Bug 1832783 - Reject oversized CGI requests

[loganrosen]

Large multipart submissions that exceed Mojolicious's message-size limit currently continue into legacy CGI dispatch with an incomplete request body. For new bug submissions, that hides the actual failure behind a misleading bug_type error.

This rejects Mojolicious message-size failures at the CGI boundary before rebuilding STDIN or executing the legacy script. Browser requests receive a generic localized HTTP 413 response, while REST, JSON-RPC, and XML-RPC requests retain machine-readable error responses. The response intentionally does not report maxattachmentsize, because that setting limits decoded attachment data rather than the complete raw HTTP message rejected by Mojolicious. Normal attachment and bug-creation behavior remains unchanged.

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1832783

Tested with test_sanity t/app-cgi-request-limit.t t/mojo-example.t.

[Copilot]

Pull request overview

Rejects oversized CGI requests with HTTP 413 before legacy CGI dispatch.

Changes:

• Adds CGI request-limit handling.
• Reports attachment limits for multipart bug submissions.
• Adds regression coverage.

Show a summary per file

File	Description
Bugzilla/App/Controller/CGI.pm	Handles oversized CGI requests.
t/app-cgi-request-limit.t	Tests oversized multipart submissions.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Medium

[dklawren]

Thanks for tackling this. The approach (short-circuit in the shared CGI wrapper) is the right place to catch it cheaply, but there are two behavioral regressions I think need to be resolved before landing, plus a cluster of message-accuracy issues and a test-coupling problem.

Blocking

1. JSON/XML-RPC endpoints lose their error contract

load_one generates this wrapper for every glob'd *.cgi, and setup_routes maps the API paths to those same wrappers ($r->any('/rest')->to('CGI#rest_cgi'), /rest.cgi/*PATH_INFO, /rest/*PATH_INFO, /jsonrpc.cgi, /xmlrpc.cgi). So an over-limit POST /rest/bug//attachment now returns:

HTTP/1.1 413
Content-Type: text/plain; charset=UTF-8

The request is too large.

where it previously returned application/json with a {"error":true,...} document. js/util.js:514 does fetch(request).then(response => response.json()) unconditionally, so Bugzilla.API rejects with a bare SyntaxError and the UI shows an opaque parse failure instead of the message you wrote. python-bugzilla and any other resp.json() client raises a decode exception the same way.

This is easy to hit in practice: base64 inflates a 12 MB attachment to ~16.7 MB, past Mojo's 16 MiB default.

We already have the convention for this in Bugzilla/App.pm:70-73 — $c->respond_to(json => {...}, any => {...}). Suggest routing the 413 through that, or checking $file against the API scripts and emitting the matching error shape.

2. is_limit_exceeded is broader than "body too big"

Mojo::Message sets the same limit flag for four causes, not just message size:

• Maximum start-line size exceeded (max_line_size)
• Maximum header size exceeded
• Maximum message size exceeded
• buffer limit

Bugzilla/App.pm:87 raises MOJO_MAX_LINE_SIZE from Mojo's 8 KB to 10 KB specifically because we hit the header limit in production with large cookie jars. Before this change, Mojo truncated the headers and show_bug.cgi still ran — worst case the user saw a logged-out page. After this change, a user whose Cookie header crosses the limit gets a bare text/plain 413 on every .cgi request, with no hint that clearing cookies would fix it. That's a full site lockout replacing a graceful degradation.

Suggest gating on the actual cause — e.g. only short-circuit when the body/message-size limit tripped ($c->req->content->is_limit_exceeded / comparing body_size against max_message_size), and letting header/line-limit requests continue to fall through as they do today.

Message accuracy

3. The message quotes a limit that didn't fire

The guard triggers on Mojo's max_message_size, but the number reported comes from maxattachmentsize. MOJO_MAX_MESSAGE_SIZE isn't set anywhere in the repo (only MOJO_MAX_LINE_SIZE is), so the real ceiling is Mojo's 16 MiB default, while maxattachmentsize is admin-configurable and its checker (Bugzilla/Config/Common.pm:266) only bounds it against MySQL's max_allowed_packet.

With maxattachmentsize=20480, an 18 MB upload fails with "The request is too large. Attachments are limited to 20 MB." — self-contradictory, and the advertised limit is unreachable. Even at 16384 the multipart and base64 overhead means an at-limit attachment fails while the message says that exact size is fine.

Reporting the limit that actually fired (or setting MOJO_MAX_MESSAGE_SIZE from maxattachmentsize plus headroom so the two agree) would fix this.

4. The hint blames attachments when there may not be one

The condition is only $file eq 'post_bug.cgi' && $content_type =~ m{^multipart/form-data}i — nothing checks whether a file part exists. enter_bug.cgi's form is always multipart because it has a file input, so a reporter pasting a multi-megabyte stack trace into Description and attaching nothing gets:

The request is too large. Attachments are limited to 10 MB.

(confirmed by POSTing multipart with a single huge comment field and no file part). No hint that the comment is the thing to trim.

5. attachment.cgi gets no guidance at all

The $file eq 'post_bug.cgi' gate excludes attachment.cgi — the "Add an attachment" flow on an existing bug, which is the far more common upload path than attaching at bug creation. Over-limit POSTs to /attachment.cgi and /process_bug.cgi both fall through to the bare The request is too large.\n.

6. Unstyled response and a third unit convention

This bypasses ThrowUserError / global/user-error.html.tmpl entirely, so a browser user lands on a bare text/plain page — no chrome, no back link, no maintainer contact, not localizable. Meanwhile the pre-existing over-limit path for the same file (template/en/default/global/user-error.html.tmpl:729) renders a proper error page.

The units also diverge. Because line 73 switches to MB only when maxattachmentsize % 1024 == 0, one installation reports its limit three different ways:

┌─────────────────────────────────┬─────────────────────────────────────────┐
│ Source │ Text │
├─────────────────────────────────┼─────────────────────────────────────────┤
│ this patch │ 4 MB │
├─────────────────────────────────┼─────────────────────────────────────────┤
│ global/user-error.html.tmpl:729 │ Attachments cannot be more than 4096 KB │
├─────────────────────────────────┼─────────────────────────────────────────┤
│ js/attachment.js:463 │ 4.0 MB │
└─────────────────────────────────┴─────────────────────────────────────────┘

Following the existing KB convention (or fixing all three at once) would avoid users wondering whether they hit one limit or three.

Tests

7. MOJO_MAX_MESSAGE_SIZE=512 also caps the test client's response parser

Mojo::Message's max_message_size defaults to $ENV{MOJO_MAX_MESSAGE_SIZE} on the response side too, and the 413 response is ~1050 bytes — 951 of it headers, dominated by the ~600-byte Content-Security-Policy-Report-Only value added in after_dispatch. I confirmed in the test image that $t->tx->res->is_limit_exceeded is 1 for this test's response.

It passes only because the whole response arrives in a single read, so Mojo::Message::parse finishes the body before the size check trips, and Mojo::UserAgent.pm:249 then overwrites the limit error with the 413 status error so post_ok never notices. Split that response across two reads — a larger CSP after enabling github_client_id/phabricator params, one extra security header, different socket timing — and content_is sees a truncated body and fails in CI with a misleading diagnostic.

$t->ua->max_response_size(0) after constructing $t (or scoping the limit to the request rather than the env) removes the coupling.

8. Two uncovered paths

Both cases POST multipart to /post_bug.cgi, so neither the bare The request is too large.\n branch nor the under-limit pass-through is exercised. A change that made the hint unconditional, or one that fired the guard for in-limit requests, would keep this file green while breaking every legacy CGI POST. Two additions would close it: one under-limit post_ok asserting a non-413 status, and one oversized POST to a different .cgi.

Non-blocking

Depth of the check. The guard only protects legacy .cgi routes; native Mojo routes still process truncated bodies. Bugzilla/App/Controller/SES.pm:106 does decode_json($self->req->body) on what may be a truncated payload, and the API/MFA/OAuth2 controllers are in the same position. The app-wide before_routes hook at Bugzilla/App.pm:60 (used for the block_user_agent rejection) is the right depth and would cover everything in one place.

No log line. Rejections are silent, unlike the templated error paths which log via Bugzilla/App/Plugin/Error.pm's _make_logfunc. When a user reports "my upload just fails," an operator can't distinguish Mojo's limit from the front-end proxy from post_bug.cgi's own file_too_large check, or measure frequency. A single WARN with path and size would make it observable.

[loganrosen]

Thanks for the detailed review. I addressed the requested changes in e51c8e0:

• Oversized REST, JSON-RPC, and XML-RPC requests now retain their existing machine-readable error formats while returning HTTP 413.
• The guard now fires only for Mojolicious’s Maximum message size exceeded error, so header, start-line, and buffer-limit behavior is unchanged.
• The response is now a generic, localized “The request is too large.” message. Browser requests render the normal Bugzilla error page, and the response no longer quotes the unrelated attachment limit or uses attachment-specific guidance.
• The test client’s response limit is decoupled from the lowered server limit, and coverage now includes under-limit CGI requests, generic CGI rejection, each legacy API format, and the exact limit-cause predicate.
• Rejections now emit a warning containing only the CGI route and Mojolicious limit reason.

I kept app-wide handling for native Mojo routes out of this fix because the reported regression occurs at the shared legacy CGI bridge; that broader hardening can be handled separately.

[Copilot]

Review details

Comments suppressed due to low confidence (2)

Bugzilla/App/Controller/CGI.pm:140

• This bypasses Bugzilla's JSON-RPC error-code translation. Bugzilla/Error.pm:128-136 adds 100000 to every Bugzilla error code before exposing it through JSON-RPC, so an unknown transient error is normally 132000; returning raw 32000 changes the endpoint's established error contract and can collide with JSON::RPC's own codes. Apply the same offset here and update the test expectation.

          code    => ERROR_UNKNOWN_TRANSIENT,

template/en/default/global/user-error.html.tmpl:1725

• The implementation now deliberately returns this generic message, but the PR description still says multipart post_bug.cgi responses show the configured attachment limit. Update the description so reviewers and release history do not promise behavior that was intentionally removed.

    The request is too large.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Medium

[dklawren]

Issues

1. PR body doesn't match the diff. Body: "Multipart post_bug.cgi responses also show the configured attachment limit." Template says only The request is too large. — no limit, no maxattachmentsize. Either code is missing or body is stale. This matters: bug 1832783's user pain is "why did it fail" — a bare "too large" barely improves on the old error. Include the limit:

[% ELSIF error == "request_too_large" %]
  [% title = "Request Too Large" %]
  The request is too large. Attachments must be smaller than
  [% Param('maxattachmentsize') FILTER html %] KB.

2. Only Maximum message size exceeded is caught — Maximum buffer size exceeded isn't. max_buffer_size (MOJO_MAX_BUFFER_SIZE, default 256KB) also sets is_limit_exceeded with a truncated body, and that request still falls through to the misleading bug_type error. Same class of bug, not fixed. Start-line/header limits are genuinely different (Mojo handles those upstream), but buffer-size belongs in this branch.

3. Detection by exact-matching a Mojolicious internal string (CGI.pm:117) is brittle across Mojo upgrades — a reworded message silently reverts to the old broken behavior with no test failure at the HTTP level (only the unit test with the hand-rolled TestRequest would catch it, and it asserts the same hardcoded literal, so it can't). Prefer a message-agnostic check:

sub _is_message_size_exceeded {
  my ($request) = @_;
  return 0 unless $request->is_limit_exceeded;
  my $max = $request->max_message_size;
  return $max && ($request->headers->content_length // 0) > $max;
}

4. Guard lives in the CGI wrapper only. Native Mojo routes (OAuth2 provider, BouncedEmails, NewRelease, ComponentGraveyard, …) still get truncated bodies. A before_dispatch hook in Bugzilla/App.pm would cover everything in one place and let you use $c->respond_to for content negotiation instead of dispatching on $file. Filename dispatch also misses github.cgi (webhook, JSON consumer — gets an HTML page).

5. _render_request_too_large reimplements Bugzilla::App::Plugin::Error::_render_error — same four-way format dispatch, same hardcoded https://bmo.readthedocs.io/en/latest/api/, same ERROR_UNKNOWN_TRANSIENT fallback. The only reason it can't reuse the helper is that _render_error hardcodes status => 200 (Error.pm:66, Error.pm:95 via the map). Adding an optional status override to user_error and calling $c->user_error('request_too_large', {}, {status => 413}) would delete ~50 lines and keep the two paths from drifting.

6. _request_too_large_message pokes request_cache directly to force the plain-text branch at user-error.html.tmpl:2054. It works, but it bypasses Bugzilla->usage_mode's setter side effects (Bugzilla.pm:508 — the setter also drives error_mode), so setting both keys by hand is load-bearing and undocumented. Add a comment saying why, or reuse the Error plugin per #5. Also: FILTER txt hard-wraps, so a longer message would land newlines inside the JSON message field; trim won't strip those.

7. REST error code is generic. ERROR_UNKNOWN_TRANSIENT (32000) with HTTP 413 contradicts REST_STATUS_CODE_MAP, which maps 32000 elsewhere. Clients keying on the internal code can't distinguish "too large" from any other transient error. Consider a dedicated code in Bugzilla/WebService/Constants.pm.

Style / conventions

• my $REQUEST_TOO_LARGE_ERROR = 'request_too_large'; → use constant matches the codebase.
• New test's MPL header omits the "Incompatible With Secondary Licenses" lines that t/mojo-example.t and the rest of t/ carry.
• Template entry is correctly alphabetized. xml_quote/trim are both exported from Bugzilla/Util.pm:18,24. ✓

Tests

Solid HTTP-level coverage of all four response formats, and the negative status_isnt(413) case is a good guard. Gaps:

• No under-limit regression test. Body claims "normal attachment and bug-creation behavior remains unchanged" but nothing asserts a small multipart POST still reaches the CGI and succeeds. This is the behavior most at risk from the new early-return — worth a test.
• $t->ua->max_response_size(0) is essential (UA reads the same MOJO_MAX_MESSAGE_SIZE=512 for responses and would truncate them) but unexplained — one comment.
• The TestRequest unit test duplicates what the HTTP tests already prove for the positive case, and its negative case assertss the implementation, so it can't catch a Mojo string change. Low value as written; higher value if #3 is adopted.

[Copilot]

Review details

Suppressed comments (2)

Bugzilla/App/Controller/CGI.pm:124

• load_one also wraps BzAPI's REST script as extensions/BzAPI/bin/rest.cgi (see extensions/BzAPI/Extension.pm:291), so oversized /bzapi and /latest requests miss this equality check and fall through to the HTML response. Recognize REST scripts by basename (or explicit endpoint type) so those REST clients retain the machine-readable 413 response too.

  if ($file eq 'rest.cgi') {

Bugzilla/App/Controller/CGI.pm:125

• This early REST response bypasses Bugzilla::WebService::Server::REST::response, which adds Access-Control-Allow-Origin: * to every normal REST response (Bugzilla/WebService/Server/REST.pm:144-155). Without that header, a cross-origin client can submit the multipart request but the browser hides this 413 JSON response, defeating the machine-readable REST error behavior. Add the normal REST CORS header here and cover it in the REST assertion.

    return $c->render(

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

Review details

Suppressed comments (1)

t/app-cgi-request-limit.t:144

• This assertion locks in raw code 58, but Bugzilla's established JSON-RPC contract adds 100000 to web-service codes (Bugzilla/Error.pm:128-140). Expect 100058 after making the oversized-request response follow the same convention.

  ->json_is('/error/code' => 58)

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Balanced

[loganrosen]

@dklawren Thanks for the second review. I addressed the actionable items in 6ab268ab0, with the REST follow-up in 7dd2bee9c:

• Updated the PR description to match the implementation. I kept the localized message generic because maxattachmentsize limits decoded attachment data, while Mojolicious rejects the complete raw request; quoting the attachment limit would still be inaccurate.
• Added Maximum buffer size exceeded to the 413 path and covered both message- and buffer-limit failures through real HTTP requests.
• Kept the exact Mojolicious reason check because the request exposes no structured limit cause. Content-Length is not equivalent to the raw-message limit, may be absent for chunked requests, and cannot identify buffer-limit failures. The HTTP tests exercise Mojolicious itself, so a dependency rewording that breaks detection will fail them.
• Kept the guard at the shared CGI boundary for this bug. Moving it app-wide would broaden the change to native controllers and webhook-specific response contracts. I also kept the protocol renderers local because the existing Error plugin does not preserve the JSON-RPC and XML-RPC wire formats.
• Added dedicated web-service error code 58 with an HTTP 413 REST mapping. The localized API message now uses the canonical usage-mode setter, with a comment explaining the scoped cache localization; FILTER txt does not wrap while rendering in web-service mode.
• Switched the tag to use constant, restored the full test license notice, documented max_response_size(0), and added an under-limit multipart post_bug.cgi regression test. I retained the focused predicate checks for the intentionally excluded header-limit case.
• The follow-up also recognizes extension-backed BzAPI rest.cgi routes and preserves the normal REST CORS headers on early 413 responses, with regression coverage for /rest and /bzapi.

I filed bug 2059957 to track app-wide handling for native Mojolicious routes separately.

[dklawren]

Coverage is CGI-only. Native Mojo routes (App/Controller/API.pm:33-62: /api, /latest, /bzapi/configuration, and native /rest/* controllers) still dispatch on a truncated body. Mostly GET today so low severity, but Glue.pm or an app-level under would cover everything in one place.

Header/start-line limits still fall through. Maximum header size exceeded (431) and Maximum start-line size exceeded leave the CGI script with a truncated request, same class of misleading failure. The test documents this as intentional, so a follow-up bug is fine, just want it tracked.

[dklawren]

Reuse Bugzilla::App::Plugin::Error. _render_request_too_large's HTML branch is nearly verbatim Bugzilla/App/Plugin/Error.pm:61-68, the JSON branch duplicates :88-95, and _request_too_large_message duplicates :44-46. Calling $c->user_error('request_too_large') with usage_mode set should let you drop most of this, keeping only the jsonrpc/xmlrpc special cases (which are going away soon).

[loganrosen]

Addressed in 5d2d66e. Browser and REST request-limit responses now call $c->user_error('request_too_large'). I added an optional explicit status option to Plugin::Error so this parser failure remains HTTP 413 in development mode as well as production, while JSON-RPC and XML-RPC retain their specialized response formats.

[dklawren]

Third copy of the CORS allow-headers list. _set_rest_cors_headers joins WebService/Server/REST.pm:145-155 and App/Controller/API.pm:107-116. Those two have already drifted apart (API.pm adds authorization, hardcodes x-bugzilla-api-key). Could we extract one shared helper rather than add a third place to maintain?

[loganrosen]

Addressed in 5d2d66e. I extracted set_rest_cors_headers into Bugzilla::WebService::Util and now use it for native API routes, normal legacy REST responses, and early CGI request-limit responses. The canonical list includes authorization plus the translated names from API_AUTH_HEADERS; the focused /rest and /bzapi tests cover those headers.

[Copilot]

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[dklawren]

Sorry to do this to you again but your PR changes some now that jsonrpc and xmlrpc are no longer in tree. So after this round we should be good.

[dklawren]

matching the literal mojo error text only covers the message and buffer limits. a request that trips Maximum header size exceeded or Maximum start-line size exceeded is also is_limit_exceeded with an unparsed body, so it still falls through to legacy cgi dispatch and reproduces the misleading bug_type error this pr is fixing. it also silently disables the guard if mojo ever rewords the string. consider keying off is_limit_exceeded and excluding only the cases you deliberately want to pass through

[loganrosen]

Fixed in f096c6cad. The CGI guard now keys directly off is_limit_exceeded, so message, buffer, header, and start-line limits are all rejected before CGI dispatch without depending on Mojolicious’s error text. The focused predicate coverage now includes all four current limit reasons, a future reworded limit error, and a non-limit control.

[dklawren]

keying the development exception page off exists $options->{status} couples two unrelated things. any future caller that passes a status also loses the dev stack trace. a dedicated option such as skip_exception_page would be clearer

[loganrosen]

Fixed in f096c6cad. Development exception rendering now depends on the dedicated skip_exception_page option rather than the presence of status, and only the parser-limit browser response opts out while retaining HTTP 413. Added coverage verifies that status alone still renders the development diagnostics page with HTTP 500.

[dklawren]

this only guards cgi-dispatched routes, so native mojo api routes still execute with a truncated body

Bugzilla::App::Controller::API registers POST /rest/reminder, POST /rest/component/*product, PUT /rest/component/:product/*component and PUT /rest/lando/uplift under its own /rest bridge, and those match before the /rest/*PATH_INFO fallback to rest.cgi. an oversized body on those endpoints still reaches the handler with partial data instead of a 413

consider moving the check into a shared before_dispatch hook so both dispatch paths get it

[loganrosen]

Fixed in 0c4f423ca. Native API bridges now check is_limit_exceeded before dispatch and return the shared JSON error with HTTP 413/code 58, so oversized requests never reach their controllers. I kept the check in the API bridge rather than a global before_dispatch hook so native routes retain their seven-header CORS contract while legacy REST keeps its broader authentication-header list. The focused test now verifies both under-limit native dispatch and oversized native rejection.

[dklawren]

these five cases do not assert anything about the code

_is_request_limit_exceeded only forwards is_limit_exceeded, and TestRequest returns the flag it was handed at construction, so the message strings are never read. the loop reads like it covers mojolicious limit-message classification but it only checks that 1 is truthy, and the negative case below only checks that 0 is falsy

the real coverage is already in the request-level assertions above, so this block and the _is_request_limit_exceeded indirection it exists for can go

[loganrosen]

Fixed in 0c4f423ca. I removed the forwarding helper, TestRequest, and the six tautological assertions. The CGI wrapper now calls $c->req->is_limit_exceeded directly, with behavior covered by the request-level tests.

[dklawren]

LGTM r=dkl