From 6b0113a534aab0a5d5db9ed5c67cafb183d7bcfd Mon Sep 17 00:00:00 2001 From: ShuaixinHuang <1836656514@qq.com> Date: Sat, 17 Oct 2026 23:23:37 +0800 Subject: [PATCH] fix(browser): preserve URL paths when removing credentials (#4731) * fix(browser): preserve URL paths when removing credentials * test(browser): verify URL userinfo through real V8 captures --- scripts/lib/browser-url-userinfo-e2e.mjs | 186 ++++++++++++++++++ scripts/verify-real-browser.mjs | 3 + src/domain/browserObservation.test.ts | 36 ++++ src/domain/browserObservation.ts | 35 +--- ...cdpBrowserProvider.targetInventory.test.ts | 44 +++-- 5 files changed, 260 insertions(+), 44 deletions(-) create mode 100644 scripts/lib/browser-url-userinfo-e2e.mjs diff --git a/scripts/lib/browser-url-userinfo-e2e.mjs b/scripts/lib/browser-url-userinfo-e2e.mjs new file mode 100644 index 0000000000..4506c04452 --- /dev/null +++ b/scripts/lib/browser-url-userinfo-e2e.mjs @@ -0,0 +1,186 @@ +import assert from "node:assert/strict"; +import { createHash } from "node:crypto"; +import { fileURLToPath } from "node:url"; +import { Client } from "@modelcontextprotocol/client"; +import { StdioClientTransport } from "@modelcontextprotocol/client/stdio"; + +import { CdpConnection } from "../../dist/browser/CdpConnection.js"; +import { parseEvidence } from "../../dist/domain/evidence.js"; +import { artifactCliEvidence } from "./artifact-e2e.mjs"; +import { closeVerifierResources } from "./verifier-cleanup.mjs"; + +/** Verify real V8 source labels, which retain spelling that navigation normalizes. */ +export async function verifyBrowserUrlUserInfo(endpoint, origin) { + const rawUrl = `${origin.replace("://", "://user:pass@")}\\path@tail/../keep.js?x=1#frag`; + const expectedUrl = `${origin}\\path@tail/../keep.js?x=1#frag`; + const sources = [rawUrl, expectedUrl].map( + (url, index) => + `globalThis.reaUserinfoUrlProbe${index} = () => 4731;\n//# sourceURL=${url}`, + ); + const response = await fetch(`${endpoint}/json/new?about:blank`, { + method: "PUT", + }); + assert.equal( + response.ok, + true, + "Browser refused the owned URL fixture target", + ); + const target = await response.json(); + assert.equal(typeof target.id, "string"); + let connection; + let transport; + let client; + let verificationError; + try { + connection = await CdpConnection.connect( + target.webSocketDebuggerUrl, + "inspect_web_page", + ); + await loadFixturePage(connection, `${origin}/app?userinfo-probe`); + const producerUrls = []; + connection.onEvent((event) => { + if (event.method === "Debugger.scriptParsed") + producerUrls.push(event.params.url); + }); + await connection.send("Debugger.enable"); + const browserUrl = await connection.send("Runtime.evaluate", { + expression: `new URL(${JSON.stringify(rawUrl)}).href`, + returnByValue: true, + }); + assert.equal(browserUrl.exceptionDetails, undefined); + assert.equal(browserUrl.result.value, new URL(rawUrl).href); + assert.notEqual(browserUrl.result.value, rawUrl); + assert.equal(new URL(browserUrl.result.value).pathname, "/keep.js"); + for (const source of sources) { + const evaluated = await connection.send("Runtime.evaluate", { + expression: source, + returnByValue: true, + }); + assert.equal(evaluated.exceptionDetails, undefined); + } + assert.ok( + [rawUrl, expectedUrl].every((url) => producerUrls.includes(url)), + "Real V8 did not retain the exact sourceURL spelling on the CDP wire", + ); + const assertScripts = (evidence) => { + const result = parseEvidence(evidence).normalized_result; + return sources.map((source) => { + const script = result.scripts.items.find( + (item) => + item.source.included && item.source.artifact.text === source, + ); + assert.ok( + script, + "Public capture omitted the retained V8 fixture script", + ); + assert.equal(script.url, expectedUrl); + assert.equal(new URL(script.url).origin, origin); + assert.equal( + script.source.artifact.sha256, + createHash("sha256").update(source).digest("hex"), + ); + return script.url; + }); + }; + const cliUrl = assertScripts( + await artifactCliEvidence("inspect-web-page", endpoint, [ + target.id, + "--allowed-origins", + origin, + "--include-script-sources", + "--observation-ms", + "0", + ]), + ); + transport = new StdioClientTransport({ + command: process.execPath, + args: [fileURLToPath(new URL("../rea.mjs", import.meta.url)), "mcp"], + env: { ...process.env, REA_LOG_LEVEL: "silent" }, + stderr: "pipe", + }); + client = new Client({ name: "browser-url-userinfo-e2e", version: "1" }); + await client.connect(transport); + const mcp = await client.callTool({ + name: "inspect_web_page", + arguments: { + cdp_endpoint: endpoint, + allowed_origins: [origin], + target_id: target.id, + include_script_sources: true, + observation_ms: 0, + }, + }); + assert.notEqual(mcp.isError, true, JSON.stringify(mcp)); + assert.deepEqual(assertScripts(mcp.structuredContent), cliUrl); + return { + cli: true, + stdio_mcp: true, + raw_v8_source_url: true, + credential_free_control: true, + original_source_digest: true, + browser_url_parser_normalizes_backslash_and_dot_segments: true, + }; + } catch (cause) { + verificationError = cause; + throw cause; + } finally { + await closeVerifierResources( + [ + async () => client?.close(), + async () => transport?.close(), + async () => connection?.close(), + async () => { + const closed = await fetch(`${endpoint}/json/close/${target.id}`); + assert.equal( + closed.ok, + true, + "Browser retained the owned URL fixture target", + ); + }, + ], + verificationError, + ); + } +} + +async function loadFixturePage(connection, url) { + const loaded = new Set(); + let loaderId; + let resolveLoaded; + let timer; + const load = new Promise((resolve) => { + resolveLoaded = resolve; + }); + const unsubscribe = connection.onEvent((event) => { + if (event.method !== "Page.lifecycleEvent" || event.params.name !== "load") + return; + loaded.add(event.params.loaderId); + if (event.params.loaderId === loaderId) resolveLoaded(); + }); + try { + await connection.send("Page.enable"); + await connection.send("Page.setLifecycleEventsEnabled", { enabled: true }); + const navigation = await connection.send("Page.navigate", { url }); + assert.equal(navigation.errorText, undefined); + assert.equal(typeof navigation.loaderId, "string"); + loaderId = navigation.loaderId; + if (!loaded.has(loaderId)) + await Promise.race([ + load, + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new Error("Owned URL fixture did not finish loading")), + 10_000, + ); + }), + ]); + assert.equal( + (await connection.send("Page.getFrameTree")).frameTree.frame.url, + url, + ); + } finally { + clearTimeout(timer); + unsubscribe(); + await connection.send("Page.setLifecycleEventsEnabled", { enabled: false }); + } +} diff --git a/scripts/verify-real-browser.mjs b/scripts/verify-real-browser.mjs index f412aafddc..54c1eaa112 100644 --- a/scripts/verify-real-browser.mjs +++ b/scripts/verify-real-browser.mjs @@ -53,6 +53,7 @@ import { artifactCliEvidence, artifactMcpResult } from "./lib/artifact-e2e.mjs"; import { verifyScenarioEnvironment } from "./lib/browser-scenario-environment-e2e.mjs"; import { verifyScenarioStorage } from "./lib/browser-scenario-storage-e2e.mjs"; import { verifyScenarioServiceWorkers } from "./lib/browser-scenario-service-workers-e2e.mjs"; +import { verifyBrowserUrlUserInfo } from "./lib/browser-url-userinfo-e2e.mjs"; const SCENARIO_SECRET_VALUE = "rea-browser-verifier-secret"; const SCENARIO_URL_SECRET_VALUE = "rea-browser-url-verifier-secret"; @@ -378,6 +379,7 @@ try { }); const moduleTrace = await verifyBrowserModules(executable); const captureMetadata = await verifyBrowserCaptureMetadataBudget(endpoint); + const urlUserInfo = await verifyBrowserUrlUserInfo(endpoint, site.origin); report = { browser: observed.value.browser.product, endpoint, @@ -405,6 +407,7 @@ try { scriptExport, moduleTrace, captureMetadata, + urlUserInfo, domDestinations, browserScenarioCli: true, browserScenarioAttachCleanup: "disconnected-external", diff --git a/src/domain/browserObservation.test.ts b/src/domain/browserObservation.test.ts index 6bcf09f686..2de2a7cd28 100644 --- a/src/domain/browserObservation.test.ts +++ b/src/domain/browserObservation.test.ts @@ -123,6 +123,42 @@ describe("browser observation sensitive surfaces and retention", () => { expect(sanitized.redacted).toBe(true); }); + it("preserves path userinfo-like text after a special-scheme backslash", () => { + const suffix = String.raw`\path@tail/../keep?x=1#frag`; + expect( + sanitizeBrowserUrl(`https://user:pass@app.example.test${suffix}`), + ).toEqual({ + url: `https://app.example.test${suffix}`, + origin: "https://app.example.test", + query_parameter_names: ["x"], + redacted: true, + }); + expect(sanitizeBrowserUrl(`https://app.example.test${suffix}`)).toEqual({ + url: `https://app.example.test${suffix}`, + origin: "https://app.example.test", + query_parameter_names: ["x"], + redacted: false, + }); + expect( + sanitizeBrowserUrl( + String.raw`custom://user:pass@host\name@tail/path?x=1#frag`, + ), + ).toEqual({ + url: "custom://tail/path?x=1#frag", + origin: null, + query_parameter_names: ["x"], + redacted: true, + }); + expect( + sanitizeBrowserUrl("https://us\ter:pass@app.example.test/path?x=1#frag"), + ).toEqual({ + url: "https://app.example.test/path?x=1#frag", + origin: "https://app.example.test", + query_parameter_names: ["x"], + redacted: true, + }); + }); + it("retains ordinary query values and malformed URL diagnostics", () => { const longName = `a${"x".repeat(400)}`; const parameters = [ diff --git a/src/domain/browserObservation.ts b/src/domain/browserObservation.ts index a1e669b267..0c513eceec 100644 --- a/src/domain/browserObservation.ts +++ b/src/domain/browserObservation.ts @@ -1,7 +1,10 @@ import { z } from "zod"; import { urlDeclaresQueryOrFragment } from "./urlComponents.js"; -import { parseUrlQueryParameterNames } from "./recordedTransportUrl.js"; +import { + parseRecordedTransportUrl, + parseUrlQueryParameterNames, +} from "./recordedTransportUrl.js"; const parseExactOrigin = (value: string): string | undefined => { let url: URL; @@ -268,32 +271,10 @@ export const sanitizeBrowserUrl = (value: string): SanitizedBrowserUrl => { }; const removeUrlUserInfo = (value: string, parsed: URL): string => { - const schemeSeparator = value.indexOf("://"); - const authorityStart = - schemeSeparator < 0 - ? value.startsWith("//") - ? 2 - : -1 - : schemeSeparator + 3; - if (authorityStart < 0) return credentialFreeHref(parsed); - const remaining = value.slice(authorityStart); - const delimiter = remaining.search(/[/?#]/u); - const authorityEnd = - delimiter < 0 ? value.length : authorityStart + delimiter; - const authority = value.slice(authorityStart, authorityEnd); - const userInfoEnd = authority.lastIndexOf("@"); - if (userInfoEnd < 0) return credentialFreeHref(parsed); - const candidate = `${value.slice(0, authorityStart)}${authority.slice(userInfoEnd + 1)}${value.slice(authorityEnd)}`; - try { - const candidateUrl = new URL(candidate); - return candidateUrl.username === "" && candidateUrl.password === "" - ? candidate - : credentialFreeHref(parsed); - } catch (cause: unknown) { - // Fallback to the already-sanitized href when the candidate is invalid. - void cause; - return credentialFreeHref(parsed); - } + const { credentials } = parseRecordedTransportUrl(value); + return credentials === null || credentials === "ambiguous" + ? credentialFreeHref(parsed) + : `${value.slice(0, credentials.start)}${value.slice(credentials.end)}`; }; const credentialFreeHref = (parsed: URL): string => { diff --git a/tests/boundary/browser/cdpBrowserProvider.targetInventory.test.ts b/tests/boundary/browser/cdpBrowserProvider.targetInventory.test.ts index 9b2053d391..9f3c3153ac 100644 --- a/tests/boundary/browser/cdpBrowserProvider.targetInventory.test.ts +++ b/tests/boundary/browser/cdpBrowserProvider.targetInventory.test.ts @@ -2,11 +2,12 @@ import { createServer } from "node:http"; import { describe, expect, it } from "vitest"; -import { CdpBrowserProvider } from "../../../src/browser/CdpBrowserProvider.js"; -import { listBrowserTargetsInputSchema } from "../../../src/domain/browserObservation.js"; +import { parseEvidence } from "../../../src/domain/evidence.js"; +import { browserTargetListSchema } from "../../../src/domain/browserObservationSchemas.js"; +import { runCliJson } from "../../fixtures/cliJsonProcess.js"; describe("CDP target inventory", () => { - it("returns every allowed target within the bounded discovery payload", async () => { + it("returns complete target inventory and original URL paths through the CLI", async () => { let port = 0; const targets = Array.from({ length: 1_101 }, (_, index) => { const id = `target-${String(index).padStart(4, "0")}`; @@ -14,7 +15,10 @@ describe("CDP target inventory", () => { id, type: "page", title: `Page ${String(index)}`, - url: `https://allowed.example/page/${String(index)}`, + url: + index === 0 + ? String.raw`https://user:pass@allowed.example\path@tail/../keep?x=1#frag` + : `https://allowed.example/page/${String(index)}`, ...(index === 0 ? {} : { attached: index === 1 ? false : true }), webSocketDebuggerUrl: `ws://127.0.0.1:${String(port)}/devtools/page/${id}`, }; @@ -52,24 +56,30 @@ describe("CDP target inventory", () => { port = address.port; const endpoint = `http://127.0.0.1:${String(port)}`; - const result = await new CdpBrowserProvider().listTargets( - listBrowserTargetsInputSchema.parse({ - cdp_endpoint: endpoint, - allowed_origins: ["https://allowed.example"], - }), + const evidence = parseEvidence( + await runCliJson([ + "list-browser-targets", + endpoint, + "--allowed-origins", + "https://allowed.example", + "--json", + ]), ); + const result = browserTargetListSchema.parse(evidence.normalized_result); - if (!result.ok) throw result.error; - expect(result.value.targets).toHaveLength(targets.length); - expect(result.value.targets[0]?.target_id).toBe("target-0000"); - expect(result.value.targets.at(-1)?.target_id).toBe("target-1100"); + expect(result.targets).toHaveLength(targets.length); + expect(result.targets[0]?.target_id).toBe("target-0000"); + expect(result.targets[0]?.url).toBe( + String.raw`https://allowed.example\path@tail/../keep?x=1#frag`, + ); + expect(result.targets[0]?.origin).toBe("https://allowed.example"); + expect(JSON.stringify(evidence)).not.toContain("user:pass"); + expect(result.targets.at(-1)?.target_id).toBe("target-1100"); expect( - result.value.targets.slice(0, 3).map(({ attached }) => attached), + result.targets.slice(0, 3).map(({ attached }) => attached), ).toEqual([null, false, true]); expect( - result.value.limitations.some((item) => - item.includes("attachment state"), - ), + result.limitations.some((item) => item.includes("attachment state")), ).toBe(true); } finally { if (server.listening)