Move user email addressing to the inbox. subdomain - #643
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds ChangesUSER_EMAIL_DOMAIN support and domain split
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🔎 Preview deployed: https://kody-pr-643.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 473e69e. Configure here.
User inboxes and outbound senders now live at
{username}@inbox.<APP_BASE_URL hostname> (e.g. kentcdodds@inbox.heykody.dev)
instead of the zone apex, with an optional USER_EMAIL_DOMAIN env override.
The subdomain replaces denylist-only separation with structural
separation: the user-controlled address namespace can never collide
with system transactional mail (kody@<apex> stays apex-only and the
apex is no longer a user inbox), and user outbound sender reputation is
isolated from the system sender. The reserved-username denylist stays
as signup/scope protection and inbound defense-in-depth.
Everything funnels through getPlatformEmailDomain, so inbound routing,
outbound from, inbox auto-provisioning, and sender-identity
provisioning all follow from the one derivation change.
473e69e to
498697b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/worker/src/email/platform-address.ts (2)
46-67: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo guard against
USER_EMAIL_DOMAINcolliding with the system/apex domain.If an operator sets
USER_EMAIL_DOMAINto the same hostname asAPP_BASE_URL's apex,getPlatformEmailDomainandgetSystemEmailDomainreturn identical values, silently collapsing the structural separation between user and system mail that this PR introduces (reserved-username denylist still protects known system locals, but the sender-reputation/namespace isolation goal is defeated without any warning). Consider rejecting (or logging/warning on) an override equal to the system domain and falling back to the derived default in that case.🤖 Prompt for 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. In `@packages/worker/src/email/platform-address.ts` around lines 46 - 67, `getPlatformEmailDomain` currently accepts `USER_EMAIL_DOMAIN` even when it matches the apex/system domain derived from `APP_BASE_URL`, which can make `getSystemEmailDomain` and the platform domain collide; update this function to detect that equality and reject or warn on the override, then fall back to the derived default instead of returning the conflicting value. Use the existing `configuredDomain`, `configuredBaseUrl`, and `defaultUserEmailSubdomainLabel` logic in `platform-address.ts` to locate the check and keep the user/system mail namespaces separated.
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
APP_BASE_URLhostname-parsing logic betweengetSystemEmailDomainandgetPlatformEmailDomain.Both functions independently do
new URL(configuredBaseUrl).hostname.toLowerCase()with the same empty-check/try-catch shape. Since these two functions define the system/user domain split this PR is built around, keeping the parsing in one place avoids future drift between them.♻️ Suggested shared helper
+function deriveHostnameFromAppBaseUrl(appBaseUrl?: string | null): string | null { + const configuredBaseUrl = appBaseUrl?.trim() + if (!configuredBaseUrl) return null + try { + const hostname = new URL(configuredBaseUrl).hostname.toLowerCase() + return hostname.length > 0 ? hostname : null + } catch { + return null + } +} + export function getSystemEmailDomain(env: { APP_BASE_URL?: string | null }): string | null { - const configuredBaseUrl = env.APP_BASE_URL?.trim() - if (!configuredBaseUrl) return null - try { - const hostname = new URL(configuredBaseUrl).hostname.toLowerCase() - return hostname.length > 0 ? hostname : null - } catch { - return null - } + return deriveHostnameFromAppBaseUrl(env.APP_BASE_URL) }Also applies to: 46-67
🤖 Prompt for 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. In `@packages/worker/src/email/platform-address.ts` around lines 19 - 30, The hostname parsing for APP_BASE_URL is duplicated in getSystemEmailDomain and getPlatformEmailDomain, so refactor the shared URL-to-hostname logic into a single helper and have both functions call it. Preserve the current trim, empty-check, lowercase conversion, and try/catch behavior in the shared helper so both domain split functions stay consistent and avoid drift.packages/worker/src/email/platform-address.node.test.ts (1)
1-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing unit test for
getSystemEmailDomain.The new
getSystemEmailDomainexport (used to gate system-inbox routing ininbound.ts) isn't unit-tested in this file; onlygetPlatformEmailDomainandbuildPlatformEmailAddressare covered. Worker-level tests exercise it indirectly, but a direct unit test here would pin down its edge cases (missing/malformedAPP_BASE_URL) alongside its sibling.✅ Suggested test
+test('getSystemEmailDomain derives the apex hostname from APP_BASE_URL', () => { + expect(getSystemEmailDomain({ APP_BASE_URL: 'https://heykody.dev' })).toBe( + 'heykody.dev', + ) + expect(getSystemEmailDomain({})).toBeNull() + expect(getSystemEmailDomain({ APP_BASE_URL: 'not a url' })).toBeNull() +})🤖 Prompt for 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. In `@packages/worker/src/email/platform-address.node.test.ts` around lines 1 - 46, Add a direct unit test for getSystemEmailDomain in this spec alongside getPlatformEmailDomain and buildPlatformEmailAddress. Cover the key edge cases called out in inbound.ts routing: valid APP_BASE_URL should derive the expected system inbox domain, while missing or malformed APP_BASE_URL should return null. Use the existing test style in platform-address.node.test.ts and reference getSystemEmailDomain explicitly so the behavior is pinned down independently of worker-level coverage.packages/worker/src/env-schema.ts (1)
174-174: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEnv schema doesn't enforce hostname format for
USER_EMAIL_DOMAIN.
optionalNonEmptyStringSchemaonly checks non-emptiness, unlikeAPP_BASE_URL'soptionalUrlStringSchemawhich enforces actual URL parseability. A malformedUSER_EMAIL_DOMAIN(e.g. containing spaces) passes this schema silently, and is only caught defensively later ingetPlatformEmailDomain'sbareHostnamePatterncheck — contradicting that function's inline comment claiming parity withAPP_BASE_URL's validation. GivengetEnv()throws at boot on schema failures with a clear message, tightening this schema (e.g. reusing the hostname regex) would fail fast on misconfiguration instead of silently falling back to the derived default.♻️ Suggested tightened schema
- USER_EMAIL_DOMAIN: optionalNonEmptyStringSchema, + USER_EMAIL_DOMAIN: createSchema<unknown, string | undefined>((value, context) => { + if (value === undefined) return { value: undefined } + if (typeof value !== 'string') return fail('Expected string', context.path) + const trimmed = value.trim().toLowerCase().replace(/\.$/, '') + if (trimmed.length === 0) return { value: undefined } + if (!/^[a-z0-9](?:[a-z0-9.-]*[a-z0-9])?$/.test(trimmed)) { + return fail('USER_EMAIL_DOMAIN must be a bare hostname.', context.path) + } + return { value: trimmed } + }),🤖 Prompt for 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. In `@packages/worker/src/env-schema.ts` at line 174, USER_EMAIL_DOMAIN currently uses only a non-empty string check, so it should be tightened to validate hostname format at the schema layer instead of relying on later fallback logic. Update the env schema in env-schema.ts by replacing optionalNonEmptyStringSchema for USER_EMAIL_DOMAIN with the same hostname-style validation used by getPlatformEmailDomain/bareHostnamePattern, so getEnv() fails fast on malformed values and stays consistent with APP_BASE_URL’s stricter parsing.packages/worker/src/email/system-email.workers.test.ts (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared domain test fixtures.
systemDomain/userDomain/platformBaseUrlconstants are now duplicated near-identically across this file,inbound.workers.test.ts,inbound-entitlements.workers.test.ts, andoutbound.workers.test.ts. A shared test-fixtures module could reduce drift risk if the derivation logic changes again.🤖 Prompt for 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. In `@packages/worker/src/email/system-email.workers.test.ts` around lines 20 - 22, The domain fixture constants are duplicated across multiple worker test files, which makes them easy to drift apart if the derivation logic changes. Extract the shared `systemDomain`, `userDomain`, and `platformBaseUrl` setup into a common test-fixtures module and update `system-email.workers.test.ts` to import and use that shared source instead of local copies. Keep the existing test names and assertions intact, but centralize the derivation logic so `inbound.workers.test.ts`, `inbound-entitlements.workers.test.ts`, and `outbound.workers.test.ts` can all reuse the same fixture helpers.
🤖 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 `@docs/use/email-primitives.md`:
- Around line 14-25: Update the email primitives docs bullet so it describes the
configured platform domain instead of implying a dedicated subdomain; in the
section covering inbound mail and unknown usernames, rephrase the user-inbox
routing text in docs/use/email-primitives.md to reference the platform domain /
USER_EMAIL_DOMAIN behavior, using the existing terms like “platform domain,”
“apex,” and “system inboxes” consistently.
---
Nitpick comments:
In `@packages/worker/src/email/platform-address.node.test.ts`:
- Around line 1-46: Add a direct unit test for getSystemEmailDomain in this spec
alongside getPlatformEmailDomain and buildPlatformEmailAddress. Cover the key
edge cases called out in inbound.ts routing: valid APP_BASE_URL should derive
the expected system inbox domain, while missing or malformed APP_BASE_URL should
return null. Use the existing test style in platform-address.node.test.ts and
reference getSystemEmailDomain explicitly so the behavior is pinned down
independently of worker-level coverage.
In `@packages/worker/src/email/platform-address.ts`:
- Around line 46-67: `getPlatformEmailDomain` currently accepts
`USER_EMAIL_DOMAIN` even when it matches the apex/system domain derived from
`APP_BASE_URL`, which can make `getSystemEmailDomain` and the platform domain
collide; update this function to detect that equality and reject or warn on the
override, then fall back to the derived default instead of returning the
conflicting value. Use the existing `configuredDomain`, `configuredBaseUrl`, and
`defaultUserEmailSubdomainLabel` logic in `platform-address.ts` to locate the
check and keep the user/system mail namespaces separated.
- Around line 19-30: The hostname parsing for APP_BASE_URL is duplicated in
getSystemEmailDomain and getPlatformEmailDomain, so refactor the shared
URL-to-hostname logic into a single helper and have both functions call it.
Preserve the current trim, empty-check, lowercase conversion, and try/catch
behavior in the shared helper so both domain split functions stay consistent and
avoid drift.
In `@packages/worker/src/email/system-email.workers.test.ts`:
- Around line 20-22: The domain fixture constants are duplicated across multiple
worker test files, which makes them easy to drift apart if the derivation logic
changes. Extract the shared `systemDomain`, `userDomain`, and `platformBaseUrl`
setup into a common test-fixtures module and update
`system-email.workers.test.ts` to import and use that shared source instead of
local copies. Keep the existing test names and assertions intact, but centralize
the derivation logic so `inbound.workers.test.ts`,
`inbound-entitlements.workers.test.ts`, and `outbound.workers.test.ts` can all
reuse the same fixture helpers.
In `@packages/worker/src/env-schema.ts`:
- Line 174: USER_EMAIL_DOMAIN currently uses only a non-empty string check, so
it should be tightened to validate hostname format at the schema layer instead
of relying on later fallback logic. Update the env schema in env-schema.ts by
replacing optionalNonEmptyStringSchema for USER_EMAIL_DOMAIN with the same
hostname-style validation used by getPlatformEmailDomain/bareHostnamePattern, so
getEnv() fails fast on malformed values and stays consistent with APP_BASE_URL’s
stricter parsing.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b0bf628-be7c-49da-817f-286145470a88
📒 Files selected for processing (12)
docs/contributing/environment-variables.mddocs/use/email-primitives.mdpackages/worker/.env.examplepackages/worker/src/email/inbound-entitlements.workers.test.tspackages/worker/src/email/inbound.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/email/platform-address.node.test.tspackages/worker/src/email/platform-address.tspackages/worker/src/email/system-email.workers.test.tspackages/worker/src/env-schema.ts
The USER_EMAIL_DOMAIN override means user mail is not always on a subdomain of APP_BASE_URL; describe the configured domain instead.
2 participants
Follow-up to #635, per product owner decision: user email addressing moves off the zone apex onto a dedicated subdomain. User inboxes and outbound senders now live at
{username}@inbox.<APP_BASE_URL hostname>—kentcdodds@inbox.heykody.devin production — with an optionalUSER_EMAIL_DOMAINenv override.Rebased on main after #642 (operator-owned system inboxes) landed; the two now compose into a clean two-domain model:
heykody.dev(apex,getSystemEmailDomain)kody@transactional sender + operator system inboxes (kody,support,abuse,postmaster,security,admin). All other apex mail rejects.inbox.heykody.dev(getPlatformEmailDomain){username}@inboxes and outbound senders. System/reserved locals reject here.Why
The apex model relied on the reserved-username denylist alone to keep system addresses safe; a denylist is inherently incomplete. A dedicated subdomain makes the separation structural:
Cloudflare Email Routing supports subdomains on all plans (free), so this is dashboard config only on the infra side.
What changed
getPlatformEmailDomain(the single chokepoint from HARD BREAK: username-based email inbox model #635) now returnsUSER_EMAIL_DOMAINwhen set, otherwiseinbox.+ theAPP_BASE_URLhostname. Inbound user routing, the outbound from address, default-inbox auto-provisioning, and sender-identity provisioning all follow from this one derivation change.getSystemEmailDomain(apex hostname):handleInboundEmailroutes Add operator-owned system email inboxes #642's system locals on the apex (next to the transactional sender whose replies they receive), user mail on the subdomain, and rejects everything else on either domain.USER_EMAIL_DOMAINadded to the env schema,.env.example, anddocs/contributing/environment-variables.md. A malformed override falls back to the derived default (same defensive pattern asgetAppBaseUrl). Production needs no config: the derived default yieldsinbox.heykody.dev.email-primitives.mdaddressing model (merged with Add operator-owned system email inboxes #642's system-inbox wording) + local-testing example updated.Tests
platform-address.node.test.ts: derived default, override normalization (case/trailing dot), malformed-override fallback, null when unconfigured.inbox.kody.example.com;system-email.workers.test.tssplits addresses across apex (system) and subdomain (user) and adds cross-domain cases: system locals on the subdomain reject as reserved, non-system locals on the apex reject as unknown, and a real username on the apex rejects.npm run validategreen locally after the rebase.Deploy ordering (important)
Before (or immediately after) this deploys, the Cloudflare zone needs Email Routing enabled for the subdomain: Email → Email Routing → Settings → Add subdomain (
inbox.heykody.dev), accept the DNS records, and point the subdomain's catch-all at the worker. Until that's done, inbound user mail has nowhere to land (apex user-mail is rejected by this change; subdomain mail isn't routed yet). Outbound fromkentcdodds@inbox.heykody.devalso depends on the subdomain's sending DNS being in place. System inboxes on the apex keep working unchanged.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@148a49c7· Head:498697b9Classification: extends — the
emailprimitive's addressing contract changes again (user mail: apex →inbox.subdomain); intentionally breaking for the ~hours-old apex user addresses, accepted pre-launch.Primitives touched
emailinbox.<apex>(orUSER_EMAIL_DOMAIN); apex = system mail onlySystem map
Before / after
Summary by CodeRabbit
USER_EMAIL_DOMAINto override the user inbox domain (inbox.<app hostname>by default).