From c4dd1ba8d3da66258f7e2843845e5443b3da5bd8 Mon Sep 17 00:00:00 2001 From: Xavier L'Hour Date: Thu, 17 Sep 2026 19:17:15 +0200 Subject: [PATCH 1/5] Bug 2072305 - Accept query-string params on Component create/update create/update only read from the JSON body. Adds the query-string merge the docs promise, reusing the same merge_request_params helper as bugs 2065171/2065173. update() now whitelists fields before set_all(), since form-urlencoded cookie-auth requests (which include Bugzilla_api_token) previously failed JSON parsing before reaching it and no longer will. --- Bugzilla/API/V1/Component.pm | 43 ++++++++++++++---------------------- Bugzilla/WebService/Util.pm | 35 +++++++++++++++++++++++++++++ qa/t/rest_components.t | 18 ++++++++++++++- 3 files changed, 68 insertions(+), 28 deletions(-) diff --git a/Bugzilla/API/V1/Component.pm b/Bugzilla/API/V1/Component.pm index ae60207c80..68fe919e07 100644 --- a/Bugzilla/API/V1/Component.pm +++ b/Bugzilla/API/V1/Component.pm @@ -10,12 +10,12 @@ package Bugzilla::API::V1::Component; use 5.10.1; use Mojo::Base qw( Mojolicious::Controller ); -use Mojo::JSON qw(decode_json false true); -use Try::Tiny; +use Mojo::JSON qw(false true); use Bugzilla::Component; use Bugzilla::Constants; -use Bugzilla::Util qw(email_filter trim); +use Bugzilla::Util qw(email_filter trim); +use Bugzilla::WebService::Util qw(merge_request_params); sub setup_routes { my ($class, $r) = @_; @@ -47,8 +47,7 @@ sub create { || return $self->user_error('auth_failure', {group => 'editcomponents', action => 'add', object => 'components'}); - my ($params, $error) = $self->_get_params(); - return $self->user_error($error) if $error; + my $params = merge_request_params($self); my $product = Bugzilla::Product->check({name => $self->param('product')}); @@ -95,21 +94,24 @@ sub update { my $component = Bugzilla::Component->check( {name => $self->param('component'), product => $product}); - my ($params, $error) = $self->_get_params(); - return $self->user_error($error) if $error; + my $params = merge_request_params($self); + + # Whitelist the documented update fields; set_all() throws unknown_method + # for any stray key (e.g. Bugzilla_api_token, include_fields). + my %values = map { $_ => $params->{$_} } + grep { exists $params->{$_} } + qw(name description default_assignee default_qa_contact default_bug_type + is_active triage_owner team_name bug_description_template); # If the user is only able to edit triage owner and nothing else, # then we only allow that field to be passed to set_all() if (!$user->in_group('editcomponents')) { - if (exists $params->{triage_owner}) { - $params = {triage_owner => $params->{triage_owner}}; - } - else { - $params = {}; - } + %values = exists $values{triage_owner} + ? (triage_owner => $values{triage_owner}) + : (); } - $component->set_all($params); + $component->set_all(\%values); $component->update(); return $self->render(json => $self->_component_to_hash($component)); @@ -143,17 +145,4 @@ sub _component_to_hash { }; } -sub _get_params { - my ($self) = @_; - my $params = {}; - my $error = ''; - try { - $params = decode_json($self->req->body); - } - catch { - $error = 'rest_malformed_json'; - }; - return ($params, $error); -} - 1; diff --git a/Bugzilla/WebService/Util.pm b/Bugzilla/WebService/Util.pm index 32b34a4bb0..f7ade2aecb 100644 --- a/Bugzilla/WebService/Util.pm +++ b/Bugzilla/WebService/Util.pm @@ -21,6 +21,8 @@ use Storable qw(dclone); use URI::Escape qw(uri_unescape); use Type::Params qw( compile ); use Types::Standard -all; +use Mojo::JSON qw(decode_json); +use Try::Tiny; use base qw(Exporter); @@ -38,6 +40,7 @@ our @EXPORT_OK = qw( params_to_objects fix_credentials set_rest_cors_headers + merge_request_params ); sub set_rest_cors_headers { @@ -297,6 +300,29 @@ sub params_to_objects { return \@objects; } +sub merge_request_params { + my ($c) = @_; + + # $c->req->params already covers the query string plus, for POST/PUT, an + # application/x-www-form-urlencoded or multipart body. Layer a JSON body + # underneath that (silently ignored if absent or not valid JSON), so + # params work from either the query string or a JSON request body. + # Query-string values win on a key collision, matching the legacy REST + # layer (see fix_credentials/_retrieve_json_params in + # Bugzilla::WebService::Server::REST) and the documented behavior in + # docs/en/rst/api/core/v1/general.rst. + my $params = $c->req->params->to_hash; + + if (length $c->req->body) { + my $body_params; + try { $body_params = decode_json($c->req->body); } + catch { $body_params = undef; }; + $params = {%$body_params, %$params} if ref $body_params eq 'HASH'; + } + + return $params; +} + sub fix_credentials { my ($params, $cgi) = @_; @@ -399,6 +425,15 @@ Helps make life simpler for WebService methods that internally create objects via both "ids" and "names" fields. Also de-duplicates objects that were loaded by both "ids" and "names". Returns an arrayref of objects. +=head2 merge_request_params + +Takes a Mojolicious controller and returns a hashref merging its query +string/form-body params (C<< $c->req->params->to_hash >>) with a decoded +JSON request body, if any. Query-string/form-body values win on a key +collision. For use by native Mojo REST controllers that need to accept +parameters from either the query string or a JSON body on non-GET +requests. + =head2 fix_credentials Allows for certain parameters related to authentication such as Bugzilla_login, diff --git a/qa/t/rest_components.t b/qa/t/rest_components.t index 2a5fadae32..99be67a7c1 100644 --- a/qa/t/rest_components.t +++ b/qa/t/rest_components.t @@ -66,6 +66,14 @@ $t->post_ok($url ->json_is('/message' => 'The Firefox product already has a component named TestComponent.'); +# Fields may also be passed entirely via the query string, with no JSON body. +$t->post_ok($url + . 'rest/component/Firefox?name=QueryStringComponent' + . '&description=Created%20via%20query%20string' + . '&default_assignee=admin%40mozilla.test&team_name=Mozilla' => + {'X-Bugzilla-API-Key' => $api_key})->status_is(200) + ->json_is('/name' => 'QueryStringComponent'); + ### Section 2: Make updates to the component my $update = { @@ -88,12 +96,20 @@ $t->put_ok($url ->json_is('/description' => 'Updated description') ->json_is('/default_assignee' => 'permanent_user@mozilla.test'); +# A query-string parameter is also accepted on PUT, and wins over a matching +# parameter in the JSON body. +$t->put_ok($url + . 'rest/component/Firefox/TestComponent?description=Query%20String%20Wins' => + {'X-Bugzilla-API-Key' => $api_key} => + json => {description => 'Should Not Be Used'})->status_is(200) + ->json_is('/description' => 'Query String Wins'); + # Retrieve the new component and verify $t->get_ok($url . 'rest/component/Firefox/TestComponent' => {'X-Bugzilla-API-Key' => $api_key})->status_is(200) ->json_is('/triage_owner' => 'admin@mozilla.test') - ->json_is('/description' => 'Updated description'); + ->json_is('/description' => 'Query String Wins'); # Update an existing user and give edittriageowners permissions my $user_update = {groups => {add => ['edittriageowners']}}; From 9f6fc0f6c7b86027128035d1a425bf863924d939 Mon Sep 17 00:00:00 2001 From: Xavier L'Hour Date: Thu, 17 Sep 2026 19:49:37 +0200 Subject: [PATCH 2/5] Bug 2072305 - Trigger CI rerun From 672f62071fafa87f6a13c5260c8b79372bfd82b3 Mon Sep 17 00:00:00 2001 From: Xavier L'Hour Date: Thu, 24 Sep 2026 16:29:19 +0200 Subject: [PATCH 3/5] Bug 2072305 - Test that the query string wins over a form body --- qa/t/rest_components.t | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/qa/t/rest_components.t b/qa/t/rest_components.t index 4bbbe1c8e3..6a3b01798b 100644 --- a/qa/t/rest_components.t +++ b/qa/t/rest_components.t @@ -120,6 +120,15 @@ $t->get_ok($url ->json_is('/triage_owner' => 'admin@mozilla.test') ->json_is('/description' => 'Query String Wins'); +# A form-urlencoded body and the query string may both carry the same field; +# the query string wins and the value stays a plain string (it used to be +# merged into an arrayref and stored as "ARRAY(0x...)"). +$t->put_ok($url + . 'rest/component/Firefox/TestComponent?description=Query%20Beats%20Form' => + {'X-Bugzilla-API-Key' => $api_key} => + form => {description => 'Form Body Loses'})->status_is(200) + ->json_is('/description' => 'Query Beats Form'); + # Update an existing user and give edittriageowners permissions my $user_update = {groups => {add => ['edittriageowners']}}; $t->put_ok($url From 327582c15b18a71ccf69e8d7ddbf5f19425175c2 Mon Sep 17 00:00:00 2001 From: Xavier L'Hour Date: Thu, 24 Sep 2026 16:29:37 +0200 Subject: [PATCH 4/5] Bug 2072305 - Test that a malformed JSON body is rejected --- qa/t/rest_components.t | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/qa/t/rest_components.t b/qa/t/rest_components.t index 6a3b01798b..202b29565f 100644 --- a/qa/t/rest_components.t +++ b/qa/t/rest_components.t @@ -129,6 +129,14 @@ $t->put_ok($url form => {description => 'Form Body Loses'})->status_is(200) ->json_is('/description' => 'Query Beats Form'); +# A malformed JSON body is rejected instead of being treated as an empty, +# successful update. +$t->put_ok($url + . 'rest/component/Firefox/TestComponent' => + {'X-Bugzilla-API-Key' => $api_key} => '{"description": ') + ->status_is(400)->json_is('/code' => 32000) + ->json_like('/message' => qr/JSON data used for the request was malformed/); + # Update an existing user and give edittriageowners permissions my $user_update = {groups => {add => ['edittriageowners']}}; $t->put_ok($url From 5a610d335accdbe066d2a91569831f60e13a3e23 Mon Sep 17 00:00:00 2001 From: Xavier L'Hour Date: Thu, 24 Sep 2026 16:29:44 +0200 Subject: [PATCH 5/5] Bug 2072305 - Coerce is_active from query-string and form values --- Bugzilla/API/V1/Component.pm | 11 +++++++++++ qa/t/rest_components.t | 16 ++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/Bugzilla/API/V1/Component.pm b/Bugzilla/API/V1/Component.pm index 197ea223c1..91d9a9ef1e 100644 --- a/Bugzilla/API/V1/Component.pm +++ b/Bugzilla/API/V1/Component.pm @@ -105,6 +105,17 @@ sub update { qw(name description default_assignee default_qa_contact default_bug_type is_active triage_owner team_name bug_description_template); + # A JSON body sends is_active as a real boolean, but the query string and a + # form body send the literal string "true" or "false", and check_boolean + # treats any non-empty string as true. + if (exists $values{is_active} && !ref $values{is_active}) { + my $is_active = lc($values{is_active} // ''); + return $self->user_error('invalid_params', + {type_error => 'is_active must be true or false'}) + if $is_active !~ /^(?:true|false|1|0)$/; + $values{is_active} = ($is_active eq 'true' || $is_active eq '1') ? 1 : 0; + } + # If the user is only able to edit triage owner and nothing else, # then we only allow that field to be passed to set_all() if (!$user->in_group('editcomponents')) { diff --git a/qa/t/rest_components.t b/qa/t/rest_components.t index 202b29565f..6c89012fd4 100644 --- a/qa/t/rest_components.t +++ b/qa/t/rest_components.t @@ -14,6 +14,7 @@ use Bugzilla; use QA::Util qw(get_config); use MIME::Base64 qw(encode_base64 decode_base64); +use Mojo::JSON qw(false true); use Test::Mojo; use Test::More; @@ -137,6 +138,21 @@ $t->put_ok($url ->status_is(400)->json_is('/code' => 32000) ->json_like('/message' => qr/JSON data used for the request was malformed/); +# is_active from the query string is the string "true"/"false", which must be +# coerced rather than treated as a truthy string. +$t->put_ok($url + . 'rest/component/Firefox/TestComponent?is_active=false' => + {'X-Bugzilla-API-Key' => $api_key})->status_is(200) + ->json_is('/is_active' => false); +$t->put_ok($url + . 'rest/component/Firefox/TestComponent?is_active=true' => + {'X-Bugzilla-API-Key' => $api_key})->status_is(200) + ->json_is('/is_active' => true); +$t->put_ok($url + . 'rest/component/Firefox/TestComponent?is_active=maybe' => + {'X-Bugzilla-API-Key' => $api_key})->status_is(400) + ->json_like('/message' => qr/is_active must be true or false/); + # Update an existing user and give edittriageowners permissions my $user_update = {groups => {add => ['edittriageowners']}}; $t->put_ok($url