Skip to content

Commit 621db8f

Browse files
steipetesteipete
committed
fix(browser): reject explicit zero cdp ports
1 parent f5e1fe9 commit 621db8f

8 files changed

Lines changed: 83 additions & 135 deletions

File tree

‎extensions/browser/src/browser/cdp.helpers.fuzz.test.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,12 @@ describe("fuzz: parseBrowserHttpUrl", () => {
349349
expect(() => parseBrowserHttpUrl(url, "test")).toThrow(/must be http\(s\) or ws\(s\)/);
350350
}
351351
});
352+
353+
it("rejects explicitly configured port zero", () => {
354+
for (const scheme of ["http", "https", "ws", "wss"]) {
355+
expect(() => parseBrowserHttpUrl(`${scheme}://127.0.0.1:0`, "test")).toThrow(/invalid port/);
356+
}
357+
});
352358
});
353359

354360
describe("fuzz: redactCdpUrl", () => {

‎extensions/browser/src/browser/cdp.helpers.ts‎

Lines changed: 2 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { parseBrowserHttpUrl, redactCdpUrl } from "openclaw/plugin-sdk/browser-config";
12
import { fetchWithSsrFGuard } from "openclaw/plugin-sdk/ssrf-runtime";
23
import WebSocket from "ws";
34
import { isLoopbackHost } from "../gateway/net.js";
@@ -6,7 +7,6 @@ import {
67
type SsrFPolicy,
78
resolvePinnedHostnameWithPolicy,
89
} from "../infra/net/ssrf.js";
9-
import { redactSensitiveText } from "../logging/redact.js";
1010
import {
1111
getDirectAgentForCdp,
1212
withManagedProxyForCdpUrl,
@@ -18,98 +18,7 @@ import { resolveBrowserRateLimitMessage } from "./rate-limit-message.js";
1818
import { withAllowedHostname } from "./ssrf-policy-helpers.js";
1919

2020
export { isLoopbackHost };
21-
22-
/**
23-
* Detects whether a raw URL string contains an explicitly written port.
24-
*
25-
* WHATWG `URL` normalizes default ports (e.g. `:80` for http, `:443` for
26-
* https) to an empty `.port` string, making it impossible to distinguish
27-
* "user wrote :80" from "user omitted the port". This helper inspects the
28-
* raw string to preserve that intent.
29-
*
30-
* Handles IPv6 bracket notation and userinfo (user:pass@host) correctly.
31-
*/
32-
function hasRawExplicitPort(raw: string): boolean {
33-
// Strip scheme (e.g. "http://") and take only the authority portion
34-
// (everything before the first /, ?, or #).
35-
const authority = raw.replace(/^[a-z][a-z0-9+.-]*:\/\//i, "").split(/[/?#]/, 1)[0] ?? "";
36-
37-
// Strip userinfo (user:pass@); the colon there is not a port separator.
38-
const hostPort = authority.includes("@")
39-
? authority.slice(authority.lastIndexOf("@") + 1)
40-
: authority;
41-
42-
// IPv6: [::1]:9222 has a port after the closing bracket.
43-
if (hostPort.startsWith("[")) {
44-
return /^\[[^\]]+\]:\d+$/.test(hostPort);
45-
}
46-
47-
// IPv4 / hostname: host:port
48-
return /:\d+$/.test(hostPort);
49-
}
50-
51-
export function parseBrowserHttpUrl(raw: string, label: string) {
52-
const trimmed = raw.trim();
53-
const parsed = new URL(trimmed);
54-
const allowed = ["http:", "https:", "ws:", "wss:"];
55-
if (!allowed.includes(parsed.protocol)) {
56-
throw new Error(`${label} must be http(s) or ws(s), got: ${parsed.protocol.replace(":", "")}`);
57-
}
58-
59-
const isSecure = parsed.protocol === "https:" || parsed.protocol === "wss:";
60-
const port =
61-
parsed.port && Number.parseInt(parsed.port, 10) > 0
62-
? Number.parseInt(parsed.port, 10)
63-
: isSecure
64-
? 443
65-
: 80;
66-
67-
// WHATWG URL rejects invalid ports (non-numeric, negative, >65535), and
68-
// the ternary above falls back to 80/443 for empty or zero parsed.port,
69-
// so this defensive guard is unreachable at runtime. Kept as a
70-
// belt-and-braces check against parser drift.
71-
/* c8 ignore next 3 */
72-
if (Number.isNaN(port) || port <= 0 || port > 65535) {
73-
throw new Error(`${label} has invalid port: ${parsed.port}`);
74-
}
75-
76-
const normalized = parsed.toString().replace(/\/$/, "");
77-
const hasExplicitPort = hasRawExplicitPort(trimmed);
78-
79-
// When the user explicitly wrote a default port (e.g. :80 for http),
80-
// WHATWG normalization drops it from the URL string. Rebuild a
81-
// port-preserving normalized form so callers don't need raw-string hacks.
82-
// Note: the URL .port setter silently discards protocol-default ports,
83-
// so we must inject the port via string surgery on the normalized form.
84-
let normalizedWithPort: string;
85-
if (hasExplicitPort && !parsed.port) {
86-
const proto = parsed.protocol + "//";
87-
const rest = normalized.slice(proto.length);
88-
// Skip userinfo (user:pass@) if present
89-
const atIdx = rest.indexOf("@");
90-
const hostStart = atIdx >= 0 ? atIdx + 1 : 0;
91-
const hostPart = rest.slice(hostStart);
92-
// Find the end of the host: IPv6 brackets, a path slash, or a port colon.
93-
const hostLen = hostPart.startsWith("[")
94-
? hostPart.indexOf("]") + 1
95-
: (() => {
96-
const idx = hostPart.search(/[:/]/);
97-
return idx < 0 ? hostPart.length : idx;
98-
})();
99-
const insertAt = hostStart + hostLen;
100-
normalizedWithPort = proto + rest.slice(0, insertAt) + ":" + port + rest.slice(insertAt);
101-
} else {
102-
normalizedWithPort = normalized;
103-
}
104-
105-
return {
106-
parsed,
107-
port,
108-
hasExplicitPort,
109-
normalized,
110-
normalizedWithPort,
111-
};
112-
}
21+
export { parseBrowserHttpUrl, redactCdpUrl };
11322

11423
/**
11524
* Returns true when the URL uses a WebSocket protocol (ws: or wss:).
@@ -179,24 +88,6 @@ export async function assertCdpEndpointAllowed(
17988
}
18089
}
18190

182-
export function redactCdpUrl(cdpUrl: string | null | undefined): string | null | undefined {
183-
if (typeof cdpUrl !== "string") {
184-
return cdpUrl;
185-
}
186-
const trimmed = cdpUrl.trim();
187-
if (!trimmed) {
188-
return trimmed;
189-
}
190-
try {
191-
const parsed = new URL(trimmed);
192-
parsed.username = "";
193-
parsed.password = "";
194-
return redactSensitiveText(parsed.toString().replace(/\/$/, ""));
195-
} catch {
196-
return redactSensitiveText(trimmed);
197-
}
198-
}
199-
20091
type CdpResponse = {
20192
id: number;
20293
result?: unknown;

‎extensions/browser/src/browser/client-fetch.loopback-auth.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,22 @@ describe("fetchBrowserJson loopback auth", () => {
208208
expect(headers.get("authorization")).toBe("Bearer loopback-token");
209209
});
210210

211+
it("does not treat explicit port zero as the default loopback bridge port", async () => {
212+
mocks.resolveBrowserControlAuth.mockReturnValueOnce({
213+
token: undefined,
214+
password: undefined,
215+
});
216+
mocks.getBridgeAuthForPort.mockReturnValueOnce({ token: "bridge-token" });
217+
const fetchMock = stubJsonFetchOk();
218+
219+
await fetchBrowserJson<{ ok: boolean }>("http://127.0.0.1:0/");
220+
221+
const init = requireFetchInit(fetchMock);
222+
const headers = new Headers(init?.headers);
223+
expect(mocks.getBridgeAuthForPort).not.toHaveBeenCalled();
224+
expect(headers.get("authorization")).toBeNull();
225+
});
226+
211227
it("preserves dispatcher timeout context without no-retry hint", async () => {
212228
mocks.dispatch.mockRejectedValueOnce(new Error("Chrome CDP handshake timeout"));
213229

‎extensions/browser/src/browser/client-fetch.ts‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { parseBrowserHttpUrl } from "openclaw/plugin-sdk/browser-config";
12
import { parseFiniteNumber } from "openclaw/plugin-sdk/number-runtime";
23
import { fetchWithSsrFGuard } from "openclaw/plugin-sdk/ssrf-runtime";
34
import { normalizeOptionalString } from "openclaw/plugin-sdk/string-coerce-runtime";
@@ -68,13 +69,7 @@ function withLoopbackBrowserAuthImpl(
6869
// Sandbox bridge servers can run with per-process ephemeral auth on dynamic ports.
6970
// Fall back to the in-memory registry if config auth is not available.
7071
try {
71-
const parsed = new URL(url);
72-
const port =
73-
parsed.port && Number.parseInt(parsed.port, 10) > 0
74-
? Number.parseInt(parsed.port, 10)
75-
: parsed.protocol === "https:"
76-
? 443
77-
: 80;
72+
const { port } = parseBrowserHttpUrl(url, "browser control URL");
7873
const bridgeAuth = deps.getBridgeAuthForPort(port);
7974
if (bridgeAuth?.token) {
8075
headers.set("Authorization", `Bearer ${bridgeAuth.token}`);

‎extensions/browser/src/browser/profiles.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ describe("getUsedPorts", () => {
125125
it("ignores invalid cdpUrl values", () => {
126126
const profiles = {
127127
bad: { cdpUrl: "notaurl" },
128+
portZero: { cdpUrl: "http://127.0.0.1:0" },
128129
};
129130
const used = getUsedPorts(profiles);
130131
expect(used.size).toBe(0);

‎extensions/browser/src/browser/profiles.ts‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { parseBrowserHttpUrl } from "openclaw/plugin-sdk/browser-config";
2+
13
/**
24
* CDP port allocation for browser profiles.
35
*
@@ -66,16 +68,7 @@ export function getUsedPorts(
6668
continue;
6769
}
6870
try {
69-
const parsed = new URL(rawUrl);
70-
const port =
71-
parsed.port && Number.parseInt(parsed.port, 10) > 0
72-
? Number.parseInt(parsed.port, 10)
73-
: parsed.protocol === "https:"
74-
? 443
75-
: 80;
76-
if (isValidTcpPort(port)) {
77-
used.add(port);
78-
}
71+
used.add(parseBrowserHttpUrl(rawUrl, "browser.profiles.*.cdpUrl").port);
7972
} catch {
8073
// ignore invalid URLs
8174
}

‎src/plugin-sdk/browser-cdp.ts‎

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,18 @@
11
import { redactSensitiveText } from "../logging/redact.js";
22

3+
function hasRawExplicitPort(raw: string): boolean {
4+
const authority = raw.replace(/^[a-z][a-z0-9+.-]*:\/\//i, "").split(/[/?#]/, 1)[0] ?? "";
5+
const hostPort = authority.includes("@")
6+
? authority.slice(authority.lastIndexOf("@") + 1)
7+
: authority;
8+
9+
if (hostPort.startsWith("[")) {
10+
return /^\[[^\]]+\]:\d+$/.test(hostPort);
11+
}
12+
13+
return /:\d+$/.test(hostPort);
14+
}
15+
316
export function parseBrowserHttpUrl(raw: string, label: string) {
417
const trimmed = raw.trim();
518
const parsed = new URL(trimmed);
@@ -9,21 +22,45 @@ export function parseBrowserHttpUrl(raw: string, label: string) {
922
}
1023

1124
const isSecure = parsed.protocol === "https:" || parsed.protocol === "wss:";
12-
const port =
13-
parsed.port && Number.parseInt(parsed.port, 10) > 0
14-
? Number.parseInt(parsed.port, 10)
15-
: isSecure
16-
? 443
17-
: 80;
25+
const hasExplicitPort = hasRawExplicitPort(trimmed);
26+
const port = parsed.port ? Number.parseInt(parsed.port, 10) : isSecure ? 443 : 80;
1827

28+
if (hasExplicitPort && !parsed.port) {
29+
const defaultPort = isSecure ? 443 : 80;
30+
if (port !== defaultPort) {
31+
throw new Error(`${label} has invalid port: ${parsed.port}`);
32+
}
33+
}
1934
if (Number.isNaN(port) || port <= 0 || port > 65_535) {
2035
throw new Error(`${label} has invalid port: ${parsed.port}`);
2136
}
2237

38+
const normalized = parsed.toString().replace(/\/$/, "");
39+
let normalizedWithPort: string;
40+
if (hasExplicitPort && !parsed.port) {
41+
const proto = parsed.protocol + "//";
42+
const rest = normalized.slice(proto.length);
43+
const atIdx = rest.indexOf("@");
44+
const hostStart = atIdx >= 0 ? atIdx + 1 : 0;
45+
const hostPart = rest.slice(hostStart);
46+
const hostLen = hostPart.startsWith("[")
47+
? hostPart.indexOf("]") + 1
48+
: (() => {
49+
const idx = hostPart.search(/[:/]/);
50+
return idx < 0 ? hostPart.length : idx;
51+
})();
52+
const insertAt = hostStart + hostLen;
53+
normalizedWithPort = proto + rest.slice(0, insertAt) + ":" + port + rest.slice(insertAt);
54+
} else {
55+
normalizedWithPort = normalized;
56+
}
57+
2358
return {
2459
parsed,
2560
port,
26-
normalized: parsed.toString().replace(/\/$/, ""),
61+
hasExplicitPort,
62+
normalized,
63+
normalizedWithPort,
2764
};
2865
}
2966

‎src/plugin-sdk/browser-subpaths.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,15 @@ describe("plugin-sdk browser subpaths", () => {
2020
expect(redactCdpUrl(parsed.normalized)).toBe("http://127.0.0.1:9222");
2121
});
2222

23+
it("preserves explicit default ports and rejects explicit port zero", () => {
24+
const parsed = parseBrowserHttpUrl("http://127.0.0.1:80/json/version", "browser.cdpUrl");
25+
expect(parsed.hasExplicitPort).toBe(true);
26+
expect(parsed.normalizedWithPort).toBe("http://127.0.0.1:80/json/version");
27+
expect(() => parseBrowserHttpUrl("http://127.0.0.1:0", "browser.cdpUrl")).toThrow(
28+
/invalid port/,
29+
);
30+
});
31+
2332
it("resolves browser control auth on the dedicated auth subpath", () => {
2433
expect(resolveBrowserControlAuth).toBeTypeOf("function");
2534
});

0 commit comments

Comments
 (0)