Skip to content

fix(import): trust a DMARC pass only from the pinned authserv-id - #248

Merged
AnnatarHe merged 2 commits into
mainfrom
claude/blissful-franklin-shhvxl
Oct 12, 2026
Merged

AnnatarHe merged 2 commits into
mainfrom
claude/blissful-franklin-shhvxl

Conversation

@AnnatarHe

Copy link
Copy Markdown
Member

Summary

This follows up clippingkk/web#247, which was merged just before this commit landed. It addresses the fifth automated review on that PR.

Sender authentication

  • A dmarc=pass now counts only when the server named by EMAIL_IMPORT_AUTHSERV_ID stamped it. Anyone can write an Authentication-Results header claiming a pass.
    • Results from any other server can still make the verdict stricter, but never grant it.
    • A pass from an unpinned server reads as unpinned.
  • Until EMAIL_IMPORT_AUTHSERV_ID is set, nothing imports. The log says which variable to set. EMAIL_IMPORT_ALLOW_MISSING_DMARC=1 is the explicit opt-out; it now also covers unpinned passes, with a WARN for each email.
  • Repeated headers joined by a comma are split apart, so a sender's evil.example; dmarc=pass can't borrow the receiver's authserv-id.
  • Headers folded across lines are unfolded first, so a genuine folded pass isn't missed.

Binding codes

  • The 3-an-hour limit per address is now per reader and address. A looser shared limit of 10 an hour per address still caps what several accounts can send to one inbox. One account asking for someone else's address therefore can't lock its owner out of verifying.

Smaller

  • A failed Wenqu search is logged as a WARN.
  • Attachment download links must be https.
  • import.ts is split: saving and book matching move to save.ts, and the logging helpers to log.ts.
  • Docs: EMAIL_IMPORT_AUTHSERV_ID is marked required, with the new code-send limits and a note that each new code resets the attempt count.

Deploy notes

Set EMAIL_IMPORT_AUTHSERV_ID before expecting imports to work.

  1. Send yourself a notebook export.
  2. Open its headers in the Resend dashboard.
  3. Use the first word of the Authentication-Results value.

If there is no such header, only EMAIL_IMPORT_ALLOW_MISSING_DMARC=1 lets imports through; see docs/email-import.md for the risk.

Test plan

  • pnpm test: 86 files, 661 tests. New cases:
    • an unpinned pass is refused by default, and the log names the env var;
    • a comma-joined pass doesn't borrow the receiver's authserv-id;
    • a folded header still reads as a pass;
    • one reader's code sends don't block the address owner.
  • pnpm lint, pnpm format:check, pnpm typecheck, pnpm typecheck:server

🤖 Generated with Claude Code

https://claude.ai/code/session_01CCcqmwTy6GqiC7gN4HCjMa


Generated by Claude Code

Fifth round of review feedback on the email import.

- A dmarc=pass now counts only when the server named by
  EMAIL_IMPORT_AUTHSERV_ID stamped it; anyone can write a header
  claiming one. Other servers' results can still make the verdict
  stricter. Until the id is set, every pass reads as `unpinned` and
  nothing imports (EMAIL_IMPORT_ALLOW_MISSING_DMARC=1 is the explicit
  opt-out, warned on per email). Comma-joined repeated headers no longer
  borrow the receiver's authserv-id, and folded headers are unfolded.
- The tight per-address limit on binding codes is now per reader and
  address (3 an hour), with a looser shared one (10 an hour), so one
  account can't lock an address's owner out of verifying.
- A failed Wenqu search is logged; attachment links must be https.
- The importer is split: saving and book matching move to save.ts and
  the logging helpers to log.ts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CCcqmwTy6GqiC7gN4HCjMa
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@claude

claude Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Review

Solid hardening pass overall. Pinning the DMARC pass to the receiver's authserv-id is the right fix, and the comma and line-fold handling is careful. The split of import.ts into save.ts and log.ts reads well. I only read the code and didn't run the tests.

Security

  1. A forged header using the receiver's own authserv-id is still trusted. dmarcResult trusts any dmarc=pass whose id matches EMAIL_IMPORT_AUTHSERV_ID, and it can't tell who wrote that header. A sender can add 'Authentication-Results: mx.resend.com; dmarc=pass' themselves.
    • The 'any non-pass wins' rule covers this only if Resend also stamps its own dmarc= result. If Resend adds none (for example, the From domain has no policy), the forged header is the only result and is trusted.
    • RFC 8601 section 5 says receivers should strip incoming headers claiming their own authserv-id. Please confirm Resend does this and note it in docs/email-import.md.
    • If you can't confirm it, trust only the first (topmost) header, since the receiver prepends its own. Joined values would need to keep their order.
  2. Rate-limit counters aren't atomic. In bindings.ts, Promise.all increments all three counters even when one denies. A blocked request still uses up the per-reader and shared budgets, so a few accounts hitting their own limits could use up the shared 10-an-hour address counter. Not dangerous, but checking sequentially (or counting only once all three pass) would be tidier.
  3. The https check is narrow. It is correct, and new URL() throwing on an invalid URL is fine as a job error. You could also check that the host belongs to Resend, to limit SSRF if the signed webhook payload were ever spoofed. Low risk.

Quality

  • stripCommentsAndQuotes loops until nothing changes, which is quadratic on deeply nested comments. Header size is bounded, so this is acceptable, but a depth cap would be cheap insurance.
  • DmarcResult gained an 'unpinned' member. Please check that every switch or message map handles it.
  • Defaulting to 'nothing imports until the env var is set' is the safe choice, and the deploy note is clear. A one-time startup WARN when the variable is unset would be more discoverable than a per-email log.

Tests
The new cases (unpinned, comma-joined, folded, reader-scoped limits) are good. I'd add:

  • a forged same-id header next to a genuine dmarc=fail, and next to no genuine result (this pins down point 1 either way);
  • authserv-ids that differ in case or have a trailing dot (mx.resend.com.);
  • an EMAIL_IMPORT_ALLOW_MISSING_DMARC=1 test that covers 'unpinned';
  • a test that the shared per-address limit still trips across different readers.

Point 1 is the main thing to settle before merging. The rest are small.

@AnnatarHe
AnnatarHe merged commit e7bdf1a into main Oct 12, 2026
2 checks passed
@AnnatarHe
AnnatarHe deleted the claude/blissful-franklin-shhvxl branch October 12, 2026 10:22
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.

2 participants