test(ci): replace mailsac with self-owned e2e inbox worker - #3574
test(ci): replace mailsac with self-owned e2e inbox worker#3574baktun14 wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds an authenticated Cloudflare E2E inbox worker with D1 storage. UI email verification now uses the worker through shared client and strategy abstractions. GitHub Actions and test configuration use the worker URL, token, and email domain instead of Mailsac. ChangesE2E inbox worker migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3574 +/- ##
==========================================
- Coverage 76.33% 75.73% -0.61%
==========================================
Files 1134 1070 -64
Lines 29598 27685 -1913
Branches 7352 6958 -394
==========================================
- Hits 22593 20966 -1627
+ Misses 6174 5915 -259
+ Partials 831 804 -27
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/deploy-web/tests/ui/fixture/test-env.config.ts`:
- Around line 18-22: Update the E2E_INBOX_API_URL schema validation to require
an absolute URL using https, while allowing http only for explicit loopback
hosts used in local development. Preserve the existing trimming and
trailing-slash normalization, and reject all other schemes or non-loopback http
addresses.
In `@apps/deploy-web/tests/ui/services/email-verification/worker-inbox.client.ts`:
- Around line 32-44: Update fetchMessages to use an AbortController and pass its
signal to fetch with a bounded timeout; clear the timeout in a finally block so
stalled Worker requests abort and polling can resume.
In `@tools/e2e-inbox-worker/src/index.ts`:
- Around line 18-20: The message lookup SELECT must filter out records older
than MESSAGE_TTL_MS, and expired rows must be purged on a scheduled path
independent of new email delivery. Update the existing cleanup flow in the
worker’s request/handler logic and add the scheduled purge hook, then revise the
README retention description to document read-time exclusion and periodic
deletion.
In `@tools/e2e-inbox-worker/wrangler.jsonc`:
- Around line 10-11: Replace the all-zero database_id placeholder in the
Wrangler configuration with the actual UUID returned by `wrangler d1 create
console-e2e-inbox`, ensuring deployment binds DB to the intended remote D1
database.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b2919b0-3707-4a30-a38d-d21e273f87c0
⛔ Files ignored due to path filters (1)
tools/e2e-inbox-worker/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
.github/actions/console-web-ui-testing/action.yml.github/workflows/reusable-deploy-test.ymlapps/deploy-web/tests/ui/actions/auth.tsapps/deploy-web/tests/ui/fixture/test-env.config.tsapps/deploy-web/tests/ui/onboarding-journey.spec.tsapps/deploy-web/tests/ui/passwordless-login.spec.tsapps/deploy-web/tests/ui/services/email-verification/email-verification.strategy.tsapps/deploy-web/tests/ui/services/email-verification/inbox-client.tsapps/deploy-web/tests/ui/services/email-verification/inbox-code.strategy.tsapps/deploy-web/tests/ui/services/email-verification/index.tsapps/deploy-web/tests/ui/services/email-verification/worker-inbox.client.tstools/e2e-inbox-worker/.gitignoretools/e2e-inbox-worker/README.mdtools/e2e-inbox-worker/package.jsontools/e2e-inbox-worker/schema.sqltools/e2e-inbox-worker/src/index.tstools/e2e-inbox-worker/tsconfig.jsontools/e2e-inbox-worker/wrangler.jsonc
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large (XL) change that introduces new e2e test infrastructure (a Cloudflare Email Worker + D1) and touches the release-gating CI pipeline/secrets, a human look would still be worthwhile before merging.
What was reviewed:
- The worker's email parsing/storage flow and its bearer-token-protected
GET /messages/:emailendpoint (tools/e2e-inbox-worker/src/index.ts) - The refactor from
MailsacCodeVerificationStrategyto the genericInboxCodeVerificationStrategy+WorkerInboxClient, and the freshness/polling logic that moved over unchanged - CI wiring in action.yml and reusable-deploy-test.yml swapping Mailsac secrets/vars for the new E2E_INBOX_* ones
- Candidate issues around the text-body fallback, HTML-to-text script handling, and fetchMessageBody's unused parameter were raised and ruled out as non-issues
Extended reasoning...
Overview
This PR replaces a third-party mailbox service (Mailsac) with a self-hosted Cloudflare Email Worker + D1 database for reading Auth0 passwordless OTP emails in the deploy-web Playwright e2e suite. It touches: a new tools/e2e-inbox-worker package (Worker source, schema, wrangler config, README runbook), the e2e test helper classes (renamed/refactored Mailsac strategy into a generic inbox-client abstraction), the env schema for the e2e suite, and two CI files (composite action + reusable workflow) that pass new secrets/vars instead of the old Mailsac ones.
Security risks
The new Worker exposes a bearer-token-protected HTTP endpoint that returns captured email bodies for a given address. The token comparison is a direct string equality check rather than constant-time, but this is a low-value target (test-only OTP mailbox scoped to a throwaway e2e email domain, not production data or credentials), so the risk is minimal. No production code paths, auth logic, or user-facing security surfaces are touched — this is purely CI/test infrastructure, and the shipped app has no changes.
Level of scrutiny
This is test/CI infrastructure rather than product code, which normally warrants lighter scrutiny. However, it's large (19 files, size:XL), introduces an entirely new deployed service with manual one-time infra setup (Cloudflare Email Routing, D1, secrets) that lives outside this repo's normal deploy pipeline, and modifies the release-gating CI workflow (reusable-deploy-test.yml) and its secrets. That combination — new external infra + CI pipeline changes for a release-gating suite — justifies a human sanity check even though no functional bugs were found in the code itself.
Other factors
The refactor of the OTP polling/matching logic (InboxCodeVerificationStrategy) is a fairly mechanical extraction of the existing Mailsac logic behind a new InboxClient interface, and looks correct. The bug-hunting pass raised a few candidate issues (blank text/plain fallback, script-tag leakage in the HTML-to-text fallback, and an apparently-unused parameter in WorkerInboxClient.fetchMessageBody) which were investigated and ruled out. No outstanding review comments exist on this PR to address.
There was a problem hiding this comment.
Beyond the two nits already flagged inline, I also checked whether pairing the 24h-TTL cleanup DELETE with the critical email-storage INSERT in the same D1 batch() call (tools/e2e-inbox-worker/src/index.ts) risks losing a freshly delivered OTP if the batch partially fails. D1 batches run as an implicit transaction, so a failed DELETE would roll back the INSERT too — but the DELETE is a simple parameterized query with no realistic failure mode here, so this isn't a real risk.
Extended reasoning...
Examined the coupling between the opportunistic TTL-purge DELETE and the email-storage INSERT in the worker's email() handler, since both run in one env.DB.batch() call. D1 batches execute sequentially inside an implicit transaction, so a failure in either statement rolls back both — meaning a failed DELETE could theoretically prevent the just-arrived OTP email from being persisted. In practice the DELETE is a simple, deterministic received_ms < ? comparison against the primary table with no foreign keys or triggers, so there's no plausible runtime failure mode that would trigger a rollback and lose the email. This is a minor architectural coupling (cleanup logic mixed into the critical write path) but not a functional bug, so it doesn't warrant an inline comment on its own.
- validate E2E_INBOX_API_URL as an absolute https URL (http only for loopback) - bound WorkerInboxClient reads with an AbortController timeout - exclude >24h messages from reads and add an hourly scheduled purge cron - share the worker Env type between src and test declarations - run the worker's tsc + vitest suite in CI on path changes
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The validate job is not a required status check and the tool is fully isolated (own lockfile, no monorepo deps), so re-running it against the merge-queue commit adds no value. merge_group also ignores the paths filter, so it ran npm ci + typecheck + test on every merge-queue entry regardless of what changed. The pull_request paths filter already scopes runs to changes under the tool and its workflow.
An empty text/plain part decodes to whitespace (e.g. a lone newline), which is truthy, so neither the nullish nor the logical-or operator would drop it in favor of the html fallback. Gate on trimmed content so an OTP that lives only in the html part is still captured for the code regex. Covered by a multipart/alternative test with an empty text part.
akash.network carries production Google Workspace mail, and Cloudflare Email Routing replaces a domain's MX records and cannot coexist with an external provider. The old example invited enabling Email Routing on the apex and breaking real email. Docs now require a dedicated domain (with a warning), and all examples and test fixtures use placeholder domains.
There was a problem hiding this comment.
I reviewed this PR and found no new bugs. Because it's a large (XL), security- and infrastructure-sensitive change — a new Cloudflare Worker with bearer-token auth backing a CI secret, plus GitHub Actions credential/permission changes — a human look is still worthwhile.
What was reviewed: the e2e-inbox-worker's email parsing/D1 storage/purge logic and its bearer-token-gated HTTP endpoint, the CI workflow's checkout/Node setup, and the Mailsac-to-worker migration across the e2e auth strategies (InboxClient/InboxCodeVerificationStrategy). Checked fetchMessageBody's unused email param — ruled out since message ids are globally unique (crypto.randomUUID()), so it's redundant, not a correctness bug. Checked the bearer-token comparison for timing-attack exposure — ruled out as impractical given it's a long random secret over TLS, not a security-critical auth boundary.
Extended reasoning...
Overview
This PR replaces the Mailsac-based e2e OTP inbox with a self-owned Cloudflare Email Worker + D1 (tools/e2e-inbox-worker), refactors the deploy-web e2e email-verification strategy from a Mailsac-specific implementation to a generic InboxClient/InboxCodeVerificationStrategy pair backed by a new WorkerInboxClient, updates CI workflows/actions to pass the new E2E_INBOX_* secrets/vars, and adds a dedicated CI workflow for the new tool. It also adds solid unit/integration test coverage for the new worker and client code.
Security risks
The main surface is the new worker's bearer-token-protected HTTP endpoint and the CI checkout for the new tool's workflow. Both were already flagged and addressed during review: the checkout now sets persist-credentials: false, and a non-constant-time token comparison was raised and ruled out as impractical for this use case (long random secret, TLS-only, test-only OTP data). The latest commit also removes akash.network as a live example domain from docs/fixtures, avoiding a documented footgun where enabling Cloudflare Email Routing on the real production domain would break its existing mail — a good defensive doc fix.
Level of scrutiny
This PR is labeled size: XL, introduces new production-adjacent infrastructure (a deployed Cloudflare Worker with its own secrets and one-time manual setup steps), and changes CI credential handling — all reasons to keep a human in the loop even with no outstanding bugs. It already went through multiple rounds of automated review with real findings (dead loopback entry, missing persist-credentials, merge_group gating, empty-text-part fallback) that were fixed, plus two nits (missing lint step, node-version-file resolution) the author explicitly declined with reasonable justification.
Other factors
Test coverage is strong: new unit specs for the URL schema, the inbox client, the verification strategy, and a Workers-runtime integration spec for the worker itself (auth, TTL purge, HTML/empty-text fallback, ordering). The remaining open nits are non-blocking and already have author responses recorded in the thread, so nothing appears to be outstanding without acknowledgment.
| it("aborts the request when the worker does not respond within the timeout", async () => { | ||
| vi.useFakeTimers(); | ||
| const { client } = setup({ hangsUntilAborted: true }); | ||
|
|
||
| const pending = client.fetchMessages("probe@e2e.example.test"); | ||
| const assertion = expect(pending).rejects.toThrow(/aborted/i); | ||
| await vi.advanceTimersByTimeAsync(10_000); | ||
|
|
||
| await assertion; | ||
| vi.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
🟡 The abort test at lines 37-47 enables vi.useFakeTimers() (line 38) but restores real timers with a manual vi.useRealTimers() on line 46, which sits after the awaited assertion (lines 42/45) — and the file's afterEach (lines 6-8) only calls vi.unstubAllGlobals(), never vi.useRealTimers(). If that assertion throws, fake timers leak into the rest of the file's tests. Move the restoration into afterEach (or a try/finally), matching the sibling inbox-code.strategy.unit.spec.ts, which does exactly this in its afterEach (lines 11-13).
Extended reasoning...
In worker-inbox.client.unit.spec.ts, the "aborts the request when the worker does not respond within the timeout" test enables fake timers at line 38 (vi.useFakeTimers()) and only restores real timers via an inline vi.useRealTimers() call on line 46. That line sits after the awaited assertion on lines 42/45 (expect(pending).rejects.toThrow(...) combined with await vi.advanceTimersByTimeAsync(10_000)). The file-level afterEach (lines 6-8) only calls vi.unstubAllGlobals() and never touches timer state.
This means restoration is entirely happy-path: it only runs if the test reaches the end without throwing. If the abort assertion on line 42 ever fails — for example if WorkerInboxClient.fetchMessages stops rejecting with an AbortError-shaped message, or the timeout wiring regresses — the test throws before line 46 executes, and fake timers (and the faked Date) stay active for the rest of the suite, including the two fetchMessageBody tests that run afterward.
This is a real gap relative to the sibling file added in the same PR, inbox-code.strategy.unit.spec.ts, which restores timers in afterEach(() => vi.useRealTimers()) (lines 11-13) precisely so cleanup happens regardless of test outcome. worker-inbox.client.unit.spec.ts does not follow that same pattern, so cleanup here is fragile in a way the rest of the PR already knows how to avoid.
To be precise about the actual blast radius (since this was raised in review): the two tests that run after the abort test are fetchMessageBody's "serves the body cached..." and "throws for a message id that was never listed". Neither depends on a real timer advancing — the first drives fetchMessages against a Response.json(...) mock that resolves on a microtask (not something vitest's fake timers intercept), and the second never calls fetch at all. So in the current test file, a timer leak would not actually cascade into hung/timed-out follow-on tests; both would likely still pass. The defect is real but narrower than "leaked timers make later tests hang" — it's a cleanup-only-on-success bug that happens to be currently harmless given what runs after it in this specific file.
Step-by-step proof of the leak itself:
- Test reaches line 38, calls
vi.useFakeTimers()— fakeDate/timers are now active for the runner. - Line 41 kicks off
client.fetchMessages(...)against afetchmock that hangs until its abort signal fires. - Line 43 advances fake time by
10_000ms, which fires theAbortControllertimeout insideWorkerInboxClient.fetchMessages, rejecting the pending call. - Line 42/45 asserts the rejection matches
/aborted/i. If this assertion is wrong — e.g. a future refactor changes the rejection message or timing — theawait assertionthrows. - Because the throw happens before line 46,
vi.useRealTimers()never runs, and noafterEachpicks up the slack either. - Every subsequent test in this file now runs under stale fake-timer state until something else restores it.
Fix: move vi.useRealTimers() into the file's afterEach (alongside the existing vi.unstubAllGlobals()) or wrap the test body in try/finally, so restoration is outcome-independent — matching the pattern already used in inbox-code.strategy.unit.spec.ts.
This is test-only, only manifests once another assertion in this same test has already failed, and (per the analysis above) does not currently cascade into confusing failures elsewhere in this file — so it is a hygiene/consistency nit, not a blocking issue.
3 participants
Why
Resolves CON-799
The console-web release pipeline is gated by Playwright e2e tests that read Auth0 passwordless OTP emails through Mailsac. Mailsac's free tier allows 1,500 API ops/month and we exceeded it (126.7% last month), so codes stop arriving and release runs fail (e.g. run 31184065073). This recurs every month the suite stays on Mailsac.
What
Replaces Mailsac with email-capture infrastructure we own:
akash-network/e2e-inbox-worker(new standalone repo): Cloudflare Email Worker + D1. A catch-all Email Routing rule on a dedicated e2e domain delivers Auth0's OTP emails to the worker, which parses them withpostal-mimeand serves them via a bearer-token-protectedGET /messages/:email. Its README contains the one-time setup runbook and local-dev instructions. Verified locally viawrangler dev+/cdn-cgi/handler/emailinjection (plain-text and HTML-only emails, auth 401s, unknown-path 404). The worker initially lived in this PR undertools/e2e-inbox-worker/and was moved to its own repo per review feedback, so its dependencies sit inside a scanned surface (own CI + CodeQL + Dependabot; Socket/Snyk enablement tracked in that repo's README checklist) with its own secrets and auto-deploy.InboxCodeVerificationStrategy(parameterized by anInboxClient); the newWorkerInboxClienttalks to the worker. Mailsac strategy andMAILSAC_API_KEYare deleted; env now needsE2E_INBOX_API_URL,E2E_INBOX_API_TOKEN,E2E_INBOX_EMAIL_DOMAIN(fail-fast zod validation).console-web-ui-testingaction andreusable-deploy-test.ymlpass the three new values; the vestigialEMAIL_VERIFICATION_STRATEGYinput (never read by test code) is removed.Before merge (infra, one-time): register a dedicated e2e mail domain as a Cloudflare zone with Email Routing,
wrangler d1 create+ deploy the worker, add the catch-all rule, then set this repo's GitHub secretE2E_INBOX_API_TOKENand variablesE2E_INBOX_API_URL/E2E_INBOX_EMAIL_DOMAIN(runbook in the e2e-inbox-worker README).MAILSAC_API_KEYcan be deleted after the first green release run.No shipped-artifact changes; the flow still exercises real Auth0 email delivery end-to-end.
Summary by CodeRabbit
New Features
Documentation
Tests