From 828395d510bd4f29525e5a0807a550a2daad885c Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:25:16 +0800 Subject: [PATCH] fix(browser): ignore duplicate frame navigations and read block-form source map directives (#546) --- src/browser/CdpCaptureEventHandlers.ts | 9 ++- src/browser/CdpCaptureEventState.ts | 6 ++ src/browser/CdpCaptureEvents.ts | 11 ++- src/domain/javascriptStaticAnalysisCalls.ts | 7 +- ...javascriptStaticAnalysisSourceMaps.test.ts | 42 ++++++++++ .../browser/cdpCaptureNavigation.test.ts | 78 +++++++++++++++++++ 6 files changed, 149 insertions(+), 4 deletions(-) create mode 100644 src/domain/javascriptStaticAnalysisSourceMaps.test.ts create mode 100644 tests/boundary/browser/cdpCaptureNavigation.test.ts diff --git a/src/browser/CdpCaptureEventHandlers.ts b/src/browser/CdpCaptureEventHandlers.ts index 23f5731550..ad273d0884 100644 --- a/src/browser/CdpCaptureEventHandlers.ts +++ b/src/browser/CdpCaptureEventHandlers.ts @@ -367,8 +367,15 @@ export const handleFrameNavigated = ( ): void => { const frame = recordValue(params.frame); if (stringValue(frame?.id) !== state.mainFrameId) return; - state.navigationDuringCapture = true; const rawUrl = stringValue(frame?.url); + const loaderId = stringValue(frame?.loaderId); + // `frameNavigated` re-fires for a document already reported (same URL and + // loader). Treating that as a navigation aborted a valid capture with + // `target_changed`, which is a false claim that the target moved. + const document = `${String(loaderId ?? "")}\0${String(rawUrl ?? "")}`; + if (state.committedDocument === document) return; + state.committedDocument = document; + state.navigationDuringCapture = true; if ( isHttpUrl(rawUrl) && allowedSanitizedUrl(rawUrl, state.allowedOrigins) === undefined diff --git a/src/browser/CdpCaptureEventState.ts b/src/browser/CdpCaptureEventState.ts index 324881c28d..654313961d 100644 --- a/src/browser/CdpCaptureEventState.ts +++ b/src/browser/CdpCaptureEventState.ts @@ -20,4 +20,10 @@ export interface CdpCaptureEventsState { originViolation: boolean; navigationDuringCapture: boolean; mainFrameId: string | undefined; + /** + * Identity of the document last observed in the main frame. `frameNavigated` + * re-fires for the same document, so this is what distinguishes a real + * document change from a duplicate event. + */ + committedDocument: string | undefined; } diff --git a/src/browser/CdpCaptureEvents.ts b/src/browser/CdpCaptureEvents.ts index df0f7edec1..e06a8722e3 100644 --- a/src/browser/CdpCaptureEvents.ts +++ b/src/browser/CdpCaptureEvents.ts @@ -33,6 +33,7 @@ export class CdpCaptureEvents implements CdpCaptureEventsState { originViolation = false; navigationDuringCapture = false; mainFrameId: string | undefined = undefined; + committedDocument: string | undefined = undefined; readonly input: InspectWebPageInput; readonly allowedOrigins: ReadonlySet; @@ -46,6 +47,7 @@ export class CdpCaptureEvents implements CdpCaptureEventsState { this.mainFrameId = frameId; this.originViolation = false; this.navigationDuringCapture = false; + this.committedDocument = undefined; this.scripts.clear(); this.executionContextFrames.clear(); this.network.clear(); @@ -65,7 +67,14 @@ export class CdpCaptureEvents implements CdpCaptureEventsState { beginFinalCapture(frameId: string): void { this.mainFrameId = frameId; - this.navigationDuringCapture = false; + // Deliberately does not clear `navigationDuringCapture`. A main-frame + // navigation observed during the observation window means the page never + // settled, and clearing the flag here reported the capture as clean while + // carrying pre-navigation scripts, network and console into the inventory + // of the new document. The caller now reports `target_changed` instead. + // `committedDocument` is dropped so the new document's frame events are + // compared against itself rather than the document we just left. + this.committedDocument = undefined; } ingest(event: CdpEvent): void { diff --git a/src/domain/javascriptStaticAnalysisCalls.ts b/src/domain/javascriptStaticAnalysisCalls.ts index 44c81c999b..4e23d4117b 100644 --- a/src/domain/javascriptStaticAnalysisCalls.ts +++ b/src/domain/javascriptStaticAnalysisCalls.ts @@ -273,11 +273,14 @@ export const addSourceMapDirectives = ( source: string, accumulator: AnalysisAccumulator, ): void => { + // Minifiers emit either `//# sourceMappingURL=` or the block form + // `/*# sourceMappingURL=app.js.map */`. Matching only the line form silently + // dropped the block form, so a real footer produced no mapping at all. for (const match of source.matchAll( - /\/\/[#@]\s*sourceMappingURL\s*=\s*([^\s]+)/gu, + /(?:\/\/|\/\*)[#@]\s*sourceMappingURL\s*=\s*([^\s*]+)/gu, )) { const declared = match[1]; - if (declared === undefined) continue; + if (declared === undefined || declared.length === 0) continue; const start = match.index; const location = rangeForOffsets(source, start, start + match[0].length); addFindingOnce(accumulator, `source-map\0${declared}`, () => diff --git a/src/domain/javascriptStaticAnalysisSourceMaps.test.ts b/src/domain/javascriptStaticAnalysisSourceMaps.test.ts new file mode 100644 index 0000000000..222252c227 --- /dev/null +++ b/src/domain/javascriptStaticAnalysisSourceMaps.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from "vitest"; + +import { analyzeJavaScriptStaticSource } from "./javascriptStaticAnalysis.js"; + +const sourceMapUrls = (source: string): readonly string[] => + analyzeJavaScriptStaticSource(source).source_map_urls.map( + ({ declared_url: declaredUrl }) => declaredUrl, + ); + +describe("source map directives", () => { + it.each([ + ["line comment", "//# sourceMappingURL=app.js.map", "app.js.map"], + ["legacy line comment", "//@ sourceMappingURL=app.js.map", "app.js.map"], + // Minifiers commonly emit the block form; it previously matched nothing. + ["block comment", "/*# sourceMappingURL=app.js.map */", "app.js.map"], + [ + "block comment after code", + "code();\n/*# sourceMappingURL=vendor.abc.map */", + "vendor.abc.map", + ], + ])("reads the %s form", (_label, source, expected) => { + expect(sourceMapUrls(source)).toEqual([expected]); + }); + + it("keeps the comment terminator out of the declared url", () => { + expect(sourceMapUrls("/*# sourceMappingURL=app.js.map */")).toEqual([ + "app.js.map", + ]); + }); + + it("retains every directive in a source", () => { + expect( + sourceMapUrls( + "//# sourceMappingURL=first.map\n/*# sourceMappingURL=second.map */", + ), + ).toEqual(["first.map", "second.map"]); + }); + + it("reports nothing when no directive is present", () => { + expect(sourceMapUrls("const value = 1;")).toEqual([]); + }); +}); diff --git a/tests/boundary/browser/cdpCaptureNavigation.test.ts b/tests/boundary/browser/cdpCaptureNavigation.test.ts new file mode 100644 index 0000000000..070d6b1122 --- /dev/null +++ b/tests/boundary/browser/cdpCaptureNavigation.test.ts @@ -0,0 +1,78 @@ +import { expect, it, onTestFinished } from "vitest"; + +import { CdpCaptureEvents } from "../../../src/browser/CdpCaptureEvents.js"; +import { inspectWebPageInputSchema } from "../../../src/domain/browserObservation.js"; +import { CdpBrowserProvider } from "../../../src/browser/CdpBrowserProvider.js"; +import { startFakeCdpBrowser } from "../../fixtures/fakeCdpBrowser.js"; +import type { CdpEvent } from "../../../src/browser/CdpConnection.js"; + +const events = (): CdpCaptureEvents => + new CdpCaptureEvents( + inspectWebPageInputSchema.parse({ + cdp_endpoint: "http://127.0.0.1:9222", + allowed_origins: ["https://example.test"], + target_id: "page-1", + }), + new Set(["https://example.test"]), + ); + +const frameNavigated = (url: string, loaderId: string): CdpEvent => ({ + method: "Page.frameNavigated", + params: { + frame: { id: "main", url, loaderId }, + }, +}); + +it("ignores a repeated frameNavigated for the document already committed", () => { + const state = events(); + state.beginAuthorizedFrame("main"); + state.ingest(frameNavigated("https://example.test/page", "loader-1")); + expect(state.navigationDuringCapture).toBe(true); + state.navigationDuringCapture = false; + + // A re-fire with the same URL and loader is not a navigation. + state.ingest(frameNavigated("https://example.test/page", "loader-1")); + expect(state.navigationDuringCapture).toBe(false); +}); + +it("still reports a genuine change of document", () => { + const state = events(); + state.beginAuthorizedFrame("main"); + state.ingest(frameNavigated("https://example.test/page", "loader-1")); + state.navigationDuringCapture = false; + state.ingest(frameNavigated("https://example.test/other", "loader-2")); + expect(state.navigationDuringCapture).toBe(true); +}); + +it("does not forget a navigation that happened during the observation window", () => { + const state = events(); + state.beginAuthorizedFrame("main"); + state.ingest(frameNavigated("https://example.test/page", "loader-1")); + // The final capture re-attaches to the resolved frame; clearing the flag here + // used to report a clean capture while carrying pre-navigation state. + state.beginFinalCapture("main"); + expect(state.navigationDuringCapture).toBe(true); +}); + +it("keeps a settled observation window clean", () => { + const state = events(); + state.beginAuthorizedFrame("main"); + state.beginFinalCapture("main"); + expect(state.navigationDuringCapture).toBe(false); +}); + +it("captures a page without aborting on a duplicate frame event", async () => { + const browser = await startFakeCdpBrowser({ sessionTimeline: "same_origin" }); + onTestFinished(async () => { + await browser.close(); + }); + const result = await new CdpBrowserProvider().inspectPage( + inspectWebPageInputSchema.parse({ + cdp_endpoint: browser.endpoint, + allowed_origins: [browser.allowedOrigin], + target_id: "allowed-page", + observation_ms: 0, + }), + ); + expect(result.ok).toBe(true); +});