Project

General

Profile

Actions

Bug #15306

closed

[API] Ensure 0 and false are accepted as false for boolean params in a www-form-encoded body or query string, if they were in rails4.

Added by Lucas Di Pentima about 3 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
06/04/2019
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

It seems that form-encoded (and maybe query string?) params are received as strings on the API on cases that should be boolean. E.g.: include_trash


Subtasks 1 (0 open1 closed)

Task #15312: Review 15306-api-nonjson-paramsResolvedLucas Di Pentima06/04/2019

Actions
Actions #1

Updated by Lucas Di Pentima about 3 years ago

Added tests at 5ea0a240e - branch 15306-api-nonjson-params

It seems that params are correctly interpreted (at least on the include_trash case), or maybe the tests aren't doing the right thing?

Actions #2

Updated by Tom Clegg about 3 years ago

If I add some debug printfs to railsAPI and do a test request with workbench->controller->railsAPI, I get this:

App 28128 output: find_objects_for_index: request original_url is "https://localhost:42701/arvados/v1/collections/zzzzz-4zz18-subprojgonecoll?include_trash=false" 
App 28128 output: find_objects_for_index: request query_parameters is {"include_trash"=>"false"}
App 28128 output: find_objects_for_index: params[:include_trash] is "false", interpreting that as true

But when the tests in note-1 use ?include_trash=false, they work, and the debug printf says "params[:include_trash] is false, interpreting that as false."

I suspect the difference is that load_required_parameters() converts strings to bools, but we don't declare the include_trash parameter for the "show collection" action, so it doesn't get converted.

Actions #3

Updated by Lucas Di Pentima about 3 years ago

Updates at 0d5c86670 expose the bug. This happens also on the 1.3-dev branch so it's not a regression caused by the rails 5 upgrade.

Actions #4

Updated by Lucas Di Pentima about 3 years ago

Trying to see if this bug was introduced as a regression, I got to 87ea4388edb977d246b09b78d9d4bfa5c2ba5170 (from #9587 - 2 years ago), and the tests fail the same. I'm stopping the digging to use the time to fix the issue.

Actions #6

Updated by Lucas Di Pentima about 3 years ago

Groups also have the same problem. Added tests and fix on 18ae48a2e
Tesst run: https://ci.curoverse.com/job/developer-run-tests/1280/

Actions #7

Updated by Lucas Di Pentima about 3 years ago

  • Target version changed from 2019-06-05 Sprint to 2019-06-19 Sprint
Actions #8

Updated by Tom Clegg about 3 years ago

Does the JSON test really pass the include_trash parameter as JSON? The "show" half of the test says :format => :json but I think that is just specifying a desired response format.

I suspect that in order to pass JSON-encoded params to show/list, you need to do a post with :_method => "GET"...

As for the fix itself, it seems like this still leaves other similar loopholes open. Nearly all actions call find_object_by_uuid, which calls find_objects_for_index, which uses params[:include_trash] -- but only show and index go through the bool-parsing code. Perhaps we could close the loophole by calling a bool_param(:include_trash) method from application_controller.rb instead of using the raw params[:include_trash] value. That method can return false (or even error out) if the param isn't already a bool. If it isn't a bool by now then it wasn't declared by _*_requires_parameters(), isn't a supported/advertised flag for the current action, and shouldn't be used.

Actions #9

Updated by Lucas Di Pentima about 3 years ago

Status update: 57eb9ae04 - Test run: https://ci.curoverse.com/job/developer-run-tests/1284/

Added a bool_param() method that returns the param if it exists and it's a boolean, or false. A wb integration test about trashed container requests started failing, so I added include_trash definitions on ContainerRequests#show & ContainerRequests#index, but the same integration test keeps failing, so I'm investigating.

Actions #10

Updated by Lucas Di Pentima about 3 years ago

Thanks to Tom for finding the obvious error I made. Fixed the previous failure by moving the param declarations from the CR model to the controller.
Also added a log warning message when receiving non boolean parameters.

Updates at e9a3a68fd - Test run: https://ci.curoverse.com/job/developer-run-tests/1285/

Actions #11

Updated by Tom Clegg about 3 years ago

LGTM, thanks

Actions #12

Updated by Lucas Di Pentima about 3 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100
Actions #13

Updated by Peter Amstutz over 2 years ago

  • Release set to 22
Actions

Also available in: Atom PDF