From a2dc21c824044008fb8675356470e00d277034c8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:54:46 -0700 Subject: [PATCH] fix(review): reject plain ws: in isSafeEndpointUrl, matching its own "secure" doc comment isSafeEndpointUrl's doc comment always said it permits "secure WebSocket endpoints (wss:, ws:)" -- but ws: is the plaintext counterpart to wss:, the same relationship http:/https: has, where isSafeHttpUrl already rejects http: for exactly that reason. The registry-logic.ts error message this guards ("must be a public HTTPS or WSS endpoint") already only ever mentioned WSS, confirming ws: support was an oversight, not an intentional carve-out. Applied identically to both sides of the safe-url.ts twin pair (content-lane + @loopover/engine). Closes #8017. --- packages/loopover-engine/src/review/safe-url.ts | 12 +++++++----- src/review/content-lane/safe-url.ts | 12 +++++++----- test/unit/content-lane-safe-url.test.ts | 9 ++++++--- test/unit/safe-url-engine.test.ts | 9 ++++++--- 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/packages/loopover-engine/src/review/safe-url.ts b/packages/loopover-engine/src/review/safe-url.ts index 4d400917bd..190ae5df63 100644 --- a/packages/loopover-engine/src/review/safe-url.ts +++ b/packages/loopover-engine/src/review/safe-url.ts @@ -7,8 +7,8 @@ // // Rejects non-HTTPS (isSafeHttpUrl), localhost / `*.localhost` / .local / .internal, and private/ // loopback/link-local IPs in any literal notation (decimal `2130706433`, hex `0x7f000001`, octal, -// short `127.1`, and the IPv6 forms). isSafeEndpointUrl additionally permits wss:/ws: for base-layer -// chain endpoints. +// short `127.1`, and the IPv6 forms). isSafeEndpointUrl additionally permits wss: (not plain ws:, +// #8017) for base-layer chain endpoints. function parseIpv4Component(part: string): number | null { if (/^0x[0-9a-f]+$/i.test(part)) return parseInt(part, 16); @@ -109,8 +109,10 @@ export function isSafeHttpUrl(raw: string): boolean { return !hostIsPrivateOrLocal(url.hostname); } -/** Like isSafeHttpUrl but also permits secure WebSocket endpoints (`wss:`, `ws:`) — base-layer chain - * endpoints (subtensor RPC/WSS/archive) are probed via JSON-RPC, not HTTP. Same SSRF host/IP guard. */ +/** Like isSafeHttpUrl but also permits the secure WebSocket endpoint (`wss:`) — base-layer chain endpoints + * (subtensor RPC/WSS/archive) are probed via JSON-RPC, not HTTP. Same SSRF host/IP guard. Plain `ws:` is + * REJECTED (#8017): it is the plaintext counterpart to `wss:`, the same relationship `http:`/`https:` has — + * and `isSafeHttpUrl` above already rejects `http:` for exactly that reason. */ export function isSafeEndpointUrl(raw: string): boolean { let url: URL; try { @@ -118,6 +120,6 @@ export function isSafeEndpointUrl(raw: string): boolean { } catch { return false; } - if (!["https:", "wss:", "ws:"].includes(url.protocol)) return false; + if (!["https:", "wss:"].includes(url.protocol)) return false; return !hostIsPrivateOrLocal(url.hostname); } diff --git a/src/review/content-lane/safe-url.ts b/src/review/content-lane/safe-url.ts index 4d400917bd..190ae5df63 100644 --- a/src/review/content-lane/safe-url.ts +++ b/src/review/content-lane/safe-url.ts @@ -7,8 +7,8 @@ // // Rejects non-HTTPS (isSafeHttpUrl), localhost / `*.localhost` / .local / .internal, and private/ // loopback/link-local IPs in any literal notation (decimal `2130706433`, hex `0x7f000001`, octal, -// short `127.1`, and the IPv6 forms). isSafeEndpointUrl additionally permits wss:/ws: for base-layer -// chain endpoints. +// short `127.1`, and the IPv6 forms). isSafeEndpointUrl additionally permits wss: (not plain ws:, +// #8017) for base-layer chain endpoints. function parseIpv4Component(part: string): number | null { if (/^0x[0-9a-f]+$/i.test(part)) return parseInt(part, 16); @@ -109,8 +109,10 @@ export function isSafeHttpUrl(raw: string): boolean { return !hostIsPrivateOrLocal(url.hostname); } -/** Like isSafeHttpUrl but also permits secure WebSocket endpoints (`wss:`, `ws:`) — base-layer chain - * endpoints (subtensor RPC/WSS/archive) are probed via JSON-RPC, not HTTP. Same SSRF host/IP guard. */ +/** Like isSafeHttpUrl but also permits the secure WebSocket endpoint (`wss:`) — base-layer chain endpoints + * (subtensor RPC/WSS/archive) are probed via JSON-RPC, not HTTP. Same SSRF host/IP guard. Plain `ws:` is + * REJECTED (#8017): it is the plaintext counterpart to `wss:`, the same relationship `http:`/`https:` has — + * and `isSafeHttpUrl` above already rejects `http:` for exactly that reason. */ export function isSafeEndpointUrl(raw: string): boolean { let url: URL; try { @@ -118,6 +120,6 @@ export function isSafeEndpointUrl(raw: string): boolean { } catch { return false; } - if (!["https:", "wss:", "ws:"].includes(url.protocol)) return false; + if (!["https:", "wss:"].includes(url.protocol)) return false; return !hostIsPrivateOrLocal(url.hostname); } diff --git a/test/unit/content-lane-safe-url.test.ts b/test/unit/content-lane-safe-url.test.ts index 2a8049d9c4..5ba05ad9d9 100644 --- a/test/unit/content-lane-safe-url.test.ts +++ b/test/unit/content-lane-safe-url.test.ts @@ -144,9 +144,8 @@ describe("isSafeHttpUrl", () => { }); describe("isSafeEndpointUrl", () => { - it("additionally permits wss / ws for chain endpoints", () => { + it("additionally permits wss for chain endpoints", () => { expect(isSafeEndpointUrl("wss://entrypoint.example.com")).toBe(true); - expect(isSafeEndpointUrl("ws://node.example.com")).toBe(true); expect(isSafeEndpointUrl("https://api.example.com")).toBe(true); }); @@ -155,7 +154,11 @@ describe("isSafeEndpointUrl", () => { expect(isSafeEndpointUrl("wss://localhost")).toBe(false); }); - it("rejects non-ws/https protocols", () => { + // #8017: plain ws: is the plaintext counterpart to wss:, the same relationship http:/https: has -- and + // isSafeHttpUrl already rejects http: for exactly that reason. The doc comment above always said "secure + // WebSocket endpoints"; the code just didn't match it until now. + it("rejects non-wss/https protocols, including plain unencrypted ws: (#8017)", () => { + expect(isSafeEndpointUrl("ws://node.example.com")).toBe(false); expect(isSafeEndpointUrl("http://example.com")).toBe(false); expect(isSafeEndpointUrl("ftp://example.com")).toBe(false); }); diff --git a/test/unit/safe-url-engine.test.ts b/test/unit/safe-url-engine.test.ts index 68b35522ea..c18b7329de 100644 --- a/test/unit/safe-url-engine.test.ts +++ b/test/unit/safe-url-engine.test.ts @@ -145,9 +145,8 @@ describe("isSafeHttpUrl", () => { }); describe("isSafeEndpointUrl", () => { - it("additionally permits wss / ws for chain endpoints", () => { + it("additionally permits wss for chain endpoints", () => { expect(isSafeEndpointUrl("wss://entrypoint.example.com")).toBe(true); - expect(isSafeEndpointUrl("ws://node.example.com")).toBe(true); expect(isSafeEndpointUrl("https://api.example.com")).toBe(true); }); @@ -156,7 +155,11 @@ describe("isSafeEndpointUrl", () => { expect(isSafeEndpointUrl("wss://localhost")).toBe(false); }); - it("rejects non-ws/https protocols", () => { + // #8017: plain ws: is the plaintext counterpart to wss:, the same relationship http:/https: has -- and + // isSafeHttpUrl already rejects http: for exactly that reason. The doc comment above always said "secure + // WebSocket endpoints"; the code just didn't match it until now. + it("rejects non-wss/https protocols, including plain unencrypted ws: (#8017)", () => { + expect(isSafeEndpointUrl("ws://node.example.com")).toBe(false); expect(isSafeEndpointUrl("http://example.com")).toBe(false); expect(isSafeEndpointUrl("ftp://example.com")).toBe(false); });