Repository navigation
feat: tell applications about received mail (inbound Phase C) - #62
Merged
Merged
Conversation
An event was always about a message SESKit sent, so email_events.email_id
was required. A message that arrives has no emails row, and everything a
webhook needs - signing, retries, auto-disable, the sweep, the deliveries
log - is keyed to an event. Giving received mail its own events and
deliveries would mean writing all of that twice, and keeping the two in
step for ever. So an event now belongs to one of two parents instead.
email_id becomes nullable and inbound_email_id is added beside it. A check
constraint holds that exactly one is set: neither would leave an event
nothing can be delivered for, since the project is found through the
parent, and both would make it ambiguous which. The database holds it, so
no path through the code can break it; tests assert both violations are
refused. Deleting a received message deletes its events, as deleting a
sent one always has.
The blast radius is small, and was checked rather than assumed. Only
queue_deliveries finds a project through the email; the metrics queries
inner-join emails, so received events are excluded from sent-mail rates,
which is correct. That lookup is the next commit.
The public payload carries inbound_id where an email event carries
email_id, and never both. A receiver switching on which field is present
has to be able to trust what it refers to, and GET /v1/emails/{id} must
never be handed an inbound id. The sent payload is asserted unchanged:
it is the contract every existing receiver relies on.
Existing rows keep their email_id and gain a NULL inbound_email_id, the
only shape that satisfies the constraint, so nothing is rewritten. The
constraint is added NOT VALID and validated separately, which takes a
weaker lock on a table that grows with every message anyone sends. The
downgrade deletes received events, since they have no email_id to restore
and reverting to NOT NULL would otherwise fail; the migration says so.
Checked offline: the SQL emits as intended in both directions and there is
one head. The check that runs it against a database and diffs it against
the models is tests/test_migrations.py, in CI.
When a received message is recorded, an email.received event is raised in the same transaction and queued for every active webhook endpoint of the project. After the commit the worker asks the queue to attempt the deliveries now; the sweep covers anything that fails to enqueue. The payload describes the message and does not carry it. No body, no headers, no attachment bytes: it stays small however large the mail is, and a receiver that wants more asks for it with its API key. The subject and sender are in it because a receiver deciding whether to bother fetching needs them. `to` is the header and `recipients` is who the message was addressed to on the wire, which is the address that received it and can be one the headers never mention. There is no url field: a receiver knows its own API host, and an absolute one would depend on PUBLIC_BASE_URL, which may be unset. A test asserts the signed wire bytes, not just the accessor, and that none of the body or headers is in them. A verdict SES did not give is null in the payload, never PASS. A receiver that filters on spam would otherwise treat an unchecked message as clean, and a test records a notification with the verdicts removed to prove it. Raised in the same transaction as the row, so a webhook cannot be lost to the process dying between the two. A duplicate returns before it gets there, so it raises no second event and queues no second delivery. occurred_at is when SES received the message: a backlog means SESKit can hear about mail long after it arrived, and created_at on the payload says when it happened. Mail for one project never reaches another project's endpoint. The payload carries the sender, the recipients and the subject, so a delivery to the wrong project is a leak, and a test registers an endpoint on a second project and asserts it is not queued. queue_deliveries finds the project through whichever parent the event has. That was the only place that assumed every event had a sent email. A failed enqueue does not keep a recorded message on the queue. The message is stored and the delivery row exists, so leaving the announcement would only make it return to be recognised as a duplicate.
GET /v1/inbound pages through what a project received, newest first,
without bodies. GET /v1/inbound/{id} returns one message in full: bodies,
headers, attachments described, and what SES concluded about it. Nothing
here writes; the worker records a message as it arrives and this says what
is there.
The list is a summary, because a page of a hundred messages with their HTML
in it is a response nobody asked for. Both shapes are built by hand from
the stored row and not with from_attributes: the row stores things in
shapes that suit the database, and a public contract should not move when
the table does.
Paged by cursor for the reason /v1/emails is. A wrong page does not raise;
it returns real rows in a plausible order and leaves some out. A message
arriving between two pages shifts every row down one under an offset and
the reader skips the one that moved across the boundary, so ids name a
fixed point instead - a test inserts mail between pages and asserts nothing
is skipped or repeated. An unknown cursor is a 404 and not an empty page,
and so is one belonging to another project: the comparison is lexical, so a
foreign id would otherwise position a page of real messages by a value the
caller cannot see. Sorted by id and not received_at; a backlog means the two
can differ and only id order is one a cursor can be stable over.
An id belonging to another project is a 404, never a 403, which would
confirm it exists, and a test asserts the response is identical to one for
an id that never existed. Nothing internal appears in a response - not the
bucket, not the storage key, not the project id - and a test greps both
shapes for them. A client has no use for where somebody's mail is stored.
HTML and headers are returned exactly as sent, with the field description
saying to treat them as hostile; a test sends a script tag and asserts it
arrives unaltered, because an API that quietly sanitises would be a second
place for a consumer to be wrong. raw_available says whether the original
is expected to still be in storage, and is true when unknown: the parsed
message outlives the bucket on purpose, so a message past retention is
still fully readable.
Regenerates the OpenAPI document and the API reference, which CI checks; the
docs tests that require every field and parameter to be described pass.
GET /v1/inbound/{id}/attachments/{index} returns one attachment's bytes and
GET /v1/inbound/{id}/raw returns the message exactly as SES received it, as
an .eml. Both fetch the stored message from the project's bucket, on the
project's own key in the project's own region.
These serve bytes a stranger wrote, from the API's own origin, and that is
the whole risk. Whatever the sender declared, the response is a download and
not a document: always application/octet-stream, Content-Disposition:
attachment, nosniff so a browser cannot decide the bytes are HTML, and a CSP
sandbox as what is left if one renders it anyway - no script, no forms, no
same-origin access. no-store, because this is somebody's private mail. A test
sends a file that declares itself text/html and contains script, and asserts
it arrives unaltered and as opaque bytes: the API contains hostile content
and does not pretend to clean it.
Filenames are the last thing between a sender's string and a header. The
parser already reduces them, and content_disposition does not rely on that:
both the plain and the encoded form are built from the same basename, so
neither can carry a path or a name made only of dots, and an empty name
becomes "attachment" in both. Probed against a quote breakout, a CRLF
Set-Cookie, a backslash path and a NUL before it was trusted, and a test
sends a message whose attachment name carries a path and a CRLF-delimited
Set-Cookie through RFC 2231, then asserts on the bytes of the response that
no such header exists.
A request that can be refused without touching S3 is refused first, because
each fetch is a transfer of up to 40 MB and one the caller could have been
told about for free is a cost. An attachment index that does not exist, a
message past retention, an id from another project and a project that is not
connected all fail before any fetch, and tests assert the store was not
asked. Ownership is checked before anything is fetched, so a stranger cannot
make the server read somebody else's mail even to be told it is not theirs.
Retention removes objects at a day boundary, so the date can say "still
there" while the bucket says otherwise; that is a 404 with the same message
and not a server error. A missing s3:GetObject is reported as a permission
failure and not as a missing message, because a user told their mail is gone
when it is a policy to fix would reasonably panic. The parsed message outlives
all of this on purpose.
The fakes for received mail go into app_client by default, for the reason the
provider and provisioner fakes are there: no test should be able to reach AWS
by forgetting to substitute something. That also closes a gap left when the
inbound provisioner dependency was added without an override.
client.inbound.get, .list, .attachment and .raw, on both SesKit and AsyncSesKit, over the endpoints the API now has. Thin like the rest of the SDK: the request builders are shared, so the two clients cannot disagree about a URL or a parameter name, and nothing here validates or interprets what the API returned. Downloads are bytes, and the transport hands them back untouched. A request now says it wants a file, and a success is returned exactly as received: decoding it as JSON fails and decoding it as text corrupts it. A test sends a payload with a NUL, an 0xff and a lone 0x80 through it and asserts it comes out identical. Errors are still JSON envelopes whichever was asked for, so a download that fails raises the same typed exception, and it shares the retry rule with every other call. An id is interpolated into a URL, and `../emails` is a valid string. Quoted, it stays one path segment that the API says it does not know, and never a different endpoint answering with a message that parses as the wrong thing. A test sends it through a stub and again through the real API, along with a slash, a query, a fragment and a CRLF. The attachment index is forced through int so nothing but a number can reach the path. (emails.get does not quote its id; that predates this and is left for its own change.) A verdict the API left null stays None and is never read as a pass. The body fields carry the sender's content untouched and the docstrings say to treat html as hostile. A list returns summaries and not full messages, as the API does, so a page of a hundred has no bodies in it. Tested twice, as the rest of the SDK is: against a stub, for the exact bytes sent, and against the real application with Postgres behind it, which is the tier that catches the SDK and the server disagreeing about a field name.
SecurityHeadersMiddleware assigned the dashboard's policy to every response, including /v1, so a route that set its own was silently overwritten. The attachment download sets `sandbox; default-src 'none'` as the layer that is left if a browser renders a stranger's file despite octet-stream, attachment and nosniff. It never reached the client: the response carried `script-src 'self'` instead. The stricter header was discarded and the looser one shipped, with nothing anywhere to say so. Found by the download tests, which assert the header on the response and not on the code that sets it - which is the only reason it was found. The middleware now adds the dashboard policy only when the route has not set one. The rest of the headers are still applied either way, and a route that sets none still gets the dashboard's, so the change cannot leave a page without a policy; a test asserts both halves. The guard was run against the old middleware and fails there. Also fixes the download tests' own helper, which passed `attachments` twice when a test overrode it.
Otitodev
force-pushed
the
feat/inbound-api
branch
from
October 5, 2026 05:36
9473751 to
38c5f14
Compare
The events reference gains `email.received`: the payload, every field, and what makes it different from the others. It carries `inbound_id` and no `email_id`, and it describes the message without carrying it - no body, no headers, no attachment - so the call that fetches the rest is named beside it. A verdict of null is documented as "Amazon SES did not say" and explicitly not a pass, since a reader filtering on spam would otherwise treat an unchecked message as clean. The page also says every active endpoint receives it and that a receiver which validates `type` against a fixed list should add it before turning receiving on, because that is the one way this could surprise somebody. The SDK reference gains the four calls, with the two things a reader most needs to be told: attachment and raw return bytes untouched and stop working when retention removes the original while the parsed message does not, and html in a received message is the sender's and has to be treated as hostile. The security model gains a section for what is a third untrusted boundary and the first one SESKit serves from: why an announcement is believed only if it is ours, why a download is a download whatever the sender declared, why the API does not clean what it returns, why verdicts are recorded and never acted on, and what is and is not logged. Its response-headers row now says a route may set a stricter policy of its own and keep it, which was not true until the previous commit, and its "not covered yet" list says plainly that received mail is not quarantined and attachments are not scanned by SESKit itself. The API reference is generated and was regenerated with the endpoints; this is the prose. A link to an endpoint uses the generator's own anchor, which mkdocs --strict confirms.
Otitodev
changed the title
Otitodev
marked this pull request as ready for review
October 5, 2026 05:43
1 task done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Phase C of receiving mail: telling applications about it. Phases A and B (#60, #61) built the plumbing and stored received messages; this makes them reachable. An
email.receivedwebhook fires when mail arrives, a/v1/inboundAPI reads it, and the Python SDK wraps both. Nothing here turns receiving on, which is Phase D, so none of it is reachable by a user yet.Six commits, one concern each:
email_idwas required. Rather than write all of that twice for received mail, an event now has exactly one of two parents, held by a check constraint.GET /v1/inbound,GET /v1/inbound/{id}).client.inbound.get / list / attachment / raw, sync and async.What to review hardest:
Content-Security-Policyto every response, so a route's own policy was silently overwritten. The download setssandbox; default-src 'none'as the layer that holds if a browser renders a stranger's file anyway. It never reached the client: responses carriedscript-src 'self'instead. The download tests assert the header on the response, which is how it was found. The middleware now keeps a policy the route set, and a test asserts both halves, so no page is left without one. The guard was run against the old code and fails there.application/octet-stream,attachment,nosniff,sandbox. The filename is reduced to a basename in both header forms. A test sends a file that declares itself HTML with script in it, and a name carrying a path and a CRLF-delimitedSet-Cookie, and asserts on the bytes of the response.verdictsthat SES did not give arenull, neverPASS, and a test records a notification with them removed.Decisions made while building, so you can overrule them: the payload includes
from,toandsubject; attachments are alwaysoctet-streamwith no allowlist;GET /v1/inbound/{id}/rawis included; there is nourlfield in the payload, since a receiver knows its own API host and an absolute one would depend onPUBLIC_BASE_URL.Existing receivers are unaffected until someone turns receiving on, since there is nothing to turn it on with yet. The events reference says a receiver that validates
typeagainst a fixed list should addemail.receivedfirst.Not yet verified against a real AWS account: that SES accepts a rule publishing to our topic without a topic policy (moto does), and whether a rule's key prefix appears in the reported
objectKey. Nothing in ingest depends on the second, but fetching does.emails.getin the SDK does not quote its id, so an id containing a slash can reach another path. That predates this PR and is left for its own change; the new inbound calls do quote.Checks
tests/test_migrations.pypasses in CI. I generated the SQL offline in both directions and checked it; I did not runalembic upgrade headagainst a database myself.uv run --group docs mkdocs build --strictis clean🤖 Generated with Claude Code