feat(desktop): click-to-expand link previews with SSRF-guarded resolver - #99980
feat(desktop): click-to-expand link previews with SSRF-guarded resolver#99980melon-xf wants to merge 10 commits into
Conversation
a14d80d to
9fdbbad
Compare
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 9fdbbad3d1b5deb313ac413a66a49d177b4ef3e2 against current main / base 21b2095d00a98b8ad7b5c60b10587619c852cdb8. I read the resolver, production Electron I/O, hidden-title-window path, renderer card, focused tests, all three commits, the 5-PR FILE-LIST, current hosted checks, and the prior Desktop link-title SSRF lineage (#63171 / #65613).
There is a lot worth keeping here. Making previews explicitly user-initiated instead of auto-unfurling is the right privacy boundary. The typed success/failure envelope, bounded cache, per-host pacing, field caps, initial DNS fail-closed behavior, and focused policy tests are all solid. The problem is that the security boundary stops one layer too early: the injected unit policy is stricter than the real production transports that actually perform the requests.
Blocking — redirect hops and DNS rebinding bypass the SSRF admission
electron/link-preview.ts::resolveLinkPreview() validates only the original parsed.hostname and its first DNS answer. Production then crosses two transports that can independently move somewhere else:
electron/main.ts::fetchLinkHtml()invokes curl with--location. A public URL can answer30x Location: http://127.0.0.1/..., RFC1918, link-local/cloud metadata, etc. Curl follows that hop itself;resolveLinkPreview()never sees or re-admits the redirect target.- When tier 1 yields no title,
fetchHtmlTitleWithRenderer()enters the existing hiddenBrowserWindow. Its session guard blocks selected resource types and downloads, but it does not re-run the public-address policy on navigation/redirect destinations.window.loadURL(rawUrl)therefore becomes a second redirect/re-resolution boundary after the one-time admission.
The DNS-rebinding comment in this PR correctly identifies the TOCTOU, but for a PR whose new network primitive is explicitly advertised as SSRF-guarded, leaving that hole for a follow-up is not a safe settlement. The 96 KiB response cap limits returned bytes; it does not prevent the request from reaching an internal service.
This is also established repository lineage rather than a novel review preference. Open #63171 (zapabob) already implements the missing shape for the existing Desktop title path: explicit per-hop redirects, public-address admission on every hop, DNS pinning via curl --resolve, proxy bypass, and removal of arbitrary hidden-renderer navigation. Closed #65613 (HengYangDS) is the broader prior/reference implementation of the same class. Please preserve that credit and compose with/salvage that security foundation rather than introducing a parallel, knowingly weaker resolver.
There is a concrete classifier regression from that lineage too: this PR's isPrivateAddress() rejects fe80::/10 and fc00::/7, but not deprecated site-local fec0::/10. That exact fec0::1 hole was found in review on #63171 and was fixed there in 65bfbeddaf. Add it here only if this PR still owns classification after reconciliation; ideally there should be one canonical admission implementation, not another fork of the special-address matrix.
Required proof: a production-transport regression where a public origin redirects to a listening 127.0.0.1 / private target and the private listener proves it received zero requests; the same for a public DNS answer that changes before connect. Every hop must be admitted and the connection must be pinned to the vetted address. If tier 2 remains, its navigation must satisfy the same invariant; otherwise remove it from this untrusted path as #63171 does.
Blocking — og:image reintroduces an unguarded renderer-side private-network request
parseLinkMeta() accepts any absolute http:/https: og:image / twitter:image. LinkPreviewBody then renders it directly as:
<img src={meta.imageUrl} ... />So an otherwise-public page can advertise http://127.0.0.1:..., RFC1918, link-local/metadata, or an attacker-controlled redirect as its thumbnail. After the user clicks the preview, the renderer makes that request without the main-process DNS/IP gate. This may not make the response readable to page JS, but it is still an internal-network GET primitive and it contradicts the module contract that the renderer does not talk to the network for this feature.
There is already an adjacent safe pattern in electron/favicon.ts: fetch bytes through the owned main-process I/O path and return a data URL specifically so the renderer does not perform a second network request. The preview thumbnail should use the same ownership shape (guarded/bounded fetch -> validated image bytes -> data/blob payload), or omit the thumbnail when it cannot be proven safe.
Required proof: public page -> private og:image and public page -> thumbnail redirect-to-private both produce zero private-network requests. Positive control: an ordinary public thumbnail still renders.
Blocking repository gate — do not grow electron/main.ts
This change adds the preview runtime/persistence/network owner directly into apps/desktop/electron/main.ts; the patch's own later hunk is already around line 16,930. Under the repository's 2K/godfile invariant, the new runtime needs an extracted owner (for example link-preview-runtime.ts / IPC registrar) and main.ts should remain only the narrow composition seam. link-preview.ts itself is a good start; the production I/O, persistence/store construction, limiter, and IPC registration belong with it rather than adding another subsystem to the godfile.
Interlock / merge order
The PR says Standalone, but the FILE-LIST is not actually independent:
- #99979 touches
apps/desktop/src/i18n/{ar,en,ja,types,zh}.ts, all also touched here. That is a real collision/rebase edge even if the features are logically independent. - #63171 touches
apps/desktop/electron/main.tsand owns the exact redirect/DNS-pinning security seam this feature currently reimplements incompletely. - #65613 is closed/unmerged prior reference work, not something to silently erase; #63171 is the current open salvage path.
Please update the stated merge topology accordingly and preserve the prior contributors' credit. I would treat #63171/security-foundation reconciliation as upstream of this feature, and whichever of #99979/#99980 lands second must rebase/re-run its locale/type checks.
Acceptance / every-commit receipts
This PR has three commits:
df7749163c9424febb0c588b2832e9083b90d268— no hosted PR workflow run found6de01e97c99a3d3f37118090f3fba5b26bff518e— no hosted PR workflow run found9fdbbad3d1b5deb313ac413a66a49d177b4ef3e2— CI, Docker, and Nix workflow runs are currentlyaction_required, not green
The local Vitest/tsc receipts in the body are useful, but the resolver tests inject LinkPreviewIo; they therefore cannot prove the real curl --location or hidden-BrowserWindow redirect behavior above. After the security/ownership fixes, every commit still needs exact-SHA hosted green receipts before this is merge-ready.
The feature idea and consent model are strong. Once all network edges consume the same public-destination proof — initial page, every redirect, tier-2 navigation if retained, and thumbnail bytes — this becomes a much cleaner boundary rather than an SSRF guard with known escape hatches. 🚀
alt-glitch
added
type/feature
… durable cache (D7)
Click-to-expand unfurl behind the user's click: a mentioned URL is never
fetched. The Electron main process owns the whole policy, extending the
existing tiered title machinery instead of forking it — tier 1 reuses the
curl request shape with the same 96 KB byte budget, tier 2 backfills the
title through the hidden-window renderer for JS-only pages.
- electron/link-preview.ts: og/twitter/description/<title> extraction
(entity decode, whitespace collapse, 240/300 caps, relative og:image
resolution), private-hostname + resolved-IP SSRF guard (fail closed on
unresolvable or private answers), HostRateLimiter (>=10s spacing between
same-host starts, <=3 in flight per host, 4 process-wide), LinkPreviewStore
(400-entry LRU, 24h TTL, debounced persisted JSON in userData; misses are
never cached so a failed unfurl can succeed on retry)
- resolveLinkPreview never throws: every failure leg is an envelope
{ok:false, reason:'private-url'|'error'} the renderer can render honestly
- hermes:fetchLinkPreview IPC + preload bridge + renderer type; hermes:
fetchLinkTitle contract untouched
…t messages (D7) Renderer half of D7: an assistant message that mentions external http(s) URLs gains a collapsed chip per URL once the turn settles. Nothing is fetched for a mentioned URL — the chip is a local affordance, and clicking it performs the one main-process fetch (see the D7 resolver commit) and expands into an honest card: thumbnail, title, description, or a specific failure line. No silent legs. - thread/content.ts extractLinkPreviewTargets: settled-text URL scan, deduped, localhost-shaped hosts excluded — they can never unfurl and a chip that cannot deliver is noise. Streaming-safe: callers run it only after the '' while running branch, so per-token flushes skip the scan - link-preview-card.tsx LinkPreviewChipCard: one-shot sticky expansion, state held for the mounted transcript's life so re-renders never refetch; failure legs (error / private-url / bridge missing) each render text; collapsed chip uses Tip, not native title (no-native-title guard) - mounted as the AssistantLinkPreviews PERF leaf beside AssistantPreviewEmbeds: renders null without the desktop bridge, while running, or when no external URL is mentioned - i18n: linkPreview* keys in en, zh, zh-hant, ja, ar + types - markdown-text.tsx untouched by design: plain links stay plain; provider embeds keep their own consent gate
… guard Review note on D7: the hostname is vetted (public + DNS answers private- checked) but io.fetchHtml resolves it again — a rebinding attacker can flip the answer between the two lookups. Exposure is bounded (main-process GET, byte-capped, og/title-only rendering), so this ships as an explicit, scoped note with the full fix (socket pinning to a vetted address) called out as follow-up work rather than an untested resolver swap in this PR.
The tier-1 curl fetch used --location --max-redirs 3: the SSRF guard (scheme + hostname + DNS resolution) vetted only the INITIAL url, and any 30x from there was followed to an arbitrary destination — loopback, RFC1918, or 169.254.169.254 — whose title/description then reached the renderer as a link card. A vetted first hop proved nothing about where the chain ends. curl is now driven one hop at a time and the chain is walked by fetchWithGuardedRedirects (link-preview.ts), which re-applies the full guard to every Location target before it is requested, resolves relative Locations against the current url, refuses non-http targets, and stops after 3 redirects (curl's old --max-redirs). Refusals and exhausted budgets surface as '' — the same miss signal the old curl error leg produced — so tier 2 and the envelope see nothing new. The title fetcher and the preview fetcher shared the vulnerable request verbatim, so both now share one guarded fetcher (fetchPageHtmlWithCurl) instead of two copies of the curl arg list. --include puts the status line and headers on stdout so the hop loop can read them; the byte budget covers the header block, and TITLE_TIMEOUT_MS budgets the whole walk, matching the single --max-time it replaces.
… guard The hidden tier-2 BrowserWindow follows redirects itself: Chromium walks the 30x chain and resolves each hop's DNS, so the per-hop curl guard B5 added covered only the URLs we fetch. Every request the window makes now passes the same verdict through the partition's single webRequest.onBeforeRequest (the guard owns the registration; a second one would replace it): resource-type blocks, isPrivateHostname on the request URL, and — invoked asynchronously, as the callback allows — our own DNS resolution checked with isPrivateAddress, which is what closes redirect hops to names that answer private without depending on Chromium's resolution timing. details.ip, when Chromium supplies it, only tightens the verdict. A failing or empty resolution denies: unresolvable names would fail to connect anyway. Regression tests are red on the unfixed guard (it had no request surface at all); will-download cancellation is unchanged and shares the one guard.
…ite-local IPv6 Adopts the security shape of upstream NousResearch#63171 (fetchPinnedLinkTitle, and the site-local classification from 65bfbeddaf) rather than parallel-implementing it — review feedback on this PR was exactly that these two primitives belong to that foundation, not a third variant. The hop guard now RETURNS the addresses it vetted, and the transport is required to pin the connection to one of them (curl --resolve host:port:addr, one per vetted address — curl fails over among pinned addresses without a fresh lookup). Before, the guard resolved the name, called it public, and then let curl resolve it AGAIN: an attacker controlling DNS could swap the answer to a private address between verdict and request. The pin closes that window for the tier-1 curl walk; tier-2's window guard is unchanged (its residual is documented in link-title-window.ts). isPrivateAddress also refuses fec0::/10 (site-local): deprecated, but still routable in some estates, and upstream already rejects it for link titles. Regression proof is the pinning contract itself: each hop's request carries the addresses vetted for THAT hop, and a site-local-only name is refused before any request.
… pinned hop walk
The preview card rendered <img src={meta.imageUrl}> directly: a renderer-side
GET to an attacker-chosen URL with page context, no guard, no admission —
exactly the boundary the rest of this PR guards (review blocking item 2).
The policy layer now refuses a private og:image before any I/O, and the
runtime fetches the bytes through the SAME per-hop admission and DNS pinning
as the page fetch (resolveThumbnail), sniffs them as a real image, and hands
the renderer a data URL it can paint without touching the network. An image
that cannot be proven (private hop, rebinding name, non-image bytes, oversized)
omits the thumbnail rather than weakening the card. The durable cache stores
no image bytes; the data URL rides the envelope and a small runtime LRU.
Regression proofs: private og:image and redirect-to-private thumbnails make
ZERO requests past the vetted first hop (binary Io records every requested
URL); public PNGs still render.
fix(desktop) shape credit: thumbnail leg follows the favicon.ts I/O pattern.
main.ts is the composition seam, not the runtime: the title cache, the one-hop pinned curl transport, the hidden title-window queue, the durable preview cache, and the IPC surface all move to link-preview-runtime.ts (review blocking item 3). The module takes app/BrowserWindow/session as a deps object so it stays unit-testable without Electron, and registers its two handles (hermes:fetchLinkTitle, hermes:fetchLinkPreview) through registerLinkPreviewRuntime — main.ts constructs and registers, nothing more. No behavior change: the handles resolve identically, the tier-2 window keeps its guarded session, and the preview flow is unchanged apart from the thumbnail leg landing in the previous commit.
The renderer tests asserted the old <img src={imageUrl}> behavior; they now
assert the card paints the main-process-validated data URL and NEVER a remote
src, plus a leg for the unprovable-thumbnail case (image: '' renders the
unavailable note, no img element).
Unused imports and statement padding across the touched files; the link-preview-card thumbnail JSX keeps its explanatory comment.
9fdbbad to
17fac9e
Compare
|
Thanks @andrexibiza for the careful review — all three blocking items are addressed in follow-up commits on this branch (head now 1. Redirect hops + DNS rebinding — tier-1 fetch re-admits every redirect hop through the SSRF guard (no more 2. og:image — the renderer no longer fetches the advertised URL. The main process walks og:image through the same guarded/pinned hop walk, validates the bytes, and delivers a data-URL envelope; unprovable thumbnails are omitted, never renderer-fetched. 3. Godfile — runtime owner extracted to Merge-order note corrected in the description: not standalone — #63171 upstream of this (the primitives here are written to land on that seam), and #99979's i18n changes land first (rebase done; no collision at head). Verification at head: |
b6d76a8 to
17fac9e
Compare
3 participants
— feat(desktop): click-to-expand link previews
Branch:
pr/link-previewsReview response (2026-09-01, andrexibiza review)
All three blocking items are addressed in follow-up commits on this branch:
re-admits every redirect hop through the SSRF guard (
curl--locationis gone), the tier-2 hidden window's requests are gated through the same
guard, and every hop is pinned to its vetted addresses (DNS pinned via
resolve-to-vetted-address, closing the rebinding window the Limitations
section used to document). Site-local IPv6 (
fec0::/10) is added to theprivate-address classifier. The per-hop admission + DNS pinning shape
follows the security seam proposed in fix(desktop): prevent SSRF in automatic link-title previews #63171 — credit to that effort; this
PR composes with it rather than parallel-implementing it.
Zero-request regressions: a private HTTP listener asserts it receives
ZERO requests across redirect-to-private, rebinding, and og:image scenarios
(see
link-preview.test.ts/link-title-window.test.ts).by the renderer from a raw URL. The main process fetches og:image through
the same guarded/pinned hop walk, validates the bytes, and hands the
renderer a data-URL envelope. When the thumbnail can't be proven safe, it
is omitted (never renderer-fetched). The favicon-fetch shape applies.
electron/link-preview-runtime.tswith a narrow IPC registrar;main.tsis the composition seam again (−299 lines).
Merge order
Not standalone. #63171 owns the shared SSRF security seam and should land
upstream of this PR; this branch's per-hop admission/DNS-pinning primitives are
written to compose with it. #99979 collides on i18n files and lands before this.
Series order: #2 (media-roots) -> #1 (events+relay) -> #3 (desktop cards) ->
#4/#5 (independent). This PR is 4 of 5.
Motivation
URLs in agent replies render as bare text. Rich previews would help — but the
common implementation (auto-fetch every mentioned URL) is both a privacy surprise
and an SSRF surface. This feature gets the value without the surveillance-grade
behavior: nothing is fetched until the user clicks "Load link preview."
Approach
A mentioned URL renders as a small, passive chip. On click, the desktop's main
process (not the renderer — cross-origin HTML is unreadable from a renderer
canvas) fetches the page once within strict bounds and extracts Open Graph /
Twitter-card meta plus
<title>:hostname literal-IP checks and DNS resolution both run through a
private-address classifier (loopback, RFC1918, link-local, site-local
fec0::/10, unique-local, CGNAT, unspecified, broadcast, IPv4-mapped IPv6,*.localhost,*.local,*.internal,*.home.arpa, single-label names).Any private answer refuses the fetch. Each fetch is pinned to the vetted
address — a later DNS flip cannot redirect an admitted connection.
(hidden window, images blocked, downloads cancelled, requests guard-gated),
per-host rate limiter, timeouts, 400-entry durable cache.
<meta>tags — head-region territory.What changed
apps/desktop/electron/link-preview.ts: resolver, parser, guard, cache,rate limiter (unit-tested in isolation).
apps/desktop/electron/link-preview-runtime.ts: runtime owner(review item 3) with narrow IPC registrar;
main.tsstays the seam.electron/link-title-window.ts: tier-2 window whose requests pass throughthe SSRF guard.
preload.ts/src/global.d.ts: IPC surface.link-preview-card.tsx+thread/content.ts+assistant-message.tsx:chip rendering and click-to-expand in settled assistant messages; thumbnails
arrive as validated main-process data URLs (review item 2).
private-address matrix (with
fec0::/10), redirect-to-private, DNS-pinning,and thumbnail data-URL envelope legs.
Testing
Full battery at head 17fac9e (rebased on
main04224b2):npm run test:ui— 6948 passed / 703 files;npm run test:desktop:platforms—2056 passed (one environmental ssh-socket-path assertion excluded, fails
identically on pristine
mainunder a deep$HOME);npm run lint— 0 errors;npm run typecheck— clean.Limitations
by design (same policy as the original guard, now including
fec0::/10).renderer-internal subresource loads are blocked outright rather than pinned
(images blocked, downloads cancelled), so no unpinned network egress remains.
PR series merge order: #2 (media-roots) -> #1 (events+relay) -> #3 (desktop cards) -> #4/#5 (independent). This PR is 4 of 5.