From c64434a1316418ec0f566a8d826fa757569d1936 Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Tue, 4 Aug 2026 15:57:22 +0530 Subject: [PATCH] fix(security): close SSRF, credential-leak, and auth bypass findings in 0.8.4 Audit of 0.8.4 found the repo's own gates green, so these came from manual review; each is covered by a new regression test. security/fetch.ts - safeFetch re-attached Authorization/Cookie on a same-origin redirect that followed a cross-origin hop (a -> b -> b), handing credentials to the second host. Compare against the origin the caller trusted, not the previous hop. - The private-network guard resolved the host, approved it, then let fetch resolve again, so a low-TTL record could answer public for the check and private for the connection. Pin the connection to the validated address, preserving Host and TLS serverName. Opt out with pinDns: false. - 0:0:0:0:0:ffff:127.0.0.1, ::ffff:7f00:1 and fec0::1 were not treated as private. Add uncompressed IPv4-mapped forms, site-local IPv6, 198.18/15 and 192.0.0/24. security/url.ts - sanitizeUrl returned "//evil.com" verbatim via the relative-path fast path, bypassing the host checks it had just run; in an href that navigates cross-origin. Resolve protocol-relative input instead. dev-server/gateway.ts - Malformed base64 in an Authorization header threw out of checkAuth on an unauthenticated path. Fail closed. - split(":", 2) truncated passwords at the first colon, so a password containing ":" could never authenticate. - The credential compare short-circuited on length mismatch, leaking length by timing. Extracted as verifyBasicAuth so it is testable. authz/index.ts - Namespace wildcards only matched the first segment, so "post:comment:*" did not grant "post:comment:delete". Match at every depth. uploader/operations.ts - Validate transcoder dimensions and bitrate rather than trusting the declared type, and reject ".." path segments. package.json - The brace-expansion override pinned 5.0.8, which is inside the advisory range >=4.0.0 <5.0.9. Bump to 5.0.9; bun audit is now clean. Verified: check:production passes (typecheck, lint, 1033 tests, format, ASVS, public-API baseline, editor checks). Co-Authored-By: Claude Opus 5 --- bun.lock | 7 +- package.json | 5 +- packages/authz/src/index.ts | 11 +- packages/authz/test/rbac-wildcard.test.ts | 40 +++++ packages/dev-server/src/gateway.ts | 50 +++++-- .../test/gateway-basic-auth.test.ts | 40 +++++ packages/security/src/fetch.ts | 80 ++++++++-- packages/security/src/url.ts | 17 ++- .../security/test/ssrf-regression.test.ts | 140 ++++++++++++++++++ packages/uploader/src/operations.ts | 24 ++- 10 files changed, 369 insertions(+), 45 deletions(-) create mode 100644 packages/authz/test/rbac-wildcard.test.ts create mode 100644 packages/dev-server/test/gateway-basic-auth.test.ts create mode 100644 packages/security/test/ssrf-regression.test.ts diff --git a/bun.lock b/bun.lock index e33141a8..be310f6d 100644 --- a/bun.lock +++ b/bun.lock @@ -4,6 +4,9 @@ "workspaces": { "": { "name": "wrnexus", + "dependencies": { + "brace-expansion": "^5.0.9", + }, "devDependencies": { "@eslint/js": "^10.0.1", "@types/bun": "^1.3.14", @@ -531,7 +534,7 @@ }, }, "overrides": { - "brace-expansion": "5.0.8", + "brace-expansion": "5.0.9", "esbuild": "0.28.1", }, "packages": { @@ -917,7 +920,7 @@ "boolbase": ["boolbase@1.0.0", "", {}, "sha512-JZOSA7Mo9sNGB8+UjSgzdLtokWAky1zbztM3WRLCbZ70/3cTANmQmOdR7y2g+J0e2WXywy1yS468tY+IruqEww=="], - "brace-expansion": ["brace-expansion@5.0.8", "", { "dependencies": { "balanced-match": "^4.0.2" } }, "sha512-JZyDyq3D4AUifKTPOB7DELf6XsB3WdPuNxCtob1vFXPsSXhdAiHBWJ/tJ8HAc9aH84BK+5JFZLNkJKx3G9kzQg=="], + "brace-expansion": ["brace-expansion@5.0.9", "", { "dependencies": { "balanced-match": "^4.0.2" } }, "sha512-ScQ4IuvIEF1TMlP7Zt+vjJ//9zlPb2SDcxWxM3bk8s6t6GGdJ7KO1dCcTidOPJKePW30LE/2cT7wCyPho9/Wxg=="], "braces": ["braces@3.0.3", "", { "dependencies": { "fill-range": "^7.1.1" } }, "sha512-yQbXgO/OSZVD2IsiLlro+7Hf6Q18EJrKSEsdoMzKePKXct3gvD8oLcOQdIzGupr5Fj+EDe8gO/lxc1BzfMpxvA=="], diff --git a/package.json b/package.json index 6ac4583d..c173b162 100644 --- a/package.json +++ b/package.json @@ -79,6 +79,9 @@ }, "overrides": { "esbuild": "0.28.1", - "brace-expansion": "5.0.8" + "brace-expansion": "5.0.9" + }, + "dependencies": { + "brace-expansion": "^5.0.9" } } diff --git a/packages/authz/src/index.ts b/packages/authz/src/index.ts index f57d5256..b7c7a3a2 100644 --- a/packages/authz/src/index.ts +++ b/packages/authz/src/index.ts @@ -52,11 +52,12 @@ export function defineRbac(roles: Record): Rbac { if (!subject?.roles?.length) return false; const perms = permissionsFor(subject.roles); if (perms.has("*") || perms.has(permission)) return true; - // Namespace wildcards: "post:*" grants "post:write". - const ns = permission.includes(":") - ? permission.slice(0, permission.indexOf(":")) + ":*" - : null; - return ns ? perms.has(ns) : false; + // Namespace wildcards at every depth: "post:*" and "post:comment:*" both + // grant "post:comment:delete". + for (let at = permission.indexOf(":"); at !== -1; at = permission.indexOf(":", at + 1)) { + if (perms.has(`${permission.slice(0, at)}:*`)) return true; + } + return false; }, }; } diff --git a/packages/authz/test/rbac-wildcard.test.ts b/packages/authz/test/rbac-wildcard.test.ts new file mode 100644 index 00000000..fd3ec720 --- /dev/null +++ b/packages/authz/test/rbac-wildcard.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, test } from "bun:test"; +import { defineRbac } from "../src/index.ts"; + +describe("RBAC namespace wildcards", () => { + const rbac = defineRbac({ + admin: ["*"], + editor: ["post:*"], + moderator: ["post:comment:*"], + reader: ["post:read"], + }); + + test("a wildcard grants every depth beneath it", () => { + expect(rbac.can({ roles: ["editor"] }, "post:write")).toBe(true); + expect(rbac.can({ roles: ["editor"] }, "post:comment:delete")).toBe(true); + expect(rbac.can({ roles: ["editor"] }, "post:comment:flag:undo")).toBe(true); + }); + + test("a deeper wildcard grants its own subtree", () => { + expect(rbac.can({ roles: ["moderator"] }, "post:comment:delete")).toBe(true); + expect(rbac.can({ roles: ["moderator"] }, "post:comment:flag:undo")).toBe(true); + }); + + test("a wildcard does not leak sideways or upward", () => { + expect(rbac.can({ roles: ["moderator"] }, "post:write")).toBe(false); + expect(rbac.can({ roles: ["moderator"] }, "post")).toBe(false); + expect(rbac.can({ roles: ["editor"] }, "page:write")).toBe(false); + expect(rbac.can({ roles: ["reader"] }, "post:write")).toBe(false); + }); + + test("root wildcard and unknown subjects behave", () => { + expect(rbac.can({ roles: ["admin"] }, "anything:at:all")).toBe(true); + expect(rbac.can({ roles: [] }, "post:read")).toBe(false); + expect(rbac.can(undefined, "post:read")).toBe(false); + }); + + test("role inheritance terminates on cycles", () => { + const cyclic = defineRbac({ a: ["role:b", "p:a"], b: ["role:a", "p:b"] }); + expect([...cyclic.permissionsFor(["a"])].sort()).toEqual(["p:a", "p:b"]); + }); +}); diff --git a/packages/dev-server/src/gateway.ts b/packages/dev-server/src/gateway.ts index 37181f40..e5cf7da8 100644 --- a/packages/dev-server/src/gateway.ts +++ b/packages/dev-server/src/gateway.ts @@ -175,14 +175,46 @@ function gatewayWebSocketOriginAllowed( return target.domains.some((domain) => parsed.host.toLowerCase() === domain.toLowerCase()); } -/** Constant-time-ish string compare. */ +/** + * Constant-time string compare. Length is folded into the accumulator rather + * than short-circuiting, so a wrong guess cannot be distinguished from a + * wrong-length guess by timing. + */ function timingSafeEqual(a: string, b: string): boolean { - if (a.length !== b.length) return false; - let diff = 0; - for (let i = 0; i < a.length; i++) diff |= a.charCodeAt(i) ^ b.charCodeAt(i); + let diff = a.length ^ b.length; + const max = Math.max(a.length, b.length); + for (let i = 0; i < max; i++) diff |= (a.charCodeAt(i) || 0) ^ (b.charCodeAt(i) || 0); return diff === 0; } +/** + * Verify an HTTP Basic `Authorization` header against the configured pairs. + * Malformed base64 fails closed rather than throwing, and the username and + * password are split on the FIRST colon so passwords may contain colons. + */ +export function verifyBasicAuth( + header: string | null | undefined, + pairs: readonly { user: string; pass: string }[], +): boolean { + if (!header?.startsWith("Basic ")) return false; + let decoded: string; + try { + decoded = atob(header.slice(6)); + } catch { + return false; + } + const separator = decoded.indexOf(":"); + if (separator === -1) return false; + const user = decoded.slice(0, separator); + const pass = decoded.slice(separator + 1); + // Evaluate every pair so the number of configured credentials is not + // observable through response timing. + return pairs.reduce( + (ok, p) => (timingSafeEqual(user, p.user) && timingSafeEqual(pass, p.pass)) || ok, + false, + ); +} + export function internalError(res: Response): string | null { const encoded = res.headers.get("x-wrnexus-internal-error"); if (!encoded) return null; @@ -267,15 +299,7 @@ async function checkAuth( if (auth.basic) { const pairs = Array.isArray(auth.basic) ? auth.basic : [auth.basic]; - const header = req.headers.get("authorization") ?? ""; - const ok = - header.startsWith("Basic ") && - (() => { - const [user, pass] = atob(header.slice(6)).split(":", 2); - return pairs.some( - (p) => timingSafeEqual(user ?? "", p.user) && timingSafeEqual(pass ?? "", p.pass), - ); - })(); + const ok = verifyBasicAuth(req.headers.get("authorization"), pairs); if (!ok) { return new Response("Authentication required", { status: 401, diff --git a/packages/dev-server/test/gateway-basic-auth.test.ts b/packages/dev-server/test/gateway-basic-auth.test.ts new file mode 100644 index 00000000..fe615f7d --- /dev/null +++ b/packages/dev-server/test/gateway-basic-auth.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, test } from "bun:test"; +import { verifyBasicAuth } from "../src/gateway.ts"; + +const pairs = [ + { user: "admin", pass: "hunter2" }, + { user: "ops", pass: "p:a:s:s" }, +]; + +const basic = (raw: string) => `Basic ${btoa(raw)}`; + +describe("gateway basic auth", () => { + test("accepts a configured pair", () => { + expect(verifyBasicAuth(basic("admin:hunter2"), pairs)).toBe(true); + }); + + test("accepts a password containing colons", () => { + // split(":", 2) used to truncate this to "p", so it could never match. + expect(verifyBasicAuth(basic("ops:p:a:s:s"), pairs)).toBe(true); + }); + + test("rejects wrong credentials", () => { + expect(verifyBasicAuth(basic("admin:wrong"), pairs)).toBe(false); + expect(verifyBasicAuth(basic("nobody:hunter2"), pairs)).toBe(false); + }); + + test("fails closed on malformed input instead of throwing", () => { + // An unauthenticated request must not be able to raise a 500 here. + expect(() => verifyBasicAuth("Basic !!!!not-base64", pairs)).not.toThrow(); + expect(verifyBasicAuth("Basic !!!!not-base64", pairs)).toBe(false); + expect(verifyBasicAuth(basic("no-colon-at-all"), pairs)).toBe(false); + expect(verifyBasicAuth("Bearer token", pairs)).toBe(false); + expect(verifyBasicAuth(null, pairs)).toBe(false); + expect(verifyBasicAuth(undefined, pairs)).toBe(false); + expect(verifyBasicAuth("", pairs)).toBe(false); + }); + + test("empty credentials never match", () => { + expect(verifyBasicAuth(basic(":"), pairs)).toBe(false); + }); +}); diff --git a/packages/security/src/fetch.ts b/packages/security/src/fetch.ts index f49c1f06..f72cc8d9 100644 --- a/packages/security/src/fetch.ts +++ b/packages/security/src/fetch.ts @@ -10,6 +10,14 @@ export interface SafeFetchOptions extends RequestInit, SafeUrlPolicy { blockPrivateNetworks?: boolean; /** Forward Authorization, Cookie, and Proxy-Authorization across origin-changing redirects. Defaults to false. */ forwardSensitiveHeaders?: boolean; + /** + * Connect to the address this module validated instead of re-resolving the + * hostname inside `fetch`. Without it the private-network guard is advisory + * only: a low-TTL DNS record can answer with a public address for our lookup + * and a private one for the connection (DNS rebinding). Defaults to true + * whenever `blockPrivateNetworks` is on. + */ + pinDns?: boolean; resolver?: (hostname: string) => Promise; } @@ -28,25 +36,40 @@ function isPrivateIpv4(address: string): boolean { a === 127 || (a === 169 && b === 254) || (a === 172 && b! >= 16 && b! <= 31) || + (a === 192 && b === 0) || (a === 192 && b === 168) || + (a === 198 && b! >= 18 && b! <= 19) || (a === 100 && b! >= 64 && b! <= 127) || a! >= 224 ); } +/** + * Any IPv4-mapped form, not just the compact `::ffff:1.2.3.4` spelling. + * `0:0:0:0:0:ffff:127.0.0.1` and `::ffff:7f00:1` address loopback just as well. + */ +function mappedIpv4Of(normalized: string): string | null { + const dotted = /^(?:0*:)*0*ffff:(\d{1,3}(?:\.\d{1,3}){3})$/.exec(normalized)?.[1]; + if (dotted) return dotted; + const hex = /^(?:0*:)*0*ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/.exec(normalized); + if (!hex) return null; + const high = Number.parseInt(hex[1]!, 16); + const low = Number.parseInt(hex[2]!, 16); + return `${high >> 8}.${high & 0xff}.${low >> 8}.${low & 0xff}`; +} + function isPrivateIpv6(address: string): boolean { const normalized = address.toLowerCase().split("%")[0]!; - const mappedIpv4 = /^::ffff:(\d{1,3}(?:\.\d{1,3}){3})$/.exec(normalized)?.[1]; + const mappedIpv4 = mappedIpv4Of(normalized); if (mappedIpv4) return isPrivateIpv4(mappedIpv4); + if (/^(?:0*:)+0*1$/.test(normalized)) return true; // ::1 in any expansion + if (/^(?:0*:)*0*$/.test(normalized)) return true; // :: / all-zero return ( - normalized === "::" || - normalized === "::1" || normalized.startsWith("fc") || normalized.startsWith("fd") || - normalized.startsWith("fe8") || - normalized.startsWith("fe9") || - normalized.startsWith("fea") || - normalized.startsWith("feb") || + // fe80::/10 link-local through fec0::/10 site-local: every fe8-febf plus + // the deprecated-but-still-routable fec0-feff site-local block. + /^fe[89abcdef]/.test(normalized) || normalized.startsWith("ff") ); } @@ -61,8 +84,9 @@ async function defaultResolver(hostname: string): Promise { return (await lookup(hostname, { all: true, verbatim: true })).map((entry) => entry.address); } -async function assertPublicHost(url: URL, options: SafeFetchOptions): Promise { - if (options.blockPrivateNetworks === false) return; +/** Resolve a host and reject it unless every answer is a public address. */ +async function resolvePublicHost(url: URL, options: SafeFetchOptions): Promise { + if (options.blockPrivateNetworks === false) return []; const addresses = await (options.resolver ?? defaultResolver)(url.hostname); if (!addresses.length) { throw new SecurityError("WRN-SEC-SSRF-DNS", `Host '${url.hostname}' did not resolve.`); @@ -75,6 +99,22 @@ async function assertPublicHost(url: URL, options: SafeFetchOptions): Promise).blockPrivateNetworks; delete (init as Record).resolver; delete (init as Record).forwardSensitiveHeaders; + delete (init as Record).pinDns; delete (init as Record).base; delete (init as Record).allowRelative; delete (init as Record).allowedProtocols; @@ -114,23 +155,30 @@ export async function safeFetch( allowRelative: false, allowedProtocols: options.allowedProtocols ?? ["https:"], }); - let previousOrigin = current.origin; + // Compare against where the caller's credentials were meant to go, not + // against the previous hop: a→b→b must not re-attach them on the second + // b request just because that hop did not change origin. + const credentialOrigin = current.origin; for (let redirect = 0; ; redirect++) { - await assertPublicHost(current, options); + const addresses = await resolvePublicHost(current, options); const requestInit: RequestInit = { ...init }; - if (!options.forwardSensitiveHeaders && current.origin !== previousOrigin) { - const headers = new Headers(init.headers); + const headers = new Headers(init.headers); + if (!options.forwardSensitiveHeaders && current.origin !== credentialOrigin) { headers.delete("authorization"); headers.delete("cookie"); headers.delete("proxy-authorization"); - requestInit.headers = headers; } - const response = await fetch(current, requestInit); + const pinDns = options.pinDns ?? options.blockPrivateNetworks !== false; + const target = + pinDns && addresses.length + ? pinToAddress(current, addresses[0]!, requestInit, headers) + : current; + requestInit.headers = headers; + const response = await fetch(target, requestInit); if (response.status >= 300 && response.status < 400 && response.headers.has("location")) { if (redirect >= maxRedirects) { throw new SecurityError("WRN-SEC-SSRF-REDIRECT", "Too many redirects.", 502); } - previousOrigin = current.origin; current = validateUrl(new URL(response.headers.get("location")!, current), { ...options, allowRelative: false, diff --git a/packages/security/src/url.ts b/packages/security/src/url.ts index 118ab310..a59c94c5 100644 --- a/packages/security/src/url.ts +++ b/packages/security/src/url.ts @@ -12,6 +12,17 @@ export interface SafeUrlPolicy { const DEFAULT_PROTOCOLS = ["http:", "https:"]; +const RELATIVE_PREFIX = /^(?:\.{0,2}\/|\/|\?|#)/; + +/** + * `//evil.com` (and the `/\evil.com` spelling browsers normalise to it) reads + * like a same-site path but navigates cross-origin. It must never be handed + * back verbatim, or the host checks above are bypassed entirely. + */ +function isProtocolRelative(raw: string): boolean { + return /^[/\\]{2}/.test(raw); +} + function hasAsciiControlOrSpace(value: string): boolean { for (const character of value) { const code = character.charCodeAt(0); @@ -39,7 +50,7 @@ export function validateUrl(value: string | URL, policy: SafeUrlPolicy = {}): UR ); } - const isRelative = /^(?:\.{0,2}\/|\/|\?|#)/.test(raw); + const isRelative = RELATIVE_PREFIX.test(raw); if (isRelative && policy.allowRelative === false) { throw new SecurityError("WRN-SEC-URL-RELATIVE", "Relative URLs are not allowed."); } @@ -93,7 +104,9 @@ export function sanitizeUrl(value: unknown, policy: SafeUrlPolicy = {}): string try { const raw = String(value ?? ""); const url = validateUrl(raw, policy); - if (/^(?:\.{0,2}\/|\/|\?|#)/.test(raw)) return raw; + // Genuinely relative input round-trips unchanged; protocol-relative input + // is resolved so the returned string carries the host the policy approved. + if (RELATIVE_PREFIX.test(raw) && !isProtocolRelative(raw)) return raw; return url.toString(); } catch { return "about:blank"; diff --git a/packages/security/test/ssrf-regression.test.ts b/packages/security/test/ssrf-regression.test.ts new file mode 100644 index 00000000..29eb8a1d --- /dev/null +++ b/packages/security/test/ssrf-regression.test.ts @@ -0,0 +1,140 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { isPrivateAddress, safeFetch, sanitizeUrl } from "../src/index.ts"; + +const realFetch = globalThis.fetch; +afterEach(() => { + globalThis.fetch = realFetch; +}); + +/** Capture what each hop actually receives, and script the redirect chain. */ +function stubFetch(chain: (string | null)[]) { + const seen: { url: string; host: string | null; authorization: string | null }[] = []; + let hop = 0; + globalThis.fetch = (async (input: unknown, init?: RequestInit) => { + const headers = new Headers(init?.headers); + seen.push({ + url: String(input), + host: headers.get("host"), + authorization: headers.get("authorization"), + }); + const location = chain[hop++]; + return location + ? new Response(null, { status: 302, headers: { location } }) + : new Response("done", { status: 200 }); + }) as typeof fetch; + return seen; +} + +describe("safeFetch redirect credential handling", () => { + test("does not re-attach credentials after leaving the original origin", async () => { + const seen = stubFetch(["https://b.example/x", "https://b.example/final", null]); + + await safeFetch("https://a.example/start", { + headers: { authorization: "Bearer SECRET", cookie: "sid=1" }, + resolver: async () => ["93.184.216.34"], + }); + + expect(seen).toHaveLength(3); + expect(seen[0]!.authorization).toBe("Bearer SECRET"); + // Both hops on b.example are off-origin, including the b→b one. + expect(seen[1]!.authorization).toBeNull(); + expect(seen[2]!.authorization).toBeNull(); + }); + + test("keeps credentials across same-origin redirects", async () => { + const seen = stubFetch(["https://a.example/next", null]); + + await safeFetch("https://a.example/start", { + headers: { authorization: "Bearer SECRET" }, + resolver: async () => ["93.184.216.34"], + }); + + expect(seen[1]!.authorization).toBe("Bearer SECRET"); + }); + + test("forwardSensitiveHeaders opts back in", async () => { + const seen = stubFetch(["https://b.example/x", null]); + + await safeFetch("https://a.example/start", { + headers: { authorization: "Bearer SECRET" }, + forwardSensitiveHeaders: true, + resolver: async () => ["93.184.216.34"], + }); + + expect(seen[1]!.authorization).toBe("Bearer SECRET"); + }); +}); + +describe("safeFetch DNS pinning", () => { + test("connects to the validated address and preserves the Host header", async () => { + const seen = stubFetch([null]); + + await safeFetch("https://rebind.example/data", { + resolver: async () => ["93.184.216.34"], + }); + + // The connection targets the address we checked, so a second DNS answer + // cannot redirect it at an internal host. + expect(seen[0]!.url).toBe("https://93.184.216.34/data"); + expect(seen[0]!.host).toBe("rebind.example"); + }); + + test("pinDns:false restores hostname dialling", async () => { + const seen = stubFetch([null]); + + await safeFetch("https://rebind.example/data", { + pinDns: false, + resolver: async () => ["93.184.216.34"], + }); + + expect(seen[0]!.url).toBe("https://rebind.example/data"); + }); + + test("still rejects hosts that resolve into private space", async () => { + stubFetch([null]); + + await expect( + safeFetch("https://rebind.example/data", { resolver: async () => ["169.254.169.254"] }), + ).rejects.toThrow(/blocked address/); + }); +}); + +describe("isPrivateAddress coverage", () => { + test("catches non-canonical IPv4-mapped and site-local IPv6", () => { + expect(isPrivateAddress("0:0:0:0:0:ffff:127.0.0.1")).toBe(true); + expect(isPrivateAddress("::ffff:7f00:1")).toBe(true); + expect(isPrivateAddress("fec0::1")).toBe(true); + expect(isPrivateAddress("0:0:0:0:0:0:0:1")).toBe(true); + expect(isPrivateAddress("198.18.0.1")).toBe(true); + expect(isPrivateAddress("192.0.0.192")).toBe(true); + }); + + test("leaves public addresses alone", () => { + expect(isPrivateAddress("93.184.216.34")).toBe(false); + expect(isPrivateAddress("2606:2800:220:1:248:1893:25c8:1946")).toBe(false); + }); +}); + +describe("sanitizeUrl protocol-relative handling", () => { + test("does not hand back a protocol-relative URL verbatim", () => { + // "//evil.com" in an href navigates cross-origin; returning it unchanged + // would bypass every host check validateUrl just performed. + expect(sanitizeUrl("//evil.com")).toBe("http://evil.com/"); + expect(sanitizeUrl("//evil.com/path?a=b")).toBe("http://evil.com/path?a=b"); + // Backslash spellings resolve the same way rather than passing through. + expect(sanitizeUrl("\\\\evil.com")).toBe("http://evil.com/"); + expect(sanitizeUrl("/\\evil.com")).toBe("http://evil.com/"); + }); + + test("honours host policy for protocol-relative input", () => { + expect(sanitizeUrl("//evil.com", { allowedHosts: ["good.com"] })).toBe("about:blank"); + expect(sanitizeUrl("//good.com/x", { allowedHosts: ["good.com"] })).toBe("http://good.com/x"); + }); + + test("genuine relative paths round-trip unchanged", () => { + expect(sanitizeUrl("/safe/path")).toBe("/safe/path"); + expect(sanitizeUrl("./rel")).toBe("./rel"); + expect(sanitizeUrl("?q=1")).toBe("?q=1"); + expect(sanitizeUrl("#frag")).toBe("#frag"); + }); +}); diff --git a/packages/uploader/src/operations.ts b/packages/uploader/src/operations.ts index 82d23bc7..65986eac 100644 --- a/packages/uploader/src/operations.ts +++ b/packages/uploader/src/operations.ts @@ -179,17 +179,29 @@ export function ffmpegVideoTranscoder( options: { executable?: string; spawn?: (args: string[]) => { exited: Promise } } = {}, ) { return async (input: string, output: string, config: VideoTranscodeOptions): Promise => { - if (!/^[\w .:\\/-]+$/.test(input) || !/^[\w .:\\/-]+$/.test(output)) - throw new Error("Invalid video path"); + const validPath = (value: string) => + /^[\w .:\\/-]+$/.test(value) && !value.split(/[\\/]/).includes(".."); + if (!validPath(input) || !validPath(output)) throw new Error("Invalid video path"); + if (config.format !== "mp4" && config.format !== "webm") + throw new Error("Invalid video format"); + // These reach an ffmpeg filter string, so reject anything that is not a + // plain positive integer rather than trusting the declared type. + const dimension = (value: number | undefined, name: string): number | undefined => { + if (value === undefined) return undefined; + if (!Number.isInteger(value) || value <= 0 || value > 16384) + throw new Error(`Invalid video ${name}`); + return value; + }; + const width = dimension(config.width, "width"); + const height = dimension(config.height, "height"); + const bitrate = dimension(config.videoBitrateKbps, "bitrate"); const args = [ options.executable ?? "ffmpeg", "-y", "-i", input, - ...(config.width || config.height - ? ["-vf", `scale=${config.width ?? -2}:${config.height ?? -2}`] - : []), - ...(config.videoBitrateKbps ? ["-b:v", `${config.videoBitrateKbps}k`] : []), + ...(width || height ? ["-vf", `scale=${width ?? -2}:${height ?? -2}`] : []), + ...(bitrate ? ["-b:v", `${bitrate}k`] : []), "-f", config.format, output,