Skip to content

fix: reject unrecognized DMARC p= and sp= policy values - #740

Open
schmug wants to merge 1 commit into
mainfrom
claude/issue-738-dmarc-policy-validation
Open

schmug wants to merge 1 commit into
mainfrom
claude/issue-738-dmarc-policy-validation

Conversation

@schmug

@schmug schmug commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Summary

  • analyzeDmarc's p= if/else-if chain had no final else: an unrecognized value (e.g. the real-world typo p=Quarntine) pushed no validation at all, so the record could report overall "pass" and grading fell into scoring.ts's "shouldn't normally reach here" fallback arm, crediting the domain with quarantine-level (C tier) enforcement it does not have.
  • The sp= branch had the same gap but a worse failure mode: it pushed a pass/"Subdomain policy explicitly set" for any non-empty value, so sp=Rejectt reported as correctly configured.
  • Added an explicit fail branch to both the p= and sp= checks for any value outside reject / quarantine / none (RFC 7489 §6.3), citing the unrecognized value in the message.

Owner / zone steps

None — code-only change, no provisioning required.

Security notes

Not security-sensitive in the auth/injection/exfiltration sense, but this PR touches src/analyzers/dmarc.ts, which is CODEOWNERS-gated (@schmug) as part of the analyzer/orchestration/scoring supply-chain surface. Auto-merge is intentionally NOT enabled on this PR — it needs human code-owner review per the CODEOWNERS path-scoping in the repo's operating rules.

Testing

  • npm test — 1521/1521 passing in the coverage-gated node project (vitest run --project=node --coverage), no per-file coverage threshold regressions (dmarc.ts 96.96% lines / 91.35% branches, scoring.ts unaffected). The one failure surfaced by the full npm test chain (test/integration/mta-sts-runtime.test.ts, a real-network fetch to mta-sts.dmarc.mx) is a pre-existing, network-dependent failure reproduced identically on unmodified main in this sandbox (no outbound network to the live domain here) — unrelated to this change.
  • npm run typecheck — clean.
  • npm run lint — clean (191 files checked, no issues).
  • New regression tests in test/dmarc.test.ts use the issue's literal fixtures (p=Quarntine, sp=Rejectt) plus two tests confirming p=REJECT (case-insensitive) and sp=none still behave exactly as before.
  • New regression test in test/scoring.test.ts confirms a DmarcResult with status: "fail" and an unparseable p= tag grades F and never produces tierReason: "Fallback — quarantine-level enforcement".

Choices made

  • Chose fail (not warn) for the unrecognized sp= case, matching the p= handling for consistency — the issue's Acceptance criteria permitted either, and Acceptance mandated fail unconditionally for p=.
  • No changes to src/shared/scoring.ts: making analyzeDmarc emit status: "fail" for these records is sufficient — it now hits the existing gatekeeper (dmarc.status === "fail" → grade F) before the fallback arm at line 281 is ever reached, per the issue's constraint to "fix only the path this bug opens."
  • Did not add a learnAnchor to the new validations — no existing /learn/dmarc anchor covers "unrecognized policy value," and the issue's pointers only expected a learnAnchor for p=none, which already has one.

Deferred

  • Out of scope per the issue: re-grading historical scan rows scored under the old behavior, other DMARC tags (pct/rua/ruf/adkim/aspf), and the wider "is the fallback arm reachable at all" question in scoring.ts.

Refs

Closes #738

🤖 Generated with Claude Code

https://claude.ai/code/session_01THpUFWMdjGGDJinJx6GzHc


Generated by Claude Code

analyzeDmarc's p= if/else-if chain had no final else, so an
unrecognized policy value (e.g. a typo like p=Quarntine) pushed no
validation at all and the record could report overall "pass". Grading
then fell into scoring.ts's "shouldn't normally reach here" fallback
arm and credited the domain with quarantine-level (C tier)
enforcement it does not have.

The sp= branch had the same gap but worse: it pushed a "pass" for any
non-empty value, so sp=Rejectt reported as correctly configured.

Add an explicit fail branch to both the p= and sp= checks for values
outside reject/quarantine/none (RFC 7489 §6.3). This makes
analyzeDmarc return status "fail" for these records, which already
routes them through scoring.ts's existing gatekeeper (dmarc.status ===
"fail" -> grade F) before the fallback arm is ever reached — no
change to scoring.ts itself.

Closes #738

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01THpUFWMdjGGDJinJx6GzHc
@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  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 f6c8efa Sep 14 2026, 12:17 PM

schmug commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Auto-merge routine: escalating to human review.

This PR modifies src/analyzers/dmarc.ts, which matches this repo's risk-path denylist (src/analyzers/**) for the auto-merge routine. Per the routine's rules, any risk-path touch escalates to needs-you and is never auto-merged, regardless of review/scan outcome.

Separately, noting for transparency: the routine's configured review/scan tooling (routine-anti-noise, security-diff-scan, pr-review-fanout, merge-pr-with-gate skills/plugins) was not available in this session, so no automated review or scan was run on this PR. The risk-path escalation above would apply either way.

This PR needs a human code-owner review before merging.


Generated by Claude Code

@schmug schmug added the needs-you label Sep 14, 2026 — with Claude
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DMARC p= and sp= accept unrecognized values without any validation

2 participants