mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-09 15:59:30 +00:00
fix(browser): reject credentialed page URLs safely (#102952)
Co-authored-by: bitkyc08 <bitkyc08@gmail.com>
This commit is contained in:
parent
436ed5f308
commit
d5fb4903f1
4 changed files with 88 additions and 19 deletions
|
|
@ -1519,6 +1519,24 @@ describe("browser tool url alias support", () => {
|
|||
expect(opts.profile).toBeUndefined();
|
||||
});
|
||||
|
||||
it("rejects credentialed open URLs before host or node dispatch", async () => {
|
||||
mockSingleBrowserProxyNode();
|
||||
const tool = createBrowserTool();
|
||||
for (const target of ["host", "node"] as const) {
|
||||
for (const url of ["https://user:secret@example.com/path", "https://user:secret@"]) {
|
||||
const error = await tool.execute?.("call-1", { action: "open", target, url }).then(
|
||||
() => new Error("credentialed URL was accepted"),
|
||||
(cause: unknown) => cause,
|
||||
);
|
||||
expect(error).toBeInstanceOf(Error);
|
||||
expect(String(error)).not.toContain("secret");
|
||||
}
|
||||
}
|
||||
|
||||
expect(browserClientMocks.browserOpenTab).not.toHaveBeenCalled();
|
||||
expect(gatewayMocks.callGatewayTool).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("tracks opened tabs when session context is available", async () => {
|
||||
browserClientMocks.browserOpenTab.mockResolvedValueOnce({
|
||||
targetId: "tab-123",
|
||||
|
|
@ -1575,6 +1593,26 @@ describe("browser tool url alias support", () => {
|
|||
expect(request.profile).toBeUndefined();
|
||||
});
|
||||
|
||||
it("rejects credentialed navigate URLs before host or node dispatch", async () => {
|
||||
mockSingleBrowserProxyNode();
|
||||
const tool = createBrowserTool();
|
||||
for (const target of ["host", "node"] as const) {
|
||||
for (const url of ["https://user:secret@example.com/path", "https://user:secret@"]) {
|
||||
const error = await tool
|
||||
.execute?.("call-1", { action: "navigate", target, url, targetId: "tab-1" })
|
||||
.then(
|
||||
() => new Error("credentialed URL was accepted"),
|
||||
(cause: unknown) => cause,
|
||||
);
|
||||
expect(error).toBeInstanceOf(Error);
|
||||
expect(String(error)).not.toContain("secret");
|
||||
}
|
||||
}
|
||||
|
||||
expect(browserActionsMocks.browserNavigate).not.toHaveBeenCalled();
|
||||
expect(gatewayMocks.callGatewayTool).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("keeps targetUrl required error label when both params are missing", async () => {
|
||||
const tool = createBrowserTool();
|
||||
|
||||
|
|
|
|||
|
|
@ -57,6 +57,7 @@ import {
|
|||
untrackSessionBrowserTab,
|
||||
} from "./browser-tool.runtime.js";
|
||||
import { DEFAULT_BROWSER_SCREENSHOT_TIMEOUT_MS } from "./browser/constants.js";
|
||||
import { parseBrowserNavigationUrl } from "./browser/navigation-guard.js";
|
||||
import { normalizeBrowserScreenshot } from "./browser/screenshot.js";
|
||||
import { describeBrowserScreenshot, neutralizeMediaDirectives } from "./browser/vision.js";
|
||||
import { wrapExternalContent } from "./sdk-security-runtime.js";
|
||||
|
|
@ -163,10 +164,11 @@ function readOptionalTargetAndTimeout(params: Record<string, unknown>) {
|
|||
}
|
||||
|
||||
function readTargetUrlParam(params: Record<string, unknown>) {
|
||||
return (
|
||||
const targetUrl =
|
||||
readStringParam(params, "targetUrl") ??
|
||||
readStringParam(params, "url", { required: true, label: "targetUrl" })
|
||||
);
|
||||
readStringParam(params, "url", { required: true, label: "targetUrl" });
|
||||
parseBrowserNavigationUrl(targetUrl);
|
||||
return targetUrl;
|
||||
}
|
||||
|
||||
function formatScreenshotShareHint(filePath: string): string {
|
||||
|
|
|
|||
|
|
@ -252,6 +252,26 @@ describe("browser navigation guard", () => {
|
|||
).rejects.toBeInstanceOf(InvalidBrowserNavigationUrlError);
|
||||
});
|
||||
|
||||
it("blocks network URLs with embedded credentials before lookup", async () => {
|
||||
const lookupFn = createLookupFn("93.184.216.34");
|
||||
const result = assertBrowserNavigationAllowed({
|
||||
url: "https://user:secret@example.com/private",
|
||||
lookupFn,
|
||||
});
|
||||
await expect(result).rejects.toThrow("URL-embedded credentials are not supported");
|
||||
await expect(result).rejects.toThrow("openclaw browser set credentials");
|
||||
await expect(result).rejects.not.toThrow("secret");
|
||||
expect(lookupFn).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("redacts malformed credential-bearing URLs from diagnostics", async () => {
|
||||
const result = assertBrowserNavigationAllowed({
|
||||
url: "https://user:secret@",
|
||||
});
|
||||
await expect(result).rejects.toThrow("Invalid URL: [redacted credential-bearing URL]");
|
||||
await expect(result).rejects.not.toThrow("secret");
|
||||
});
|
||||
|
||||
it("validates final network URLs after navigation", async () => {
|
||||
const lookupFn = createLookupFn("127.0.0.1");
|
||||
await expect(
|
||||
|
|
|
|||
|
|
@ -15,16 +15,14 @@ import { matchesHostnameAllowlist, normalizeHostname } from "../sdk-security-run
|
|||
|
||||
const NETWORK_NAVIGATION_PROTOCOLS = new Set(["http:", "https:"]);
|
||||
const SAFE_NON_NETWORK_URLS = new Set(["about:blank"]);
|
||||
const BROWSER_NAVIGATION_CREDENTIALS_BLOCKED_MESSAGE =
|
||||
"Navigation blocked: URL-embedded credentials are not supported for page navigation. Set HTTP Basic auth with `openclaw browser set credentials <username> <password>` or use an authenticated browser profile.";
|
||||
|
||||
function isAllowedNonNetworkNavigationUrl(parsed: URL): boolean {
|
||||
// Keep non-network navigation explicit; about:blank is the only allowed bootstrap URL.
|
||||
return SAFE_NON_NETWORK_URLS.has(parsed.href);
|
||||
}
|
||||
|
||||
function normalizeNavigationUrl(url: string): string {
|
||||
return url.trim();
|
||||
}
|
||||
|
||||
/** Raised when a browser navigation URL fails syntax or policy validation. */
|
||||
export class InvalidBrowserNavigationUrlError extends Error {
|
||||
constructor(message: string) {
|
||||
|
|
@ -33,6 +31,27 @@ export class InvalidBrowserNavigationUrlError extends Error {
|
|||
}
|
||||
}
|
||||
|
||||
/** Parse a page-navigation URL and reject credentials before any transport dispatch. */
|
||||
export function parseBrowserNavigationUrl(url: string): URL {
|
||||
const rawUrl = url.trim();
|
||||
if (!rawUrl) {
|
||||
throw new InvalidBrowserNavigationUrlError("url is required");
|
||||
}
|
||||
|
||||
let parsed: URL;
|
||||
try {
|
||||
parsed = new URL(rawUrl);
|
||||
} catch {
|
||||
const diagnostic = rawUrl.includes("@") ? "[redacted credential-bearing URL]" : rawUrl;
|
||||
throw new InvalidBrowserNavigationUrlError(`Invalid URL: ${diagnostic}`);
|
||||
}
|
||||
|
||||
if (parsed.username || parsed.password) {
|
||||
throw new InvalidBrowserNavigationUrlError(BROWSER_NAVIGATION_CREDENTIALS_BLOCKED_MESSAGE);
|
||||
}
|
||||
return parsed;
|
||||
}
|
||||
|
||||
/** Policy inputs applied to browser page navigation checks. */
|
||||
export type BrowserNavigationPolicyOptions = {
|
||||
ssrfPolicy?: SsrFPolicy;
|
||||
|
|
@ -107,17 +126,7 @@ export async function assertBrowserNavigationAllowed(
|
|||
lookupFn?: LookupFn;
|
||||
} & BrowserNavigationPolicyOptions,
|
||||
): Promise<void> {
|
||||
const rawUrl = normalizeNavigationUrl(opts.url);
|
||||
if (!rawUrl) {
|
||||
throw new InvalidBrowserNavigationUrlError("url is required");
|
||||
}
|
||||
|
||||
let parsed: URL;
|
||||
try {
|
||||
parsed = new URL(rawUrl);
|
||||
} catch {
|
||||
throw new InvalidBrowserNavigationUrlError(`Invalid URL: ${rawUrl}`);
|
||||
}
|
||||
const parsed = parseBrowserNavigationUrl(opts.url);
|
||||
|
||||
if (!NETWORK_NAVIGATION_PROTOCOLS.has(parsed.protocol)) {
|
||||
if (isAllowedNonNetworkNavigationUrl(parsed)) {
|
||||
|
|
@ -174,7 +183,7 @@ export async function assertBrowserNavigationResultAllowed(
|
|||
lookupFn?: LookupFn;
|
||||
} & BrowserNavigationPolicyOptions,
|
||||
): Promise<void> {
|
||||
const rawUrl = normalizeNavigationUrl(opts.url);
|
||||
const rawUrl = opts.url.trim();
|
||||
if (!rawUrl) {
|
||||
return;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue