fix(recapture): refuse a redirect to a loopback or private address#1887

• X
• Mastodon
• Reddit
• LinkedIn

Reviewed commit: 028361afb46722f2cbe79bae7b7a91328dc78755

PR #1887 (draft, opened 2026-10-17T03:28:50Z) already exists at this head. Its live body matches /tmp/ce-1726-pr-body.md except for one trailing blank line, so I checked the controls against the live PR. Head was still 028361af when I finished. origin/main moved to 3d9ef9a9 (#1874, a signals refactor), which touches none of this branch's files.

Failure scenario for 1 and 2. A public page answers 302 Location: http://127.0.0.1.nip.io:8080/, or redirects to any name the author points at 192.168.1.1. checkHop calls refusedAddressClass, then localAddressKind. ipv4Kind splits the hostname into 7 dot-separated parts and returns null (localAddress.ts:37-40), so the hop is followed. The final-URL backstop returns null for the same reason (recapture.ts:149), and captureLifecycle.ingest stores the page (recapture.ts:186).

Failure scenario for 3. A public page embeds <iframe src="https://public.example/f">, which answers 302 to http://192.168.1.1/. will-redirect fires with isMainFrame: false, checkHop returns at backgroundRenderer.ts:415, and the frame's document is serialised into the stored MHTML and the screenshot. Not verified: whether Chromium's own local-network-access checks block a public-to-private frame in Electron 44.7.0. No fixture here has a public origin.

Body claims checked against the source (all hold except 1 and 5 above):

• Classification moved unchanged. ipv4Kind and ipv6Kind match the removed ipv4Class and ipv6Class line for line. The localhost and trailing-dot rule moved into localAddressKind (localAddress.ts:91-94). The viewer's messages are unchanged (guestLink.ts:59-61), and e2e/mhtml-link-interactivity.spec.ts:489 still expects "Points at a loopback address" and passes.
• Switch: app?.isPackaged === false && env === '1' (localAddressPolicy.ts:13-15). It admits only loopback and this-computer (:25), and a mocked packaged build refuses 127.0.0.1 with the switch set (tests/main/services/localAddressPolicy.test.ts:33-37). This covers the brief's "production refuses 127.0.0.1" test. The fixture is test-scoped and passes process.env through (e2e/fixtures/electronApp.ts:62-66).
• The renderer's third race promise reuses the timeout cleanup path (backgroundRenderer.ts:564-577). Listeners are registered before the first await (:476-477 precede :498). refused is subscribed by Promise.race, so a late reject is not unhandled.
• A Recapture of an existing Capture goes through the same check: RECAPTURE_ENQUEUE_CAPTURES, then recaptureService.enqueue (src/main/ipcHandlers.ts:974-980), then validateUrl (recapture.ts:275).
• The in-flight entry is replaced by url plus source (src/renderer/stores/appStore.ts:265-268). Both new skipped events use job.url (recapture.ts:155, :236). The body's note that the excluded-host event uses finalUrl is accurate (recapture.ts:178).
• AC4: the excluded-host check still runs after the new check (recapture.ts:167-183).
• renderPageInHiddenWindow has one wiring (src/main/index.ts:610-611). The global web-contents-created guards cover webviews only (index.ts:356-357), so the new handlers are the only ones on the hidden window.
• The four renderer call sites surface rejected[0].reason, so an enqueue-time refusal is shown to the operator (AddUrlsBox.tsx:84, CaptureMenu.tsx:78, CaptureSelectionBar.tsx:78, useLinkMenuTarget.ts:54). The activity feed shows skipReason (CaptureHealth.tsx:94).

