Repository navigation
Bug 2059957 - Reject truncated request bodies before native endpoint dispatch - #2767
Conversation
…dispatch Reject body-related and unknown parser limits before native endpoint actions run, preserving response formats, REST CORS, and existing CGI handling. Document the REST 413 response and add focused regression coverage. Co-authored-by: Copilot App <[email protected]>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation safely blocks affected native actions and provides focused coverage for response formats, CORS, and parser-limit classification.
Review effort: Balanced
Findings: None
What changed in this PR
Adds centralized rejection of truncated request bodies before native endpoint dispatch while preserving legacy CGI behavior and REST response contracts.
Changes:
- Adds an app-wide request-limit action hook with safe logging and format-specific 413 responses.
- Configures REST, JSON, and CSP endpoints with explicit error formats.
- Adds request-limit coverage and REST API documentation.
| File | Description |
|---|---|
Bugzilla/App/Plugin/RequestLimit.pm |
Implements native request-limit rejection. |
Bugzilla/App.pm |
Registers the new plugin. |
Bugzilla/App/Controller/API.pm |
Marks native REST requests for REST-formatted errors. |
Bugzilla/App/Controller/Main.pm |
Marks announcement responses as JSON. |
Bugzilla/App/Controller/CSPReport.pm |
Marks CSP responses as empty. |
t/app-cgi-request-limit.t |
Tests dispatch prevention and response contracts. |
docs/en/rst/api/core/v1/general.rst |
Documents REST 413 behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return $c->user_error(REQUEST_TOO_LARGE_ERROR); | ||
| } | ||
|
|
||
| $c->stash->{request_limit_format} = 'rest'; |
There was a problem hiding this comment.
this drops the old 413 for REST requests that hit the header or start-line limit, so /rest endpoints now run with half-parsed headers (e.g. api key cut off) and no body. maybe keep failing closed for any is_limit_exceeded when the format is rest and add a test for it
There was a problem hiding this comment.
Thanks for catching this. REST routes now continue to reject header and start-line parser limits with the existing 413 response; non-REST handling remains unchanged. I added an HTTP regression test covering the REST header-limit case, including the 413 response, error envelope, and CORS header.
Keep rejecting every Mojolicious parser-limit failure on REST routes, matching the prior API guard. Add an HTTP regression test for a truncated REST header. Co-authored-by: Copilot App <[email protected]>
Mojolicious can mark a request as limit-exceeded while leaving a truncated body available to application code. The legacy CGI bridge already rejects these requests, but other native routes could still execute their endpoint actions. This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=2059957.
Add an app-wide action hook that rejects message-size and buffer-size failures before native endpoint actions run. Unknown parser-limit failures also fail closed, with a generic log reason. Known start-line and header limits retain their existing fallback behavior, and legacy CGI handling is unchanged. Rejection logs include only the matched route pattern and a safe limit reason, not request paths, query strings, or bodies.
Preserve the standard REST 413 error envelope and CORS headers, with explicit response metadata for native JSON and empty-response endpoints. Document the REST 413 response and error code without promising a fixed byte threshold.
Coverage verifies under-limit dispatch, oversized native JSON action non-execution, REST error/CORS contracts, CSP responses, and parser-limit classification. The focused request-limit test and progressive Perl critic pass on the final code changes. Repository compile and Perl-quality checks passed before the fail-closed refinement.