From 45bee4b484db5ec7c1a82334a4e784d0dcf69cfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Wed, 30 Sep 2026 14:47:57 +0200 Subject: [PATCH] fix(api): reject request-derived storage keys over platform limits before use GET /api/versions/exists read KV before its try block with an unbounded key, so an overlong v threw into the catch-all as an anonymous 500 and a Sentry event. Guard it and the other anonymously reachable key sites (demo ids, /d subpaths against R2, Tier-2 session ids, sourcemap keys). Co-Authored-By: Claude Sonnet 5.5 --- runner/pipeline/o11y-symbolicate.test.mjs | 23 ++ runner/pipeline/storage-key-guards.test.mjs | 234 +++++++++++++++++++ runner/workers/api/src/index.ts | 14 ++ runner/workers/api/src/share.ts | 7 +- runner/workers/api/src/storage-key.ts | 36 +++ runner/workers/o11y/src/drain/symbolicate.ts | 6 +- 6 files changed, 318 insertions(+), 2 deletions(-) create mode 100644 runner/pipeline/storage-key-guards.test.mjs create mode 100644 runner/workers/api/src/storage-key.ts diff --git a/runner/pipeline/o11y-symbolicate.test.mjs b/runner/pipeline/o11y-symbolicate.test.mjs index 5585ebd26..dc2e08aa7 100644 --- a/runner/pipeline/o11y-symbolicate.test.mjs +++ b/runner/pipeline/o11y-symbolicate.test.mjs @@ -120,6 +120,29 @@ test("symbolicateResourceLogs leaves a frame unresolved when no map exists (neve assert.equal(resolved.scopeLogs[0].logRecords[0].body.stringValue, record.scopeLogs[0].logRecords[0].body.stringValue); }); +test("symbolicateResourceLogs never asks for a map whose R2 key would exceed 1024 bytes", async () => { + // The frame path is browser-supplied; R2 throws on an overlong key, which would surface as a fetch_error. + const frameLine = formatStackFrame({ + filename: `https://demos.handsontable.com/assets/${"a".repeat(1100)}.js`, + function: "fn", + lineno: 1, + colno: 1, + }); + const record = exceptionRecord(["Error: x", frameLine]); + const asked = []; + + const [resolved] = await symbolicateResourceLogs([record], { + getMap: async (key) => { + asked.push(key); + if (Buffer.byteLength(key, "utf8") > 1024) throw new Error("The specified object name is not valid. (10020)"); + return null; + }, + }); + + assert.deepEqual(asked, []); + assert.equal(resolved.scopeLogs[0].logRecords[0].body.stringValue, record.scopeLogs[0].logRecords[0].body.stringValue); +}); + test("symbolicateResourceLogs never touches a non-exception record", async () => { const record = { resource: { diff --git a/runner/pipeline/storage-key-guards.test.mjs b/runner/pipeline/storage-key-guards.test.mjs new file mode 100644 index 000000000..63a01aabb --- /dev/null +++ b/runner/pipeline/storage-key-guards.test.mjs @@ -0,0 +1,234 @@ +// Request-derived KV keys, R2 keys and DO names that would exceed a platform +// limit must be answered with a clean 4xx (or a miss) before any storage +// access. The KV fake here throws on a key over 512 bytes the way real Workers +// KV does, so a route that forgets the guard fails these specs instead of +// passing on a permissive Map. +// +// Run: node --experimental-strip-types --test pipeline/storage-key-guards.test.mjs + +import test from "node:test"; +import assert from "node:assert/strict"; +import { register } from "node:module"; + +import { ctx, demoRow, fakeKV, makeEnv } from "./fixtures/worker-harness.mjs"; +import { setSandboxFactory } from "./fixtures/cloudflare-sandbox-stub.mjs"; + +register("./fixtures/worker-hooks.mjs", import.meta.url); + +const { default: worker } = await import("../workers/api/src/index.ts"); +const { captures } = await import("./fixtures/sentry-cloudflare-stub.mjs"); +const { kvKeyFits, r2KeyFits } = await import("../workers/api/src/storage-key.ts"); + +const bytes = (s) => Buffer.byteLength(s, "utf8"); + +/** fakeKV that rejects like the real thing and records every rejected or accepted key. */ +function strictKV() { + const inner = fakeKV(); + const accessed = []; + const rejected = []; + const guard = (op, key) => { + accessed.push(`${op} ${key}`); + const n = bytes(key); + if (n === 0 || n > 512) { + rejected.push(key); + throw new Error(`KV ${op} failed: 414 UTF-8 encoded length of ${n} exceeds key length limit of 512.`); + } + }; + return { + accessed, + rejected, + inner, + async get(key, type) { guard("GET", key); return inner.get(key, type); }, + async put(key, value, options) { guard("PUT", key); return inner.put(key, value, options); }, + async delete(key) { guard("DELETE", key); return inner.delete(key); }, + async list(options) { return inner.list(options); }, + }; +} + +/** R2 fake that throws on an object key over 1024 bytes, like real R2. */ +function strictR2(r2) { + const rejected = []; + const guard = (key) => { + if (bytes(key) > 1024) { + rejected.push(key); + throw new Error(`R2 get failed: The specified object name is not valid. (10020)`); + } + }; + return { + rejected, + puts: r2.puts, + async get(key) { guard(key); return r2.get(key); }, + async put(key, value, options) { guard(key); return r2.put(key, value, options); }, + delete: (key) => r2.delete(key), + list: (options) => r2.list(options), + }; +} + +function setup(seedRows = [], seedArtifacts = {}) { + const made = makeEnv(seedRows, [], seedArtifacts); + const kv = strictKV(); + made.env.CACHE = kv; + const r2 = strictR2(made.env.ARTIFACTS); + made.env.ARTIFACTS = r2; + return { ...made, kv, r2 }; +} + +const call = (env, method, path, init) => + worker.fetch(new Request(`https://demos.handsontable.com${path}`, { method, ...init }), env, ctx); + +// ---- unit: bytes, not characters ------------------------------------------ + +test("key helpers measure UTF-8 bytes, not UTF-16 units", () => { + const multibyte = "é".repeat(300); // 300 chars, 600 bytes + assert.ok(multibyte.length < 512 && bytes(multibyte) > 512); + assert.equal(kvKeyFits(multibyte), false); + assert.equal(kvKeyFits("a".repeat(512)), true); + assert.equal(kvKeyFits("a".repeat(513)), false); + assert.equal(kvKeyFits(""), false); + assert.equal(r2KeyFits("a".repeat(1024)), true); + assert.equal(r2KeyFits("a".repeat(1025)), false); +}); + +// ---- GET /api/versions/exists ---------------------------------------------- + +test("versions/exists: an overlong v is a 400, with no KV access, no throw and no Sentry capture", async () => { + const { env, kv } = setup(); + const before = captures.length; + const res = await call(env, "GET", `/api/versions/exists?v=${"1".repeat(600)}`); + assert.equal(res.status, 400); + assert.match((await res.json()).error, /valid version/); + assert.deepEqual(kv.accessed, [], "the guard must run before any KV access"); + assert.equal(captures.length, before, "an anonymous malformed request must not reach Sentry"); +}); + +test("versions/exists: a multibyte v under 512 characters but over 512 bytes is a 400", async () => { + const { env, kv } = setup(); + const v = "é".repeat(300); + assert.ok(v.length < 512 && bytes(`version-exists:${v}`) > 512); + const before = captures.length; + const res = await call(env, "GET", `/api/versions/exists?v=${encodeURIComponent(v)}`); + assert.equal(res.status, 400); + assert.deepEqual(kv.accessed, []); + assert.equal(captures.length, before); +}); + +test("versions/exists: a valid version still asks npm once, caches the answer and serves the repeat from KV", async () => { + const { env, kv } = setup(); + const realFetch = globalThis.fetch; + const asked = []; + globalThis.fetch = async (input) => { + if (String(input).startsWith("https://registry.npmjs.org/")) asked.push(String(input)); + return new Response("{}", { status: 200 }); + }; + try { + const first = await call(env, "GET", "/api/versions/exists?v=17.0.0-next-abc123"); + assert.equal(first.status, 200); + assert.deepEqual(await first.json(), { exists: true }); + const second = await call(env, "GET", "/api/versions/exists?v=17.0.0-next-abc123"); + assert.deepEqual(await second.json(), { exists: true }); + } finally { + globalThis.fetch = realFetch; + } + assert.deepEqual(asked, ["https://registry.npmjs.org/handsontable/17.0.0-next-abc123"]); + assert.deepEqual(kv.rejected, []); +}); + +test("versions/exists: an empty v is still a 400", async () => { + const { env } = setup(); + const res = await call(env, "GET", "/api/versions/exists?v="); + assert.equal(res.status, 400); +}); + +// ---- demo ids (getDemo cache key) ------------------------------------------- + +test("GET /d/:id with an overlong id is a 404, not a KV throw", async () => { + const { env, kv } = setup(); + const before = captures.length; + const res = await call(env, "GET", `/d/${"a".repeat(600)}/`); + assert.equal(res.status, 404); + assert.deepEqual(kv.rejected, []); + assert.equal(captures.length, before); +}); + +test("GET /d/:id with a multibyte id under 512 characters but over 512 bytes is a 404", async () => { + const { env, kv } = setup(); + const id = "é".repeat(300); + const res = await call(env, "GET", `/d/${encodeURIComponent(id)}/`); + assert.equal(res.status, 404); + assert.deepEqual(kv.rejected, []); +}); + +test("GET /api/demos/:id and /api/demos/:id/source with an overlong id are 404s", async () => { + const { env, kv } = setup(); + const before = captures.length; + for (const path of [`/api/demos/${"b".repeat(600)}`, `/api/demos/${"b".repeat(600)}/source`]) { + const res = await call(env, "GET", path); + assert.equal(res.status, 404, path); + } + assert.deepEqual(kv.rejected, []); + assert.equal(captures.length, before); +}); + +test("GET /d/:id for a real demo still serves its index.html", async () => { + const { env } = setup([demoRow()], { "demos/abc123/index.html": "ok" }); + const res = await call(env, "GET", "/d/abc123/"); + assert.equal(res.status, 200); + assert.match(await res.text(), /ok/); +}); + +// ---- R2 key from the /d/:id subpath ------------------------------------------ + +test("GET /d/:id/ does not hand R2 a key over 1024 bytes", async () => { + const { env, r2 } = setup([demoRow()], { "demos/abc123/index.html": "ok" }); + const before = captures.length; + const res = await call(env, "GET", `/d/abc123/${"p".repeat(1100)}`); + assert.equal(res.status, 200, "falls through to the SPA index like any other unknown path"); + assert.deepEqual(r2.rejected, []); + assert.equal(captures.length, before); +}); + +// ---- Tier-2 session ids (tombstone/meter KV keys, DO name) ------------------- + +test("session subroutes with an overlong id are a 400 before any KV or sandbox access", async () => { + const { env, kv } = setup(); + let sandboxTouched = 0; + setSandboxFactory(() => { sandboxTouched += 1; return {}; }); + const id = "s".repeat(600); + for (const [method, path, init] of [ + ["POST", `/api/session/${id}/file`, { body: JSON.stringify({ path: "a.js", contents: "x" }), headers: { "Content-Type": "application/json" } }], + ["DELETE", `/api/session/${id}/file?path=a.js`], + ["GET", `/api/session/${id}/status`], + ]) { + const res = await call(env, method, path, init); + assert.equal(res.status, 400, `${method} ${path.slice(0, 40)}`); + } + assert.deepEqual(kv.accessed, [], "no tombstone or meter read for an invalid id"); + assert.equal(sandboxTouched, 0); +}); + +test("DELETE /api/session/:id with an overlong id is a 400 and touches nothing", async () => { + const { env, kv } = setup(); + let sandboxTouched = 0; + setSandboxFactory(() => { sandboxTouched += 1; return { destroy: async () => {} }; }); + const res = await call(env, "DELETE", `/api/session/${"s".repeat(600)}`); + assert.equal(res.status, 400); + assert.deepEqual(kv.accessed, []); + assert.equal(sandboxTouched, 0); +}); + +test("POST /api/session with an overlong client-supplied sessionId is a 400 and touches nothing", async () => { + const { env, kv } = setup(); + let sandboxTouched = 0; + setSandboxFactory(() => { sandboxTouched += 1; return {}; }); + const res = await call(env, "POST", "/api/session", { + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + framework: "react-js", + files: { "package.json": "{}" }, + sessionId: "s".repeat(600), + }), + }); + assert.equal(res.status, 400); + assert.deepEqual(kv.rejected, []); + assert.equal(sandboxTouched, 0); +}); diff --git a/runner/workers/api/src/index.ts b/runner/workers/api/src/index.ts index abe397fe0..99514c2e1 100644 --- a/runner/workers/api/src/index.ts +++ b/runner/workers/api/src/index.ts @@ -45,6 +45,7 @@ import { } from "./session-lifecycle.js"; import { refAmbiguousMessage, refUnknownMessage } from "./session-listing.js"; import { ImportError, MAX_PAYLOAD_CHARS, importFromUrl, validatePayloadFiles } from "./import-url.js"; +import { VERSION_QUERY_RE, sessionIdFits } from "./storage-key.js"; import { BuildFailure, buildFailureTags, createDemo, createPendingDemo, demoBuildState, getDemo, getDemoSource, hasCachedBuild, invalidateDemo, isUserBuildError, serveDemoAsset, shortId, updateDemo, userBuildErrorDetail, withEntryScript, type DemoRow } from "./share.js"; import { BuildJobBase, scheduleSnapshotBuild } from "./snapshot-jobs.js"; import { @@ -912,6 +913,10 @@ async function handleNonProxyRequest(request: Request, env: Env, ctx: ExecutionC const cfg = BUILD_CONFIG[body.framework]; if (!dev || !cfg) return json({ error: `Tier-2 not wired for framework: ${body.framework}` }, 400); const files = validateFiles(body.files); + // The id becomes a KV key suffix and a DO name; an overlong one is a client mistake, not a 500. + if (typeof body.sessionId === "string" && body.sessionId.trim() && !sessionIdFits(body.sessionId.trim())) { + return json({ error: "invalid session id" }, 400); + } // `session.start` (contract §5, "all outcomes"): one point per create // attempt regardless of how it ends, timed from here. const createStartedAt = Date.now(); @@ -1244,6 +1249,10 @@ async function handleNonProxyRequest(request: Request, env: Env, ctx: ExecutionC // covered by default instead of each hand-copying the check. if (parts[0] === "api" && parts[1] === "session" && parts.length >= 4) { const sessionId = parts[2]!; + // An overlong id makes the tombstone/meter KV reads throw, which those + // helpers swallow as "no marker" / "metered", waving an invented id + // through to a sandbox boot. + if (!sessionIdFits(sessionId)) return json({ error: "invalid session id" }, 400); const gateRequest = { method: request.method, sub: parts[3] }; const refuse = (verdict: ReturnType) => verdict === "noop" ? cors(new Response(null, { status: 204 })) : json({ error: "session closed" }, 410); @@ -1337,6 +1346,7 @@ async function handleNonProxyRequest(request: Request, env: Env, ctx: ExecutionC // discards the response); the admin panel's kill button goes through the // same `teardownLiveSession`. if (request.method === "DELETE" && parts[0] === "api" && parts[1] === "session" && parts.length === 3) { + if (!sessionIdFits(parts[2]!)) return json({ error: "invalid session id" }, 400); await teardownLiveSession(env, parts[2]!); return cors(new Response(null, { status: 204 })); } @@ -1948,6 +1958,10 @@ async function handleNonProxyRequest(request: Request, env: Env, ctx: ExecutionC if (request.method === "GET" && parts[0] === "api" && parts[1] === "versions" && parts[2] === "exists") { const v = url.searchParams.get("v")?.trim() ?? ""; if (!v) return json({ error: "v is required" }, 400); + // Shape-check before the KV read: a KV key is capped at 512 bytes, so an + // overlong `v` makes `get` throw and an anonymous caller would mint a 500. + // Real versions and dist-tags are short ASCII. + if (!VERSION_QUERY_RE.test(v)) return json({ error: "v is not a valid version" }, 400); const cacheKey = `version-exists:${v}`; const cached = await env.CACHE.get(cacheKey, "json"); if (cached) return cors(cacheableJson(cached)); diff --git a/runner/workers/api/src/share.ts b/runner/workers/api/src/share.ts index d2a2c4093..788e7118b 100644 --- a/runner/workers/api/src/share.ts +++ b/runner/workers/api/src/share.ts @@ -18,6 +18,7 @@ import { recordContainerUsage, SESSION_INSTANCE_TYPE } from "./budget.js"; import { htmlEntryLoadsModule, snapshotBuildCommand } from "./build-command.js"; import { htMajorFromVersion, injectLiteHtml } from "./monitor-inject.js"; import { emitPoint } from "./telemetry/points.js"; +import { kvKeyFits, r2KeyFits } from "./storage-key.js"; type SandboxLike = { mkdir(path: string, opts?: { recursive?: boolean }): Promise; @@ -723,6 +724,8 @@ export async function updateDemo( export async function getDemo(env: Env, id: string): Promise { const cacheKey = `demo:${id}`; + // A KV key is capped at 512 bytes; no minted id is near that, so an overlong one is a miss. + if (!kvKeyFits(cacheKey)) return null; const cached = await env.CACHE.get(cacheKey, "json"); if (cached) return cached as DemoRow; const row = await env.DB.prepare("SELECT * FROM demos WHERE id = ?").bind(id).first(); @@ -888,7 +891,9 @@ export async function serveDemoAsset( if (isDocRequest) record(404, 0); return new Response("Not found", { status: 404 }); } - const candidates = clean === "" ? ["index.html"] : [clean, `${clean}/index.html`, "index.html"]; + // R2 throws on an object key over 1024 bytes; an overlong path cannot name an object, so skip it. + const candidates = (clean === "" ? ["index.html"] : [clean, `${clean}/index.html`, "index.html"]) + .filter((c) => r2KeyFits(row.r2_prefix + c)); let obj: R2ObjectBodyText | null = null; let hitPath = "index.html"; diff --git a/runner/workers/api/src/storage-key.ts b/runner/workers/api/src/storage-key.ts new file mode 100644 index 000000000..d959d9723 --- /dev/null +++ b/runner/workers/api/src/storage-key.ts @@ -0,0 +1,36 @@ +// Platform limits on storage keys and object names built from request input. +// Import-free so pipeline specs can load it directly under strip-types. + +const encoder = new TextEncoder(); + +/** Workers KV rejects a key over 512 UTF-8 bytes (and an empty one) by throwing. */ +export const KV_KEY_MAX_BYTES = 512; + +/** R2 rejects an object key over 1024 UTF-8 bytes by throwing. */ +export const R2_KEY_MAX_BYTES = 1024; + +/** Longest client-supplied Tier-2 session id we accept; minted ids are ~30 bytes. */ +export const SESSION_ID_MAX_BYTES = 128; + +const byteLength = (value: string): number => encoder.encode(value).length; + +/** True when `key` is a legal KV key, measured in UTF-8 bytes rather than UTF-16 units. */ +export function kvKeyFits(key: string): boolean { + const bytes = byteLength(key); + return bytes > 0 && bytes <= KV_KEY_MAX_BYTES; +} + +/** True when `key` is a legal R2 object key. */ +export function r2KeyFits(key: string): boolean { + const bytes = byteLength(key); + return bytes > 0 && bytes <= R2_KEY_MAX_BYTES; +} + +/** True when a client-supplied session id is short enough to become a KV key suffix and a DO name. */ +export function sessionIdFits(sessionId: string): boolean { + const bytes = byteLength(sessionId); + return bytes > 0 && bytes <= SESSION_ID_MAX_BYTES; +} + +/** What `/api/versions/exists` accepts: a semver or npm dist-tag shape, ASCII only. */ +export const VERSION_QUERY_RE = /^[0-9A-Za-z][0-9A-Za-z._+-]{0,63}$/; diff --git a/runner/workers/o11y/src/drain/symbolicate.ts b/runner/workers/o11y/src/drain/symbolicate.ts index 6138d5d41..9b3239d5d 100644 --- a/runner/workers/o11y/src/drain/symbolicate.ts +++ b/runner/workers/o11y/src/drain/symbolicate.ts @@ -63,6 +63,8 @@ function isBabelChunk(filename: string): boolean { } } +const R2_KEY_MAX_BYTES = 1024; + /** `sourcemaps//.map` (ADR §C.3). * `null` when `filename` is not a parseable URL — a preview-host frame * correctly never resolves to a real map. */ @@ -73,7 +75,9 @@ function mapKeyFor(filename: string, serviceVersion: string): string | null { // without this the symbolicator would fetch a nonexistent map and // report it as missing. if (url.pathname.endsWith("/")) return null; - return `sourcemaps/${serviceVersion}${url.pathname}.map`; + const key = `sourcemaps/${serviceVersion}${url.pathname}.map`; + // R2 throws on a key over 1024 bytes; the frame's path is browser-supplied, so it cannot name a map. + return new TextEncoder().encode(key).length > R2_KEY_MAX_BYTES ? null : key; } catch { return null; }