feat(request): add FormFileWithConfig for size and content-type limits - #44
Conversation
Uploads previously had no framework-level guard: callers hand-rolled io.LimitReader plus magic-byte sniffing, and getting either wrong is a security problem rather than a cosmetic one. FormFileWithConfig applies the size limit with http.MaxBytesReader before ParseMultipartForm runs, so an oversized upload is rejected without first being buffered to memory or spooled to disk. A post-hoc fh.Size check would have let a client stream gigabytes to /tmp before receiving a 413; that check remains only as a backstop for a form some middleware already parsed. AllowedTypes is matched against the type detected from the file's leading bytes via http.DetectContentType, never the multipart part's Content-Type header, which is client-supplied and trivially forged. Since sniffing has to open the file anyway, the opened multipart.File is returned rewound so callers don't reopen it. Also fixes FormFile flattening every failure into 400: an oversized body now returns 413 and a non-multipart request 415, matching BindMultipart. The two `err.Error() == "http: request body too large"` string comparisons in form.go are replaced with errors.As on *http.MaxBytesError.
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds ChangesMultipart upload handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FormFileWithConfig
participant MultipartParser
participant sniffContentType
participant fileError
Client->>FormFileWithConfig: Send multipart upload
FormFileWithConfig->>MultipartParser: Parse bounded request
MultipartParser-->>FormFileWithConfig: Return file and metadata
FormFileWithConfig->>sniffContentType: Detect type from file bytes
sniffContentType-->>FormFileWithConfig: Return normalized media type
FormFileWithConfig->>fileError: Map limit or type failure
fileError-->>Client: Return structured HTTP error
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@request/file.go`:
- Around line 15-123: Update the FormFileWithConfig documentation to state that
callers using a non-standard HTTP server or transport must call
r.MultipartForm.RemoveAll() to clean up multipart temporary files, while
retaining the existing caller responsibility to close the returned file.
- Around line 106-111: Update the sniffContentType error branch and fileError
default branch to preserve the underlying err by using the errors package’s
supported cause-wrapping API (such as WithCause or Wrap), while keeping the
existing client-facing messages unchanged. Verify both returned API errors
retain errors.Is/errors.As traceability and remove any silent discard of the
original error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2706f26-470e-47a1-aaef-dda5e533436f
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!CHANGELOG.md
📒 Files selected for processing (4)
README.mdrequest/file.gorequest/file_test.gorequest/form.go
| // DefaultMaxUploadSize is the default body limit applied by FormFileWithConfig | ||
| // when FileConfig.MaxBytes is zero. | ||
| const DefaultMaxUploadSize = 10 << 20 // 10 MB | ||
|
|
||
| // sniffLen is the number of leading bytes inspected to detect a file's real | ||
| // content type. http.DetectContentType never looks at more than this. | ||
| const sniffLen = 512 | ||
|
|
||
| // FileConfig constrains an uploaded file. | ||
| type FileConfig struct { | ||
| // MaxBytes is the maximum allowed request body size in bytes. It is | ||
| // enforced before the body is read, so an oversized upload is rejected | ||
| // without being buffered to memory or disk. | ||
| // Defaults to DefaultMaxUploadSize if zero. | ||
| MaxBytes int64 | ||
|
|
||
| // MaxMemory is the maximum memory (in bytes) used for parsing the | ||
| // multipart form before spilling to disk. | ||
| // Defaults to DefaultMaxMultipartMemory if zero. | ||
| MaxMemory int64 | ||
|
|
||
| // AllowedTypes is an allowlist of media types, matched against the type | ||
| // detected from the file's own bytes rather than the client-supplied | ||
| // Content-Type header. Entries may be exact ("image/png"), a wildcard | ||
| // subtype ("image/*"), or "*/*"; matching is case-insensitive. | ||
| // An empty slice allows any content type. | ||
| // | ||
| // Note that http.DetectContentType recognises a fixed set of signatures | ||
| // and falls back to "application/octet-stream" for formats it does not | ||
| // know, so an allowlist for an exotic type may need that entry (or a | ||
| // caller-side check) to accept legitimate files. | ||
| AllowedTypes []string | ||
| } | ||
|
|
||
| // FormFileWithConfig returns the first file for the given form field, enforcing | ||
| // a size limit and an optional content-type allowlist. | ||
| // | ||
| // Unlike FormFile it also returns the opened file, positioned at the start: | ||
| // | ||
| // f, fh, err := request.FormFileWithConfig(r, "avatar", request.FileConfig{ | ||
| // MaxBytes: 5 << 20, | ||
| // AllowedTypes: []string{"image/png", "image/jpeg"}, | ||
| // }) | ||
| // if err != nil { | ||
| // return err | ||
| // } | ||
| // defer f.Close() | ||
| // | ||
| // The caller is responsible for closing the returned file. | ||
| // | ||
| // Content types are detected with http.DetectContentType over the first 512 | ||
| // bytes of the file. The multipart part's own Content-Type header is not | ||
| // consulted, since it is supplied by the client and trivially forged. | ||
| func FormFileWithConfig(r *http.Request, field string, cfg FileConfig) (multipart.File, *multipart.FileHeader, error) { | ||
| maxBytes := cfg.MaxBytes | ||
| if maxBytes <= 0 { | ||
| maxBytes = DefaultMaxUploadSize | ||
| } | ||
| maxMemory := cfg.MaxMemory | ||
| if maxMemory <= 0 { | ||
| maxMemory = DefaultMaxMultipartMemory | ||
| } | ||
|
|
||
| // Limit the body before anything reads it. Once the form has been parsed | ||
| // the bytes are already spooled, so a limit applied afterwards would be | ||
| // no protection at all. | ||
| if r.MultipartForm == nil { | ||
| if r.Body == nil || r.Body == http.NoBody { | ||
| return nil, nil, errors.BadRequest("Request body is required") | ||
| } | ||
| r.Body = http.MaxBytesReader(nil, r.Body, maxBytes) | ||
|
|
||
| if err := r.ParseMultipartForm(maxMemory); err != nil { | ||
| return nil, nil, fileError(field, err) | ||
| } | ||
| } | ||
|
|
||
| f, fh, err := r.FormFile(field) | ||
| if err != nil { | ||
| return nil, nil, fileError(field, err) | ||
| } | ||
|
|
||
| // Backstop for a form parsed before this call (by middleware, or by an | ||
| // earlier FormFile), where the body limit above was never applied. | ||
| if fh.Size > maxBytes { | ||
| _ = f.Close() | ||
| return nil, nil, errors.New(errors.CodeRequestTooLarge, | ||
| fmt.Sprintf("File %q exceeds the maximum size of %d bytes", field, maxBytes)). | ||
| WithStatus(http.StatusRequestEntityTooLarge) | ||
| } | ||
|
|
||
| if len(cfg.AllowedTypes) > 0 { | ||
| detected, err := sniffContentType(f) | ||
| if err != nil { | ||
| _ = f.Close() | ||
| return nil, nil, errors.BadRequest(fmt.Sprintf("Failed to read file field %q", field)) | ||
| } | ||
| if !mediaTypeAllowed(detected, cfg.AllowedTypes) { | ||
| _ = f.Close() | ||
| return nil, nil, errors.New(errors.CodeUnsupportedMedia, | ||
| fmt.Sprintf("File type %s is not allowed, expected one of: %s", | ||
| detected, strings.Join(cfg.AllowedTypes, ", "))). | ||
| WithStatus(http.StatusUnsupportedMediaType). | ||
| WithField(field, "unsupported file type") | ||
| } | ||
| } | ||
|
|
||
| return f, fh, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Document multipart temp-file cleanup expectations.
FormFileWithConfig calls r.ParseMultipartForm, which can spool files to disk. For a request served through the standard net/http.Server, the server calls req.MultipartForm.RemoveAll() after the handler returns, so this is normally not a leak. However, that guarantee is server-specific (and historically inconsistent across HTTP/1 vs HTTP/2 paths) and isn't mentioned anywhere in this file's docs. Since this is a general-purpose library function, consider documenting on FormFileWithConfig (similar to the existing "caller is responsible for closing" note) that callers using a non-stdlib server/transport are responsible for calling r.MultipartForm.RemoveAll() themselves.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@request/file.go` around lines 15 - 123, Update the FormFileWithConfig
documentation to state that callers using a non-standard HTTP server or
transport must call r.MultipartForm.RemoveAll() to clean up multipart temporary
files, while retaining the existing caller responsibility to close the returned
file.
…rors The sniff-failure branch and fileError's fallback branch both discarded the original error, so an unexpected multipart failure surfaced as a bare 400 with nothing to debug from. Both now Wrap it. Error.Err is json:"-" and Message is unchanged, so this is invisible to clients; it only restores errors.Is/As traceability for logs. Covered by a test asserting both halves: the cause is reachable, and it does not appear in the serialized response.
golangci-lint infers the analysis language version from go.mod (1.22), and at that version staticcheck does not model t.Fatal as terminating, so it reported two false 'possible nil pointer dereference' warnings in httpclient/httpclient_test.go. Genuine SA5011 findings are still reported at 1.23.
Applies to BindMultipart, FormFile, FormFiles, and FormFileWithConfig alike, so it belongs on the package rather than any one helper.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
request/file_test.go (1)
314-339: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAdd
t.Parallel()to the test and its subtests.As per path instructions, tests should use
t.Parallel()where safe.TestMediaTypeAllowedis a pure function test without shared state, making it safe for parallel execution.⚡ Proposed fix
func TestMediaTypeAllowed(t *testing.T) { + t.Parallel() tests := []struct { name string detected string allowed []string want bool }{ {"exact match", "image/png", []string{"image/png"}, true}, {"case insensitive", "image/png", []string{"IMAGE/PNG"}, true}, {"second entry", "image/jpeg", []string{"image/png", "image/jpeg"}, true}, {"no match", "text/plain", []string{"image/png"}, false}, {"wildcard match", "image/gif", []string{"image/*"}, true}, {"wildcard no match", "video/mp4", []string{"image/*"}, false}, {"wildcard is not a prefix match", "imagexml/foo", []string{"image/*"}, false}, {"empty allowlist", "image/png", nil, false}, {"match anything", "application/octet-stream", []string{"*/*"}, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() if got := mediaTypeAllowed(tt.detected, tt.allowed); got != tt.want { t.Errorf("mediaTypeAllowed(%q, %v) = %v, want %v", tt.detected, tt.allowed, got, tt.want) } }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@request/file_test.go` around lines 314 - 339, Add t.Parallel() at the start of TestMediaTypeAllowed and within each t.Run subtest, preserving the existing table-driven assertions.Source: Path instructions
request/file.go (1)
68-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the multipart cleanup note to
FormFileWithConfigCallers using a non-standard HTTP server need to callr.MultipartForm.RemoveAll()after use to release temp files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@request/file.go` at line 68, Add a documentation note to FormFileWithConfig stating that callers using a non-standard HTTP server must call r.MultipartForm.RemoveAll() after processing the uploaded file to release temporary files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@request/file_test.go`:
- Around line 314-339: Add t.Parallel() at the start of TestMediaTypeAllowed and
within each t.Run subtest, preserving the existing table-driven assertions.
In `@request/file.go`:
- Line 68: Add a documentation note to FormFileWithConfig stating that callers
using a non-standard HTTP server must call r.MultipartForm.RemoveAll() after
processing the uploaded file to release temporary files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9473b869-6737-4817-a370-f059aa50deb6
📒 Files selected for processing (3)
.golangci.ymlrequest/file.gorequest/file_test.go
response.File and response.Reader write the body in a single pass, so neither can back a <video> that needs to seek or a resumable download. Both new helpers delegate the transfer to http.ServeContent, which owns Range parsing, conditional GETs, HEAD, and 416 handling, and layer the Content-Disposition, ETag, and Cache-Control handling on top. File now shares the Content-Disposition builder rather than repeating it.
Follow-up to 393ddeb, which staged only tracked files and so omitted the two new ones.
2 participants
Summary
Uploads previously had no framework-level guard: callers hand-rolled io.LimitReader plus magic-byte sniffing, and getting either wrong is a security problem rather than a cosmetic one.
FormFileWithConfig applies the size limit with http.MaxBytesReader before ParseMultipartForm runs, so an oversized upload is rejected without first being buffered to memory or spooled to disk. A post-hoc fh.Size check would have let a client stream gigabytes to /tmp before receiving a 413; that check remains only as a backstop for a form some middleware already parsed.
AllowedTypes is matched against the type detected from the file's leading bytes via http.DetectContentType, never the multipart part's Content-Type header, which is client-supplied and trivially forged. Since sniffing has to open the file anyway, the opened multipart.File is returned rewound so callers don't reopen it.
Also fixes FormFile flattening every failure into 400: an oversized body now returns 413 and a non-multipart request 415, matching BindMultipart. The two
err.Error() == "http: request body too large"string comparisons in form.go are replaced with errors.As on *http.MaxBytesError.Motivation
Fixes #
Changes
Checklist
make all)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores