fix(api): guard request-derived KV/R2/DO keys against platform length limits - #399
Merged
Merged
Conversation
…fore 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
GET /api/versions/exists?v=<about 600 chars>readenv.CACHE.get("version-exists:" + v)before its own try block. Workers KV rejects keys over 512 bytes (KV GET failed: 414 UTF-8 encoded length of 615 exceeds key length limit of 512.), so the call threw into the fetch catch-all and an anonymous caller got a 500 that was also captured in Sentry, meaning anyone can generate Sentry noise and 5xx metrics./api/payload/:idalready shape-checks its id for the same reason; this PR follows that precedent. This retires the criterion-11 probe trigger used on 2026-09-30 (DEV-3093): that probe no longer produces a 500 or a Sentry event.What changed:
/api/versions/exists:vmust match^[0-9A-Za-z][0-9A-Za-z._+-]{0,63}$(semver / dist-tag shape, ASCII), otherwise 400{"error":"v is not a valid version"}before any KV access and with no Sentry capture. The 4xx contract is documented in the route's code comment only, because no doc underrunner/docslists this route's statuses. The client (checkVersionExists) already treats any non-ok response as "exists" (fails open), so a 400 changes nothing for it.getDemo(demo:<id>cache key): an id whose key is over 512 bytes or empty is a miss (null), so/d/:id,/embed/:id,GET /api/demos/:id,/api/demos/:id/source,/api/demos/:id/access,PATCH/DELETE /api/demos/:idand the MCP demo routes answer their normal 404 instead of throwing.serveDemoAsset: candidate R2 keys (r2_prefix+ URL subpath) over 1024 bytes are skipped, so/d/<real id>/<very long path>falls through to the SPA index exactly like any other unknown path instead of R2 throwing.session-tombstone:<id>and the meter key in KV, and the Durable Object name): over 128 bytes is a 400{"error":"invalid session id"}onPOST /api/session(client-suppliedsessionId),DELETE /api/session/:idand every/api/session/:id/*subroute. This one is worse than a 500: the tombstone and meter reads swallow the KV error (.catch(() => null)/.catch(() => "1")), so an overlong invented id was treated as a live metered session and waved through to a sandbox call for an attacker-chosen DO name.symbolicate.ts: a browser-supplied frame path that would make the sourcemap R2 key exceed 1024 bytes is not looked up (it was a countedfetch_errorskip, not a crash).workers/api/src/storage-key.tsmeasures UTF-8 bytes, not string length.Every place a KV key, R2 key or DO name is built from request-derived input (workers/api/src and workers/o11y/src):
version-exists:<v>KV (index.ts)?vdemo:<id>KV ingetDemo(share.ts)/d,/embed,/api/demos/:id[/source]demo:<id>KV,/api/demos/:id/access,PATCH/DELETE /api/demos/:id,/api/mcp/demos/:id[/status]getDemoguardr2_prefix + subpathinserveDemoAsset/d/:id/<subpath>session-tombstone:<id>, meter key,getSandbox(<id>)DO namePOST /api/sessionbodysessionId; path id on/api/session/:id[/*]payload:<id>KV/api/payload/:id^[a-z0-9]{1,32}$), leftchat-rl:<bucket>:<m/d>:<ip>:<time>KV (chat.ts)cf-connecting-ipanalytics:salt:<day>,versionscatalog, last-good latest, settings, budget state, watchdog stateKV_METER_PREFIX + sessionId(admin.ts)listresultdemos/<id>/...andavatars/<uuid>R2 putsshortId()/randomUUID()BuildJobDOidFromName(demoId)(snapshot-jobs.ts)getDemosucceededbuild--<shortId>getByName("main"/"box"), inboxidFromName("main")sourcemaps/<version>/<path>.mapR2 getO11Y_INBOX.get(key)in the drainproxyToSandboxNot fixed, noted for follow-up:
sessionIdcharacters are still not restricted to the preview-hostname grammar ([a-z0-9-]), only bounded in length.Types of changes
runner/) changeHow was this verified?
Nothing here was validated in production or against any production host; everything ran locally under
node --testagainst fakes. The newrunner/pipeline/storage-key-guards.test.mjs(13 tests) and one case ino11y-symbolicate.test.mjsdrive the real default export ofworkers/api/src/index.tswith a KV fake that throws on keys over 512 bytes (and an R2 fake that throws over 1024), like the real services. They assert: overlongvgives 400 with no KV access, no throw and no Sentry capture; a multibyte string under 512 characters but over 512 bytes is rejected (bytes, not characters); a validvstill asks npm once and is then served from KV; overlong demo ids give 404; an overlong/dsubpath does not reach R2; overlong session ids give 400 with no KV or sandbox access on create, delete and subroutes; the symbolicator does not request an overlong map key.Revert proof: with
index.ts,share.tsandsymbolicate.tschecked out from the base commit (git checkout 23ebd1881 -- <paths>, then restored), 10 of the 14 new tests fail (the baseversions/existscase fails with the real messageKV GET failed: 414 ... length of 615and status 500 instead of 400); the other 4 (unit test of the byte helpers, validv, emptyv, real demo still served) are behavior-preserving guards that pass before and after by design.pnpm --filter @handsontable/demo-runtime build: oknode --experimental-strip-types --test pipeline/*.test.mjs: 2589 pass, 0 failtsc --noEmitinworkers/apiandworkers/o11y: cleannpx wrangler deploy --dry-run --containers-rollout=noneinworkers/api: bundles (the plain dry run needs Docker, which was not available)Checklist
pnpm build(andpnpm dev) in the affected example/server-example locally: not applicable, no example changed; runner tests and typechecks run as listed aboveRelated issue(s):
Note
Medium Risk
Touches public session and version routes and fixes a session-id bypass that could reach sandbox RPCs; behavior for valid inputs is unchanged and guards only add early 4xx/404/miss paths.
Overview
Adds
storage-key.tshelpers that measure UTF-8 byte length (not string length) and validates request-derived identifiers before Workers KV, R2, or Durable Object names are built.API worker:
GET /api/versions/existsnow shape-checksvwithVERSION_QUERY_REand returns 400 before any KV read (fixes anonymous 500s and Sentry noise from overlong?v=). Demo lookups treat overlongdemo:<id>cache keys as a miss (404).serveDemoAssetdrops R2 candidate paths whose keys would exceed 1024 bytes. Tier-2 session routes reject ids over 128 bytes with 400 on create, delete, and all/api/session/:id/*subroutes—blocking a path where KV tombstone/meter errors were swallowed and could boot a sandbox for an attacker-chosen id.O11y:
symbolicate.tsskips sourcemap R2 lookups when a browser-supplied frame would produce an overlong map key.Tests: New
storage-key-guards.test.mjs(strict KV/R2 fakes) plus a symbolication case asserting no overlong map fetch.Reviewed by Cursor Bugbot for commit 45bee4b. Bugbot is set up for automated code reviews on this repo. Configure here.