fix(desktop): prevent SSRF in automatic link-title previews

[zapabob]

Summary

• Validate automatic link-title targets as public HTTP(S) destinations and reject credentials, local names, private, link-local, multicast, documentation, and carrier-grade addresses.
• Resolve the hostname once per request hop and pin every approved address with curl --resolve, preventing DNS rebinding.
• Handle redirects explicitly, revalidating and pinning every destination; proxy use is disabled.
• Remove the hidden BrowserWindow fallback so untrusted preview pages are never rendered for title extraction.
• Bound the queue, concurrent fetches, redirect count, timeouts, and response body size.

Security impact

Untrusted links may enter the desktop client through agent/tool output. Previously automatic preview fetching could reach a local or otherwise non-public address, including through redirects or DNS rebinding. This change makes the title-preview path fail closed for those targets.

Validation

• npm exec -- tsx --test electron/link-title-fetch.test.ts — 5 passed
• npm run typecheck — passed
• npm run build — passed
• git diff --check upstream/main...HEAD — passed

npm run lint has pre-existing unrelated import-order errors in the upstream renderer. The changed-file lint has no errors. The existing platform script invokes TypeScript test sources with plain node --test on this host, so focused tests use the repository's installed tsx runner.

No fork-only plugins or _docs files are included.

[teknium1]

Thanks for addressing a real exposed path. The SSRF premise is confirmed on current main: apps/desktop/electron/main.ts:3950 follows redirects with curl, and main.ts:4095-4099 falls back to BrowserWindow.loadURL(rawUrl).

Problems

• Blocking: apps/desktop/electron/link-title-fetch.ts:126-132 does not reject fec0::/10. Consequently, a DNS answer such as fec0::1 passes isPublicIpv6() and reaches the pinned curl request, so the proposed public-address boundary is incomplete.

Suggested changes

• Reject fec0::/10 and add a resolver regression test for fec0::1.
• Preserve current main's Vitest electron-test wiring (apps/desktop/vitest.config.ts:15-20, apps/desktop/package.json:47) when salvaging this stale-base test change.

Automated hermes-sweeper review.

[null-runner]

I reproduced a user-visible consequence of the hidden BrowserWindow fallback on Windows 11.

When a restored Desktop transcript contains a docs.google.com link, automatic link-title resolution can load Google in the hidden renderer. Google then invokes WebAuthn and Windows Security displays a native “Choose a passkey for google.com” dialog, even though the user did not open the link or initiate authentication.

This is separate from the SSRF premise, but it supports this PR's approach of removing the renderer fallback: DNS/network pinning alone would not prevent a public page from invoking credential UI.

For immediate local mitigation I blocked google.* before both curl and renderer resolution, blocked redirects to Google in the isolated session, and returned deterministic labels such as Google Docs, Google Sheets, and Google Drive. I am not opening a duplicate PR because this PR removes the underlying arbitrary-page renderer path.

Suggested regression coverage: assert that Google Docs URLs never reach BrowserWindow.loadURL, while retaining a readable non-network fallback label.

[GottZ]

This was generated by AI during triage.

Summary

Two PRs address the automatic Desktop link-title preview attack surface. #63171 is a compact curl-only hardening that validates and DNS-pins every hop, while #65613 is a broader implementation that centralizes URL admission and routes curl through a DNS-pinned SOCKS gateway; both remove the hidden renderer fallback.

Related pull requests

• #63171 related — (+506/-197) — overlapping fix with a concrete salvage path: validates public HTTP(S) targets, pins approved DNS answers with curl --resolve, revalidates redirects, bounds resource use, and removes arbitrary-page BrowserWindow navigation. The contributor keep_open review correctly identifies this as an exposed path, but its claimed fec0::/10 gap is contradicted by the diff: /^fe[c-f]/ rejects that range, and the included resolver test already expects fec0::1 to be rejected.
• #65613 [closed] duplicate — (+2164/-415) — closed broader reference implementation: adds shared canonical URL admission, comprehensive special-address coverage, pinned DNS caching, a loopback SOCKS transport for curl, explicit redirect admission, deadline and shutdown handling, and removes the renderer fallback. The contributor keep_open review flagged renderer navigation and an unregistered E2E test on an earlier revision; the final diff addresses both by deleting the renderer path and its E2E file, but the PR remains relevant only as a reference because the author requested closure.

Duplicates

#63171 and #65613 substantially duplicate the same SSRF and hidden-renderer fix; #65613 additionally implements a larger shared URL-admission and pinned SOCKS transport architecture.

Suggested consolidation

Keep #63171 open with a salvage path: preserve its smaller curl-only change, explicit per-hop DNS pinning, bounded queue, renderer-fallback removal, and focused regression coverage, while checking its test wiring against the current Desktop Vitest configuration during rebase. Use closed #65613 as the reference for broader canonical URL and special-address test cases if they can be adopted without importing its full transport architecture; leave #65613 closed as the duplicate/reference rather than reopening it over the earlier keep_open review and the author's explicit closure request.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup63171 ["PRs duplicating each other"]
        P63171["PR #63171 (open)"]
        P65613["PR #65613 (closed)"]
    end
    class P63171 open
    class P65613 closed
    class P63171 target
    click P63171 "https://github.com/NousResearch/hermes-agent/pull/63171"
    click P65613 "https://github.com/NousResearch/hermes-agent/pull/65613"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 111 kB of PR diffs, 3 kB of issue/PR text, 7 kB of discussion (10 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[egilewski]

too large to review safely

This PR changes 703 production lines before tests and docs. Please split it or add a focused justification if it should stay together.

Signed: GPT-5.6-luna-high in Codex

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(desktop): prevent SSRF in automatic link-title previews

1. apps/desktop/electron/link-title-fetch.ts:parseCurlOutput — headers and body share stdout (--dump-header - + --output -), and the parser scans for HTTP/ tokens to delimit header blocks. A response body that itself begins with HTTP/ (text pages, plain error dumps, markdown beginning with 'HTTP/1.1...') would be mis-consumed as another header block: its first line is parsed as a status line and the body is truncated at the next \n\n. Title-only impact, but the heuristic is fragile — using --dump-header <tempfile> would give a clean separation instead of sniffing the byte stream.

2. IPv4 special-use ranges not blocked in isPublicIpv4: 192.0.0.8/9/10 (DS-Lite, Proxy ARP, IETF anycast) and 192.31.196.0/24 (AS112). These are reserved but publicly routable, so the practical risk is low — however, since this is a security boundary, consider aligning with a standard special-use list (e.g. iana-ipv4-special-registry) rather than hand-rolling the ranges.

3. --noproxy '*' — the pinning correctly forces direct egress (prevents a rogue/proxy MITM and proxy-based redirects), but it also bypasses legitimate corporate proxy configurations, so title previews will silently fail in environments that only allow proxied egress. Worth a note (or a follow-up config toggle) since the old code honored the system proxy.

4. Behavior change worth documenting: removing the hidden-BrowserWindow fallback means JS-rendered/SPA pages and bot-walled pages that previously produced titles via the renderer will now return none. The removal is the right call security-wise (the WebAuthn/passkey repro comment shows a user-visible consequence of the renderer path), but the title-quality regression for those sites should be a conscious tradeoff — also confirm link-title-window.ts/its guards are fully dead-code-removed and not just orphaned.

5. Minor: the redirect loop runs <= maxRedirects with TITLE_MAX_REDIRECTS = 3, i.e. up to 4 fetches — same as the old --max-redirs 3, just worth a comment since the name reads like '3 total fetches'.