You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
security: bound scan fan-out with a shared DNS budget + overall deadline
Orchestrator-level DoS umbrella for GHSA-f828-8wf8-vqp2. scan()/scanStreaming()
previously awaited Promise.all across all analyzers with only a per-query 3s DNS
timeout — no aggregate cap on total DNS queries or wall-clock. A single request
combining a large DKIM selector list with a rua/ruf-stuffed _dmarc record on an
attacker-controlled domain could drive a large outbound-DNS burst and long
wall-clock on one rate-limit token.
Adds two backstops, both threaded through every analyzer:
- ScanBudget (src/dns/scan-budget.ts): one shared per-scan DNS-query pool that
every queryTxt/queryMx/queryDoh draws from; exhaustion throws a DnsLookupError
subclass so analyzers degrade to "could not verify" instead of crashing.
- Overall deadline: one AbortController + setTimeout; each settled analyzer is
raced against it, degrading to its synthetic fallback on a breach. The budget
also holds the signal, so no new query is issued past the deadline.
Defaults (DEFAULT_SCAN_LIMITS): 150 queries / 12s — generous for real
multi-analyzer scans, overridable via an optional `limits` param (tests).
Preserves the #378 per-analyzer settle contract and scanStreaming SSE semantics
(every protocol still streams exactly once). DnsLookupError moved to
src/dns/errors.ts so the budget can subclass it without depending on the DNS
client (which tests mock).
Refs GHSA-f828-8wf8-vqp2. Umbrella over the per-analyzer caps in #539.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: schmug <38227427+schmug@users.noreply.github.com>
-`src/dns/client.ts` — DNS abstraction over node:dns (NXDOMAIN returns null)
29
29
-`src/analyzers/` — One module per protocol (dmarc, spf, dkim, bimi, mta-sts, mx, security-txt)
30
-
-`src/orchestrator.ts` — Runs all analyzers in parallel and isolates each one: a single analyzer rejection surfaces as a synthetic `status: "fail"` result (with an `analyzer_error` validation; `lookup_error.code` on the types that carry it) instead of aborting the whole scan. Implemented via a per-analyzer `settle` wrapper so one failure never takes down its siblings, for both `scan` and `scanStreaming` (#378).
30
+
- `src/orchestrator.ts` — Runs all analyzers in parallel and isolates each one: a single analyzer rejection surfaces as a synthetic `status: "fail"` result (with an `analyzer_error` validation; `lookup_error.code` on the types that carry it) instead of aborting the whole scan. Implemented via a per-analyzer `settle` wrapper so one failure never takes down its siblings, for both `scan` and `scanStreaming` (#378). Both entrypoints also enforce two DoS backstops (GHSA-f828-8wf8-vqp2): a single overall deadline (one `AbortController` + `setTimeout`; each settled analyzer is raced against it via `raceDeadline`, degrading to its synthetic fallback on a breach) and one shared per-scan DNS-query budget (`ScanBudget`, threaded into every analyzer's DNS calls), so neither total outbound queries nor wall-clock can scale with attacker input. Limits come from `src/dns/scan-budget.ts` (`DEFAULT_SCAN_LIMITS`); both are overridable via the optional `limits` parameter (used by tests). A breach degrades gracefully — partial results with a note, never a throw — and `scanStreaming` still streams every protocol exactly once.
31
+
-`src/dns/scan-budget.ts` — `ScanBudget` (shared per-scan DNS-query pool + deadline-`AbortSignal`) and `DEFAULT_SCAN_LIMITS` (`maxDnsQueries`, `deadlineMs`). `queryTxt`/`queryMx`/`queryDoh` call `budget?.consume()` before any outbound query; exhaustion/deadline throw `ScanBudgetError`/`ScanDeadlineError` (both subclass `DnsLookupError`, so analyzers surface them as "could not verify" rather than a false "not configured"). `DnsLookupError` itself lives in `src/dns/errors.ts` (re-exported from `client.ts`) so the budget can subclass it without depending on the DNS client, which tests frequently mock.
31
32
-`src/shared/scoring.ts` — Grade computation (F if no DMARC or p=none). Knobs are configurable per-deploy via the `SCORING_CONFIG` env var (`src/shared/scoring-config.ts` parses/validates it; absent/invalid → shipped defaults, so hosted dmarc.mx is unaffected). `computeGrade`/`computeGradeBreakdown` take an optional `Partial<ScoringConfig>`; `scan`/`scanStreaming` require it (compile-time enforcement that every call site threads the active config)
32
33
-`src/shared/learn-anchors.ts` — Single source of truth for validation→learn-page "How to fix" deep links (#524): analyzers set the optional `Validation.learnAnchor`, `src/views/learn.ts` renders the matching `id=` attributes from the same constants, and `test/learn-anchors.test.ts` asserts both sides so anchor ids can't drift
33
34
-`src/cache.ts` — SSE result caching
@@ -85,6 +86,7 @@ Live at dmarc.mx | Repo: github.com/schmug/dmarcheck
85
86
- **Branch protection:** `main` is governed by the `main-protection` repository ruleset: requires a PR, requires the `check` status check, and blocks deletions and non-fast-forward pushes. CodeQL (`Analyze (actions)`, `Analyze (javascript-typescript)`) still runs on every PR but must **NOT** be re-added as a required status check — requiring those contexts deadlocked all merges (the "ruleset merge trap"); they were deliberately removed. `required_approving_review_count` is **0** by design — autonomous Claude Code routines (see the `claude-routines` repo) open and auto-merge PRs unattended. The human-review gate is **path-scoped**, not blanket: `require_code_owner_review` is on, so any PR touching a path in `.github/CODEOWNERS` (CI, lockfiles, security invariants, input validation, redirect posture, rate limiting, DB migrations, analyzer modules, orchestration, scoring) requires a code-owner approval before merge. This hybrid keeps routine PRs autonomous while forcing a human on the security-sensitive minority. **CODEOWNERS scope decision (issue #300):** `src/analyzers/**`, `src/orchestrator.ts`, and `src/shared/scoring.ts` are intentionally gated — a malicious-issue-driven PR adding a new analyzer or modifying orchestration/scoring could exfiltrate DNS data or manipulate grades without this gate. Enforcement is only live once the bot-identity split (#299) lands (routines currently run as admin, which bypasses CODEOWNERS). The repo Admin role bypasses the ruleset (`bypass_actors`, mode `always`) — **the autonomous bot must run as a distinct non-admin identity, never the admin account**, or the ruleset is advisory for it and CODEOWNERS cannot be satisfied (no self-approval).
86
87
-**Secret scanning:** Secret scanning, push protection, non-provider patterns, and validity checks are all enabled in repo settings. Never commit `.env`, tokens, or wrangler secrets.
87
88
-**Input validation:** User-supplied domains are restricted to `[a-z0-9.-]` in `normalizeDomain` (`src/index.ts`). DKIM selectors are restricted to `[A-Za-z0-9._-]` in `parseSelectors`. HTML output never interpolates raw user input into inline `<script>` blocks — use `data-*` attributes via `esc()` instead.
89
+
-**Scan fan-out cap (DoS, GHSA-f828-8wf8-vqp2):** the orchestrator bounds every scan with one overall deadline AND one shared per-scan DNS-query budget (`ScanBudget` from `src/dns/scan-budget.ts`, threaded through every analyzer into `queryTxt`/`queryMx`/`queryDoh`). This is the umbrella over the per-analyzer caps (DKIM selectors, MTA-STS body, DMARC rua/ruf): combining a large selector list with a rua/ruf-stuffed `_dmarc` record on an attacker-controlled domain cannot drive total outbound DNS or wall-clock past the limits, all on one rate-limit token. Do NOT remove the budget threading or the deadline race when editing the orchestrator or analyzers; keep DNS query calls drawing from the shared pool. A breach degrades gracefully (partial results + a note), never throws.
88
90
-**MTA-STS fetch redirect mode:**`src/analyzers/mta-sts.ts` uses `redirect: "manual"` for the policy fetch. Do NOT change it to `"error"` — that throws in the Cloudflare Workers fetch runtime and breaks every scan (regressed twice via PRs #58 and #92). `"manual"` is RFC 8461 §3.3-compliant: redirects yield an opaque-redirect `Response` rejected by the existing `resp.type === "opaqueredirect"` / `!resp.ok` guards.
89
91
-**security.txt fetch redirect mode:**`src/analyzers/security-txt.ts` deliberately uses `redirect: "follow"` (not `"manual"`) — RFC 9116 §3 does not forbid following redirects, and real-world deployments commonly redirect (e.g. gov.uk → www.gov.uk → vdp.cabinetoffice.gov.uk). MTA-STS's `manual` posture is a security requirement of RFC 8461 §3.3 specifically; security.txt has no equivalent rule, so the user-friendly choice is to follow.
90
92
-**DCO sign-off & squash-merge default:**`.github/workflows/dco.yml` enforces a `Signed-off-by:` trailer on every non-bot PR commit (OSPS LE-01.01). The commit that lands on `main` keeps those trailers because the repo's squash default is `squash_merge_commit_message = COMMIT_MESSAGES` (GitHub pre-fills the squash body with the concatenated commit messages). **Do not change that setting to `PR_BODY`** — it would move the sign-off requirement to the PR description and reopen the gap. Bots are identified by `[bot]` in their author name, the `github-actions` author name, or the `cursoragent@cursor.com` author email (Cursor's cloud agent authors as "Cursor Agent" with no `[bot]` suffix); all other commits — including those using GitHub's privacy noreply email (`12345678+alice@users.noreply.github.com`) — are subject to DCO enforcement (#434).
// External lookup required — enforce the shared cap before querying.
70
-
if(budget.remaining<=0){
71
-
if(!budget.capReported){
76
+
// External lookup required — enforce the per-analyzer cap before querying.
77
+
if(authBudget.remaining<=0){
78
+
if(!authBudget.capReported){
72
79
validations.push({
73
80
status: "warn",
74
81
message: `More than ${MAX_REPORT_AUTH_LOOKUPS} external report destinations configured (rua/ruf) — additional destinations were not verified for report authorization`,
0 commit comments