fix: close remaining Workers best-practices audit findings (#474) - #477
Merged
Conversation
Covers all 5 HIGH and 17 MEDIUM findings from the Workers-best-practices audit as 22 independent, per-package tasks.
Verified package.json directly instead of leaving it as a check for the task implementer.
davidwkeith
enabled auto-merge (squash)
July 31, 2026 01:18
davidwkeith
commented
Jul 31, 2026
davidwkeith
left a comment
davidwkeith
left a comment
Owner
Author
There was a problem hiding this comment.
Reviewed the diff (79 files, 33 commits) plus process conformance against CONTRIBUTING.md. Overall this is well-scoped per-package work with good hygiene: every affected publishable package has a matching .changeset/*.md, every changed package has a colocated test update, commit messages are clean Conventional Commits, and none of the cross-standard libs (@dwk/rdf, @dwk/dpop, @dwk/oauth, @dwk/log, @dwk/ldn, @dwk/http-signatures, @dwk/safe-fetch, @dwk/store, @dwk/wac, @dwk/mf2) or @dwk/deno-host were touched, so the Cloudflare-confinement rule holds. Left three inline comments on things worth fixing or double-checking before merge, plus one nit. Two process items below.
Correctness (inline comments on the code):
packages/atproto-pds/src/object.ts(#importRepo, mirrored from the pre-existing#uploadBlob): the declared-Content-Lengthsize check only protects against an honest header —request.arrayBuffer()unconditionally buffers the full body before the post-hoc size check runs, so the DoS this audit finding is meant to close isn't actually closed for a client that omits or understatesContent-Length.packages/solid-oidc/src/body.ts'sreadRequestBodyCappedin this same PR is the correct pattern (a real streaming cap) — worth applying to both#importRepoand#uploadBlob.packages/solid-pod/src/pod.ts(#broadcaston the Delete path, both the direct handler and its WebDAV-batch twin): the new WAC-filter runs after the resource is already deleted from the store, so a resource-specific ACL grant may no longer be resolvable at notification time. Not obviously wrong, but untested — the three new WAC-broadcast tests only cover Create/PUT.packages/webauthn/src/cbor.ts:MAX_DEPTH = 32combined withdepth > MAX_DEPTHactually permits 33 levels of nesting, not 32 — pure off-by-one, not exploitable, but worth matching the stated constant.
(I checked the timingSafeEqualHex length-mismatch rejection in packages/mastodon-api/src/encoding.ts that an automated pass flagged as contradicting its own doc comment — it doesn't; the doc comment and the later "reject malformed hex before timing-safe comparison" commit both correctly explain why rejecting a malformed-format length mismatch up front doesn't reintroduce the timing leak the constant-time comparison exists to prevent. No issue there.)
Process/CONTRIBUTING.md conformance:
- PR title:
fix: close remaining Workers best-practices audit findings (#474)omits a scope. PerCONTRIBUTING.md§6, scope is "omitted only for a repo-wide change" — this PR lists 13 specific non-private packages in "Packages affected," so it isn't repo-wide in that sense, even though in spirit it's one coordinated audit-closure effort. Given this repo squash-merges (the PR title becomes the permanentgit logentry), worth a deliberate call on whether a scope list is warranted here or whether "repo-wide" is being read to include "touches most of the repo for one coordinated reason" — either is defensible, just flagging the letter of the rule. - Body/checklist otherwise fully conforms to
PULL_REQUEST_TEMPLATE.md(headings intact, the one inapplicable checklist item left unchecked with a reason rather than deleted). spec/packages/solid-pod.mdwas updated for the WAC-broadcast behavior change, but a couple of other behavior changes in this PR (micropub'swaitUntil-backgrounded syndication, solid-oidc's new token-body size cap) don't have correspondingspec/packages/*.mdupdates. These may be intentionally-internal robustness details rather than protocol-contract changes, but worth a one-line confirmation that's a deliberate call rather than an oversight, per the "spec is the requirement" ground rule.
Generated by Claude Code
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…filter Add a granted non-owner subscriber (and an ungranted authenticated one) so the `authorize()` call inside `#allowedToRead` is actually exercised — the owner case short-circuits on the owner bypass and the anonymous case on default-deny, so neither caught an implementation that denied every non-owner outright. Also document the consequence for browser subscribers: the `WebSocket` API cannot set `Authorization`, so browser-originated subscriptions are anonymous and now see only publicly-readable changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Add missing withSession method to wrapped D1Database in test - Run prettier to fix line length violations Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#processVerifications and #resolveInbox parsed remote actor/verification documents via response.json() directly, buffering the full body regardless of a lying or missing Content-Length. Route both through readBodyCapped(response, ACTOR_PROFILE_MAX_BODY_BYTES) before JSON.parse, matching the capped-read discipline the rest of object.ts already follows.
Reject an oversized importRepo CAR by its declared Content-Length before resolving the source signing key or buffering the body, mirroring the existing #uploadBlob size check so a hostile Content-Length cannot push the DO past its memory ceiling or trigger a wasted DID-resolution fetch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The deploy-to-cloudflare handler mounts both @dwk/webfinger and @dwk/host-meta, so env must satisfy both Env type fragments simultaneously (intersection, not union). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
crypto.subtle.timingSafeEqual is a real but Cloudflare-Workers-proprietary SubtleCrypto extension; it does not exist on Node. Four packages (indieauth, webauthn, mastodon-api, conformance-target) call it for constant-time comparisons, so every one of those code paths threw a TypeError when composed into @dwk/server (the Node self-hosting host) — invisible until now because the checked-in dist/ was stale. Add installTimingSafeEqual, an idempotent pure-JS polyfill matching installCryptoDigestStream's pattern, and wire it into @dwk/server's startup alongside the other runtime-global installers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every sibling package's log-event taxonomy is a const object + derived union type, not a TypeScript enum (spec/observability.md says "exported as a constant"). Convert SolidOidcLogEvent to match, and re-export it from index.ts alongside every other endpoint package's taxonomy so a composing app can reference the stable event names. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
hexToBytes silently dropped a trailing odd hex character and coerced
invalid hex characters to 0, so timingSafeEqualHex("abcd", "abcde") and
timingSafeEqualHex("zz", "zy") both incorrectly returned true. Reject
malformed input (odd length, non-hex characters, or a length mismatch)
up front — a malformed format is not a secret, so this doesn't
reintroduce a timing leak.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The fail-closed catch (Task 8) called config.logger.error directly, skipping the emit() helper that the fail-open branch uses to also call config.metrics.count. An operator counting micropub.media.metadata_failed therefore only saw non-fatal occurrences and missed every case where the upload was actually rolled back. Widen emit() to accept "error" and use it at the fail-closed call site for logger/metrics parity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…he Node WebSocket shim
Task 4 (earlier in this branch) made solid-pod's WebSocket notification
broadcast WAC-filter per subscriber by calling
`server.serializeAttachment({ agent })` right after
`ctx.acceptWebSocket(server)`, and later `ws.deserializeAttachment()`
in `#broadcast`. That's correct on workerd, which implements both
methods as part of the Hibernation API, but `EmulatedWebSocket` (the
Node self-hosting shim `@dwk/server` installs) had neither, so the
call threw `TypeError: server.serializeAttachment is not a function`
on every WebSocket upgrade — surfacing as an unhandled 500 through
solid-pod's unguarded `fetch()` and the front door's unguarded DO
`fetch()` call.
Add both methods backed by a plain in-memory field: this shim never
actually hibernates, so there's nothing to persist across a restart —
`serializeAttachment` sets the field, `deserializeAttachment` reads it
back (`null` if never set).
Also fixes `@dwk/server`'s "delivers change notifications over a
WebSocket subscription" integration test, which was hanging even once
this shim was fixed: it opened the WebSocket unauthenticated, so
`#broadcast`'s (correct, intentional) WAC filter never allowed the
notification through for the private `/doc` resource. Authenticate the
subscription as the owner via the test's `x-test-as` hook, matching
`@dwk/solid-pod`'s own WAC-filtering test coverage.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
davidwkeith
force-pushed
the
claude/issue-474-3fcfa6
branch
from
July 31, 2026 01:35
1fff1b5 to
c894a19
Compare
# Conflicts: # packages/mastodon-api/src/handler.ts
davidwkeith
force-pushed
the
claude/issue-474-3fcfa6
branch
from
July 31, 2026 01:37
c894a19 to
64a94a2
Compare
…ering first The declared-Content-Length pre-check on #uploadBlob and #importRepo only helped when the header was present and truthful; a request omitting or understating Content-Length still buffered the whole body via request.arrayBuffer() before the post-hoc size check ran, so an unauthenticated-shaped client could still push the DO past its 128 MB memory ceiling. Add a local readRequestBodyCapped helper (body.ts) that reads the body incrementally and cancels the reader the instant the running total exceeds the cap, wired into both call sites. #importRepo's signing-key resolution now runs after the capped read (previously before) so an oversized/hostile body never triggers a network DID-resolution fetch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`depth > Reader.MAX_DEPTH` let a 33-level-deep CBOR structure decode even though MAX_DEPTH is 32 and the doc comment promises a 32-level maximum. Switch to `depth >= MAX_DEPTH` so the enforced boundary matches what's documented, and add tests pinning the exact accepted/rejected boundary (the existing 40-deep test only exercised well past either boundary). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… loop readRequestBodyCapped's tail merge loop held the whole `chunks` array live while allocating and filling `merged`, so peak memory during a successful capped read was ~2x the body size instead of ~1x (a regression versus the old `new Uint8Array(await request.arrayBuffer())`, which was a view, not a copy). Drop each chunk's reference the moment it's copied into `merged` so it becomes collectible immediately, bringing peak back to ~1x. Added a multi-chunk reassembly test (distinct per-chunk byte patterns) to prove the chunk-release loop still reassembles bytes correctly and in order. Also softened the doc comments (body.ts, the #uploadBlob/#importRepo call sites, and the changeset) that implied this fix alone makes the DO immune to its 128 MB ceiling — it doesn't. `maxImportCarSizeBytes` still defaults to 128 MiB, the same order of magnitude as the ceiling itself, so a fully honest, in-cap import at the default remains a real memory risk; this fix only closes the "buffer first, check later" gap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… JSON body reads Five call sites still buffered the whole body via a bare `request.json()` with no size cap at all: `#createSession`, `#updateHandle`, `#createRecord`, `#putRecord`, `#deleteRecord`. Four run `#requireAuth` first, but `#createSession` is the login endpoint — reachable with no authentication whatsoever — making it a strictly broader DoS vector than the `#uploadBlob`/`#importRepo` gap fixed in the previous patch. Add `readJsonBodyCapped` (body.ts): reads the body via the existing incremental `readRequestBodyCapped`, then `JSON.parse`s it. A malformed body still throws the same native SyntaxError as before (surfacing as the existing generic 500 InternalServerError) — only the size-cap behavior is new. Add config.maxJsonBodyBytes (default 2 MiB, well below maxBlobSizeBytes/maxImportCarSizeBytes since these are small structured-data endpoints, not bulk-transfer ones) and wire it into all five call sites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…k-then-merge The previous fix (50b204e) tried to bound peak memory by dropping each chunk's reference right after copying it into a `merged` buffer, but that doesn't work: `new Uint8Array(total)` is one big upfront allocation, so at the moment it runs every chunk in `chunks` is still live — peak is ~2x regardless of when the chunks are released afterward, since nothing is actually collectible until a GC runs, and no allocation happens during the release loop to trigger one. Rewritten to allocate a single buffer sized to `maxBytes` (the cap, known upfront) before reading anything, and write each chunk directly into it at the running offset — checking the cap before each write instead of after. No `chunks` array, no separate merge step. Returns a zero-copy `subarray` trimmed to the bytes actually received. This gives a genuine, provable single-allocation bound (~1x the cap, not the body) instead of the chunks-plus-copy shape that always cost ~2x. Trade-off called out in the comments/changeset: the pre-allocated buffer costs `maxBytes` momentarily on every call regardless of how much data actually arrives (e.g. a 10-byte body against a 2 MiB cap still allocates 2 MiB up front). That's the deliberate cost of a provable bound over an accumulate-and-copy approach, and normal for the cap sizes in this file (2 MiB JSON, 5-128 MiB blobs/CARs) well under the 128 MB isolate limit on its own. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Round 2 (pre-allocating a Uint8Array sized to maxBytes on every call) traded
round 1's problem (accumulate-then-copy briefly costing ~2x the actual body
size) for a worse one: ~1x the FULL CONFIGURED CAP, unconditionally, on
every single call regardless of how little data actually arrived. Since
#importRepo's default cap equals the DO's memory ceiling (128 MiB), every
importRepo call — even for a 1-byte body — momentarily allocated 128 MiB.
It also made createSession (unauthenticated) and uploadBlob cost their full
cap's worth of allocation from a request that sends almost nothing, a
bandwidth-free DoS amplification that didn't exist before either fix.
Replaced with a resizable ArrayBuffer (`new ArrayBuffer(0, { maxByteLength:
maxBytes })`) plus a length-tracking Uint8Array view. Each chunk grows the
backing buffer by exactly its own size via `resize()` right before being
written in, so memory committed tracks the bytes actually received, not the
cap — while resize() is never asked to grow past maxBytes (checked, and the
reader cancelled, before each resize), so the cap is still a hard ceiling.
The length-tracking view already has exactly the right length once the loop
ends, so it's returned directly — no chunks array, no second "merged"
allocation, no subarray trim. This is genuinely better than both prior
versions: memory proportional to actual bytes received, with a provable cap
that can never be exceeded.
Requires this package's tsconfig.json/tsconfig.build.json to override `lib`
to include ES2024.ArrayBuffer (the shared tsconfig.base.json pins ES2022,
which predates resizable ArrayBuffer); TypeScript's `lib` array replaces
rather than merges with the parent, so both configs repeat the full array.
Confirmed working under the real vitest-pool-workers/workerd runtime (all
169 @dwk/atproto-pds tests pass), not just Node.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…o-pds's resizable buffer Both packages path-map @dwk/atproto-pds straight to source for pre-build typechecking, which pulls body.ts's resizable ArrayBuffer usage into their type-check program too. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…tion Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 4, 2026
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.
Summary
Closes all 5 HIGH and 17 MEDIUM findings from issue #474 (a Workers-best-practices audit across every mountable worker). LOW severity findings are left for a follow-up.
Highlights:
ctx.waitUntilinstead of blocking the create-post response; a D1 failure no longer leaks its raw error message to the client.CodeStoreconstruction per isolate, and wired the injected logger/metrics seam at security-relevant rejection points.crypto.subtle.timingSafeEqual, using Cloudflare's documented never-short-circuit-on-length-mismatch pattern.crypto.randomUUID()instead ofMath.random()for channel ids.readBodyCapped.Logger); atproto-pds also caps migration CAR buffering and surfaces unhandled XRPC errors.Envtype that only checked one of the two mounted packages' binding requirements.Also fixed, found during final-review verification (not in the original audit):
crypto.subtle.timingSafeEqualis a Cloudflare Workers-only API. Four of the above fixes adopted it without a fallback, which broke@dwk/server(the Node self-hosting host) — every IndieAuth-authenticated request, every WebAuthn ceremony, and Mastodon client auth would throw on Node. Added a polyfill to@dwk/cf-shimsand wired it into@dwk/server. Discovering this also surfaced a second regression: the solid-pod WebSocket fix above callsserializeAttachment, which the Node WebSocket shim didn't implement — added it, verified with the previously-broken@dwk/serverintegration test now passing.Packages affected
@dwk/webauthn, @dwk/solid-pod, @dwk/mastodon-api, @dwk/micropub, @dwk/solid-oidc, @dwk/indieauth, @dwk/microsub, @dwk/activitypub, @dwk/vc, @dwk/webdav, @dwk/remotestorage, @dwk/atproto-pds, @dwk/conformance-target (private, no changeset), @dwk/cf-shims, examples/deploy-to-cloudflare (private, no changeset)
Checklist
spec/packages/and updated them if behaviour changed (spec/packages/solid-pod.mdupdated for the new per-subscriber WebSocket filtering behavior)src/*.test.ts)pnpm lint && pnpm format:check && pnpm typecheck && pnpm build && pnpm test(full repo-wide run: 254 test files / 3103 tests passing)pnpm changeset) if this touches a publishable packagecatalog.json/conformance/status.json— not applicable, no new mountable worker or conformance-status change🤖 Generated with Claude Code