Skip to content

fix: add auth guard to email notify endpoint - #116

Open
gusye1234 wants to merge 2 commits into
mainfrom
fix/email-notify-auth
Open

fix: add auth guard to email notify endpoint#116
gusye1234 wants to merge 2 commits into
mainfrom
fix/email-notify-auth

Conversation

@gusye1234

Copy link
Copy Markdown
Contributor

Summary

  • Adds X-Internal-Secret header validation to /api/email/notify for non-service-binding requests
  • In production, Cloudflare Service Binding already provides isolation (internal-only)
  • This fixes the dev-mode fallback path which was completely unauthenticated
  • Both email-worker callers (index.ts and imap-poller-do.ts) now pass the secret on fallback

Test plan

  • Existing route tests pass (updated to use internal URL)
  • Verify email notify works in dev mode with the secret header
  • Verify unauthenticated requests to /api/email/notify return 401
  • Verify production service-binding path still works (no header check for internal URLs)

Closes #106

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
@gusye1234
gusye1234 requested a review from a team as a code owner May 25, 2026 04:35
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 gusye1234 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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://internal URL 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_KEY as an auth token is pragmatic but dual-purposes the key. If ENCRYPTION_KEY ever gets rotated or leaked, it now also compromises this auth path. A dedicated INTERNAL_API_SECRET would provide better isolation — but for an internal dev fallback path, this is acceptable.
  • Minor: String comparison secret !== cfEnv.ENCRYPTION_KEY is technically vulnerable to timing attacks. For an internal endpoint this is fine, but crypto.timingSafeEqual would be the hardened approach.

Tests Coverage

  • Test updated to pass through the guard (uses http://internal URL)
  • 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

  1. 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.

  2. Guard skipped when ENCRYPTION_KEY is falsy: The condition cfEnv.ENCRYPTION_KEY means 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.

GenerQAQ pushed a commit that referenced this pull request Jul 30, 2026
…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).
GenerQAQ pushed a commit that referenced this pull request Jul 30, 2026
…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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: Unauthenticated Email Notify Endpoint

1 participant