security: move DNSBL DQS key off the DoH query string

[schmug]

Summary

• queryDnsbl (src/dns/client.ts) issued its Spamhaus DQS lookup as a DoH GET, putting the query name — which embeds the secret DQS key — in the request URL's query string. Workers Traces records outbound fetch spans' url.full/url.query with no scrubbing hook, so enabling the DNSBL analyzer and Workers Traces together would leak the key into every captured span.
• Switched to an RFC 8484 DoH POST with an RFC 1035 wire-format query body: the request URL is now the bare https://cloudflare-dns.com/dns-query endpoint with no query string at all, and the query name travels in the POST body instead, which traces do not record.
• Update (2nd commit, Replace #732's hand-rolled DNS wire codec with the vetted dns-packet #736): the initial commit added a small hand-rolled wire-format encoder/decoder. Per owner review, replaced it with the vetted dns-packet library (runtime dependency) — no hand-rolled binary DNS parsing remains. queryDnsbl's external behavior, request URL/method/body shape, and every existing test assertion are unchanged. queryDoh is untouched either way, per the issue's scope: its names are user-supplied domains, not secrets.

Owner / zone steps

None — this is a code-only transport change. DNSBL_DQS_KEY remains unset in production per the issue's "Current state (not exploitable)" note.

Security notes

• Closes the gap described in DNSBL DQS key travels in a DoH URL query string, which Workers Traces captures #728: the DQS key can no longer land in a URL that any URL-logging layer (Workers Traces, proxies, etc.) might capture.
• Existing redaction behavior is unchanged: breadcrumbs still log only <reversed-ip>.<key>.<zone>, and thrown errors stay generic (defense-in-depth even though the URL itself is now static and carries nothing sensitive).
• DNSBL remains informational-only — src/shared/scoring.ts is untouched — and MAX_DNSBL_HOSTNAMES/MAX_DNSBL_IPS caps plus ScanBudget threading in src/analyzers/dnsbl.ts are untouched.
• dns-packet.decode() throws on a malformed/truncated buffer; that's caught and surfaces as the same generic DnsLookupError as any other failure — never an unhandled throw and never a false "not listed". dns-packet's types don't distinguish A records from other "string data" record types, so queryDnsbl's isARecordAnswer guard re-validates data is actually a string at runtime.
• This PR touches CLAUDE.md and package.json/package-lock.json, all CODEOWNERS-gated paths — not auto-merging, opening for human review per repo policy.

Testing

• npm test — full suite: 1553 passing, 1 failing (pre-existing, unrelated — test/integration/mta-sts-runtime.test.ts is a live-network integration test that fails identically on unmodified main in this environment). Coverage-gated node re-run: 1518 passing, 0 failing, per-file floors on src/analyzers/** and src/shared/scoring.ts unaffected (neither file was touched).
• npm run typecheck — clean, no output.
• npm run lint — Checked 191 files in 373ms. No fixes applied.
• test/dns-client.test.ts's queryDnsbl tests (added in the first commit) pass unmodified against the dns-packet-based implementation — including the assertion that the request URL contains neither the key nor the reversed IP.
• New test/integration/dnsbl-wire-codec.test.ts runs in the workers vitest project (real workerd, not Node) and proves the dns-packet encode/decode round-trip for the exact query shape queryDnsbl uses, plus that a malformed buffer throws rather than silently returning garbage.

Choices made

• The original issue (DNSBL DQS key travels in a DoH URL query string, which Workers Traces captures #728) stated the "do not set this secret while traces are on" warnings already landed via feat: make the nightly cron observable (Workers Logs/Traces + awaited scheduled handler) #724, and asked for them to be removed in this PR. feat: make the nightly cron observable (Workers Logs/Traces + awaited scheduled handler) #724 is still an open, unmerged PR — those warnings don't exist yet in wrangler.toml/client.ts/CLAUDE.md, so there is nothing to remove. Updated the CLAUDE.md DNSBL bullet to describe the new transport instead.
• Landed Replace #732's hand-rolled DNS wire codec with the vetted dns-packet #736's library swap on this same branch/PR rather than a third PR, per the owner's explicit suggestion in review.

Deferred

• Removing feat: make the nightly cron observable (Workers Logs/Traces + awaited scheduled handler) #724's (not-yet-existent) "traces + DNSBL key" warnings — not actionable until feat: make the nightly cron observable (Workers Logs/Traces + awaited scheduled handler) #724 merges.
• Turning the DNSBL analyzer on in production — explicitly out of scope.

Refs

Closes #728
Closes #736

🤖 Generated with Claude Code

https://claude.ai/code/session_0165np2hMSKNJBp3a2coEmm1

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
✅ Deployment successful! View logs	dmarcheck	5765aeb	Sep 12 2026, 05:15 PM

[schmug]

Auto-merge routine: escalating to human review — not merging.

Two independent reasons, either alone sufficient to block auto-merge:

1. Risk-path touch. This PR modifies CLAUDE.md, which .github/CODEOWNERS explicitly lists as @schmug-owned/security-sensitive. The PR's own description also flags this ("touches a CODEOWNERS-gated path — not auto-merging"). Per the routine's risk-path check, any CODEOWNERS-gated path touch escalates regardless of scan results.
2. Required tooling unavailable. This routine's mandatory review + vulnerability scan step (security-diff-scan, pr-review-fanout) and merge-gate step (merge-pr-with-gate, routine-anti-noise) are shipofclaudius plugin skills that are not installed/enabled in this session — confirmed via both the enabled-skills listing and a full catalog search. Without them, the routine cannot confirm the diff passes the required scan, so per policy ("merge nothing that fails review/scan") it does not merge.

Note for a human reviewer: this touches src/dns/client.ts (DNSBL DoH transport — moves the Spamhaus DQS key from a GET query string into a POST body, per #728) and test/dns-client.test.ts, in addition to the CLAUDE.md doc update. Worth a manual look at the new wire-format encoder/decoder (encodeDnsQueryA/decodeDnsResponseA) for correctness, since it's hand-rolled DNS message parsing.

Separately: the missing shipofclaudius plugin skills mean this auto-merge routine currently cannot evaluate any PR's review/scan step, not just this one — worth checking why they aren't installed for this account.

---

Generated by Claude Code

[schmug]

Held pending #736, by owner decision on 2026-09-12.

The transport change here is accepted — moving the DQS key out of the URL into an RFC 8484 POST body is the right fix for #728. What is not accepted is the hand-rolled RFC 1035 codec (encodeDnsQueryA, decodeDnsResponseA, skipDnsName), per the standing guardrail to add a vetted library rather than hand-roll.

I checked viability rather than assuming it. Both candidates encode and decode this exact query shape inside workerd (the workers vitest project, not the Node pool): dns-packet@5.6.1 (MIT, one dependency, Buffer-based and fine because nodejs_compat is already on) and @leichtgewicht/dns-packet@6.0.3 (MIT, Buffer-free but six dependencies). dns-packet is the recommendation. The probe passed 2/2 and has been removed; it was scratch, not committed.

To be fair to this PR: no defect was found in the hand-rolled code. The call was made on the guardrail, and it trades ~60 scoped lines for a runtime dependency.

#736 carries the full swap as an implementable prompt, including the constraint that this PR's test/dns-client.test.ts assertions must keep passing unmodified. Suggested path is to land the replacement on this branch rather than open a third PR.

Not merging this, and not closing it.

[schmug]

Pushed the swap requested in review (and detailed in #736): encodeDnsQueryA, decodeDnsResponseA, and skipDnsName are gone, queryDnsbl now uses dns-packet (5.6.1, runtime dependency) for both encode and decode.

• test/dns-client.test.ts's queryDnsbl tests pass unmodified against the new implementation, including the URL/key assertion.
• Added test/integration/dnsbl-wire-codec.test.ts, which runs in the workers vitest project (real workerd) and proves the encode/decode round-trip for the exact query shape, plus that a malformed buffer throws rather than returning garbage.
• npm test (1553/1554 passing — the 1 failure is the pre-existing, unrelated live-network MTA-STS integration test that also fails on unmodified main in this environment), npm run lint, and npm run typecheck all green.

This PR now touches CLAUDE.md and package.json/package-lock.json (CODEOWNERS-gated), so it stays open for your review rather than auto-merging.

---

Generated by Claude Code

---

Generated by Claude Code

[schmug]

#736 is implemented in #737, which merges this PR's commit (c085b10, unmodified) on top of current main and replaces the hand-rolled RFC 1035 codec with dns-packet on top of it — same transport fix, same tests, plus the library swap.

I didn't push the replacement directly onto this branch as suggested in the comment above: this session's designated working branch is fixed to claude/relaxed-cray-79uw6y (not claude/relaxed-cray-vctqnc), so a new PR was the available path instead. Net effect is the same — one PR carries both the transport fix and the codec swap — just via #737 rather than more commits here.

Recommend closing this PR once #737 merges. Leaving that to you rather than doing it myself, since you'd previously asked for it to stay open pending #736.

---

Generated by Claude Code