Skip to content

Commit 0e26cb0

Browse files
MarvaeMarvae
committed
fix(browser): encapsulate explicit-port detection in parseBrowserHttpUrl
The previous implementation checked parsed.parsed.port in resolveProfile, which missed WHATWG-normalized default ports (:80/:443) and was vulnerable to false positives on IPv6 hosts and userinfo colons. Move explicit-port detection into parseBrowserHttpUrl as hasExplicitPort using a robust raw-string parser that handles: - IPv6 bracket notation ([::1] vs [::1]:9222) - userinfo (user:pass@host — colon is not a port) - WHATWG default-port normalization (:80/:443 → empty .port) Return normalizedWithPort alongside normalized so callers never need raw-string URL hacks. resolveProfile now reads: explicit URL port → use it no URL port + cdpPort → inject cdpPort no URL port + no cdpPort → protocol default Consolidate scattered port-precedence tests into a focused describe block covering all 9 cases including IPv6, default ports, stale WS, and error.
1 parent 9c22f25 commit 0e26cb0

3 files changed

Lines changed: 275 additions & 37 deletions

File tree

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

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,38 @@ import { withAllowedHostname } from "./ssrf-policy-helpers.js";
1515

1616
export { isLoopbackHost };
1717

18+
/**
19+
* Detects whether a raw URL string contains an explicitly written port.
20+
*
21+
* WHATWG `URL` normalizes default ports (e.g. `:80` for http, `:443` for
22+
* https) to an empty `.port` string, making it impossible to distinguish
23+
* "user wrote :80" from "user omitted the port". This helper inspects the
24+
* raw string to preserve that intent.
25+
*
26+
* Handles IPv6 bracket notation and userinfo (user:pass@host) correctly.
27+
*/
28+
function hasRawExplicitPort(raw: string): boolean {
29+
// Strip scheme (e.g. "http://") and take only the authority portion
30+
// (everything before the first /, ?, or #).
31+
const authority =
32+
raw
33+
.replace(/^[a-z][a-z0-9+.-]*:\/\//i, "")
34+
.split(/[/?#]/, 1)[0] ?? "";
35+
36+
// Strip userinfo (user:pass@) — the colon there is NOT a port separator.
37+
const hostPort = authority.includes("@")
38+
? authority.slice(authority.lastIndexOf("@") + 1)
39+
: authority;
40+
41+
// IPv6: [::1]:9222 → port after closing bracket
42+
if (hostPort.startsWith("[")) {
43+
return /^\[[^\]]+\]:\d+$/.test(hostPort);
44+
}
45+
46+
// IPv4 / hostname: host:port
47+
return /:\d+$/.test(hostPort);
48+
}
49+
1850
export function parseBrowserHttpUrl(raw: string, label: string) {
1951
const trimmed = raw.trim();
2052
const parsed = new URL(trimmed);
@@ -40,10 +72,42 @@ export function parseBrowserHttpUrl(raw: string, label: string) {
4072
throw new Error(`${label} has invalid port: ${parsed.port}`);
4173
}
4274

75+
const normalized = parsed.toString().replace(/\/$/, "");
76+
const hasExplicitPort = hasRawExplicitPort(trimmed);
77+
78+
// When the user explicitly wrote a default port (e.g. :80 for http),
79+
// WHATWG normalization drops it from the URL string. Rebuild a
80+
// port-preserving normalized form so callers don't need raw-string hacks.
81+
// Note: the URL .port setter silently discards protocol-default ports,
82+
// so we must inject the port via string surgery on the normalized form.
83+
let normalizedWithPort: string;
84+
if (hasExplicitPort && !parsed.port) {
85+
const proto = parsed.protocol + "//";
86+
const rest = normalized.slice(proto.length);
87+
// Skip userinfo (user:pass@) if present
88+
const atIdx = rest.indexOf("@");
89+
const hostStart = atIdx >= 0 ? atIdx + 1 : 0;
90+
const hostPart = rest.slice(hostStart);
91+
// Find end of host: IPv6 brackets or first : / /
92+
const hostLen = hostPart.startsWith("[")
93+
? hostPart.indexOf("]") + 1
94+
: (() => {
95+
const idx = hostPart.search(/[:/]/);
96+
return idx < 0 ? hostPart.length : idx;
97+
})();
98+
const insertAt = hostStart + hostLen;
99+
normalizedWithPort =
100+
proto + rest.slice(0, insertAt) + ":" + port + rest.slice(insertAt);
101+
} else {
102+
normalizedWithPort = normalized;
103+
}
104+
43105
return {
44106
parsed,
45107
port,
46-
normalized: parsed.toString().replace(/\/$/, ""),
108+
hasExplicitPort,
109+
normalized,
110+
normalizedWithPort,
47111
};
48112
}
49113

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

Lines changed: 207 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -556,41 +556,218 @@ describe("browser config", () => {
556556
expect(profile?.cdpIsLoopback).toBe(true);
557557
});
558558

559-
it("prefers explicit cdpPort when cdpUrl has no port", () => {
560-
// cdpUrl "http://127.0.0.1" (no port) should not override
561-
// the explicit cdpPort with default port 80.
562-
const resolved = resolveBrowserConfig({
563-
profiles: {
564-
openclaw: {
565-
cdpPort: 18800,
566-
cdpUrl: "http://127.0.0.1",
567-
color: "#FF4500",
568-
driver: "openclaw",
559+
describe("cdpPort vs cdpUrl port precedence", () => {
560+
it("URL with non-default port wins over cdpPort", () => {
561+
const resolved = resolveBrowserConfig({
562+
profiles: {
563+
openclaw: {
564+
cdpPort: 18800,
565+
cdpUrl: "http://127.0.0.1:9222",
566+
color: "#FF4500",
567+
driver: "openclaw",
568+
},
569569
},
570-
},
570+
});
571+
const profile = resolveProfile(resolved, "openclaw");
572+
expect(profile?.cdpPort).toBe(9222);
573+
expect(profile?.cdpUrl).toBe("http://127.0.0.1:9222");
571574
});
572-
const profile = resolveProfile(resolved, "openclaw");
573-
expect(profile?.cdpPort).toBe(18800);
574-
expect(profile?.cdpUrl).toBe("http://127.0.0.1:18800");
575-
});
576575

577-
it("prefers cdpPort over stale WebSocket devtools cdpUrl when both are set", () => {
578-
const resolved = resolveBrowserConfig({
579-
profiles: {
580-
"chrome-cdp": {
581-
cdpPort: 9222,
582-
cdpUrl: "ws://127.0.0.1:9222/devtools/browser/old-stale-id",
583-
attachOnly: true,
584-
color: "#F59E0B",
576+
it("URL with explicit default port :80 wins over cdpPort", () => {
577+
const resolved = resolveBrowserConfig({
578+
profiles: {
579+
openclaw: {
580+
cdpPort: 18800,
581+
cdpUrl: "http://127.0.0.1:80",
582+
color: "#FF4500",
583+
driver: "openclaw",
584+
},
585585
},
586-
},
586+
});
587+
const profile = resolveProfile(resolved, "openclaw");
588+
expect(profile?.cdpPort).toBe(80);
589+
expect(profile?.cdpUrl).toBe("http://127.0.0.1:80");
590+
});
591+
592+
it("URL with explicit default port preserves normalized URL details", () => {
593+
const resolved = resolveBrowserConfig({
594+
profiles: {
595+
secure: {
596+
cdpPort: 18800,
597+
cdpUrl: "https://user:[email protected]:443/json/version?token=abc#frag",
598+
color: "#0066CC",
599+
driver: "openclaw",
600+
},
601+
websocket: {
602+
cdpPort: 18800,
603+
cdpUrl: "wss://remote-browser.example.com:443/json/version?token=abc",
604+
color: "#0066CC",
605+
driver: "openclaw",
606+
},
607+
ipv6: {
608+
cdpPort: 18800,
609+
cdpUrl: "http://[::1]:80/json/version?token=abc",
610+
color: "#0066CC",
611+
driver: "openclaw",
612+
},
613+
},
614+
});
615+
616+
const secure = resolveProfile(resolved, "secure");
617+
expect(secure?.cdpPort).toBe(443);
618+
expect(secure?.cdpUrl).toBe(
619+
"https://user:[email protected]:443/json/version?token=abc#frag",
620+
);
621+
622+
const websocket = resolveProfile(resolved, "websocket");
623+
expect(websocket?.cdpPort).toBe(443);
624+
expect(websocket?.cdpUrl).toBe(
625+
"wss://remote-browser.example.com:443/json/version?token=abc",
626+
);
627+
628+
const ipv6 = resolveProfile(resolved, "ipv6");
629+
expect(ipv6?.cdpPort).toBe(80);
630+
expect(ipv6?.cdpUrl).toBe("http://[::1]:80/json/version?token=abc");
631+
});
632+
633+
it("userinfo colons without a URL port defer to cdpPort", () => {
634+
const resolved = resolveBrowserConfig({
635+
profiles: {
636+
openclaw: {
637+
cdpPort: 18800,
638+
cdpUrl: "http://user:[email protected]/json/version",
639+
color: "#FF4500",
640+
driver: "openclaw",
641+
},
642+
},
643+
});
644+
const profile = resolveProfile(resolved, "openclaw");
645+
expect(profile?.cdpPort).toBe(18800);
646+
expect(profile?.cdpUrl).toBe("http://user:[email protected]:18800/json/version");
647+
});
648+
649+
it("URL without port defers to cdpPort", () => {
650+
const resolved = resolveBrowserConfig({
651+
profiles: {
652+
openclaw: {
653+
cdpPort: 18800,
654+
cdpUrl: "http://127.0.0.1",
655+
color: "#FF4500",
656+
driver: "openclaw",
657+
},
658+
},
659+
});
660+
const profile = resolveProfile(resolved, "openclaw");
661+
expect(profile?.cdpPort).toBe(18800);
662+
expect(profile?.cdpUrl).toBe("http://127.0.0.1:18800");
663+
});
664+
665+
it("URL with non-default port, no cdpPort configured", () => {
666+
const resolved = resolveBrowserConfig({
667+
profiles: {
668+
openclaw: {
669+
cdpUrl: "http://127.0.0.1:9222",
670+
color: "#FF4500",
671+
driver: "openclaw",
672+
},
673+
},
674+
});
675+
const profile = resolveProfile(resolved, "openclaw");
676+
expect(profile?.cdpPort).toBe(9222);
677+
expect(profile?.cdpUrl).toBe("http://127.0.0.1:9222");
678+
});
679+
680+
it("URL without port and no cdpPort falls back to protocol default", () => {
681+
const resolved = resolveBrowserConfig({
682+
profiles: {
683+
openclaw: {
684+
cdpUrl: "https://remote-browser.example.com",
685+
color: "#FF4500",
686+
driver: "openclaw",
687+
},
688+
},
689+
});
690+
const profile = resolveProfile(resolved, "openclaw");
691+
expect(profile?.cdpPort).toBe(443);
692+
expect(profile?.cdpUrl).toBe("https://remote-browser.example.com");
693+
});
694+
695+
it("no URL + cdpPort constructs URL from defaults", () => {
696+
const resolved = resolveBrowserConfig({
697+
profiles: {
698+
openclaw: {
699+
cdpPort: 9222,
700+
color: "#FF4500",
701+
driver: "openclaw",
702+
},
703+
},
704+
});
705+
const profile = resolveProfile(resolved, "openclaw");
706+
expect(profile?.cdpPort).toBe(9222);
707+
expect(profile?.cdpUrl).toContain(":9222");
708+
});
709+
710+
it("no URL + no cdpPort throws", () => {
711+
const resolved = resolveBrowserConfig({
712+
profiles: {
713+
bad: {
714+
color: "#FF4500",
715+
driver: "openclaw",
716+
},
717+
},
718+
});
719+
expect(() => resolveProfile(resolved, "bad")).toThrow(
720+
'must define cdpPort or cdpUrl',
721+
);
722+
});
723+
724+
it("stale WS devtools URL + cdpPort drops path and uses cdpPort", () => {
725+
const resolved = resolveBrowserConfig({
726+
profiles: {
727+
"chrome-cdp": {
728+
cdpPort: 9222,
729+
cdpUrl: "ws://127.0.0.1:12345/devtools/browser/old-stale-id",
730+
attachOnly: true,
731+
color: "#F59E0B",
732+
},
733+
},
734+
});
735+
const profile = resolveProfile(resolved, "chrome-cdp");
736+
expect(profile?.cdpUrl).toBe("http://127.0.0.1:9222");
737+
expect(profile?.cdpPort).toBe(9222);
738+
});
739+
740+
it("IPv6 URL without port defers to cdpPort", () => {
741+
const resolved = resolveBrowserConfig({
742+
profiles: {
743+
openclaw: {
744+
cdpPort: 18800,
745+
cdpUrl: "http://[::1]",
746+
color: "#FF4500",
747+
driver: "openclaw",
748+
},
749+
},
750+
});
751+
const profile = resolveProfile(resolved, "openclaw");
752+
expect(profile?.cdpPort).toBe(18800);
753+
expect(profile?.cdpUrl).toBe("http://[::1]:18800");
754+
});
755+
756+
it("IPv6 URL with explicit port wins over cdpPort", () => {
757+
const resolved = resolveBrowserConfig({
758+
profiles: {
759+
openclaw: {
760+
cdpPort: 18800,
761+
cdpUrl: "http://[::1]:9222",
762+
color: "#FF4500",
763+
driver: "openclaw",
764+
},
765+
},
766+
});
767+
const profile = resolveProfile(resolved, "openclaw");
768+
expect(profile?.cdpPort).toBe(9222);
769+
expect(profile?.cdpUrl).toBe("http://[::1]:9222");
587770
});
588-
const profile = resolveProfile(resolved, "chrome-cdp");
589-
// cdpPort produces a stable HTTP endpoint; the stale WS session ID is dropped.
590-
expect(profile?.cdpUrl).toBe("http://127.0.0.1:9222");
591-
expect(profile?.cdpPort).toBe(9222);
592-
expect(profile?.cdpIsLoopback).toBe(true);
593-
expect(profile?.attachOnly).toBe(true);
594771
});
595772

596773
it("preserves profile host when dropping stale devtools WS path", () => {

‎extensions/browser/src/browser/config.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -502,13 +502,10 @@ export function resolveProfile(
502502
} else if (rawProfileUrl) {
503503
const parsed = parseBrowserHttpUrl(rawProfileUrl, `browser.profiles.${profileName}.cdpUrl`);
504504
cdpHost = parsed.parsed.hostname;
505-
// When the URL has an explicit port, always use it. When the URL omits the
506-
// port (e.g. "http://127.0.0.1"), prefer an already-configured cdpPort over
507-
// the protocol default (80/443) to avoid binding Chrome to a privileged
508-
// port.
509-
if (parsed.parsed.port) {
505+
// Port precedence: explicit URL port > configured cdpPort > protocol default.
506+
if (parsed.hasExplicitPort) {
510507
cdpPort = parsed.port;
511-
cdpUrl = parsed.normalized;
508+
cdpUrl = parsed.normalizedWithPort;
512509
} else if (cdpPort) {
513510
// URL omitted the port but we have an explicit cdpPort — inject it while
514511
// preserving the rest of the URL (path, query, credentials, etc.).

0 commit comments

Comments
 (0)