feat(server): wire-input hardening — shape guards, inbound limits, sanitized execute errors (ADR-0012)#9
Open
grrowl wants to merge 4 commits into
Conversation
Per ADR-0012 (to be written): - wellFormed() shape-guard runs after decode, before any SQL binding so no arbitrary decoded value reaches lookupTx or sql.exec. Drops + logs bad frames (mirrors the undecodable-frame stance). - maxFrameBytes (1 MiB) checked before decode; mirrors Cloudflare's own cap and makes the bound testable/overrideable. - maxOpsPerMutation (128): reject-don't-truncate at the top of handleMut; a partial apply would silently drop client writes. - maxSubsPerSocket (256): cap checked in handleSub; re-subs (same subId) replace rather than count against the cap (SubscriptionRegistry.add semantics). Over-limit → reset + console.error. - SubscriptionRegistry.countFor(ws) accessor added for cap enforcement. - Execute errors sanitized: full detail logged server-side, generic "mutation failed"/"command failed" + EXECUTE_FAILED sent to client. Authorize errors in handleMut kept user-facing unchanged (README API). - plan-001 error-paths test updated: command boom test now asserts the generic message/code and verifies the detail does NOT leak. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds tests/wire-hardening.test.ts pinning the ADR-0012 invariants: 1. Malformed frame shapes are silently dropped; socket survives. 2. ops over maxOpsPerMutation → LIMIT_EXCEEDED, nothing applied. 3. Subscription cap: third sub on 2-cap DO gets reset; first two subs continue receiving deltas. 4. Oversized frame (>1 MiB) dropped before decode; socket survives (workerd passed the frame to webSocketMessage — the DO's own maxFrameBytes guard fired at 1,048,666 bytes). 5. Execute error → generic "mutation failed"/EXECUTE_FAILED; SQLite constraint detail does not leak; authorize error passes through. Also fixes the wellFormed shape guard to treat null == absent for optional fields (the client transport sends null, not omit, for absent fields in MessagePack serialisation — previous commit missed this and broke all existing sub-bearing tests on the full suite). LimitsTestDO (maxOpsPerMutation=2, maxSubsPerSocket=2) added to tests/test-worker.ts, wired in vitest.config.ts and tests/env.d.ts, following the MaintTestDO/SlowTickDO pattern exactly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Records decisions from plan-005: shape-guard drop policy (drop+log, no
reply — mirrors undecodable bytes), the three limits and their
defaults/override pattern, execute-error sanitization with authorize
exempt (mut path), and the explicit deferral of dedup identity binding.
Also applies one finding from the codex adversarial review (gpt-5.5):
Finding 3 — APPLIED: JSON.stringify(decoded) in the wellFormed drop
path could throw on bigint values (MessagePack useBigInt64 may decode
bigints). Fixed by using a replacer that stringifies bigints as strings,
with a fallback to String(decoded) on any other error. The drop+log
behaviour is now crash-safe for all decoded values.
Rebuttals for findings NOT applied:
Finding 1 — NOT a gap: replay stores the SANITIZED message ("mutation
failed") because rejectTx writes the generic string to recordTx before
the dedup record is written. Old records (pre-upgrade) with SQLite detail
would only replay if they exist at deploy time; that is a deploy-time
concern, not a code gap.
Finding 2 — BY DESIGN: "no mutation handler for '...:...'" is the
authorize-path catch, kept user-facing per the plan (README: "throw to
deny"). Exposing the collection:type name is consistent with the library's
author-configuring-the-DO model; the attacker is already authed. Noted
in ADR for future review.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Test 4 ("oversized frame is dropped") previously only filtered for
`rejected` frames referencing txId "wh-big1", which passes even when
the guard is disabled (the DO processes the payload and emits
`committed`, not `rejected`).
Two stronger assertions added:
- `expect(droppedFrames).toHaveLength(0)` — this socket has no
subscriptions so ANY frame arriving signals the guard failed;
a `committed` frame now causes the test to fail correctly.
- `runInDurableObject` DB check: `SELECT COUNT(*) FROM messages
WHERE id='big'` must be 0 — the strongest signal the write
was never applied.
Verified falsifiability: temporarily setting `if (false && byteLen >
this.maxFrameBytes)` causes the test to fail with
"expected [{ t: 'committed' }] to have length 0"; restoring the guard
makes it pass. Full suite: 183 tests, 43 files, all pass.
Co-Authored-By: Claude Fable 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
1 participant
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.
Executes plan 005 from the advisor audit. Stacked on #4 (plan 001) — it updates three of that PR's test assertions in the same commit as the sanitization change.
What
wellFormed) after decode, before any SQL binding — malformed frames are dropped + logged (bigint-safe logging), socket stays alive. Optional fields treatnull == absent(the client transport serialises omitted fields as MessagePack null — documented deviation, accepted on merit).protected readonlytunables:maxOpsPerMutation=128(reject-don't-truncate,LIMIT_EXCEEDED),maxSubsPerSocket=256(refused withreset; an existing subId is exempt so resubscribe-after-reconnect can't starve),maxFrameBytes=1MiB(drop + log pre-decode)."mutation failed"/"command failed"+EXECUTE_FAILED.authorizethrows stay user-facing by design (README: "throw to deny").Review notes
One revision round: the reviewer falsifiability-tested the oversized-frame test (disabled the guard → test failed → restored), forcing it to actually bind to the guard. Known leftover, deliberately out of scope: the
NON_SERIALIZABLEreject path still carries the codec error message.🤖 Generated with Claude Code