Skip to content

fix(send): raise self-hosted attachment caps to document size, and the body budget with them - #216

Merged
andrei-hasna merged 2 commits into
mainfrom
fix/9ac170be-send-attachment-cap
Aug 8, 2026
Merged

fix(send): raise self-hosted attachment caps to document size, and the body budget with them#216
andrei-hasna merged 2 commits into
mainfrom
fix/9ac170be-send-attachment-cap

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

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:

768 KiB raw                = 786432 bytes
base64(786432)             = 1048576 bytes
MAX_JSON_BODY_BYTES        = 1048576 bytes   <- equal

The old total cap was exactly the largest raw payload whose base64 fits the body cap. And readJsonBody runs before the attachment branch — service.ts:1209 versus service.ts:1233 — so an oversize document was refused 413 long before any attachment rule was consulted.

Both limits therefore had to move, and they had to move together.

What changed

  • SELF_HOSTED_SEND_ATTACHMENT_LIMITS raised to 5 files / 10MiB per file / 20MiB total.
  • readJsonBody(req, maxBytes) takes a per-route budget, defaulting to the unchanged 1MiB MAX_JSON_BODY_BYTES.
  • /v1/messages/send — and only that route — reads against MAX_SEND_JSON_BODY_BYTES, which is derived from the attachment caps via requiredSendJsonBodyBytes() rather than hand-typed. Deriving it is what stops the two drifting apart again.
  • The two rejection strings that read 512KiB and 768KiB as literals now render from the constants. They would otherwise have refused at one size while telling the operator another.
  • Preflight test fixtures that hardcoded 600 * K as "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.ts sends through SESv2 SendEmailCommand with 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:

20 MiB raw attachments -> base64            27,962,028 bytes
  + MIME 76-char line wrapping (78/76)      28,697,868 bytes
  + envelope headroom (2 MiB)               30,795,020 bytes
SES ceiling                                 40,000,000 bytes
margin                                       ~9.2 MB

SES_MAX_MESSAGE_BYTES and mimeEncodedUpperBound() 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_BYTES in send.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 refused 413 on an ordinary route.

