fix(web): close the Postmark reply holes — authorization, idempotency, parsing - #190
Conversation
…, parsing Split out of the delivery-durability work so the email door is reviewable and revertable on its own. Three concerns, one subject: what the inbound webhook does with a message that claims to be a reply. 1. AUTHORIZATION (issue #189 — live on main, pre-existing). The MailboxHash path resolved a ticket with findUnique({ where: { displayId } }): no source filter, so an email could append to a Discord, Slack, Teams or GitHub ticket; and no sender check, so anyone knowing a displayId could append and reopen. The reasoning that a token we mint is trustworthy does not survive the fact that we minted it and then published it — generateTicketId draws 8 chars from a 32-char alphabet with Math.random(), and the bots posted "Ticket TKT-XXXXXXXX created" into public Discord and Slack threads until #172 removed that. Those threads still carry the IDs. Resolution is now scoped to source: 'EMAIL' and gated on the same isTicketParticipant rule the header path already used — reused unchanged, so there is no second, subtly different definition of participant. Failing either gate means UNRESOLVED, not dropped: the mail falls through to orphan handling, so a legitimate sender writing from an unrecognised address still gets a ticket. The hash and header paths were also mutually exclusive via if/else, so an unresolvable hash skipped header matching entirely. A mail can legitimately carry both; an unresolved hash now falls through to headers before orphan handling. Not currently exploitable: production and staging both hold 0 EMAIL tickets, so Postmark inbound has never created one. Safe by luck, not by construction. 2. IDEMPOTENCY. appendReplyToTicket did a bare message.create while the new-ticket path was guarded. Postmark retries every non-2xx and this route's own catch returns 500, so a retried reply duplicated the customer's message AND re-ran the reopen — putting a resolved ticket back in the queue twice for one email. Dedup is a real column, Message.sourceMessageId with @@unique([ticketId, sourceMessageId]), not a JSON path: jsonb cannot carry a unique constraint, and attachments.postmarkMessageId would have added a second unindexed scan to the hot path of every reply. A pre-read short-circuits the common retry on an index hit and the constraint is the authority on a race — the same two-level pattern the new-ticket path already uses. The reopen runs only on the branch that actually inserted, so a redelivery cannot reopen a ticket a human resolved in between, and insert plus reopen share one transaction. That closes the mirror gap a naive dedup opens, where the message commits, the reopen is lost, and every subsequent retry is a silent no-op that never reaches a human. A duplicate answers 200 with duplicate: true — 2xx so Postmark stops, marked so retry storms stay legible. Migration 20260811230000 ran an unguarded CREATE UNIQUE INDEX over historical data, and nothing deduped before it, so a database with duplicate EMAIL sourceIds would fail to build it — which under set -e in start.sh means the worker never boots. It now dedupes deterministically first: oldest ticket per sourceId keeps the key, later ones get sourceId = NULL with the value preserved at additionalInfo.duplicateEmailSourceId. No rows deleted, IF NOT EXISTS, fully re-runnable. A tolerant index was rejected as a constraint with holes that reads as enforced. Edited in place rather than renamed, which is safe here: that migration is applied nowhere. Production holds 0001_init, 20260715 and 20260812220000; staging holds the first two. Only a developer who ran the pre-split branch locally needs their _prisma_migrations row refreshed. 3. PARSING. getHeaderValue read only the first matching header. References is legitimately split across multiple lines by some clients, so a reply whose matching Message-ID sat in a later segment failed to thread and became a new ticket. getHeaderValues now reads all occurrences, and MAX_REPLY_MESSAGE_IDS is checked in the innermost token loop so it bounds the total across every occurrence of both headers rather than per-header — the widened input feeds a sender-supplied IN (...), so the cap has to hold after the change, and a test feeds 200 References lines to prove it. HtmlBody was never read, so an HTML-only email — Outlook, routinely — produced an empty description, an empty opening message, and an AI job over an empty question that the pipeline answered with its apology fallback. resolveMessageBody now falls back StrippedTextReply -> TextBody -> htmlToText(HtmlBody). No dependency added: nothing suitable is in the tree (jsdom is dev-only, entities is transitive under react-markdown), so htmlToText is a conservative tag-strip plus entity-decode. Its documented limits are real and worth reading before trusting it on rich mail — no DOM so table cells flatten to lines, newline runs collapse, an unterminated <style> leaks CSS, a > inside an attribute ends a tag early, hrefs are dropped, and the named-entity list is curated. A proper parser is the better answer if this path ever carries volume. An email with no usable text after every fallback still creates a ticket — a real customer message must not vanish, and the subject plus attachments are evidence — but enqueues NO AI job. An apology fallback would be delivered and would mark the ticket answered, spending its one response on nothing. Verification: turbo typecheck 10/10, turbo test 19/19 (1923 tests). Every fix carries red-green verification; 14 mutations were each observed RED and restored. The migrations were applied twice against a real Postgres engine with duplicates seeded, confirming they are re-runnable and leave DISCORD keys and NULL rows alone. Note on composition: the three fixes were developed independently, each verified green on its own, and all three patches applied with no text conflict — and the combination still failed four tests. C1's authorization correctly refused replies whose fixtures predated it. Nothing was wrong with either change; a clean merge is not evidence of correct composition. The fixtures now carry participant data. Follow-up work split out of this same review, tracked separately: worker correctness (the poll loop that never binds per-type concurrency limits), the AI response state machine (recovery paths that drop an owed escalation, and no sweeper for stranded PENDING responses), and the /health rework, which cannot land until the worker poll loop is fixed because the poll loop is what makes it report 503 on a healthy worker.
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
Reviewed the three concerns independently against the diff, not the description.
Authorization (the actual #189 fix) — correct. Both reply paths now converge on the same two gates: source: 'EMAIL' scoping and isTicketParticipant. Two details I specifically checked and liked:
isTicketParticipantrefuses to infer a domain fromuser.emailor a message author, and only trusts the human-setAccount.domain. That is the difference between a fix and a re-opened hole — deriving the domain would have made everygmail.comsender a participant on most consumer tickets.normalizeParticipantEmailgating on a real address regex means the non-email authors already inMessage.author(Outpost AI,System,slack:U123,octocat (583231)) can't be impersonated into participation.
Failing a gate falling through to header matching and then to orphan handling — rather than dropping — is the right call, and the hash/header paths no longer being mutually exclusive is a genuine bug fix on its own.
findUnique({ where: { displayId, source: 'EMAIL' } }) is valid here — extendedWhereUnique has been GA since Prisma 5 and this repo is on 6.2.
Idempotency — correct, and the reopen placement is the load-bearing part. Running the reopen only on the branch that actually inserted, inside the same transaction as the insert, closes both directions: no reopen of a ticket a human resolved in between, and no committed-message-without-reopen that every later retry silently no-ops. Answering 200 with duplicate: true is right — a non-2xx here would keep Postmark retrying a delivery we have fully processed.
Both migrations dedupe before constraining, and neither deletes a row. Deterministic winner (ORDER BY "createdAt", id), value preserved at additionalInfo.duplicateEmailSourceId, IF NOT EXISTS, re-runnable. Given prisma migrate deploy runs under set -e in the worker start script, an unguarded CREATE UNIQUE INDEX really would have been a boot-blocking landmine, so this matters more than it looks.
Parsing. Moving the MAX_REPLY_MESSAGE_IDS check into the innermost token loop so it bounds the total rather than per-header is the right fix, and the 200-References-lines test asserting exactly 50 survive pins it.
On htmlToText — you flagged this as the part you most wanted a second opinion on. I'd ship it. The limits are documented precisely rather than optimistically, decoding after stripping is the correct order (an escaped <p> never gets re-read as a tag), and comments are removed first so Outlook conditional-comment layouts don't survive as text. For a path with zero current traffic, a curated tag-strip behind a swappable signature beats a supply-chain add. Revisit if it ever carries volume.
Two non-blocking follow-ups, neither worth holding this:
findTicketByReplyMessageIdsstill resolves appended messages throughattachments.postmarkMessageId— up to 50 OR'd unindexed jsonb path scans — even though this PR addssourceMessageIdas an indexed column and backfills it.sourceMessageId: { in: messageIds }would be one indexed lookup. The jsonb read is only still needed for the historicalrn > 1rows the backfill deliberately left NULL, so a union of the two (or just accepting that gap) would drop the hot-path cost.getHeaderValueis now referenced only from the test file — the production code all goes throughgetHeaderValues. Worth deleting so nobody reaches for the first-match-only version later, which is exactly the bug this PR fixed.
Approving. This closes the only open security issue on the launch path, and outpost#215 (Postmark DNS for support.copilotkit.ai) is now unblocked to follow.
CPK-7923 1. Merge PR #190 — Postmark reply integrity (closes the security hole)
PR #190 — Merge this one first. Two reasons: it closes outpost#189, where an inbound email carrying any ticket's display ID can append to and reopen that ticket on any channel — the only open security issue on the launch path. And it is the smallest of the three splits, so it costs the other two the least rebase. Its only overlap with #191 is Ordering constraint elsewhereoutpost#215 — configuring Postmark DNS for |
Resolves one conflict in packages/outpost/db/prisma/schema.prisma, in `model Message`. Both sides added distinct fields in the same region and neither touched the other's, so the resolution is a union: - this branch: `sourceMessageId` + @@unique([ticketId, sourceMessageId]), the indexed dedup slot for inbound provider message IDs (Postmark today). - main (#191): `responseKey`, `responseState`, `responseJobId`, `responseError`, `deliveryConfirmed`, `escalationRequiredReason`, @@unique([ticketId, responseKey]), and @@index([responseState, createdAt]) for PENDING_RESPONSE_SWEEP. The shared tail (`attachments`, `createdAt`) is kept once. The two unique constraints cover different columns and do not interact. Verified on the merged result, not assumed: - prisma validate (workspace-pinned binary): schema is valid - all five migrations present from both sides; none dropped in the merge - typecheck 10/10 - full suite green: 1075 shared, 679 web, every package passing
2 participants
One of three PRs split out of #187. Scoped to the inbound email door so it can be reviewed and reverted on its own. Siblings and deferred work are named at the bottom — I'll fill in their numbers as they open.
Three concerns, one subject: what the Postmark webhook does with a message claiming to be a reply.
1. Authorization — closes #189, live on
maintodayThe
MailboxHashpath resolved a ticket withfindUnique({ where: { displayId } }):source: 'EMAIL'filter — an email could append to a Discord, Slack, Teams or GitHub ticketdisplayIdcould append and reopenThe reasoning that a token we mint is trustworthy doesn't survive the fact that we minted it and then published it.
generateTicketId()draws 8 characters from a 32-character alphabet withMath.random(), and the bots posted🎫 Ticket TKT-XXXXXXXX createdinto public Discord and Slack threads until #172 removed that. Those threads still carry the IDs.Resolution is now scoped to
source: 'EMAIL'and gated on the sameisTicketParticipantrule the header path already used — reused unchanged, so there's no second, subtly different definition of "participant."Failing either gate means unresolved, not dropped: the mail falls through to orphan handling, so a legitimate sender writing from an unrecognised address still gets a ticket.
The hash and header paths were also mutually exclusive via
if/else, so an unresolvable hash skipped header matching entirely even though a mail can legitimately carry both. Fixed.Not currently exploitable — production and staging both hold 0 EMAIL tickets, so Postmark inbound has never created one. Safe by luck, not construction.
2. Idempotency
appendReplyToTicketdid a baremessage.createwhile the new-ticket path was guarded. Postmark retries every non-2xx and this route's own catch returns 500 — so a retried reply duplicated the customer's message and re-ran the reopen, putting a resolved ticket back in the queue twice for one email.Dedup is a real column —
Message.sourceMessageIdwith@@unique([ticketId, sourceMessageId])— not a JSON path. jsonb can't carry a unique constraint, andattachments.postmarkMessageIdwould have added a second unindexed scan to the hot path of every reply. A pre-read short-circuits the common retry on an index hit; the constraint is the authority on a race. Same two-level pattern the new-ticket path already uses.The reopen runs only on the branch that actually inserted, so a redelivery can't reopen a ticket a human resolved in between, and insert + reopen share one transaction. That closes the mirror gap a naive dedup opens: message commits, reopen is lost, and every later retry is a silent no-op that never reaches a human.
A duplicate answers 200 with
duplicate: true— 2xx so Postmark stops, marked so retry storms stay legible.The migration was a deploy landmine
20260811230000ran an unguardedCREATE UNIQUE INDEXover historical data, and nothing deduped before it. A database with duplicate EMAILsourceIds would fail to build it — and underset -einstart.sh, the worker never boots.It now dedupes deterministically first: oldest ticket per
sourceIdkeeps the key, later ones getsourceId = NULLwith the value preserved atadditionalInfo.duplicateEmailSourceId. No rows deleted,IF NOT EXISTS, fully re-runnable. A tolerant index was rejected — a constraint with holes that reads as enforced is worse than none.Edited in place rather than renamed, which is safe: that migration is applied nowhere. Production holds
0001_init,20260715…,20260812220000; staging holds the first two. Only a developer who ran the pre-split branch locally needs their_prisma_migrationsrow refreshed.3. Parsing
Repeated headers.
getHeaderValueread only the first match, butReferencesis legitimately split across lines by some clients — so a reply whose matching Message-ID sat in a later segment failed to thread and became a new ticket. Now reads all occurrences, andMAX_REPLY_MESSAGE_IDSmoved into the innermost token loop so it bounds the total across every occurrence of both headers rather than per-header. The widened input feeds a sender-suppliedIN (…), so a test feeds 200Referenceslines and asserts exactly 50 survive.HTML-only mail.
HtmlBodywas never read, so an Outlook-style HTML-only email produced an empty description, an empty opening message, and an AI job over an empty question that the pipeline answered with its apology fallback.resolveMessageBodynow falls backStrippedTextReply → TextBody → htmlToText(HtmlBody).No dependency added, and this is the part I'd most like a second opinion on. Nothing suitable is in the tree, so
htmlToTextis a conservative tag-strip plus entity-decode with real limits: no DOM so table cells flatten to lines, newline runs collapse, an unterminated<style>leaks CSS, a>inside an attribute ends a tag early, hrefs are dropped, and the named-entity list is curated. A proper parser (html-to-text) is the better answer if this path ever carries volume — the tradeoff is a supply-chain add for a path with currently zero traffic.No usable text after every fallback still creates a ticket — a real customer message must not vanish, and the subject plus attachments are evidence — but enqueues no AI job. An apology fallback would be delivered and would mark the ticket answered, spending its one response on nothing.
Tests
1,923 pass, typecheck 10/10. 14 mutations each observed RED and restored to GREEN. Both migrations were applied twice against a real Postgres engine with duplicates seeded, confirming they're re-runnable and leave DISCORD keys and NULL rows alone.
Two tests that encoded the vulnerable behaviour were inverted rather than deleted, and the
findUniquemock now enforceswhere.sourcethe way Postgres would — without that, the cross-channel test would have passed against the vulnerable code.A note on composition, for whoever reviews the siblings
The three fixes here were developed independently, each verified green on its own, and all three patches applied with no text conflict — and the combination still failed four tests. The authorization fix correctly refused replies whose test fixtures predated it. Nothing was wrong with either change. A clean merge is not evidence of correct composition; the full suite after consolidation is.
Siblings and follow-ups
Split from #187, which stays open until all three exist and is then closed pointing at them.
AI_RESPONSEwork. Also the reclaim predicate, silentcount === 0on fenced writes, and unregistered types being terminally FAILED. (PR number to follow)(ticketId, responseKey)claim, recovery paths that return success when the escalation CAS queued nothing, no sweeper for strandedPENDINGresponses, andresponseErrorbeing nulled over the error just written. (PR number to follow)/healthboot rework — deferred a second time, and correctly. Four reviewers found it makes a healthy worker report 503 and get restarted mid-job, becauseSTALE_POLL_MSis 60s against 120s/300s job timeouts. It cannot land until the worker poll loop is fixed, since the poll loop is what makes it wrong.postResponsestill throws, so an email ticket's answer is generated, stored, undeliverable, and escalated) · Decide the generator's empty-response behaviour #178 generator empty-response behaviour · Document the Prisma partial-index drift on Ticket_email_sourceId_key #179 the raw-SQL partial index invisible tomigrate diff