Evidence gate. Blocking-tier hits: src/main/services/recapture.ts and src/main/services/backgroundRenderer.ts (existing rows), plus the two new rows. PR labels are evidence-affecting and agent-authored. The issue carries evidence-affecting. The Evidence impact section is substantive apart from findings 3 and 5. The known-answer table in tests/shared/localAddress.test.ts and the refusal tests in tests/main/services/recapture.test.ts:288-360 exercise the changed decision. Backward verification: the diff touches no verifier, manifest, or package code, and the full suite, including the verify tests, passes. The brief asked for a blocking row for the shared module, which is present (assessment :209). The policy row is also needed, because no src/main/services/* glob exists.

Controls.

Verification (local, Node v20.20.2, bash tool shell; no shell scripts changed, so no bash-semantics check was needed).

• pnpm lint: exit 0. 46 warnings, none in changed files.
• pnpm typecheck: exit 0.
• pnpm build: exit 0.
• BIRDBRAIN_TEST_SYSTEM_SHARP=1 BIRDBRAIN_REQUIRE_OPENSSL=1 BIRDBRAIN_REQUIRE_JQ=1 pnpm test:coverage: exit 0. 400 files passed and 1 skipped; 6490 tests passed and 17 skipped (6507). Matches the body.
• pnpm coverage:diff: exit 0, 100.00% of 67 lines. Matches the body.
• E2E under xvfb (X11): recapture-local-redirect, recapture, recapture-permissions and recapture-consent gave 5 passed. mhtml-link-interactivity gave 5 passed.
• CI at 028361af: lint, typecheck, test, build, e2e and hunks pass. merge-gate fails with "evidence-affecting with no sign-off at head", which is the expected maintainer sign-off. agent/pre-pass is pending (this review).

Could not check:

• A real packaged build refusing 127.0.0.1. Only a mocked app.isPackaged was tested.
• Whether Chromium opens any speculative connection to a redirect target before the will-redirect throttle. The e2e targets are unroutable, so the e2e runs cannot show the address was never contacted.
• Whether Chromium's local-network-access checks block the finding 3 frame in this Electron version.

Fix round at 32b1033b, on top of 028361af.

• 1 and 5: PR body, Summary and Evidence impact.
• 3: tests/main/services/backgroundRenderer.test.ts, the sub-frame test removed; the PR body's Changes block lists frames and resolved names under "left for the maintainer".
• 4: src/renderer/components/captures/useLinkMenuTarget.ts.

Reviewed commit: 32b1033b2da096466db0b1a880515a7fb25af006

Round: second pre-pass, fix-round delta 028361af..32b1033b plus the rewritten PR body. The delta rewrites one comment (src/renderer/components/captures/useLinkMenuTarget.ts:17-19) and deletes one test from tests/main/services/backgroundRenderer.test.ts. Deleting a test is executable, so this is a normal round, not an ADR-0025 prose-only delta. Under the dispatcher's scope I re-ran the touched and related suites rather than full preflight, and took CI at head as authoritative for the full suite. Head was still 32b1033b when I finished; origin/main is 3d9ef9a9.

The comment this round rewrote, checked against the code. useLinkMenuTarget.ts:17-19 says the queue "refuses the same literal local addresses the menu does, from the same classification, and the hidden render refuses a main-frame redirect to one". The queue calls refusedAddressClass (recapture.ts:104-105), which calls localAddressKind (localAddressPolicy.ts:22-26). The menu calls localAddressKind too (guestLink.ts:60-61). The render checks main-frame hops at backgroundRenderer.ts:414-420, with the listeners attached at :476-477. The comment holds. The one difference between queue and menu is the test switch, which only an unpackaged build reads (localAddressPolicy.ts:13-15, :25).

PR body claims re-derived:

• Summary 1: "lands on the name localhost or on an address ... written as numbers". localAddress.ts:93 catches localhost and any *.localhost, and :94 catches literal IPv4 and IPv6. This holds, and it understates *.localhost.
• Summary 2, "stops each such redirect before any request goes to the address": the probe below confirms it for the main frame. "The capture queue refuses a link that points at one" is recapture.ts:104-105, reached from enqueue at :275. "Checks where the render ended before anything is stored" is :149-160, which runs before ingest at :186.
• Summary 3 (the limits): holds, apart from finding 2.
• Summary 4, refusals "with the kind of address named": Points at ${label} (recapture.ts:105), Redirects to ${class} (recapture.ts:157, localAddressPolicy.ts:35).
• Evidence impact 2, "a refused final URL stops after the render and before ingest": the render is at recapture.ts:141, the refusal at :149-160 and ingest at :186, and the temp file is removed in finally at :252.
• The Changes bullet for the renderer tests matches the tests at head (:392-421). The bullet on the menu comment holds. The excluded-host follow-up candidate holds: recapture.ts:178 emits rendered.finalUrl.
• Verification block: the marker sha is head. Its 6489 passed and 17 skipped (6506) is one less than round one's 6490 (6507), which is the one deleted test. CI's test job (117304593690) ran on merge ref a7cf685a and reports 6485 passed, 17 skipped (6502), with 399 files passed and 1 skipped. The 4-test, 1-file gap is main's #1874: git diff 58997988 origin/main -- tests e2e shows 4 removed it( cases, 3 of them in the deleted tests/components/CreateSelectorCard.test.tsx. CI diff coverage, 67/67 (100.00%), matches the body.

Probe: does a refused hop open a connection? Round one could not check this. I wrote a standalone script in /tmp/ce1726-probe/main.js and ran it with the repository's Electron 44.7.0 binary under xvfb-run. The target was a raw TCP server on 127.0.0.2 that counts accepted sockets. The handler mirrors checkHop's shape (details.url, details.isMainFrame, details.preventDefault()), and each mechanism ran once without and once with preventDefault:

So Electron fires the event with isMainFrame set, and a prevented hop opens no TCP connection, preconnects included. The probe tests the Electron contract the code relies on, not renderPageInHiddenWindow itself. Not probed: form POST navigations, and sub-frame redirects, which this PR leaves out of scope.

Observation, not a finding: both refusal paths emit the same text ("Redirects to a private address", localAddressPolicy.ts:35 and recapture.ts:157). The e2e spec (e2e/recapture-local-redirect.spec.ts:119-126) therefore cannot show which path fired. The probe shows the hop path fires.

Evidence gate. The blocking-tier hits are src/main/services/recapture.ts, src/main/services/backgroundRenderer.ts, src/shared/localAddress.ts and src/main/services/localAddressPolicy.ts (assessment rows :213, :214, :209, :215). The delta touches none of them: useLinkMenuTarget.ts and the test files are not listed. The PR's labels are evidence-affecting and agent-authored, and the issue's are ready-for-agent, evidence-affecting and queued. The Evidence impact section is substantive and now carries the before-ingest wording and the frame-and-image limit. The known-answer test, tests/shared/localAddress.test.ts, is unchanged this round and passes. Backward verification: the full diff touches no verifier, manifest or package code.

Controls.

Verification (bash tool shell, Node v20.20.2).

• bash .github/scripts/dispatch/checks.sh --wait 540 1887: exit 0, with no failing and no pending checks. It passed lint, typecheck, test, build, e2e, hunks, changes, seed, adr-numbers, Dependency audit, Font licence check, Registry publish guard and Secret scan. It ignored merge-gate, which failed as expected because it reads this verdict and the evidence sign-off.
• BIRDBRAIN_TEST_SYSTEM_SHARP=1 pnpm test tests/main/services/backgroundRenderer.test.ts: exit 0, 17 passed.
• BIRDBRAIN_TEST_SYSTEM_SHARP=1 pnpm test on tests/shared/localAddress.test.ts, tests/main/services/localAddressPolicy.test.ts and tests/main/services/recapture.test.ts: exit 0, 3 files, 71 passed.
• pnpm test tests/renderer/guestLink.test.ts: exit 0.
• eslint and prettier --check on the two files the delta touches: exit 0 each.
• Not re-run locally: full preflight, the full coverage suite, build and e2e. That was the dispatcher's scope, and CI is green on all of them at this head.

Could not check:

• A real packaged build refusing 127.0.0.1, carried from round one. Only a mocked app.isPackaged is tested.
• Whether Chromium's local-network-access checks already block a public page's frame or image from a private address. That is out of scope by the dispositions above, and no fixture has a public origin.
• Whether the maintainer has seen the open question. The issue thread has no ruling after 2026-10-04.

Reviewed commit: 32b1033b2da096466db0b1a880515a7fb25af006

The commit was head at the start and at the end of the review.

Correctness traces

• Hop refusal (backgroundRenderer.ts:406-420, 476-477, 566). The handlers are attached inside render() before its first await. Promise.race is built in the same tick. refuseHop rejects synchronously inside the Electron event handler, so refused settles before the aborted loadURL rejection gets through render()'s finally. The job therefore ends skipped, not failed; e2e asserts this against a real 302. If the hop comes after the race has settled (window alive until RenderedPage.cleanup), preventDefault still stops it. The late rejection is already handled by the race, so there is no unhandled rejection.
• Refusal routing (recapture.ts:146-160, 233-242). The final-URL refusal destroys the stream, emits skipped with job.url and returns. The finally at :252 calls rendered.cleanup(). RefusedAddressError thrown from the renderer or through the fallback race becomes skipped. Nothing reaches captureLifecycle.ingest.
• Enqueue (recapture.ts:100-109). The scheme check runs first, then the classification. The only producer is enqueue (ipcHandlers.ts:961, :980), and the only renderer is wired at index.ts:611. No path reaches the renderer without validation.
• Switch (localAddressPolicy.ts:13-15). It needs app.isPackaged === false and the value exactly '1'. It admits only loopback and this-computer. Unit tests see app undefined (recapture.test.ts) or without isPackaged (backgroundRenderer.test.ts:153-156), so it is off there.
• Classification move. ipv4Kind and ipv6Kind match the removed ipv4Class and ipv6Class exactly, and the labels match the old strings. captureLinkBlockReason messages are unchanged.
• Real-Electron probe (my own script under xvfb, Electron 44.7.0, not committed). For a meta refresh, location.replace, a form POST and an a.click() to 10.255.255.1, will-navigate fired with isMainFrame=true. preventDefault kept the page on its origin. Chromium blocked a cross-origin iframe's top.location without user activation before any main-frame event.
• Bounded work: not applicable, since no stored or uploaded bytes are processed.

Evidence gate

• Touched blocking-tier paths: recapture.ts (:213), backgroundRenderer.ts (:214), and new rows localAddress.ts (:209) and localAddressPolicy.ts (:215). No advisory-tier entry was touched.
• Labels: the PR and #1726 both carry evidence-affecting.
• The Evidence impact section is substantive. It names the refusal, the literal-only and main-frame-only limits, and what verification does not cover.
• KAT: the classification table is in tests/shared/localAddress.test.ts. Refusal-writes-nothing tests are in recapture.test.ts:286-360. No hashing, manifest or verifier code is touched.
• Backward verification: no verifier, manifest or package code changed. The full suite, verify tests included, passes.

Verification (local, Node v20.20.2, detached worktree at head)

• test:coverage: exit 0, 400 files passed and 1 skipped, 6489 tests passed and 17 skipped, thresholds met.
• coverage:diff: PASS, 67/67 changed lines (100%).
• lint 0, typecheck 0, build 0, lint:boundaries 0. build:extension not needed.
• vale-prose: could not check (vale not installed here).
• Live CI: all passed except merge-gate (see 4).

Completion controls

• Commit trailers: no co-author trailer on either commit.
• auto_merge: null.
• Attribution line: present and last.
• Head sha unchanged at the end.

Scope

The guestLink refactor is the shared-classification move, the e2e switch is needed for the loopback fixtures, and the path-list rows follow from the new files. Nothing goes beyond #1726.