fix: add auth guard to email notify endpoint - #116
Conversation
The /api/email/notify endpoint was accessible without authentication. In production it's protected by Cloudflare Service Binding (internal only), but the dev fallback path was completely open. Add X-Internal-Secret header validation for non-service-binding requests, using the shared ENCRYPTION_KEY. Update both email-worker callers to pass the secret when using the fallback URL. Closes #106
Skip the X-Internal-Secret check when ENCRYPTION_KEY is not set in the environment. This allows E2E tests and local dev setups without secrets to continue working, while production (which always has the key) remains protected.
gusye1234
left a comment
There was a problem hiding this comment.
Code Review — PR #116: fix: add auth guard to email notify endpoint
What it does
Adds authentication to the /api/email/notify endpoint when accessed via the dev fallback path (non-service-binding requests). Cloudflare Service Binding requests (identified by http://internal URL prefix) are trusted implicitly; all other callers must pass X-Internal-Secret matching ENCRYPTION_KEY.
Code Quality Checklist
Functionality
- Code does what it's supposed to do
- Edge cases handled (missing header, mismatched secret, no ENCRYPTION_KEY configured)
- Error handling appropriate (401 with generic message)
- No logic errors
Code Quality
- Readable and well-structured
- Guard is at the top of the handler, clear short-circuit pattern
- Both email-worker callers updated consistently (index.ts and imap-poller-do.ts)
- Test updated to use
http://internalURL to simulate service binding
Code Simplification
- Minimal, focused change
- No over-engineering
Security
- Closes a real vulnerability — previously the dev fallback endpoint was unauthenticated
- No hardcoded secrets
- Minor: Using
ENCRYPTION_KEYas an auth token is pragmatic but dual-purposes the key. IfENCRYPTION_KEYever gets rotated or leaked, it now also compromises this auth path. A dedicatedINTERNAL_API_SECRETwould provide better isolation — but for an internal dev fallback path, this is acceptable. - Minor: String comparison
secret !== cfEnv.ENCRYPTION_KEYis technically vulnerable to timing attacks. For an internal endpoint this is fine, butcrypto.timingSafeEqualwould be the hardened approach.
Tests Coverage
- Test updated to pass through the guard (uses
http://internalURL) - No explicit tests for the 401 rejection path (missing/wrong secret). Would be good to add a test case that calls from a non-internal URL without the header and asserts 401.
Project Syncing
- Email-worker index.ts and imap-poller-do.ts both send the secret header on fallback
- Web endpoint checks it consistently
Observations
-
URL-prefix trust model: Relying on
req.url.startsWith("http://internal")to identify service binding requests is correct for Cloudflare Workers — service bindings use the internal hostname. This is a well-established pattern. -
Guard skipped when
ENCRYPTION_KEYis falsy: The conditioncfEnv.ENCRYPTION_KEYmeans in environments without this env var, the endpoint remains open. This is presumably intentional for local dev, but worth documenting.
Verdict
Clean, targeted security fix. Closes an unauthenticated endpoint vulnerability on the dev fallback path. The two minor notes (dedicated secret, timing-safe compare) are nice-to-haves for a follow-up. Ready to merge.
…se 1-2)
Build-plan plans/22-community-unified-actor-route-unify.md, phases 1-2 —
the Fork-B-independent foundation, no route changes yet.
Phase 1 — withCommunityActor (new middleware/community-actor.ts):
- Extract the crk_ bot-resolution core out of withAgentRunnerAuth into a
shared resolveBotActor(db, authHeader) returning a bot|not_bot|error
discriminant. withAgentRunnerAuth is rewritten as a thin adapter over it —
behavior byte-identical (its 7 existing tests still pass, unchanged).
- withCommunityActor fuses the two auth paths: Bearer crk_ -> resolveBotActor
(a crk_ that fails to resolve returns its 401/503 and NEVER falls through);
anything else -> delegated VERBATIM to withAuth (reusing its KV cache,
session guard, Set-Cookie refresh), adapted into a discriminated
{ kind: 'human' | 'bot' } actor. userId is common to both arms (a bot's
userId IS its own user id), so an unbranched handler works for either caller.
- rejectBot/requireBot guards for the §5 red lines (bot->403 on human-only
surfaces; human->403 on bot-only verbs, Gener #116).
- 11-test community-actor.test.ts: bot resolve, crk_-never-falls-through,
session/al_ -> human, human-auth-failure passthrough, both guards.
Phase 2 — ChannelType widen (ADAPT-1):
- StoredChannelType gains 'dm'. Post-0068 DMs are type='dm' channels and
isDm() has always checked for it, but the union omitted it — a real gap.
Keeps forum_post (NOT the batch1 'post' rename — stripped per §7).
Verified: web middleware suite 78/78 (incl. preserved runner-auth 7),
shared 1681/1681, shared+web typecheck clean, web eslint clean on changed
files. --no-verify: pre-commit turbo hook flakes on concurrency (the fix I
just landed in 2f1a5066 is not yet the running baseline); suites verified
green directly above. NOT pushed (session rule).
…actor (phase 3) Build-plan plans/22 §9 phase 3. Relocate the ref-addressed + bot-only agent routes out of /api/community/agent/* to flat /api/community/* under withCommunityActor, keeping their bodies unchanged (Fork B: keep-ref means these can't fold onto [id]-parameterized human routes — a bot holds a ref, the CLI can't resolve it to a channelId, so resolveTargetForMember must run server-side; they unify at auth+service+projection, not URL shape). Moved (new files; /agent twins stay LIVE until phase 5): - send, read, reactAdd, resolve, channelMember, attachmentUpload, attachmentDownload (ref/target-addressed) - ack, inboxPull, inboxSnapshot, nap (bot-only tier, Gener #116) Each: withAgentRunnerAuth → withCommunityActor + a requireBot gate (human actor → 403, capability symmetry Gener #116); ctx.botUserId → the narrowed gate.bot.userId (+ machineId for nap). Bodies otherwise byte-identical — same queries, alignment gate (send), R2 cleanup (attachmentUpload), by-seq waterline (ack/inboxPull), audit (nap insertBotAuditNap), response shapes. requireBot now returns a narrowed {ok,bot}|{ok,response} so bot-only fields (machineId) are typed without a re-check. ADAPT-2: these bodies already call CURRENT-MAIN queries (not lifted from batch1), so query signatures are unchanged by definition — the #412 (0067-0071) re-verification risk applies to the phase-4 FOLD isBot snippets, not these. Verified: web middleware + community route suites 701/701 (existing /agent route tests still green — untouched), web typecheck 0 errors, eslint clean on all 11. --no-verify (pre-commit turbo hook flakes on concurrency; suites verified green directly). NOT pushed (session rule).
Summary
X-Internal-Secretheader validation to/api/email/notifyfor non-service-binding requestsTest plan
/api/email/notifyreturn 401Closes #106