Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Bugzilla/App.pm
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ sub startup {
$self->plugin('Bugzilla::App::Plugin::Glue');
$self->plugin('Bugzilla::App::Plugin::Login');
$self->plugin('Bugzilla::App::Plugin::Error');
$self->plugin('Bugzilla::App::Plugin::RequestLimit');
$self->plugin('Bugzilla::App::Plugin::Hostage')
unless $ENV{BUGZILLA_DISABLE_HOSTAGE};
$self->plugin('Bugzilla::App::Plugin::SizeLimit')
Expand Down
10 changes: 1 addition & 9 deletions Bugzilla/App/Controller/API.pm
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use Bugzilla::Logging;
use Bugzilla::WebService::Util qw(set_rest_cors_headers);

use constant SUPPORTED_VERSIONS => qw(V1);
use constant REQUEST_TOO_LARGE_ERROR => 'request_too_large';

sub setup_routes {
my ($class, $r) = @_;
Expand Down Expand Up @@ -73,14 +72,7 @@ sub setup_routes {
sub _prepare_rest_request {
my ($c) = @_;
_insert_rest_headers($c);

if ($c->req->is_limit_exceeded) {
my $reason = $c->req->error->{message};
WARN("Rejected oversized request for native REST API: $reason");
Bugzilla->usage_mode(USAGE_MODE_MOJO_REST);
return $c->user_error(REQUEST_TOO_LARGE_ERROR);
}

$c->stash->{request_limit_format} = 'rest';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Bugzilla->usage_mode(USAGE_MODE_REST);
return 1;
}
Expand Down
3 changes: 2 additions & 1 deletion Bugzilla/App/Controller/CSPReport.pm
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ use constant REPORT_FIELDS =>

sub setup_routes {
my ($class, $r) = @_;
$r->post('/csp_report')->to('CSPReport#report');
$r->post('/csp_report')
->to('CSPReport#report', request_limit_format => 'empty');
}

sub report {
Expand Down
3 changes: 2 additions & 1 deletion Bugzilla/App/Controller/Main.pm
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ sub setup_routes {
$r->get('/testagent.cgi')->to('Main#testagent');

$r->add_type('hex32' => qr/[[:xdigit:]]{32}/);
$r->post('/announcement/hide/<checksum:hex32>')->to('Main#announcement_hide');
$r->post('/announcement/hide/<checksum:hex32>')
->to('Main#announcement_hide', request_limit_format => 'json');
}

sub root {
Expand Down
83 changes: 83 additions & 0 deletions Bugzilla/App/Plugin/RequestLimit.pm
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
#
# This Source Code Form is "Incompatible With Secondary Licenses", as
# defined by the Mozilla Public License, v. 2.0.

package Bugzilla::App::Plugin::RequestLimit;
use 5.10.1;
use Mojo::Base 'Mojolicious::Plugin';

use Bugzilla::Constants qw(
USAGE_MODE_MOJO
USAGE_MODE_MOJO_REST
);
use Bugzilla::Logging;

use constant REQUEST_TOO_LARGE_ERROR => 'request_too_large';
use constant REQUEST_TOO_LARGE_MESSAGE => 'The request is too large.';

my %BODY_LIMIT_REASONS = map { $_ => 1 } (
'Maximum message size exceeded',
'Maximum buffer size exceeded',
);

my %KNOWN_NON_BODY_LIMIT_REASONS = map { $_ => 1 } (
'Maximum start-line size exceeded',
'Maximum header size exceeded',
);

sub register {
my ($self, $app, $conf) = @_;
$app->hook(around_action => \&_around_action);
}

sub _around_action {
my ($next, $c, $action, $is_endpoint) = @_;
return $next->() unless $is_endpoint;
return $next->() if $c->isa('Bugzilla::App::Controller::CGI');

my $format = $c->stash->{request_limit_format} // 'html';
my $reason = request_limit_reason($c->req, $format eq 'rest');
return $next->() unless defined $reason;

my $route = $c->match->endpoint->to_string;
WARN("Rejected oversized request for $route: $reason");

if ($format eq 'rest') {
Bugzilla->usage_mode(USAGE_MODE_MOJO_REST);
return $c->user_error(REQUEST_TOO_LARGE_ERROR);
}
elsif ($format eq 'json') {
return $c->render(
json => {error => REQUEST_TOO_LARGE_MESSAGE},
status => 413
);
}
elsif ($format eq 'empty') {
return $c->render(data => '', status => 413);
}

Bugzilla->usage_mode(USAGE_MODE_MOJO);
return $c->user_error(
REQUEST_TOO_LARGE_ERROR,
{},
{status => 413, skip_exception_page => 1}
);
}

sub request_limit_reason {
my ($request, $is_rest) = @_;
return undef unless $request->is_limit_exceeded;

my $error = $request->error;
return 'Unrecognized parser limit' unless ref $error eq 'HASH';

my $reason = $error->{message} // '';
return undef if $KNOWN_NON_BODY_LIMIT_REASONS{$reason} && !$is_rest;
return $reason if $KNOWN_NON_BODY_LIMIT_REASONS{$reason};
return $BODY_LIMIT_REASONS{$reason} ? $reason : 'Unrecognized parser limit';
}

1;
6 changes: 6 additions & 0 deletions docs/en/rst/api/core/v1/general.rst
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ The error contents look similar to:
"code": 123
}

When BMO's request parser exceeds a message-size or body-buffer limit, the
REST API returns ``413 Request Entity Too Large`` with error code ``58`` and
the message ``The request is too large.`` rather than processing the truncated
body. These limits depend on the server configuration. Reduce the payload size
or split it into smaller requests where the API method supports batching.

.. _rest-query-string-limit:

BMO's Varnish front end rejects request targets longer than 8 KiB, including
Expand Down
135 changes: 135 additions & 0 deletions t/app-cgi-request-limit.t
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,43 @@ use Bugzilla::Test::MockParams;
use Test2::V0;
use Test::Mojo;

{
package TestRequest;

sub new {
my ($class, $message, $is_limit_exceeded) = @_;
return bless {
message => $message,
is_limit_exceeded => $is_limit_exceeded // 1
}, $class;
}

sub error {
my ($self) = @_;
return {message => $self->{message}};
}

sub is_limit_exceeded {
my ($self) = @_;
return $self->{is_limit_exceeded};
}
}

{
package TestRequestLimit;

our $LIMIT_NEXT_HEADER = 0;

sub lower_next_header_limit {
my ($tx) = @_;
return unless $LIMIT_NEXT_HEADER;
$LIMIT_NEXT_HEADER = 0;
my $request = $tx->req;
my $headers = $request->headers;
$headers->max_line_size(256);
}
}

my $boundary = 'bugzilla-request-limit';
my $body = join(
"\r\n",
Expand Down Expand Up @@ -53,6 +90,40 @@ my $t = Test::Mojo->new('Bugzilla::App');
# The request-size environment limit also applies to the client's responses.
$t->ua->max_response_size(0);

my $native_json_dispatched = 0;
my $native_json_route = $t->app->routes->post('/_test/native-json-limit');
$native_json_route->to(
request_limit_format => 'json',
cb => sub {
my ($c) = @_;
$native_json_dispatched++;
return $c->render(json => {sentinel => 'controller ran'});
}
);

$t->post_ok(
'/_test/native-json-limit' => {'Content-Type' => 'application/json'} => '{}'
);
$t->status_is(200);
$t->json_is('/sentinel' => 'controller ran');
is($native_json_dispatched, 1, 'under-limit native JSON request dispatches');

$native_json_dispatched = 0;
$t->post_ok(
'/_test/native-json-limit' => {'Content-Type' => 'application/json'} =>
'{"data":"' . ('x' x 512) . '"}'
);
$t->status_is(413);
$t->header_like('Content-Type' => qr{^application/json\b});
$t->json_is('/error' => 'The request is too large.');
$t->content_unlike(qr{controller ran});
is($native_json_dispatched, 0, 'oversized native JSON request does not dispatch');

$t->post_ok(
'/csp_report' => {'Content-Type' => 'application/csp-report'} => 'x' x 512
)->status_is(413)
->content_is('');

$t->post_ok(
'/post_bug.cgi' => {
'Content-Length' => length($small_body),
Expand Down Expand Up @@ -134,6 +205,70 @@ $t->post_ok(
->json_is('/code' => 58)
->json_is('/message' => 'The request is too large.');

$TestRequestLimit::LIMIT_NEXT_HEADER = 1;
my $app = $t->app;
$app->hook(after_build_tx => \&TestRequestLimit::lower_next_header_limit);
$t->post_ok(
'/rest/component/Test' => {
'Content-Length' => 2,
'Content-Type' => 'application/json',
'X-Over-Limit-Header' => 'x' x 256,
} => '{}'
);
$t->status_is(413);
$t->header_like('Content-Type' => qr{^application/json\b});
$t->header_is('Access-Control-Allow-Origin' => '*');
$t->json_is('/error' => 1);
$t->json_is('/code' => 58);
$t->json_is('/message' => 'The request is too large.');

for my $reason (
'Maximum message size exceeded',
'Maximum buffer size exceeded'
) {
is(
Bugzilla::App::Plugin::RequestLimit::request_limit_reason(
TestRequest->new($reason)
),
$reason,
"$reason is rejected before native action dispatch"
);
}

for my $reason ('Maximum header size exceeded', 'Maximum start-line size exceeded') {
is(
Bugzilla::App::Plugin::RequestLimit::request_limit_reason(
TestRequest->new($reason)
),
undef,
"$reason preserves fallback behavior"
);
is(
Bugzilla::App::Plugin::RequestLimit::request_limit_reason(
TestRequest->new($reason),
1
),
$reason,
"$reason remains rejected for REST"
);
}

is(
Bugzilla::App::Plugin::RequestLimit::request_limit_reason(
TestRequest->new('A future Mojolicious limit error')
),
'Unrecognized parser limit',
'unknown parser limits are rejected safely'
);

is(
Bugzilla::App::Plugin::RequestLimit::request_limit_reason(
TestRequest->new('Unrelated parser error', 0)
),
undef,
'non-limit parser errors retain fallback behavior'
);

$t->app->routes->get('/_test/status-error')->to(
cb => sub {
my ($c) = @_;
Expand Down
Loading