Conversation
add() only read from the JSON body. Reuse the same merge_request_params helper as bugs 2065171/2065173/2072305 instead of writing another copy. No whitelist needed here: create() only reads three named keys, not the whole params hash.
| # 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; |
There was a problem hiding this comment.
to_hash returns an arrayref for any key that appears more than once
POST /rest/reminder?bug_id=123¬e=a¬e=b&reminder_ts=2024-06-08 makes note ['a','b'], and note has no entry in Bugzilla::Reminder::VALIDATORS, so run_create_validators passes it straight through and the DB gets the literal string ARRAY(0x...)
bug_id and reminder_ts are shielded by their validators, note is not — and since this helper is meant for reuse by 2065171/2065173/2072305, every future caller inherits it
suggest collapsing to scalars, e.g. building the hash from $c->req->params->names plus $c->req->param($_)
There was a problem hiding this comment.
Good catch. Fixed as sugested: replaced $c->req->params->to_hash with $params->{$_} = $c->req->param($_) for @{$c->req->params->names} so a repeated key collapses to a scalar instead of an arrayref
| catch { | ||
| return $self->user_error('rest_malformed_json'); | ||
| }; | ||
| my $params = merge_request_params($self); |
There was a problem hiding this comment.
this is where the rest_malformed_json response is lost — see the comment on merge_request_params
if the helper keeps swallowing decode errors, this call site needs its own check so a bad JSON body still returns the right error
There was a problem hiding this comment.
Addressed by the merge_request_params change above => add() now does my ($params, $error) = merge_request_params($self); return $self->user_error($error) if $error;, so a malformed body still gets rest_malformed_json.
…lformed JSON from merge_request_params
| my $params = {}; | ||
| $params->{$_} = $c->req->param($_) for @{$c->req->params->names}; | ||
|
|
||
| if (length $c->req->body) { |
There was a problem hiding this comment.
the decode is gated on length $c->req->body, not on the content type, so any non-JSON body is now rejected
POST /rest/reminder?bug_id=123 with Content-Type: application/x-www-form-urlencoded and body note=hello returns rest_malformed_json, because decode_json is handed the raw form string
that contradicts the comment three lines above, which promises $c->req->params "already covers the query string plus, for POST/PUT, an application/x-www-form-urlencoded or multipart body" — the helper collects those params and then rejects the request they came from. same for multipart
the legacy layer doesn't have this problem: CGI.pm only populates POSTDATA/PUTDATA for non-form content types, so _retrieve_json_params never tries to decode a form body
and since 2065171/2065173/2072305 all reuse this helper, every future caller inherits it
suggest gating the decode on the content type — only decode when $c->req->headers->content_type matches application/json — or skipping it when $c->req->body_params->names is non-empty
| return $usage_mode == USAGE_MODE_JSON || $usage_mode == USAGE_MODE_REST; | ||
| return $usage_mode == USAGE_MODE_JSON | ||
| || $usage_mode == USAGE_MODE_REST | ||
| || $usage_mode == USAGE_MODE_MOJO_REST; |
There was a problem hiding this comment.
this is a real bug and worth fixing — without it every native Mojo REST error message over 72 chars gets \ns injected by the error_message filter in Bugzilla/Template.pm:866, which is exactly what the new test would trip over — but it doesn't belong in bug 2072313
i_am_webservice has 14 call sites and this flips all of them for every USAGE_MODE_MOJO_REST endpoint at once, not just Reminders
most are unreachable under MOJO_REST — native REST auth goes through Bugzilla/App/Plugin/Login.pm, not Bugzilla::Auth::Login::Cookie/APIKey — so the practical risk is low. but extensions/RestrictComments/Extension.pm:58 does change: object_end_of_set_all fires for Bugzilla/API/V1/Github.pm:364 and extensions/PhabBugz/lib/API/V1/PhabBugz.pm:235, and will now stop force-clearing restrict_comments on those updates. that's arguably the correct behavior, but it's untested and unrelated to this bug
suggest either splitting it into its own bug, or dropping the commit here and relaxing the new assertion to json_like so the test doesn't depend on the wrapping
This branch has not been deployed
2 participants
Summary
Reminders.pm's add() only read parameters from the JSON POST body. Adds the query-string merge that the REST docs promise, reusing the same merge_request_params helper as bugs 2065171/2065173/2072305.
Depends on 2072305 per the bug dependency (both introduce the same shared helper; will collapse to one copy once either merges).
Changes
merge_request_paramsnow returns($params, $error)instead of a bare hashref, so a request body that fails to decode as JSON is reported back asrest_malformed_jsonrather than silently ignoredmerge_request_params: build params from$c->req->params->names/param()instead of->to_hash, so a repeated query-string key (e.g.?note=a¬e=b) collapses to a scalar instead of an arrayref reaching an unvalidated fielduser_error('rest_malformed_json')if the body doesn't decodeTest plan
rest_malformed_jsonReferences
rest_malformed_jsoninstead of being silently treated as empty params