Skip to content

Commit 0e1f69c

Browse files
lidge-junlidge-jun
committed
fix(browser): reject credentialed navigation urls
1 parent a99cbf2 commit 0e1f69c

5 files changed

Lines changed: 110 additions & 4 deletions

File tree

‎extensions/browser/src/browser-tool.test.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1298,6 +1298,19 @@ describe("browser tool url alias support", () => {
12981298
expect(opts.profile).toBeUndefined();
12991299
});
13001300

1301+
it("rejects embedded credentials before opening a tab", async () => {
1302+
const tool = createBrowserTool();
1303+
await expect(
1304+
tool.execute?.("call-1", {
1305+
action: "open",
1306+
url: "https://user:[email protected]/path",
1307+
}),
1308+
).rejects.toThrow("embedded credentials");
1309+
1310+
expect(browserClientMocks.browserOpenTab).not.toHaveBeenCalled();
1311+
expect(gatewayMocks.callGatewayTool).not.toHaveBeenCalled();
1312+
});
1313+
13011314
it("tracks opened tabs when session context is available", async () => {
13021315
browserClientMocks.browserOpenTab.mockResolvedValueOnce({
13031316
targetId: "tab-123",
@@ -1354,6 +1367,20 @@ describe("browser tool url alias support", () => {
13541367
expect(request.profile).toBeUndefined();
13551368
});
13561369

1370+
it("rejects embedded credentials before navigating a tab", async () => {
1371+
const tool = createBrowserTool();
1372+
await expect(
1373+
tool.execute?.("call-1", {
1374+
action: "navigate",
1375+
url: "https://user:[email protected]/path",
1376+
targetId: "tab-1",
1377+
}),
1378+
).rejects.toThrow("embedded credentials");
1379+
1380+
expect(browserActionsMocks.browserNavigate).not.toHaveBeenCalled();
1381+
expect(gatewayMocks.callGatewayTool).not.toHaveBeenCalled();
1382+
});
1383+
13571384
it("keeps targetUrl required error label when both params are missing", async () => {
13581385
const tool = createBrowserTool();
13591386

‎extensions/browser/src/browser-tool.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import { DEFAULT_BROWSER_SCREENSHOT_TIMEOUT_MS } from "./browser/constants.js";
5151
import { normalizeBrowserScreenshot } from "./browser/screenshot.js";
5252
import { describeBrowserScreenshot, neutralizeMediaDirectives } from "./browser/vision.js";
5353
import { wrapExternalContent } from "./sdk-security-runtime.js";
54+
import { assertNoBrowserNavigationUrlUserInfo } from "./browser/navigation-url-userinfo.js";
5455

5556
const browserToolDeps = {
5657
browserAct,
@@ -150,10 +151,11 @@ function readOptionalTargetAndTimeout(params: Record<string, unknown>) {
150151
}
151152

152153
function readTargetUrlParam(params: Record<string, unknown>) {
153-
return (
154+
const targetUrl =
154155
readStringParam(params, "targetUrl") ??
155-
readStringParam(params, "url", { required: true, label: "targetUrl" })
156-
);
156+
readStringParam(params, "url", { required: true, label: "targetUrl" });
157+
assertNoBrowserNavigationUrlUserInfo(targetUrl);
158+
return targetUrl;
157159
}
158160

159161
const LEGACY_BROWSER_ACT_REQUEST_KEYS = [

‎extensions/browser/src/browser/navigation-guard.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,36 @@ describe("browser navigation guard", () => {
251251
).rejects.toBeInstanceOf(InvalidBrowserNavigationUrlError);
252252
});
253253

254+
it("blocks network URLs with embedded credentials before lookup", async () => {
255+
const lookupFn = createLookupFn("93.184.216.34");
256+
await expect(
257+
assertBrowserNavigationAllowed({
258+
url: "https://user:[email protected]/private",
259+
lookupFn,
260+
}),
261+
).rejects.toThrow("embedded credentials");
262+
await expect(
263+
assertBrowserNavigationAllowed({
264+
url: "https://user:[email protected]/private",
265+
lookupFn,
266+
}),
267+
).rejects.not.toThrow("secret");
268+
expect(lookupFn).not.toHaveBeenCalled();
269+
});
270+
271+
it("redacts URL credentials from invalid URL diagnostics", async () => {
272+
await expect(
273+
assertBrowserNavigationAllowed({
274+
url: "https://user:secret@",
275+
}),
276+
).rejects.toThrow("https://[redacted]@");
277+
await expect(
278+
assertBrowserNavigationAllowed({
279+
url: "https://user:secret@",
280+
}),
281+
).rejects.not.toThrow("secret");
282+
});
283+
254284
it("validates final network URLs after navigation", async () => {
255285
const lookupFn = createLookupFn("127.0.0.1");
256286
await expect(

‎extensions/browser/src/browser/navigation-guard.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ import {
66
type SsrFPolicy,
77
} from "../infra/net/ssrf.js";
88
import { matchesHostnameAllowlist, normalizeHostname } from "../sdk-security-runtime.js";
9+
import {
10+
BROWSER_NAVIGATION_USERINFO_BLOCKED_MESSAGE,
11+
hasBrowserNavigationUrlUserInfo,
12+
redactBrowserNavigationUrlForDiagnostics,
13+
} from "./navigation-url-userinfo.js";
914

1015
const NETWORK_NAVIGATION_PROTOCOLS = new Set(["http:", "https:"]);
1116
const SAFE_NON_NETWORK_URLS = new Set(["about:blank"]);
@@ -102,7 +107,9 @@ export async function assertBrowserNavigationAllowed(
102107
try {
103108
parsed = new URL(rawUrl);
104109
} catch {
105-
throw new InvalidBrowserNavigationUrlError(`Invalid URL: ${rawUrl}`);
110+
throw new InvalidBrowserNavigationUrlError(
111+
`Invalid URL: ${redactBrowserNavigationUrlForDiagnostics(rawUrl)}`,
112+
);
106113
}
107114

108115
if (!NETWORK_NAVIGATION_PROTOCOLS.has(parsed.protocol)) {
@@ -114,6 +121,10 @@ export async function assertBrowserNavigationAllowed(
114121
);
115122
}
116123

124+
if (hasBrowserNavigationUrlUserInfo(parsed)) {
125+
throw new InvalidBrowserNavigationUrlError(BROWSER_NAVIGATION_USERINFO_BLOCKED_MESSAGE);
126+
}
127+
117128
// Browser proxy routing hides the final connect target from this process.
118129
// Only block when the browser profile is known to be proxy-routed; Gateway
119130
// provider proxy env alone is not proof of browser page proxy behavior.

‎extensions/browser/src/browser/navigation-url-userinfo.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
const NAVIGATION_USERINFO_PROTOCOLS = new Set(["http:", "https:"]);
2+
3+
export const BROWSER_NAVIGATION_USERINFO_BLOCKED_MESSAGE =
4+
"Navigation blocked: URLs with embedded credentials are not supported";
5+
6+
export function hasBrowserNavigationUrlUserInfo(parsed: URL): boolean {
7+
return (
8+
NAVIGATION_USERINFO_PROTOCOLS.has(parsed.protocol) &&
9+
(parsed.username.length > 0 || parsed.password.length > 0)
10+
);
11+
}
12+
13+
export function assertNoBrowserNavigationUrlUserInfo(url: string): void {
14+
try {
15+
if (hasBrowserNavigationUrlUserInfo(new URL(url.trim()))) {
16+
throw new Error(BROWSER_NAVIGATION_USERINFO_BLOCKED_MESSAGE);
17+
}
18+
} catch (err) {
19+
if (err instanceof Error && err.message === BROWSER_NAVIGATION_USERINFO_BLOCKED_MESSAGE) {
20+
throw err;
21+
}
22+
}
23+
}
24+
25+
export function redactBrowserNavigationUrlForDiagnostics(url: string): string {
26+
try {
27+
const parsed = new URL(url);
28+
if (hasBrowserNavigationUrlUserInfo(parsed)) {
29+
parsed.username = "";
30+
parsed.password = "";
31+
}
32+
return parsed.toString();
33+
} catch {
34+
return url.replace(/([a-z][a-z0-9+.-]*:\/\/)([^/?#\s@]+)@/gi, "$1[redacted]@");
35+
}
36+
}

0 commit comments

Comments
 (0)