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 stops limit-exceeded requests at the CGI boundary and returns HTTP 413 before rebuilding STDIN or executing the legacy script. Multipart post_bug.cgi responses also show the configured attachment limit, while 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.