Enforce attachment size limit on the byte stream#6
Open
imkp1 wants to merge 1 commit into
Open
Conversation
The 50 MB attachment cap was only checked against the client-declared size (header.Size), while the actual write used an unbounded io.Copy and ParseMultipartForm's argument is an in-memory threshold, not a hard limit. A size-spoofed or chunked upload could therefore spill large temp files to disk during parsing and be written unbounded before the post-hoc size-mismatch check removed it. - handler: wrap the request body with http.MaxBytesReader so an oversized upload is refused up front (413) rather than spilled to disk while parsing the multipart form. The cap has headroom above the file size so a legitimate max-size file still fits. - service: copy through io.LimitReader(maxSize+1) and reject when the stream exceeds the cap, so enforcement no longer trusts the declared size. The cap is a struct field (defaulted to the const) to keep the guard testable without streaming 50 MB. Adds tests for both layers.
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
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
Enforces the 50 MB attachment cap on the actual byte stream, not just the
client-declared size.
Why
Two gaps let an upload exceed the cap:
r.ParseMultipartForm(maxUploadBytes)treats its argument as anin-memory threshold, not a hard request limit; a larger multipart body
spills to temp files on disk during parsing, before any size check runs.
header.Size(
in.Size > maxAttachmentSize), and the write itself used an unboundedio.Copy. A size-spoofed or chunked upload (e.g.Size: 0) was written todisk in full, and only the post-hoc
written != in.Sizecheck removed it.Net effect: a client could cause large temp/attachment files to hit disk
regardless of the 50 MB limit.
Changes
attachment_handler.go— wrap the request body withhttp.MaxBytesReaderso an oversized upload is refused up front with 413rather than spilled to disk while parsing. The request cap
(
maxRequestBytes) sits a little above the file cap, leaving headroom formultipart framing + the small form fields, so a legitimate max-size file
still fits.
attachment_service.go— copy throughio.LimitReader(in.Reader, maxSize+1)and reject when the stream exceeds thecap, so enforcement no longer trusts the declared size. The cap is a struct
field defaulted to the existing
maxAttachmentSizeconst, purely so theguard is testable without streaming 50 MB.
Tests
TestStore_RejectsOversizeStream— aSize: 0upload that streams more bytesthan the (shrunk) cap is rejected, and no partial file is left on disk.
TestUpload_RejectsOversizeBody— a request body overmaxBodyreturns413 and the service is never called.
Existing attachment tests (jail/sanitize/happy-path) still pass.
Verification