Bug #15306

[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 3 months ago. Updated 3 months 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:
-

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

Task #15312: Review 15306-api-nonjson-paramsResolvedLucas Di Pentima

Associated revisions

Revision de3ced15
Added by Lucas Di Pentima 3 months ago

Merge branch '15306-api-nonjson-params'
Closes #15306

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Lucas Di Pentima 3 months 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?

#2 Updated by Tom Clegg 3 months 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.

#3 Updated by Lucas Di Pentima 3 months 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.

#4 Updated by Lucas Di Pentima 3 months 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.

#6 Updated by Lucas Di Pentima 3 months ago

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

#7 Updated by Lucas Di Pentima 3 months ago

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

#8 Updated by Tom Clegg 3 months 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.

#9 Updated by Lucas Di Pentima 3 months 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.

#10 Updated by Lucas Di Pentima 3 months 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/

#11 Updated by Tom Clegg 3 months ago

LGTM, thanks

#12 Updated by Lucas Di Pentima 3 months ago

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

Also available in: Atom PDF