diff --git a/CHANGELOG.md b/CHANGELOG.md index 939dacd6..e6e3c093 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### An IPv6 address in `AGENT_ENDPOINT_ALLOWED_HOSTS` now matches however it is written + +The endpoint check compares the list against the address as the URL parser spells it, compressed +and lower-case, while the list kept each IPv6 entry as the operator wrote it. `[0:0:0:0:0:0:0:1]:8443` +was therefore a line that silently never matched, the failure the list's other refusals exist to +prevent. Stripping the brackets on both sides also folded two different names into one, so naming +`[fd00::1:8443]`, an address, admitted `[fd00::1]:8443`, another address on a port, and the other way +round. Bracketed entries are now stored in the parser's spelling, with the port kept as written, and +compared with their brackets on; an entry the parser does not read as an address is refused at boot, +naming the entry, as a URL or a wildcard already was. Names and IPv4 entries are unaffected. ### A Bot's own decline is only recorded against a Bot the caller may reach A Bot reports that it declined a request through the person's session, and the audit row says diff --git a/server/src/agents/endpoint.ts b/server/src/agents/endpoint.ts index 39c979d4..e7978524 100644 --- a/server/src/agents/endpoint.ts +++ b/server/src/agents/endpoint.ts @@ -48,7 +48,10 @@ function namedAsAllowed( } catch { return false; } - const hostname = url.hostname.toLowerCase().replace(/^\[|\]$/g, ""); + // As the parser spells them, brackets included: `[::1]:8443` names an address and a port and + // `[::1:8443]` names an address, and with the brackets gone the two read as one. The list is + // stored in the same spelling (see `normalizeAllowedHost` in config.ts). + const hostname = url.hostname.toLowerCase(); const host = url.port ? `${hostname}:${url.port}` : hostname; return allowedHosts.has(host) || allowedHosts.has(hostname); } diff --git a/server/src/config.ts b/server/src/config.ts index 7c873985..915fb170 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -668,23 +668,43 @@ function agentEndpointAllowedHosts( `AGENT_ENDPOINT_ALLOWED_HOSTS entry "${entry}" must name one host. Patterns are not accepted: list each address instead.`, ); } - hosts.add(normalizeAllowedHost(host)); + hosts.add(normalizeAllowedHost(entry, host)); } return hosts; } -function normalizeAllowedHost(host: string): string { - // IPv6 is bracketed as [host] or [host]:port. Strip the brackets and keep the port. - if (host.startsWith("[")) { - const close = host.indexOf("]"); - if (close === -1) return host.replace(/^\[/, "").replace(/\]$/, ""); - const ipv6 = host.slice(1, close).toLowerCase(); - const rest = host.slice(close + 1); - if (!rest) return ipv6; - if (rest.startsWith(":")) return `${ipv6}${rest.toLowerCase()}`; - return `${ipv6}${rest.toLowerCase()}`; - } - return host; +/** + * An IPv6 entry, spelled the way the endpoint check will see it. + * + * `namedAsAllowed` compares against `URL.hostname`, which the parser canonicalises: compressed, + * lower-case, in brackets. An entry kept as written matched only when the operator happened to + * write it that way, so `[0:0:0:0:0:0:0:1]:8443` was a line that silently never matched, which is + * the failure the URL and wildcard refusals above exist to prevent. Stripping the brackets instead + * folded two different names into one: `[::1]:8443`, an address and a port, and `[::1:8443]`, an + * address, both became `::1:8443`, so naming either admitted the other. + * + * The address goes through the URL parser rather than a hand-written normaliser, so the spelling + * here is the parser's own and cannot drift from it. The port is kept as written, since the parser + * drops a scheme's default port and an operator who wrote `:80` meant that port. A bracketed entry + * the parser refuses is not an address, and is refused the way a URL is: at boot, naming the entry. + */ +function normalizeAllowedHost(entry: string, host: string): string { + if (!host.startsWith("[")) return host; + const close = host.indexOf("]"); + const address = close === -1 ? host : host.slice(0, close + 1); + const port = close === -1 ? "" : host.slice(close + 1); + const refusal = () => + new Error( + `AGENT_ENDPOINT_ALLOWED_HOSTS entry "${entry}" must be a host, optionally with a port, and not a URL.`, + ); + if (port && !/^:\d{1,5}$/.test(port)) throw refusal(); + let hostname: string; + try { + hostname = new URL(`http://${address}`).hostname; + } catch { + throw refusal(); + } + return `${hostname}${port}`; } function privateHostsAllowed(environment: Environment): boolean { diff --git a/server/tests/agent-endpoint.test.ts b/server/tests/agent-endpoint.test.ts index 92911df4..a2ec786d 100644 --- a/server/tests/agent-endpoint.test.ts +++ b/server/tests/agent-endpoint.test.ts @@ -634,6 +634,42 @@ describe("private addresses named one at a time", () => { ).toBeFalse(); }); + test("an IPv6 address matches however the endpoint spells it", () => { + // The list holds the parser's spelling (config.ts canonicalises it); the endpoint may arrive + // uncompressed or upper-case and the parser folds both to the same name. + for (const address of [ + "http://[0:0:0:0:0:0:0:1]:8443/ag-ui", + "http://[::1]:8443/ag-ui", + "http://[::1]:8443/ag-ui".toUpperCase().replace("HTTP", "http"), + ]) { + expect( + checkAgentEndpoint(address, { allowedHosts: named("[::1]:8443") }) + .allowed, + ).toBeTrue(); + } + }); + + test("an IPv6 address is not confused with an address and a port", () => { + // `[fd00::1:8443]` is an address on the private network, and `[fd00::1]:8443` is another one + // with a port. Naming either must not admit the other, which is what stripping the brackets + // from both did: each became `fd00::1:8443`. + expect( + checkAgentEndpoint("http://[fd00::1]:8443/ag-ui", { + allowedHosts: named("[fd00::1:8443]"), + }).allowed, + ).toBeFalse(); + expect( + checkAgentEndpoint("http://[fd00::1:8443]/ag-ui", { + allowedHosts: named("[fd00::1]:8443"), + }).allowed, + ).toBeFalse(); + expect( + checkAgentEndpoint("http://[fd00::1:8443]/ag-ui", { + allowedHosts: named("[fd00::1:8443]"), + }).allowed, + ).toBeTrue(); + }); + test("the metadata address cannot be named back in", () => { /* * The property that makes this safe to ship. The never-allowed list is checked before the diff --git a/server/tests/config.test.ts b/server/tests/config.test.ts index be2f76e9..45061e0f 100644 --- a/server/tests/config.test.ts +++ b/server/tests/config.test.ts @@ -818,6 +818,33 @@ describe("AGENT_ENDPOINT_ALLOWED_HOSTS", () => { expect([...hosts].sort()).toEqual(["10.0.0.42:9000", "agents.internal"]); }); + test("an IPv6 address is stored the way the endpoint check spells it", () => { + // The check compares against `URL.hostname`: compressed, lower-case, in brackets. An entry kept + // as the operator wrote it was a line that silently never matched. + const hosts = loadConfig({ + ...base(), + AGENT_ENDPOINT_ALLOWED_HOSTS: + "[0:0:0:0:0:0:0:1]:8443, [FE80::1], [::1:8443]", + }).agentEndpointAllowedHosts; + expect([...hosts].sort()).toEqual([ + "[::1:8443]", + "[::1]:8443", + "[fe80::1]", + ]); + }); + + test("a bracketed entry that is not an address is refused, naming the entry", () => { + expect(() => + loadConfig({ + ...base(), + AGENT_ENDPOINT_ALLOWED_HOSTS: "[not-an-address]", + }), + ).toThrow(/must be a host/); + expect(() => + loadConfig({ ...base(), AGENT_ENDPOINT_ALLOWED_HOSTS: "[::1]junk" }), + ).toThrow(/must be a host/); + }); + test("a URL is refused, naming the entry", () => { expect(() => loadConfig({