feat(email): validate the sender domain and log why a send failed #117

Sammanfogat

supernaut sammanfogade 1 incheckning från feat/113-115-mail-sender-validation-and-diagnostics in i main

2026-07-31 12:58:14 +00:00

supernaut kommenterad

2026-07-31 12:20:10 +00:00

Ägare

Closes #115. Part of #113 — the portal half; the infra half is gitborg/gitborg-infra#283.

Two failures from today's mail.gitborg.se move motivated this, and neither was diagnosable from the portal's own output.

Sender validation (#113)

EMAIL_FROM is hardcoded here while the same address is configured independently in bitborg-infra, and nothing made them agree. They drifted: infra moved, the portal kept sending as email.gitborg.se, and every portal mail was rejected.

senderAllowed() now checks the sender's domain against EMAIL_ALLOWED_SENDER_DOMAINS (supplied by infra from the same variable that drives forgejo_mailer_from), and sendEmail refuses before contacting the provider — naming the real problem instead of returning whatever status the provider happens to pick.

An unset allowlist is deliberately distinct from a rejecting one. Absent means "infra has not told us yet" and still sends, so deploying this in either order relative to #283 cannot break mail. Only a populated list that excludes our domain refuses.

The From stays hardcoded on purpose — an env-set From could send as an unverified domain. The fix is detection, not configurability.

Failure diagnostics (#115)

sendEmail kept res.status and discarded the body, so a wrong-account API key surfaced as a bare status 422. The provider had explained itself; we threw it away, and diagnosis needed account knowledge rather than logs.

The body is now captured, capped at 512 bytes, and rendered by describeSendFailure() into one line per failure mode:

Reason	Line
`config`	`not attempted: SWEEGO_API_KEY is unset`
`sender`	`refused before sending: …`
`error` + status	`provider rejected (status 422): {"error":…}`
`error`, no status	`provider unreachable or timed out`

The route now logs the config case too, which it previously skipped. That skip is why an absent log line meant either delivered or never attempted — a systematically broken sender was indistinguishable from a healthy quiet one, and that ambiguity is exactly what let this run unnoticed until a user reported no email.

Built test-first

src/lib/email.test.ts is new — the mail path had no unit tests at all, itself part of why this stayed hidden. Each test was watched failing before the code existed. 19 tests: allowlist parsing (whitespace, case, multi-entry, empty), refusal before send with an assertion that the provider was never called, unset-allowlist passthrough, body capture, the 512-byte cap, and every failure sentence.

Dependencies are injected following the health.ts pattern rather than mocking globals, using in rather than ?? so an explicit undefined cannot silently pick up an ambient env var and make a result depend on the developer's shell.

Verification

vitest run — 114 tests pass (12 files)
astro check — 0 errors (the 20 hints are pre-existing)
eslint + stylelint — clean

Not covered

detail is operator telemetry and must never reach a user — it is provider output and may echo the request. It is returned for logging only; no template renders it.

#116 (a resend path for a failed setup link) is separate and still open — this makes the failure visible, not recoverable.

Closes #115. Part of #113 — the portal half; the infra half is gitborg/gitborg-infra#283. Two failures from today's mail.gitborg.se move motivated this, and neither was diagnosable from the portal's own output. ## Sender validation (#113) `EMAIL_FROM` is hardcoded here while the same address is configured independently in bitborg-infra, and nothing made them agree. They drifted: infra moved, the portal kept sending as `email.gitborg.se`, and every portal mail was rejected. `senderAllowed()` now checks the sender's domain against `EMAIL_ALLOWED_SENDER_DOMAINS` (supplied by infra from the same variable that drives `forgejo_mailer_from`), and `sendEmail` **refuses before contacting the provider** — naming the real problem instead of returning whatever status the provider happens to pick. **An unset allowlist is deliberately distinct from a rejecting one.** Absent means "infra has not told us yet" and still sends, so deploying this in either order relative to #283 cannot break mail. Only a populated list that excludes our domain refuses. The From stays hardcoded on purpose — an env-set From could send as an unverified domain. The fix is detection, not configurability. ## Failure diagnostics (#115) `sendEmail` kept `res.status` and discarded the body, so a wrong-account API key surfaced as a bare `status 422`. The provider had explained itself; we threw it away, and diagnosis needed account knowledge rather than logs. The body is now captured, capped at 512 bytes, and rendered by `describeSendFailure()` into one line per failure mode: | Reason | Line | | --- | --- | | `config` | `not attempted: SWEEGO_API_KEY is unset` | | `sender` | `refused before sending: …` | | `error` + status | `provider rejected (status 422): {"error":…}` | | `error`, no status | `provider unreachable or timed out` | The route now logs the `config` case too, which it previously skipped. That skip is why an absent log line meant either *delivered* or *never attempted* — a systematically broken sender was indistinguishable from a healthy quiet one, and that ambiguity is exactly what let this run unnoticed until a user reported no email. ## Built test-first `src/lib/email.test.ts` is **new** — the mail path had no unit tests at all, itself part of why this stayed hidden. Each test was watched failing before the code existed. 19 tests: allowlist parsing (whitespace, case, multi-entry, empty), refusal before send with an assertion that the provider was **never called**, unset-allowlist passthrough, body capture, the 512-byte cap, and every failure sentence. Dependencies are injected following the `health.ts` pattern rather than mocking globals, using `in` rather than `??` so an explicit `undefined` cannot silently pick up an ambient env var and make a result depend on the developer's shell. ## Verification - `vitest run` — **114 tests pass** (12 files) - `astro check` — **0 errors** (the 20 hints are pre-existing) - `eslint` + `stylelint` — clean ## Not covered `detail` is operator telemetry and must never reach a user — it is provider output and may echo the request. It is returned for logging only; no template renders it. #116 (a resend path for a failed setup link) is separate and still open — this makes the failure *visible*, not *recoverable*.

supernaut lade till 1 incheckning

2026-07-31 12:20:10 +00:00

feat(email): validate the sender domain and log why a send failed

ci / ci (pull_request) Successful in 1m16s

Detaljer

da3478d077

Closes #115. Part of #113 (the portal half; infra supplies the allowlist).

Two failures from the mail.gitborg.se move motivated this, and neither was
diagnosable from the portal's own output.

SENDER VALIDATION (#113). EMAIL_FROM is hardcoded here while the same address is
configured independently in gitborg-infra, and nothing made them agree. They
drifted: infra moved to mail.gitborg.se, the portal kept sending as
email.gitborg.se, and every portal mail was rejected. senderAllowed() now checks
the sender's domain against an allowlist infra supplies as
EMAIL_ALLOWED_SENDER_DOMAINS, and sendEmail refuses BEFORE contacting the provider
— naming the real problem rather than returning whatever status the provider picks.

An unset allowlist is deliberately distinct from a rejecting one: absent means
"infra has not told us yet" and still sends, so deploying this cannot break mail
during rollout. Only a populated list that excludes our domain refuses.

FAILURE DIAGNOSTICS (#115). sendEmail kept res.status and discarded the response
body, so a wrong-account API key surfaced as a bare `status 422` and took account
knowledge rather than logs to diagnose. The body is now captured, capped at 512
bytes, and rendered by describeSendFailure() into one line per failure mode.

The route also logs the "config" case, which it previously skipped. That skip meant
an absent log line covered both "delivered" and "never attempted" — a
systematically broken sender was indistinguishable from a healthy quiet one, which
is how this went unnoticed until a user reported no email.

Built test-first: src/lib/email.test.ts is new. The mail path had no unit tests at
all, itself part of why this stayed hidden. 19 tests cover allowlist parsing,
refusal before send, unset-allowlist passthrough, body capture and capping, and
each failure sentence. Dependencies are injected following the health.ts pattern,
using `in` rather than `??` so an explicit undefined cannot silently pick up an
ambient env var and make a test depend on the developer's shell.

114 tests pass, eslint and stylelint clean, astro check reports 0 errors.

supernaut refererade denna ändringsförfrågan från en incheckning

2026-07-31 12:56:36 +00:00

feat(mail): derive every sender from one sending-domain variable (#283)

supernaut sammanfogade incheckning 5714a29ab9 till main

2026-07-31 12:58:14 +00:00

supernaut tog bort grenen feat/113-115-mail-sender-validation-and-diagnostics

2026-07-31 12:58:14 +00:00

supernaut refererade denna ändringsförfrågan från en incheckning

2026-07-31 12:58:15 +00:00

feat(email): validate the sender domain and log why a send failed (#117)

supernaut refererade denna ändringsförfrågan från en incheckning

2026-08-03 09:41:50 +00:00