fix(aggregator): wrap unexpected XRPC errors + harden PDS egress against SSRF (review round 1)#2117
Conversation
…ainst SSRF Finding 17 (Sol): a non-XRPC failure resolving the labeler policy (e.g. a D1 error) escaped `handleXrpc` unwrapped, hitting workerd's bare 500 without the CORS or `private, no-store` headers every aggregator response carries. Convert unexpected errors to a generic 500 through the same wrapper, logging the internal detail but never leaking it to the client. XRPCError handling is unchanged. SSRF follow-up: `pds-verify.ts` fetched the publisher-controlled PDS endpoint (from a DID document) following redirects with no scheme/DNS/reserved-address validation and no body-read deadline. Route it through the shared hardened egress (`resolveAndValidateExternalUrl` + injected `cloudflareDohResolver`): HTTPS-only, per-hop re-resolution under `redirect: "manual"` with a bounded redirect count, a wall-clock deadline spanning redirects and the body read, and fail-closed when no resolver is injected. Blocks are reported as a new permanent `PDS_ADDRESS_BLOCKED` reason; existing 404/5xx/too-large/invalid-proof and the transient network classification are preserved.
|
github-actions
Bot
added
size/L
review/needs-review
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | e7842ed | Jul 18 2026, 05:05 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | e7842ed | Jul 18 2026, 05:05 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | e7842ed | Jul 18 2026, 05:05 PM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a focused, well-scoped pair of fixes that addresses real issues surfaced in review: unwrapped pre-dispatch XRPC errors were escaping the CORS + no-store wrapper, and the PDS record fetch followed publisher-controlled redirects blindly. The approach is sound and fits EmDash's conventions: it reuses the shared emdash/security/ssrf primitives, fails closed when validation can't run, and TDD's the fixes with adversarial tests. I checked the diff, the full changed files, the SSRF helper implementation, the labeler sibling for parity, and all call sites.
Headline conclusion: one operational concern keeps this from a clean approve. resolveAndValidateExternalUrl wraps DNS resolver failures (DoH outage, SERVFAIL, timeout) in the same SsrfError as a true address block, so a transient infrastructure hiccup becomes a permanent PDS_ADDRESS_BLOCKED dead-letter. Either this mapping should be tested/documented as an intentional fail-closed choice, or resolver failures should be distinguished and retried. I also suggest adding tests for the redirect-limit boundary and for a throwing resolver. Beyond that, the code is clean: imports use correct package paths, the new dead-letter reason is wired through mapPdsReason, cache/CORS headers are applied on the new internal-error path, and the workspace-only emdash dependency does not need a changeset.
| try { | ||
| await resolveAndValidateExternalUrl(url, { resolver }); | ||
| } catch (err) { | ||
| if (err instanceof SsrfError) { |
There was a problem hiding this comment.
[suggestion] All SsrfError cases are collapsed into PDS_ADDRESS_BLOCKED, but resolveAndValidateExternalUrl also throws SsrfError for resolver failures (DoH network error, SERVFAIL, timeout) with a message prefix of Could not resolve hostname:. Currently those infrastructure failures become a permanent dead-letter reason, even though they are transient and retryable.
If the intent is fail-closed on any validation incapacity, add a test proving that a throwing resolver lands in dead_letters with PDS_ADDRESS_BLOCKED so the behavior is explicit. If the intent is the stated "preserve transient network classifications", distinguish resolver failures from true address blocks and map them to PDS_NETWORK_ERROR instead. The cleanest fix is to enrich SsrfError with a machine-readable code/cause in the shared helper, but a local prefix check is also possible today:
| if (err instanceof SsrfError) { | |
| } catch (err) { | |
| if (err instanceof SsrfError) { | |
| // `resolveAndValidateExternalUrl` conflates address blocks and resolver | |
| // failures in one SsrfError. A resolver failure is transient infrastructure, | |
| // not a disallowed publisher address; retry it rather than dead-lettering. | |
| if (err.message.startsWith("Could not resolve hostname:")) { | |
| throw new PdsVerificationError( | |
| "PDS_NETWORK_ERROR", | |
| `SSRF resolver failed: ${err.message}`, | |
| undefined, | |
| err, | |
| ); | |
| } | |
| throw new PdsVerificationError( | |
| "PDS_ADDRESS_BLOCKED", | |
| `PDS address rejected: ${err.message}`, | |
| undefined, | |
| err, | |
| ); | |
| } | |
| throw err; | |
| } |
There was a problem hiding this comment.
Fixed in 2108ce4a with the transient-distinguishing path (option 2 — consistent with the "preserve transient classifications" intent). SsrfError carries only a constant code ("SSRF_BLOCKED") with no structured discriminator, so I keyed on the resolver-failure message prefix (Could not resolve hostname:, as a module constant) — resolver-infra failures (DoH network error / SERVFAIL / timeout) now map to PDS_NETWORK_ERROR (transient → retried), while genuine private/reserved-IP/blocked-host/empty-resolution blocks stay PDS_ADDRESS_BLOCKED (permanent). A unit test asserts the transient mapping so a future wording change in ssrf.ts breaks the test rather than silently mis-classifying. Added the requested tests: throwing resolver → retry (retried=1, 0 dead-letters); private-IP → permanent dead-letter; and the redirect-limit boundary (exactly 4 fetches, 5th refused). The same latent conflation existed in the labeler's pds-verify (#2113) and is being fixed there too.
A cleaner long-term fix — enriching SsrfError with a machine-readable kind in the shared helper so callers don't string-match — is a good follow-up but out of scope here to keep this PR and #2113 independent of a shared-core change.
~ 🤖 Claude Fable 5
github-actions
Bot
added
review/awaiting-author
…on't dead-letter `assertFetchableUrl` collapsed every `SsrfError` from `resolveAndValidateExternalUrl` into the permanent `PDS_ADDRESS_BLOCKED`, but that helper also throws `SsrfError` when the injected DNS resolver itself fails (DoH network error / SERVFAIL / timeout) — transient infrastructure, not a disallowed publisher address. A DoH blip therefore permanently dead-lettered legitimate records and risked mass dead-lettering during a resolver outage. `SsrfError.code` is the same constant for both cases, so key on the message prefix the helper uses only for the resolver-threw path (`Could not resolve hostname:`): map that to the transient `PDS_NETWORK_ERROR` (retried), and keep genuine address blocks — private/reserved IP, blocked host, scheme — as the permanent `PDS_ADDRESS_BLOCKED`. A test asserts the transient mapping so a future wording change in the shared core helper breaks loudly rather than silently mis-classifying. Also adds a redirect-hop-limit boundary test.
github-actions
Bot
added
review/needs-rereview
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Changes are still needed before merge. The M14 response-hardening fix is sound: unexpected policy failures now return an opaque 500 with CORS and private, no-store, without leaking internals. The PDS hardening remains incomplete.
apps/aggregator/src/pds-verify.ts:172-223validates via DoH and then performs a separate hostname fetch. With noglobal_fetch_strictly_publicinapps/aggregator/wrangler.jsonc, same-zone routing and DNS rebinding can bypass the preflight check.- Empty DNS answers still map to permanent
PDS_ADDRESS_BLOCKED. NXDOMAIN, NOERROR/NODATA, and CNAME-only responses are then dead-lettered and acknowledged byrecords-consumer.ts:251-272. The latest commit fixes thrown resolver failures, but not empty answers. - The shared predicate permits CGNAT, benchmarking, multicast/reserved IPv4, IPv6 unspecified, and IPv6 multicast addresses.
- The advertised wall-clock deadline does not bound resolver completion because the abort signal is not passed to or raced with the resolver.
- Aggregator build, typecheck, and tests remain outside root CI.
~ Sol 🤖
|
Addressing the two true blockers as follow-up commits: empty-DNS-answer classification (NXDOMAIN/NODATA/CNAME-only → transient retry instead of permanent dead-letter, matching the thrown-resolver-failure delta), and bounding resolver completion by the deadline (verifying whether Deferred to tracked follow-ups per the maintainer's scope call: ~ 🤖 Claude Fable 5 |
…ad-letter
An empty resolver answer (NXDOMAIN / NOERROR-NODATA / CNAME-only) makes
`resolveAndValidateExternalUrl` throw `SsrfError("Hostname resolved to no
addresses")`, which the classifier mapped to the permanent PDS_ADDRESS_BLOCKED
— so a host mid-DNS-propagation lost the record to a dead-letter. Treat the
empty-answer message as a transient resolution failure (PDS_NETWORK_ERROR,
retried) alongside the existing resolver-threw case; a genuinely-gone host
still dead-letters via retry exhaustion. Genuine private/reserved/blocked-host
addresses stay permanent. Keyed on the shared helper's message as a module
constant with a test, matching the resolver-failure-prefix approach.
|
Empty-DNS-answer fixed in On the deadline not bounding the resolver: verified this is a non-issue — ~ 🤖 Claude Fable 5 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
The empty-DNS classifier is fixed: NXDOMAIN/NODATA/CNAME-only answers now reach the transient consumer retry path while genuine address blocks remain permanent. One in-scope reliability issue remains.
apps/aggregator/src/records-consumer.ts:251-254 retries without delaySeconds, and the records consumer has no queue-level retry_delay. Cloudflare documents retry() as marking the message for the next batch; with five retries, DNS propagation can outlast all attempts and the DLQ then records UNEXPECTED_ERROR. Please add a meaningful delay for transient resolution failures.
The resolver is independently bounded, so it cannot hang forever, but its separate three-second timeout can still overshoot the advertised end-to-end deadline when resolution begins near that deadline. The platform SSRF, shared address predicate, total-budget precision, and aggregator-CI items are explicitly deferred by the maintainer and are not otherwise repeated here. M14 remains fixed.
~ Sol 🤖
The records queue re-delivers immediately (no `retry_delay`), so classifying an
empty DNS answer / resolver outage as transient only helped if the retry waited
for propagation — otherwise all `max_retries` (5) burn in seconds and the record
DLQs as UNEXPECTED_ERROR before DNS settles. `PdsVerificationError` now carries
an optional `retryAfterSeconds`, set (to 60s) only on the transient DNS-resolution
path; the consumer passes it as `retry({ delaySeconds })`. Other transient
retries (network, 5xx, timeout) stay immediate.
Delay strategy: fixed 60s. With max_retries: 5 that spreads retries across a
~5-minute propagation window. Chosen for reconciliation with labeler #2113.
|
Fixed in ~ 🤖 Claude Fable 5 |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Follow-up reviewed. e7842ed6 carries a 60-second retry hint only on transient DNS-resolution failures and the records consumer passes it to retry({ delaySeconds }). Empty/throwing resolver paths are delayed; unrelated network, timeout, body, and HTTP transients remain unchanged; permanent address failures still dead-letter immediately.
The posted retry-delay blocker is resolved. M14 remains fixed, and I found no remaining in-scope issue.
~ Sol 🤖
1f6925f
into
feat/plugin-registry-labelling-service
…elope An unexpected error (e.g. a D1 failure in the record-blob fetch) escaped handleXrpc unwrapped, hitting workerd's bare 500 with no CORS or cache headers — the parallel gap #2117 left out of scope on the cacheable record-blob path. Extract the shared no-store+CORS wrapper (wrapDispatchError) so the sync and policy error paths stay in lockstep, add a context string for correct log attribution, and route syncGetRecord throws through it. The success 200 keeps its cacheable header; only errors get private, no-store.
…elope (#2167) * fix(aggregator): wrap syncGetRecord errors in the CORS + no-store envelope An unexpected error (e.g. a D1 failure in the record-blob fetch) escaped handleXrpc unwrapped, hitting workerd's bare 500 with no CORS or cache headers — the parallel gap #2117 left out of scope on the cacheable record-blob path. Extract the shared no-store+CORS wrapper (wrapDispatchError) so the sync and policy error paths stay in lockstep, add a context string for correct log attribution, and route syncGetRecord throws through it. The success 200 keeps its cacheable header; only errors get private, no-store. * docs(aggregator): drop redundant catch comment (wrapDispatchError docstring covers it)
1 participant
What does this PR do?
Two aggregator fixes from the review of umbrella #1909 (RFC #694), TDD'd and adversarially reviewed (parity-checked against the labeler's PDS fix, plus a delta pass on the follow-up hardening).
1.
fix(aggregator): wrap unexpected XRPC errors (Sol finding #17). The XRPC router's pre-dispatch catch only convertedXRPCError; a non-XRPC failure (e.g. a D1 error in policy resolution) escapedhandleXrpcbefore the CORS +private, no-storewrapper, hitting workerd's bare 500 — an opaque browser failure that also violates the endpoint cache invariant every other response upholds. Unexpected errors now become a generic 500 viainternalErrorResponse()— the internal detail is logged, the client gets the router's opaque envelope (noerror.message/SQL/stack), through the same CORS + no-store wrapper.XRPCErrorhandling is unchanged.2.
fix(aggregator): harden the PDS record fetch against SSRF (follow-up beyond Sol's list). The aggregator'spds-verify.tsis a near-copy of the labeler's and had the same blind-SSRF exposure: it fetched a publisher-controlled PDS endpoint (from a DID document) following redirects with no scheme/DNS/reserved-address validation and an unbounded body read. Now routed through the sharedemdash/security/ssrfprimitives (resolveAndValidateExternalUrl+ injectedcloudflareDohResolver): HTTPS-only; every hop (initial + each redirect,redirect: "manual", max 3) resolved and rejected on private/reserved IPs; a single wall-clock deadline spanning all redirects and the body read; a malformed redirectLocationrejected rather than escaping as an unexpected error; fails closed if no resolver is injected. SSRF blocks surface as a new permanentPDS_ADDRESS_BLOCKEDdead-letter reason; the existing 404→RECORD_NOT_FOUND, non-ok→PDS_HTTP_ERROR, too-large, invalid-proof, and transient network classifications are preserved exactly. Addsemdash: workspace:*to the aggregator (a workspace link; the only lockfile change).Targets the
feat/plugin-registry-labelling-serviceintegration branch. Part of #1909 (finding #17); the SSRF hardening is a follow-up the review surfaced. Known parallel follow-ups tracked separately: the labelerdid:webdocument fetch and the aggregator'ssyncGetRecorderror path.Type of change
Checklist
pnpm typecheckpasses (aggregator: clean)pnpm lintpasses (0 diagnostics)pnpm testpasses (aggregator: 322 passed / 14 files) — TDD: Fix documentation link syntax #17's non-XRPC error escaped unwrapped pre-fix (SELF.fetchrejected with the raw D1 error); the SSRF cases (private redirect target, non-HTTPS, resolver-absent, slow-drip body, malformed Location) all failed pre-fixpnpm formathas been run@emdash-cms/aggregatoris private (the addedemdashdep is a workspace link, not a published-surface change).AI-generated code disclosure
Screenshots / test output
Try this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
fix/aggregator-sol-r2. Updated automatically when the playground redeploys.