feat(api): report what each domain still needs to receive mail - #247
Open
Waiel5 wants to merge 3 commits into
Open
feat(api): report what each domain still needs to receive mail#247Waiel5 wants to merge 3 commits into
Waiel5 wants to merge 3 commits into
Conversation
saasmail has no concept of a domain. Inboxes are keyed by full address —
`sender_identities.email`, `emails.recipient` — so an operator who has
just pointed a new domain at their deployment has nothing to look at, and
when mail bounces there is no page that says why. The answer is almost
always a DNS record that was never added, and finding that out currently
means leaving the product for `dig`.
`GET /api/domains` derives the list from the same universe the inbox list
uses: DISTINCT `emails.recipient` UNION `sender_identities.email`, split
at the `@` and lower-cased so `Sales@Acme.com` and `sales@acme.com` are
one inbox on one domain. Domains are implied by the addresses that exist,
so there is no table and no migration to drift out of sync with them.
Each row carries the inbox count, the received-message count, and the DNS
status.
The live check is the part worth building. Workers have no raw DNS, but
they can reach Cloudflare's DoH resolver over HTTPS, which is enough to
read MX and TXT and say whether the domain routes to Cloudflare
(`*.mx.cloudflare.net` — the per-zone hostnames vary, so the suffix is
the test, not `route1`), somewhere else, or nowhere, and whether SPF
exists and includes `_spf.mx.cloudflare.net`. What is still missing comes
back as `{name, type, value}` records rather than prose, so the worker
ships data and the client renders and copies it.
Every failure path answers `unknown` rather than a verdict: a thrown
request, a non-JSON body, and any DNS status that is neither NOERROR nor
NXDOMAIN. Telling an operator their DNS is wrong when the resolver simply
did not answer sends them to edit records that were already correct,
which is worse than admitting the check did not happen. `unknown` also
emits no missing records, since there is nothing to base them on. MX and
TXT resolve independently, so one failing does not cloud the other.
Answers are held in `caches.default` for 60 seconds. A dashboard refresh
reuses them instead of re-querying, and a record the operator has just
fixed still appears within the minute. That cache outlives the per-test
database reset, so each test owns distinct domains.
Admin-only, guarded the same way suppressions and webhook config are: it
reads deployment-wide configuration rather than anyone's mail.
`oauth-scope-policy.ts` does not exist on this base, so there is no rule
to add here. When this meets `feat/oauth-bearer-api` the route needs
classifying as `admin` scope with `requiresAdminRole` — unmatched routes
are denied to bearer callers, so without it the endpoint is unreachable
over OAuth.
Four defects in the domain DNS check, in descending order of how much damage they do to someone who acts on the output. The SPF advice was dangerous. When a domain already published an SPF record that did not include Cloudflare, `missingRecords` told the operator to add `v=spf1 include:_spf.mx.cloudflare.net ~all` as a new TXT record. Only one SPF record per name is legal — a name with two is a `permerror`, which fails the check the operator was trying to pass and takes their existing outbound mail down with it. An existing record is now merged instead: the include is spliced in ahead of the `all` mechanism and the record comes back with `action: "replace"`, so a client can say "replace this" rather than "add this". The qualifier is left exactly as written, since rewriting someone's `-all` to `~all` quietly weakens a policy they chose. A record with no `all` mechanism gets the include appended; one that already carries it produces no entry at all. That last case now holds for the right reason. The include test matched a bare substring, so `include:_spf.mx.cloudflare.net.example.com` read as ours and the domain showed green. It matches whole terms now, with any qualifier stripped, and the same predicate decides both the status and whether a merge is needed — they cannot disagree. DKIM was not checked at all. Email Routing needs a third record beyond MX and SPF, a TXT at `cf2024-1._domainkey.<domain>` (its own selector, separate from Email Sending's `cf-bounce._domainkey`), so a domain Cloudflare considers unconfigured reported a clean bill of health. `dkim` joins `routing` and `spf` with the same four states, resolved independently. The value is deliberately `null`: Cloudflare generates that key per zone, so nothing here can know it, and a guessed DKIM key is worse than an absent one — it publishes a public key that matches no private key and turns every signed message into a verification failure. The record ships as name + type with a `note` telling the operator to copy the value from Email Routing → Settings. A TXT sitting on that name that is not a `v=DKIM1` record, or a `v=DKIM1` record whose `p=` is empty (DKIM's revocation signal — present, verifying nothing), reads as `elsewhere` rather than configured. Reassembling TXT values became load-bearing with DKIM in the mix. The splitter stripped quotes but left the space between them, so a value the resolver returned as several strings came back with spaces injected at the chunk boundaries. Nothing shorter than 255 characters noticed; a 2048-bit DKIM key is always split. Chunks now join with no separator, which is what the wire format means. A rejected body read defeated the module's own promise. `cached.text()` and `res.text()` sat outside the try/catch that produces "unknown", so a body that failed after its headers had arrived rejected `lookupDomainDns`, which took down the `Promise.all` in the router and 500'd the entire endpoint — every domain lost because one zone's response was cut short. Both reads, and the cache calls around them, are inside the catch now. And a comment claimed `missingRecords` was "Empty while anything is unknown", which the code never did: the lookups resolve independently, so a domain can be routing `none` with spf `unknown` and still carry the three MX records. The behaviour is right — a failed lookup should not suppress what the other two learned — so the claim is corrected rather than the code, here and in the CHANGELOG, and a test pins the case that disproved it.
`GET /api/domains` has been serving the native app since the DNS check landed, and the web dashboard — the surface the person who deployed this actually opens — had no page for it. The owner went looking for domain management on their own site and found nothing. This is parity, not a new feature. The page shows, per domain, whether mail can arrive at all, how many inboxes and messages sit under it, and the records still needed, each value copyable through the existing `CodeBlock` helper rather than a second clipboard implementation. Everything on it is rendered exactly as the worker sends it, which is the whole trick. `action: "replace"` means edit the record already at that name rather than adding a second, and an SPF entry arrives pre-merged; the native screen originally recomputed that merge itself and, because DKIM is also TXT, wrote the merged SPF string into the DKIM row. So `record.value` goes out verbatim and the client decides nothing. A null value happens only on DKIM, whose key Cloudflare generates per zone: that row shows `record.note` and offers no copy button for a value that does not exist. `unknown` is "the lookup did not finish", never a verdict about the zone, so it reads "Couldn't check" in a neutral tone and never in red — and an empty `missingRecords` is not reported as an all-clear while any of routing/spf/dkim is still unknown, which is the one case where saying nothing would be read as saying everything is fine. The Cloudflare section states the part that cost this deployment hours. DNS only gets mail as far as Cloudflare; Email Routing then needs its catch-all rule Active with the action "Send to a Worker" pointed at this deployment's Worker, and until it is, every message bounces 550 5.1.1 however correct the records above look. That step is also the one that cannot be automated away: wrangler will not set a Worker as the catch-all action, so it can only be done in the dashboard. Both links go through `dash.cloudflare.com/?to=`, whose `:account` and `:zone` placeholders Cloudflare resolves for whoever is signed in, so no ids of ours are baked into the page. They are the same URLs the native screen uses. Reachability is the defect this exists to fix, so the route and the nav entry are gated the way the existing admin surfaces are: `/admin/domains` alongside `/admin/users` and `/admin/suppressions`, the entry inside the `isAdmin` block of both the desktop dropdown and the mobile menu, and the page redirecting a non-admin to `/` the way InboxesPage does. The fetch is skipped for non-admins as well — the endpoint is admin-only behind `requireAdmin`, so a member landing there would only collect a 403.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Independent of my other PRs — branches from
main.Why
saasmail has no concept of a domain: inboxes are keyed by full address. So an operator who has just pointed a new domain at their deployment has no way to see whether it actually works, and nothing tells them what is wrong when mail bounces.
That is not hypothetical — on my own instance Email Routing was enabled and
ready, the MX records were correct, and every message still bounced550 5.1.1because the catch-all rule wasdisabled, action: drop. Nothing in saasmail could have told me that.What
GET /api/domains, admin-only. Domains are derived from the addresses that already exist —DISTINCT emails.recipientUNIONsender_identities.email, split at the@— so there is no new table and no migration. Per domain: inbox count, message count, and a live DNS status.The DNS part is the useful half. A Worker cannot do raw DNS, but it can query Cloudflare's DoH resolver, so MX, SPF and DKIM are each resolved independently and reported as
cloudflare/elsewhere/none/unknown, with the exact records still missing.Two things I want to draw attention to because getting them wrong is worse than not shipping the feature:
SPF is merged, never appended. Only one SPF record per name is legal, so a domain already carrying
v=spf1 include:_spf.google.com ~allgets back that record with the Cloudflare include spliced in before the terminalall, markedaction: "replace". An earlier revision emitted the canonical standalone value, which an operator following it literally would have published as a second record — breaking their outbound deliverability. Cloudflare's own docs say the same: "If you have existing SPF records, merge them".DKIM's value is
null, deliberately. The selectorcf2024-1._domainkeyis documented and checked, but thep=key is generated per zone, so the response reports the record as required with a null value and a note telling the operator to copy it from the dashboard. A wrong DKIM value is worse than an absent one.unknownis never a verdict about the zone. A resolver timeout, a non-2xx, an unparseable body and SERVFAIL all read as "could not check", and NXDOMAIN reads as "no record" — those are different answers and are not conflated. The three checks resolve independently, so a failed TXT lookup does not suppress what MX established.Tests
23 in
worker/src/__tests__/domains-router.test.ts.fetchis stubbed and the helper throws on any domain not declared, so an accidental real lookup fails loudly rather than passing by luck.Two defects found while writing them, both fixed here:
includes(), soinclude:_spf.mx.cloudflare.net.evil.exampleread as ours and the domain showed green. It now matches whole terms with the qualifier stripped, and the same predicate decides both the status and whether a merge is needed, so the two cannot disagree.txtValueclaimed to concatenate long TXT strings and actually stripped quotes, which corrupts any record split across chunks.yarn tsc --noEmitclean.Note on scope
worker/src/lib/oauth-scope-policy.tsdoes not exist onmain— it arrives with #238. When these meet,GET /api/domainsneeds anadminrule withrequiresAdminRole, since unclassified routes are denied to bearer callers. Happy to fold that in whenever #238 lands.