Skip to content

chore(dmarc): address Grok review feedback on PR #3878#3886

Merged
aterga merged 1 commit into
feat/dmarc-alignmentfrom
claude/address-grok-suggestions-9S5mB
May 12, 2026
Merged

chore(dmarc): address Grok review feedback on PR #3878#3886
aterga merged 1 commit into
feat/dmarc-alignmentfrom
claude/address-grok-suggestions-9S5mB

Conversation

@aterga
Copy link
Copy Markdown
Collaborator

@aterga aterga commented May 12, 2026

Addresses the two actionable "easy-to-change improvements" from Grok's review on #3878 (#3878 (comment)). Targets feat/dmarc-alignment so it can land as a follow-up commit on that PR.

(Tried to push directly to feat/dmarc-alignment per the maintainer's note from #3877 / #3885, but my account got a 403 against that branch — falling back to a stacked draft PR.)

What changed

  1. dmarc/mod.rs — consolidated # Security model docblock. Calls out the three deliberate deviations from "stock" DMARC, each with a one-paragraph rationale:

    • No Public Suffix List — label-anchored suffix check prevents evilexample.com-style spoofs at the cost of closing multi-domain orgs (gmail.comgooglemail.com). Safe direction for a recovery surface. Design doc §6.4.
    • No SPF — recovery proves mailbox control, not path-of-delivery. DKIM gives the cryptographic binding; SPF would need a source IP the gateway payload doesn't carry. Design doc §6.5.
    • Fail-closed everywhere — every malformed/unknown step collapses to Unverified. No quarantine, no downgrade.
  2. dmarc/test_vectors.rsverifies_end_to_end_when_dmarc_record_has_unknown_tags. Runs the full verify_email against a DMARC record carrying rua=, ruf=, fo=, and a synthetic vendorext= tag. Pins the ignore-unknown-tags behaviour through the public entry point, not just the parser unit (which already has a unknown_tags_are_ignored test in dmarc/parse.rs).

Not actioned

The four strategic recommendations (observability metrics, frontend error surface, SPF wiring, README) are explicitly "nice-to-have" / "future" and belong in later PRs of the stack.

Tests

  • cargo test -p internet_identity --bin internet_identity dmarc:: — 49 pass (was 48, +1 e2e unknown-tag test).
  • cargo clippy -p internet_identity --bin internet_identity --tests -- -D warnings — clean.
  • cargo check -p internet_identity --target wasm32-unknown-unknown — clean.
  • cargo fmt --check on the two touched files — clean (other pre-existing fmt drifts in dnssec/, openid/, tests/integration/, types/attributes.rs are unrelated and predate this branch).

Generated by Claude Code

Acts on the two actionable "easy-to-change improvements" from Grok's
review on #3878:

- dmarc/mod.rs: consolidated security-model docblock under a `# Security
  model` heading, explicitly calling out (a) no PSL — label-anchored
  suffix check prevents `evilexample.com`-style spoofs at the cost of
  closing multi-domain orgs, (b) no SPF — DKIM gives the cryptographic
  binding we need for a mailbox-control proof, no source-IP context to
  check anyway, and (c) fail-closed: every malformed/unknown step
  collapses to Unverified.
- dmarc/test_vectors.rs: new end-to-end fixture-driven test
  `verifies_end_to_end_when_dmarc_record_has_unknown_tags` that runs the
  full `verify_email` against a DMARC record carrying `rua=`, `ruf=`,
  `fo=`, and a synthetic vendor-extension tag — pins the
  ignore-unknown-tags behaviour through the public entry point, not just
  the parser unit (which already had a `unknown_tags_are_ignored`
  test).

Grok's first item ("parser must ignore unknown tags") is already
implemented and unit-tested — `dmarc/parse.rs` uses lookup-by-name, so
unknown tags pass through silently. The new e2e test simply pins it.

Tests: 49 dmarc tests pass (+1). Clippy clean. wasm32 builds clean.
@aterga aterga marked this pull request as ready for review May 12, 2026 15:23
@aterga aterga requested a review from a team as a code owner May 12, 2026 15:23
@aterga aterga requested a review from Copilot May 12, 2026 15:23
@aterga aterga merged commit 271edb2 into feat/dmarc-alignment May 12, 2026
39 checks passed
@aterga aterga deleted the claude/address-grok-suggestions-9S5mB branch May 12, 2026 15:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Follow-up to the DMARC alignment work to incorporate prior review feedback by improving security documentation and strengthening end-to-end test coverage around DMARC tag parsing behavior.

Changes:

  • Consolidates and expands the DMARC module’s security-model documentation (PSL/SPF/fail-closed rationale).
  • Adds an end-to-end test ensuring verify_email succeeds when the DMARC record contains unknown/reporting tags (ignored per RFC 7489).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/internet_identity/src/dmarc/mod.rs Adds a consolidated “Security model” module doc section explaining intentional deviations from stock DMARC.
src/internet_identity/src/dmarc/test_vectors.rs Adds an end-to-end test covering “ignore unknown DMARC tags” behavior through the public verifier entry point.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +192 to +193
// extension we've never heard of, and a duplicate-looking-but-
// different reporting tag — verification must still succeed.
sea-snake pushed a commit that referenced this pull request May 13, 2026
Addresses the two actionable "easy-to-change improvements" from Grok's
review on #3878
(#3878 (comment)).
Targets `feat/dmarc-alignment` so it can land as a follow-up commit on
that PR.

(Tried to push directly to `feat/dmarc-alignment` per the maintainer's
note from #3877 / #3885, but my account got a 403 against that branch —
falling back to a stacked draft PR.)

## What changed

1. **`dmarc/mod.rs` — consolidated `# Security model` docblock.** Calls
out the three deliberate deviations from "stock" DMARC, each with a
one-paragraph rationale:
- **No Public Suffix List** — label-anchored suffix check prevents
`evilexample.com`-style spoofs at the cost of closing multi-domain orgs
(`gmail.com` ↔ `googlemail.com`). Safe direction for a recovery surface.
Design doc §6.4.
- **No SPF** — recovery proves mailbox control, not path-of-delivery.
DKIM gives the cryptographic binding; SPF would need a source IP the
gateway payload doesn't carry. Design doc §6.5.
- **Fail-closed everywhere** — every malformed/unknown step collapses to
`Unverified`. No quarantine, no downgrade.

2. **`dmarc/test_vectors.rs` —
`verifies_end_to_end_when_dmarc_record_has_unknown_tags`.** Runs the
full `verify_email` against a DMARC record carrying `rua=`, `ruf=`,
`fo=`, and a synthetic `vendorext=` tag. Pins the ignore-unknown-tags
behaviour through the public entry point, not just the parser unit
(which already has a `unknown_tags_are_ignored` test in
`dmarc/parse.rs`).

## Not actioned

- **Grok item #1 ("parser must ignore unknown tags").** Already
implemented and unit-tested before this PR. `dmarc/parse.rs` is
lookup-by-name, so unknown tags pass through silently. The PR
description even calls this out: "ignores unknown / reporting tags". The
new end-to-end test is the explicit confirmation Grok asked for under
item #3.

The four strategic recommendations (observability metrics, frontend
error surface, SPF wiring, README) are explicitly "nice-to-have" /
"future" and belong in later PRs of the stack.

## Tests

- `cargo test -p internet_identity --bin internet_identity dmarc::` — 49
pass (was 48, +1 e2e unknown-tag test).
- `cargo clippy -p internet_identity --bin internet_identity --tests --
-D warnings` — clean.
- `cargo check -p internet_identity --target wasm32-unknown-unknown` —
clean.
- `cargo fmt --check` on the two touched files — clean (other
pre-existing fmt drifts in `dnssec/`, `openid/`, `tests/integration/`,
`types/attributes.rs` are unrelated and predate this branch).


---
_Generated by [Claude
Code](https://claude.ai/code/session_01NZmvbHgzN5NQc7Hqx9ZzpP)_

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants