Repository navigation
[SCR-585] Add email send-gate worker - #586
itsryanarmani wants to merge 1 commit into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
|
There was a problem hiding this comment.
Reviewed head: 9fd10c4c78591f7414eaec288fe5cf6fa69fab3e
No Critical or Major defects. Nothing pushed. Remaining items are Minor and open (author's call):
- Minor, open:
loadKeys(src/index.js:20) swallows a malformedKEYS_JSONand returns{}. It fails closed with 401, but a misconfigured secret is indistinguishable from a bad key. Considerconsole.errorthere. - Minor, open:
typeof parsed === 'object'accepts arrays. Only a bearer token like0could then match an index key, so it is contrived. Add!Array.isArray(parsed)for hygiene. - Minor, open:
from.nameis sent asundefinedwhen a persona has noname. Omit the field when unset. - Rejected: the claim that SHA-256 timing leaks key length. Digest time on short inputs is negligible and the keys are opaque tokens, so there is no practical trigger.
Proof: the ## How I verified section names commands with results (6 tests, 233 pass). I could not re-run them in this review environment, so I'm relying on the author's report. No deployed or Preview walk exists yet. The PR says the worker is not deployed, so run a /health and /send check after wrangler deploy.
PR standard: the diff is small and within the caps. The title and body format are fine. The commit subject is 53 characters, over the 50-character limit. The PR title is what pr-standards checks, so this is only a note.
🧑⚖️ Council
| Model | Lens | Result |
|---|---|---|
| GPT-5.6 (Codex) | correctness | 2 confirmed (Minor), 1 rejected |
| Kimi K3 | security | error (HTTP 429), no findings |
9fd10c4 to
fac6f43
Compare
There was a problem hiding this comment.
Worker logic looks right (hashed constant-time key compare over every key, fail-closed 401, per-binding allowed_sender_addresses as the real enforcement). Not merging yet: (1) wrangler.jsonc still lists three teammates by first name, and the public-name rewrite turned their domains into private-repo-xxxx.com placeholders, so a wrangler deploy from main would bind senders on non-existent domains; keep only the commented template in the public repo and put the real bindings in a private overlay or deploy-time config. (2) The current head was rewritten after the last CI run, so no tests/pr-standards run exists on this SHA; push a fresh commit so CI runs.
Reviewed-by: claude:claude-opus-5-5
Verdict: 1 major (committed per-person config is broken by the name rewrite and exposes teammate names)
Closes #585
What
A Cloudflare Worker (
workers/email-send-gate/) exposingPOST /sendwith per-person opaque keys. Each key maps to asend_emailbinding pinned withallowed_sender_addresses, so Cloudflare itself refuses to send as any other From address — a leaked key cannot impersonate another sender.Why
Teammates currently hold
Email Sending WriteAPI tokens, which are account-wide: any of them can send as any onboarded domain. This worker is the only way to grant per-mailbox send access — revocation is deleting one key, enforcement is platform-level, not code-level.How I verified
node --test workers/email-send-gate/send-gate.test.mjs— 6 tests, 0 failures (key comparison, 401/400 paths, correct binding+sender).node --test pr-standards.test.mjs assisted-by.test.mjs— 233 pass. Not yet deployed;wrangler deployafter merge.Assisted-by: t3-code:swe-2-high