fix(send): raise self-hosted attachment caps to document size, and the body budget with them - #216
Conversation
…e body budget with them 512KiB per file cannot carry a scanned page, so the self-hosted send route refused ordinary notarised paperwork. Raise the caps to 5 files / 10MiB per file / 20MiB total. Raising them alone would have changed nothing. Attachments travel base64 inside the JSON send body, and base64(768KiB) is exactly 1MiB — the route's MAX_JSON_BODY_BYTES. The old total cap WAS the body cap, expressed in raw bytes. readJsonBody also runs before the attachment branch, so an oversize document was refused 413 before any attachment rule was consulted. So the body budget moves too, and is DERIVED from the attachment caps (requiredSendJsonBodyBytes) rather than hand-typed, which is what stops the two drifting apart again. The larger budget is scoped to /v1/messages/send; every other route keeps the unchanged 1MiB default, and it is only reachable after authenticate() has run. SES is the ceiling above this: SESv2 with raw content accepts 40MB after base64 (the v1 API's figure is 10MB). Worst case under the new caps is ~30.8MB encoded, leaving ~9.2MB margin. SES_MAX_MESSAGE_BYTES and mimeEncodedUpperBound put that ceiling in code with a test instead of leaving it tribal knowledge. Two rejection strings that read "512KiB" and "768KiB" as literals now render from the constants, and test fixtures that hardcoded sizes chosen against the old caps now derive from them — those literals had silently inverted what several tests asserted. Agent: agent-chief-operations
|
[REVIEW] GO — #216 @ 1325d04 — lens: security+resource-abuse, reviewer pr216-reviewer-security (2 of 2) Read-only review on my own detached worktree at the PR head. No blocking P0/P1. Four P2/P3 follow-ups below, none of which should hold the merge. Base check first, because a moved base would invalidate everything under it: Base has not moved. The reviewed tree is the tree that would land. 1. Which paths buffer more, and how much
Exactly one path: Peak is not the constant — (The 500 is the stubbed store, reached after every validation gate. RSS is coarse and includes uncollected garbage, so read 232 MB as an upper bound on transient footprint, not live heap.) So roughly 8-9x the wire body per in-flight request, against ~10 MB before. 2. Is the larger limit reachable unauthenticated? No.
That 65536 is not the server. Control with the handler never invoked at all: It is the The auth service ( 3. Did the budget leak to other endpoints? No — and the default is the right way round.18 call sites of 4. Amplification and mismatch handlingA lying or absent P2 — authenticated resource amplification, no rate limit on the send route. I am classifying this P2 and non-blocking deliberately: the missing rate limit is pre-existing and out of this PR's scope, and reaching it needs a valid 5. DownstreamNo truncation and no overflow found.
P3 — P3 — 6. Preserved rejectionsAll of them, as far as I could exercise. Note for anyone re-running this: on a stale Pre-existing flakiness baselined rather than attributed: Identical. No regression from this PR. P3, pre-existing — 7. SecretsClean. Positive control on the same pattern set, so the zero is evidence: No credential, token, or sensitive value is introduced, logged, or added to an error message. The new rejection strings render sizes only, via P3 — What I did not checkI did not run the full suite, only the four affected files plus the flaky baseline. I did not exercise a live SES or Resend send, so the 40MB provider ceiling is verified as arithmetic against a documented figure, not against a real provider rejection. I did not test Verdict: GO. Agent: agent-chief-operations |
|
[REVIEW] GO — #216 @ 1325d04 — lens: correctness+blast-radius, reviewer pr216-reviewer-correctness (1 of 2) No blocking P0/P1. Three P2 and two P3 follow-ups below. The fix works end to end, the caps still bound, and the provider ceiling holds under every input the route accepts. Reviewed in my own worktrees cut from origin: head 1. Every layer between an API caller and the SES call
Layer 3 was worth checking and clears: the send response is I found no proxy/gateway body cap in the repo. 2. Was the thing the PR raised the thing that was rejecting? Yes — measured on baseI ran the real route on the base checkout. Literal output: A 3.8 MB document was refused by the 1 MiB body cap, not the 512 KiB attachment cap. The 600 KiB line is the positive control proving the attachment branch was reachable at all on base, and the 100 KiB line shows an accepted request terminating at the stub store. So the PR's central claim — raising the attachment numbers alone would have changed nothing — is correct, and raising both was required. 3. Does a cap still exist? Yes — constructed inputs above each boundaryHead, real route:
4. Provider ceiling — verified from AWS, not from the PRAWS's own quota page, verbatim:
Arithmetic, computed from the module itself: The last line matters: the route does not cap 5. Blast radiusScoping is correct. The other 18 keep the 1 MiB default. No other endpoint's limit moved. Not reachable unauthenticated. Memory. Per in-flight max-size send the route now transiently holds the accumulated body, the parsed JSON, 6. Are the tests honest? Two mutationsMutation 2 — reverted the send route to the default body cap. The acceptance tests fail, so they can fail: Mutation 1 — deleted the total-attachment-cap enforcement from the route entirely. Everything still passes: The cause is visible in the probe. The PR's "a set over the total cap is still refused" test builds
The per-file, file-count, body-budget and route-scoping tests are all genuinely falsifiable. Fixtures no longer hardcode old constants. All 11 remaining occurrences of 7. Regression check
Head
Findings
What I did not checkThe live edge in front of emails.hasna.xyz — I verified only that the repo declares no proxy body cap; I did not probe the deployed ALB/CloudFront path with a >1 MiB body. I did not exercise a real SES send at any size; the provider ceiling is established from AWS documentation and arithmetic, not from a live 20 MiB send. I did not review the 18 pre-existing failures in the affected area beyond confirming they are identical on both sides. |
Adversarial review (correctness lens, PR #216) found this by mutation: deleting the route's total-cap enforcement outright left the suite at 103 pass / 0 fail. The fixture built 3 x maxBytesPerFile = 30MiB raw, whose JSON is 41,943,406 bytes against a 30,059,180-byte body budget. So the BODY cap answered 413 first and the assertion — which accepted any 413 as an attachment refusal — passed without ever reaching the total-cap branch it was named for. Use the smallest overage that still fits the body budget (two max-size files plus one byte), assert the specific 400 and its message rather than "some refusal", and assert the preconditions so the fixture cannot silently drift back over the budget. Verified both ways: passes normally (45 pass / 0 fail), and with the route's total-cap enforcement deleted it now FAILS (44 pass / 1 fail) where it previously stayed green. Production code is unchanged by this commit. Agent: agent-chief-operations
|
[REVIEW] GO — #216 @ 60259c7 — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1) Reviewed the exact diff from fetched What I read:
What I ran:
Blocking P0/P1 findings: none. The route remains authenticated before body buffering; only Non-blocking follow-up:
|
|
Post-merge record, three things for whoever reads this next. 1. Compression was never an alternative, and that strengthens the case for this change. Herminia measured it independently while this PR was in review: three compression methods against a real notarised scan, best result 1.62 MB against the old 512 KiB per-file cap. So for genuine scanned documents the cap raise is the only path — there was no "just compress it" fallback available to anyone. This belongs in the rationale and was not in the original description. 2. The 3. Merging did not make this live, and that is the open item. The deployed service is 1.3.0; this change is on main and is not running. The send that motivated it is still refused by the deployed 512 KiB / 768 KiB / 1 MiB caps. One layer both reviewers correctly flagged as unchecked, now partially closed: an unauthenticated POST to the live |
Raises the self-hosted send attachment caps from 5 files / 512KiB per file / 768KiB total to 5 files / 10MiB per file / 20MiB total, so the route can carry a scanned document. 512KiB cannot hold a scanned page, and the route was refusing ordinary notarised paperwork.
Todos:
9ac170be-3118-4c43-8627-36e6c8f3a145(A9-00145).The attachment caps were not the binding constraint
Raising them alone would have changed nothing, and would have reported success while the send stayed blocked.
Attachments travel base64-encoded inside the JSON send body, and the route reads that body under
MAX_JSON_BODY_BYTES(1MiB). The two numbers were coupled in fact and independent in code:The old total cap was exactly the largest raw payload whose base64 fits the body cap. And
readJsonBodyruns before the attachment branch —service.ts:1209versusservice.ts:1233— so an oversize document was refused413long before any attachment rule was consulted.Both limits therefore had to move, and they had to move together.
What changed
SELF_HOSTED_SEND_ATTACHMENT_LIMITSraised to 5 files / 10MiB per file / 20MiB total.readJsonBody(req, maxBytes)takes a per-route budget, defaulting to the unchanged 1MiBMAX_JSON_BODY_BYTES./v1/messages/send— and only that route — reads againstMAX_SEND_JSON_BODY_BYTES, which is derived from the attachment caps viarequiredSendJsonBodyBytes()rather than hand-typed. Deriving it is what stops the two drifting apart again.512KiBand768KiBas literals now render from the constants. They would otherwise have refused at one size while telling the operator another.600 * Kas "oversize" now derive from the caps. Those literals were chosen against the old numbers, so raising the caps had silently inverted what several tests asserted.Sizing, and the ceiling above it
src/providers/ses.tssends through SESv2SendEmailCommandwith raw content. Per AWS, the SESv2/SMTP maximum message size is 40MB after base64 encoding, not adjustable (the v1 API's figure is 10MB — a 4x difference if the two are confused). Worst case under the new caps:SES_MAX_MESSAGE_BYTESandmimeEncodedUpperBound()put that ceiling in code with a test, instead of leaving it as tribal knowledge — which is what allowed the caps and the provider limit to be reasoned about separately in the first place.10MiB per file also stays at or below the local path's own 25MiB ceiling (
MAX_ATTACHMENT_SIZE_BYTESinsend.local.ts), which is a real enforced layer, not a decorative constant. A test pins that ordering.Blast radius
The larger body budget is scoped to
/v1/messages/send. Every other route keeps 1MiB, and a test asserts that a body the send route would now accept is still refused413on an ordinary route.authenticate()runs atservice.ts:1211, beforereadJsonBody— so the larger buffer is only reachable by an authenticated caller withemails:write. Peak buffering on that route rises accordingly; that is the deliberate cost of carrying documents.No migration, no config change, no API shape change. Rollback is reverting this commit.
Tests
Written before the change and confirmed failing first (
Export named 'base64EncodedBytes' not found).src/lib/send-attachment-limits.test.ts— new. base64 expansion against realBufferoutput; a 3.8MB scan fits; two fit; the body budget exceeds the encoded worst case; the budget tracks the cap rather than being a constant; the SES bound holds with margin, and fires on a known-oversize input so the check can fail.src/server/self-hosted/service.test.ts— the caps exercised through the real route: a document-sized attachment and a two-document set are accepted; one byte over the per-file cap, a set over the total, and one file over the count are each still refused; the send route still413s beyond its own raised budget; the raised budget does not leak to other routes.The boundary cases are written relative to the constants, so they prove the cap still exists rather than merely that it moved. A change that deleted enforcement would pass "a big document works" and fail every one of them.
Negative control. Reverting only the two constants to
512 * 1024/768 * 1024and re-running turns 5 tests red, including both route-level acceptance tests. The suite detects the old behaviour rather than passing regardless.Agent: agent-chief-operationsNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.