Add request body size limits to plugin HTTP endpoints#464
Add request body size limits to plugin HTTP endpoints#464nevyangelova merged 3 commits intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughGlobal request body size limit (1 MB) added and applied via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/http.go (1)
85-85: Stabilize the over-limit response instead of relying onMaxBytesReaderside effects.The new tests already had to allow both
400and413, which means the final status now depends on the concreteResponseWriterand each handler's generic decode/read error path. If this limit is part of the HTTP contract, catch*http.MaxBytesErrorexplicitly in the body-reading handlers and return one consistent status (ideally413) everywhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/http.go` at line 85, The current use of r.Body = http.MaxBytesReader(w, r.Body, maxRequestBodySize) relies on MaxBytesReader side effects; update all request body-reading/decoding functions (the handlers or helper(s) that call r.Body.Read/Decode after setting MaxBytesReader) to explicitly detect a *http.MaxBytesError when reading/parsing the body and return a consistent 413 response (instead of letting generic decode errors produce 400). Concretely, after reading/decoding (e.g., in your JSON/body decode helper), check errors.Is(err, http.ErrBodyReadAfterClose) or type-assert err to *http.MaxBytesError and if matched call http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) / w.WriteHeader(http.StatusRequestEntityTooLarge), otherwise proceed with existing error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/http.go`:
- Line 85: The current use of r.Body = http.MaxBytesReader(w, r.Body,
maxRequestBodySize) relies on MaxBytesReader side effects; update all request
body-reading/decoding functions (the handlers or helper(s) that call
r.Body.Read/Decode after setting MaxBytesReader) to explicitly detect a
*http.MaxBytesError when reading/parsing the body and return a consistent 413
response (instead of letting generic decode errors produce 400). Concretely,
after reading/decoding (e.g., in your JSON/body decode helper), check
errors.Is(err, http.ErrBodyReadAfterClose) or type-assert err to
*http.MaxBytesError and if matched call http.Error(w, "request body too large",
http.StatusRequestEntityTooLarge) /
w.WriteHeader(http.StatusRequestEntityTooLarge), otherwise proceed with existing
error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5a75d59f-c8e9-4da7-bfa4-fe77cab081a1
📒 Files selected for processing (2)
server/http.goserver/webhook_test.go
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
Changes look good! Just have one (non-blocking) comment about closing a small gap
| p.SetAPI(api) | ||
|
|
||
| largeBody := make([]byte, maxWebhookBodySize+100) | ||
| largeBody := make([]byte, maxRequestBodySize+100) |
There was a problem hiding this comment.
I see that the webhook payload test has been updated to reference maxRequestBodySize instead of maxWebhookBodySize, so we might want to remove maxWebhookBodySize from webhook.go entirely to solely use this new constant in https://github.com/mattermost/mattermost-plugin-zoom/pull/446/changes#diff-e3302fa7fe65284d10a8f46bad75c6e182a9b39a0400fb09f888d0dde5a79bf9R42
There was a problem hiding this comment.
+1 to this, the rest looks good to me :)
Made-with: Cursor
3 participants
Summary
Ticket
https://mattermost.atlassian.net/browse/MM-68164
Change Impact: 🟡 Medium
Reasoning: The PR enforces a global 1 MB request body size limit for all plugin HTTP endpoints via
http.MaxBytesReaderin the centralServeHTTPhandler—an isolated, focused change but one that introduces a new behavioral constraint across multiple endpoints which may break legitimate larger payloads.Regression Risk: Medium. The change uniformly affects all plugin endpoints (including webhook, OAuth flows, deauthorization, and API routes) where previously only some handlers had explicit limits; tests were added/updated but response status behavior can vary (400 vs 413), and callers relying on larger request bodies or specific status codes may be impacted.
QA Recommendation: Perform manual and automated tests sending payloads around the 1 MB boundary (e.g., ~900 KB, 1 MB, 1.1 MB) against all public endpoints—prioritize
/webhook,/deauthorization,/oauth2/complete, and/api/v1/*—and verify consistent error responses and client compatibility. Skip manual QA only if telemetry confirms all requests are well below 1 MB and clients tolerate 400/413 responses.Generated by CodeRabbitAI