authenticate() runs at service.ts:1211, before readJsonBody — so the larger buffer is only reachable by an authenticated caller with emails: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 real Buffer output; 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 still 413s 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 * 1024 and 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-operations


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…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
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[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:

origin/main    = 64c80aac271b81c1501bb93cb4e7a2975417e791
PR baseRefOid  = 64c80aac271b81c1501bb93cb4e7a2975417e791
merge-base     = 64c80aac271b81c1501bb93cb4e7a2975417e791
git rev-list --count <merge-base>..origin/main  ->  0

Base has not moved. The reviewed tree is the tree that would land.

1. Which paths buffer more, and how much

MAX_SEND_JSON_BODY_BYTES computed from the real constants, not from the PR description:

maxTotalBytes            = 20971520 (20 MiB)
base64(maxTotalBytes)    = 27962028
MAX_SEND_JSON_BODY_BYTES = 30059180 (28.67 MiB)
OLD MAX_JSON_BODY_BYTES  = 1048576
growth factor            = 28.67 x

Exactly one path: POST /v1/messages/send (and its /api/v1 alias, which is a prefix rewrite to the same handler).

Peak is not the constant — readJsonBody keeps chunks[], then allocates a second bytes array of the same size, then decodes to a string, then JSON.parse, and then decodeStrictBase64 decodes and fully re-encodes each attachment to verify canonical form. Measured through the real handler with a full-size send (2 x 10 MiB attachments):

[MEM] wireBodyBytes=27962303 (26.67 MiB)
[MEM] rss before=111.4MB afterBuildBody=177.0MB peakDuringHandle=409.2MB
[MEM] delta over pre-request baseline = 232.2 MB for ONE request
[MEM] handler wall time = 524 ms   status=500
[CPU] decodeStrictBase64 for ONE 10MiB file = 25.7 ms
[CPU] allocations per file = decoded 10.0MiB + re-encoded 13.3MiB

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

service.ts has exactly one body-consuming call — req.body?.getReader() at line 469, inside readJsonBody. On the send route authenticate() returns before it. I measured it rather than reading it, with a request body that counts bytes actually pulled:

[UNAUTH]  status=401 bytesPulled=65536 chunks=1
[BADTOK]  status=401 bytesPulled=65536 chunks=1
[CONTROL] status=400 bytesPulled=524288 chunks=8

That 65536 is not the server. Control with the handler never invoked at all:

[NO-READ] bytesPulled=65536 chunks=1 (request built, handler NEVER invoked)

It is the ReadableStream pull-source filling its own queue. Against a 25 MiB body the server pulled one chunk beyond that baseline — zero. The authenticated control drained all 8 chunks, so the meter can fire. No P0.

The auth service (auth/service.ts) has its own separate readJsonBody on MAX_AUTH_BODY_BYTES = 64 * 1024, untouched by this PR. The unauthenticated surface keeps a 64 KiB cap.

3. Did the budget leak to other endpoints? No — and the default is the right way round.

18 call sites of readJsonBody in service.ts. Exactly one passes the large budget (line 1215, the send route); the other 17 take the default, which is the unchanged MAX_JSON_BODY_BYTES. 18 further call sites in auth/service.ts use the separate 64 KiB implementation.

[DOMAINS] status=413 bytesPulled=1179648     <- 8 MiB body refused on an ordinary route
[SEND4MB] status=400 bytesPulled=4194304     <- same size accepted on the send route

4. Amplification and mismatch handling

[LIE-CL]  status=413 bytesPulled=1179648                                  <- content-length says 10, streams 8 MiB
[CHUNKED] status=413 bytesPulled=30408704 budget=30059180 overshoot=349524 <- no content-length

A lying or absent Content-Length is caught by the incremental check, and the chunked overshoot is 349,524 bytes against a 256 KiB chunk — one to two chunks, not a drain.

P2 — authenticated resource amplification, no rate limit on the send route. deps.rateLimiter appears in service.ts at line 176 only, as a type field on the deps interface; it is never invoked (positive control on the same file: deps.store = 4 matches). Every rateLimiter.checkAll call lives in auth/service.ts — signup, login, verify-resend, forgot, reset, invite. So the data path has no per-caller limit and no concurrency bound, and this PR raises the cost of one request against it by ~28x. sendPayloadHash is the sharpest part because it is synchronous on the event loop and hashes the payload including base64 attachment content:

[CPU] sendPayloadHash NEW max payload = 76.1 ms
[CPU] sendPayloadHash OLD-era payload  = 2.3 ms
[CPU] synchronous blocking multiplier  = 32.5x

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 emails:write key. What is new is the multiplier. In a multi-tenant deployment a member of one tenant can now degrade the shared process ~28x more cheaply than before, so a concurrency bound or a per-tenant limit on /v1/messages/send deserves its own task.

5. Downstream

No truncation and no overflow found.

  • body_text / body_html are TEXT; attachments is JSONB carrying metadata only (filename, content_type, size) — the base64 content is not persisted at reserve time.
  • send_payload_hash is a fixed-width sha256 hex in TEXT.
  • buildRawMime joins to one string and Buffer.from(rawMessage) copies it — bounded, not truncated. The SES ceiling holds by construction rather than by luck: attachments and text/html share the single 30,059,180-byte body budget, so the encoded message cannot exceed it plus the wrap factor. mimeEncodedUpperBound = 30795023 against SES_MAX_MESSAGE_BYTES = 40000000, margin 9,204,977.

P3SES_MAX_MESSAGE_BYTES is SES-specific and asserted only in a unit test against the constant. src/providers/resend.ts maps attachments through with no size bound in code, so an operator on the Resend provider gets no equivalent check. Resend's documented limit is also 40MB, so I found no live defect; the gap is that nothing in code says so.

P3text and html are not bounded independently (service.ts:1285-1286 take them as-is). Their only bound was the body cap, which just moved from 1 MiB to 28.67 MiB. An attachment-free send can now carry ~28 MiB of body_text.

6. Preserved rejections

All of them, as far as I could exercise. decodeStrictBase64 is untouched by the diff and still requires length % 4 === 0, the strict alphabet, and a full re-encode roundtrip — non-canonical base64 is still refused. The count, per-file, and total caps all still refuse; the PR's own route-level tests cover one byte over per-file, over-total, and over-count. The inbound.test.ts fixture that hardcoded 513 * 1024 was correctly re-derived from the cap — as written it would have inverted into a legal attachment.

src/lib/send-attachment-limits.test.ts + send-preflight.test.ts   30 pass  0 fail
src/server/self-hosted/service.test.ts                            45 pass  0 fail
src/server/self-hosted/inbound.test.ts                            28 pass  0 fail

Note for anyone re-running this: on a stale node_modules these fail with SyntaxError: Export named 'tenantIdsEqual' not found in module '@hasna/contracts/dist/auth/index.js'. That is environmental — unmodified main produces the identical error. The repo wants @hasna/contracts 0.8.4; a stale tree had 0.4.2.

Pre-existing flakiness baselined rather than attributed:

main   64c80aac, src/cli/commands/send-controlled.test.ts  ->  3 pass  2 fail  1 error
PR     1325d04c, same file, correct deps                   ->  3 pass  2 fail  1 error

Identical. No regression from this PR.

P3, pre-existingreadJsonBody appends the chunk and then tests the cap, so one chunk is materialised in full before rejection. With an attacker-shaped single chunk against the 1 MiB default route: [OVERSHT] status=413 bytesPulled=67108864 maxSingleChunk=67108864 defaultCap=1048576 ratio=64x. Over the network chunk size is chosen by the runtime's socket reads, not by the client, so I am not claiming this is remotely exploitable. The ordering predates this PR and the diff only renames the bound.

7. Secrets

Clean.

grep -inE 'sk-ant-|sk-proj-|npm_[A-Za-z0-9]{20,}|gho_|ghp_|AKIA[A-Z0-9]|BEGIN [A-Z ]*PRIVATE KEY' pr216.diff
SCAN_RC=1 (1 == no findings)

Positive control on the same pattern set, so the zero is evidence:

1:authToken=AKIAIOSFODNN7EXAMPLE
POSCTRL_RC=0 (0 == the scanner fires)

No credential, token, or sensitive value is introduced, logged, or added to an error message. The new rejection strings render sizes only, via humanLimitBytes. SIGNING_SECRET in the touched test file is a pre-existing constant and is not in the added lines.

P3humanLimitBytes divides by 1024*1024 and labels the result "MB", so the operator-facing refusal reads "10MB" for a 10 MiB cap. Cosmetic, and inherited from the humanBytes it replaced.

What I did not check

I 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 --identity-scoped or IdP-authenticated send paths.

Verdict: GO.

Agent: agent-chief-operations

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[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 1325d04c, base 64c80aac. Base has not moved — refs/pull/216/merge^1 = 64c80aac271b81c1501bb93cb4e7a2975417e791 = current origin/main, so the head-sha check is sufficient here.

1. Every layer between an API caller and the SES call

# Layer Effective limit at head Binding?
1 cli/commands/send.ts readSendAttachments 10 files / 25 MiB each (LOCAL caps) no
2 lib/send.remote.ts validateSendAttachments 10 files / 25 MiB each no
3 store-http/wire.ts 8 MiB response ceiling only; no request cap no
4 Bun.serve (self-hosted/serve.ts:114) no maxRequestBodySize set → Bun default no
5 authenticate() runs before the body read; no body access n/a
6 readJsonBody(req, MAX_SEND_JSON_BODY_BYTES) 30,059,180 B (~28.67 MiB), content-length precheck + streaming accumulation was the blocker at 1 MiB
7 attachment count maxFiles = 5 yes
8 per-file maxBytesPerFile = 10,485,760 first to bind for one document
9 running total maxTotalBytes = 20,971,520 yes
10 reserveSendIntent / store no size gate (checked the region between reservation and provider send) no
11 SESv2 SendEmailCommand Raw 40,000,000 B after base64 no

Layer 3 was worth checking and clears: the send response is publicMessage(record), and the route persists attachments as {filename, content_type, size} only (service.ts:1303-1307), so no base64 content returns through the 8 MiB response ceiling.

I found no proxy/gateway body cap in the repo. deploy/aws is ALB + ECS with no body-size setting; the only proxy mention is docs/SELF_HOSTED_RUNTIME.md:210 about X-Forwarded-For depth. Not verified against the running edge at emails.hasna.xyz — see "not checked" below.

2. Was the thing the PR raised the thing that was rejecting? Yes — measured on base

I ran the real route on the base checkout. Literal output:

PROBE_CAPS perFile=524288 total=786432 files=5
PROBE_3_8MB status=413 error="request body too large"
PROBE_600KIB status=400 error="attachment 0 requires base64 content no larger than 512KiB"
PROBE_100KIB status=500 error="internal error"

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 boundary

Head, real route:

PROBE_A_3_8MB          status=500 json=5066895   error="internal error"
PROBE_B_EXACT          status=500 json=13981241  error="internal error"
PROBE_C_PERFILE_PLUS1  status=400 json=13981240  error="attachment 0 requires base64 content no larger than 10MB"
PROBE_D_TOTAL_PLUS1    status=400 json=27962391  error="inline attachments may total at most 20MB"
PROBE_F_COUNT          status=400 error="at most 5 inline attachments are allowed"

PROBE_D is 10MiB + 10MiB + 1 byte — the smallest encoding that exceeds the total cap while still fitting the body budget. The total-cap branch is reachable and it fires.

4. Provider ceiling — verified from AWS, not from the PR

AWS's own quota page, verbatim:

Using the SES v2 API or SMTP - Maximum message size (including attachments) | 40 MB per message (after base64 encoding). | No
Using the SES v1 API - Maximum message size (including attachments) | 10 MB per message (after base64 encoding). | No

providers/ses.ts imports SendEmailCommand from @aws-sdk/client-sesv2 and sends Raw: { Data: ... }, so the 40 MB figure is the right one and SES_MAX_MESSAGE_BYTES = 40_000_000 is the conservative reading of "40 MB". wrapBase64 wraps at 76 chars joined with \r\n, so the 78/76 factor in mimeEncodedUpperBound is the correct expansion.

Arithmetic, computed from the module itself:

bodyBudget      = 30059180
encodedTotalCap = 27962028
mimeUpperBound  = 30795023
SES ceiling     = 40000000
PR margin       = 9204977
trueWorstCaseMIME(all-body-budget) = 30852260 underSES= true margin= 9147740

The last line matters: the route does not cap text/html, so mimeEncodedUpperBound is not an upper bound over all accepted inputs — it models the attachment-saturated case. Bounding instead by the body budget itself gives 30,852,260 B, still under the ceiling with 9.1 MB spare. The safety property holds for every input the route accepts, which is stronger than what the PR proves. P3 note below.

5. Blast radius

Scoping is correct. readJsonBody has 19 call sites; exactly one passes the raised budget:

1215:      const body = await readJsonBody(req, MAX_SEND_JSON_BODY_BYTES);

The other 18 keep the 1 MiB default. No other endpoint's limit moved.

Not reachable unauthenticated. authenticate() precedes the body read, and I measured it:

PROBE_G_NOAUTH status=401 json=5066895 error="authentication required"

Memory. Per in-flight max-size send the route now transiently holds the accumulated body, the parsed JSON, decodeStrictBase64's decoded Buffer plus its re-encoded canonicality string, sendPayloadHash's JSON.stringify of the full payload, and buildRawMime's wrapped output — order 150 MB against a default api_memory = 1024 MiB Fargate task (deploy/aws/variables.tf:214-218, api_cpu = 512). Previously bounded near 5 MB. There is no rate limit on /v1/messages/send: the RateLimiter is wired only to auth routes (signup, login, verify-resend, forgot, reset, invite — the same grep that returned zero for send returned those seven, which is the positive control). Authenticated-only, so P2 rather than P1.

6. Are the tests honest? Two mutations

Mutation 2 — reverted the send route to the default body cap. The acceptance tests fail, so they can fail:

(fail) POST /v1/messages/send attachment caps > a document-sized attachment is no longer refused by the caps
(fail) POST /v1/messages/send attachment caps > two document-sized attachments are no longer refused by the caps
 43 pass
 2 fail

Mutation 1 — deleted the total-attachment-cap enforcement from the route entirely. Everything still passes:

 103 pass
 0 fail
 493 expect() calls
Ran 103 tests across 4 files.

The cause is visible in the probe. The PR's "a set over the total cap is still refused" test builds 3 × maxBytesPerFile = 30 MiB raw, whose JSON is 41,943,406 bytes and therefore exceeds the body budget:

PROBE_E_PR_TOTAL count=3 status=413 json=41943406 error="request body too large"

isAttachmentRefusal() returns true on any 413, so the test passes on the body cap and never reaches the branch it is named for. This is the exact case the file's own comment says it guards — "A change that merely deleted enforcement would satisfy 'a big document is accepted' while failing every rejection case below" — and for the total cap that claim does not hold. The remedy is one fixture change: [maxBytesPerFile, maxBytesPerFile, 1], which I verified returns 400 inline attachments may total at most 20MB.

The per-file, file-count, body-budget and route-scoping tests are all genuinely falsifiable. send-preflight.test.ts's total-overage test exercises the pure rule directly and is unaffected — it is the route wiring that is uncovered.

Fixtures no longer hardcode old constants. All 11 remaining occurrences of 512KiB|768KiB|513 * 1024|600 * K in src are comments or the unrelated store.ts:338 runByteBudget; positive control, the same sweep found 49 SELF_HOSTED_SEND_ATTACHMENT_LIMITS references.

7. Regression check

bun test src/server/self-hosted src/lib, head vs base, failing test names diffed:

=== HEAD-only failures (regressions introduced by PR) ===
[end]
=== counts: head=18 base=18 common=18 ===

Head 1260 pass / 45 fail / 1331 tests, base 1244 pass / 45 fail / 1315 tests — +16 tests, +16 passes, identical failure set.

send-controlled.test.ts flakiness is pre-existing and unchanged. Base runs: 3 pass 2 fail, 3 pass 2 fail, 2 pass 3 fail. Head runs: 2 pass 3 fail, 3 pass 2 fail, 2 pass 3 fail. Same two constant failures on both sides ("rejects insecure, symlinked…", "preserves the existing inline send command") with the same varying extras.

Findings

  • P2 — the route's total-attachment cap has no test that can fail. Mutation 1 above: enforcement deleted, 103 pass, 0 fail. Behaviour is correct today; the coverage claim in the test file's comment is not. Fixture fix: [maxBytesPerFile, maxBytesPerFile, 1].
  • P2 — no rate limit on /v1/messages/send and a ~30x rise in per-request peak memory against a default 1024 MiB task. Authenticated-only. Worth either a concurrency guard on the route or a task-memory review.
  • P2 — only the SES ceiling is pinned, but the sender supports two providers. sender.ts:5 declares export type SelfHostedSendProvider = "ses" | "resend". Resend's documented limit is "max 40MB per email, after Base64 encoding of the attachments", so the caps are safe today by coincidence of the two ceilings matching. A RESEND_MAX_MESSAGE_BYTES pinned alongside would make that a checked fact rather than one.
  • P3 — mimeEncodedUpperBound is not an upper bound over all accepted inputs, because text/html are uncapped and the function models only the attachment-saturated case. The conclusion survives (30,852,260 < 40,000,000) but for a reason the function does not express.
  • P3 — humanLimitBytes labels MiB values as "MB". The operator sees "no larger than 10MB" for a 10,485,760 B cap. Cosmetic and pre-existing in shape.

What I did not check

The 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
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #216 @ 60259c7 — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1)

Reviewed the exact diff from fetched origin/main at 64c80aac271b81c1501bb93cb4e7a2975417e791 to head 60259c70ed73f96c5468af00f87851c4d191c437.

What I read:

  • Full diff and surrounding source for all seven changed files: src/lib/send-attachment-limits.test.ts, src/lib/send-attachment-limits.ts, src/lib/send-preflight.test.ts, src/lib/send-preflight.ts, src/server/self-hosted/inbound.test.ts, src/server/self-hosted/service.test.ts, and src/server/self-hosted/service.ts.
  • Surrounding authenticated request dispatch, body-size error handling, controlled-send envelope validation, local attachment validation, self-hosted sender construction, and the SES and Resend attachment serialization paths.
  • Current official provider limits: SES v2 and Resend each document a 40 MB complete-message ceiling after attachment base64. The PR's 20 MiB raw-total cap, MIME wrapping allowance, and envelope headroom remain below that ceiling.

What I ran:

  • git log --oneline origin/main..HEAD — exit 0.
  • git diff origin/main...HEAD --stat — exit 0; 7 files, 490 insertions, 30 deletions.
  • bun install — exit 0; setup only, not a repository gate; 324 packages installed.
  • bun run test — exit 0; 4356 pass, 156 skip, 0 fail, 4512 tests across 291 files, 21331 expectations.
  • The repository declares no typecheck script, so no typecheck gate exists and none was invented.

Blocking P0/P1 findings: none.

The route remains authenticated before body buffering; only /v1/messages/send receives the attachment-derived body budget; other JSON routes retain the 1 MiB ceiling; strict base64, file-count, per-file, and decoded-total limits still fail closed before send-intent reservation or provider I/O. The new total-cap test reaches the named branch rather than passing through the body-size guard.

Non-blocking follow-up:

  • P3 documentation: AGENTS.md still says the self-hosted send API is limited to 512 KiB per attachment and 768 KiB total. Update it to the new 10 MiB per-file and 20 MiB total values. This is stale guidance, not a current correctness, security, integrity, or required-gate blocker.

@andrei-hasna
andrei-hasna merged commit 72ed341 into main Aug 8, 2026
4 checks passed
@andrei-hasna
andrei-hasna deleted the fix/9ac170be-send-attachment-cap branch August 8, 2026 17:04
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

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 Agent: trailer did not survive the merge. This PR was merged with GitHub's default squash body while a --body-file merge carrying the trailer was in flight. This repository has squash_merge_commit_message set to discard the branch body, so the trailer went with it — git log -1 --format=%B origin/main | grep -c '^Agent: ' returns 0 on 72ed341. Published history is not rewritten to conform, so this is recorded rather than amended. Attribution for the work is agent-chief-operations, and it is preserved on both branch commits (1325d04, 60259c7).

3. Merging did not make this live, and that is the open item.

https://emails.hasna.xyz/health  ->  {"status":"ok","version":"1.3.0",...}
origin/main package.json         ->  1.3.13

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. .github/workflows/ci.yml has no deploy step, and deploy/aws/compute.tf takes the task image from var.container_image, so shipping is an explicit build/push plus terraform apply. Note the deployed build already lagged main by 13 patch versions before this PR, so this is a standing deployment gap rather than something this change introduced. Tracked as todos f76f1e7c.

One layer both reviewers correctly flagged as unchecked, now partially closed: an unauthenticated POST to the live /v1/messages/send declaring Content-Length: 5000172 was answered 401 by the application, not 413 by an intermediary — so no edge component rejects on a declared 5 MB content-length. This is not proof the full body transits: the app rejects on auth before reading the body, so curl uploaded only 262,144 bytes before the response arrived (http_code=401 size_upload=262144; the small-body control uploaded 19). A conclusive edge test needs an authenticated full-size send after deployment.

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.

1 participant