Listing resources with invalid filters should result in a 400

Bug #1654084 reported by Richard on 2017-01-04
20
This bug affects 2 people
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
Wishlist
Unassigned
openstack-api-sig
Medium
Ed Leafe

Bug Description

Using devstack all in one and running the get request:

curl -X GET 'localhost:35357/v3/users/?cake=isgood'

gives me back the entire user list instead of an error.
I also ran this query with httpie and this is what the neat
version of the output looks like:
http://paste.openstack.org/show/593916/

This affects all list operations in keystone that support
querying.

I'm not sure if there is a standard for validating query strings, but I figured I'd triage this since it has an impact on user-experience for sure.

Thanks Richard!

tags: added: user-experience
summary: - Invalid Get request with non-existent filter returns all users
+ Listing users with non-existent filter returns all users
Changed in keystone:
status: New → Confirmed
importance: Undecided → Low
Steve Martinelli (stevemar) wrote :

According to the HTTP spec the query args are part of the URL, these are invalid and should result in a 4xx error. However, even the great google doesn't adhere to that, try the following:

https://www.google.com/#q=search+for+something&invalid=param&more=stuff

With that said, I tried looking up what the API working group has to say about this:

http://specs.openstack.org/openstack/api-wg/guidelines/pagination_filter_sort.html

But no luck, let's bounce this bug off of them and see what they have to say.

Chris Dent (cdent) wrote :

We haven't formalized this yet for query strings (that I can find), but have discussed it and agree that 400 is the right response here as it is the only way of letting the client side know that something was less than correct about the query.

There's some related discussion in http://specs.openstack.org/openstack/api-wg/guidelines/http.html#failure-code-clarifications about incorrect object attributes in bodies which uses the same logic:

    ...the server should return a 400 Bad Request response. Do not handle the request as normal by ignoring the bad attribute. Returning an error allows the client side to know which attribute is wrong and have the potential to fix a bad request or bad code.

So I reckon there probably needs to be a generic guideline about this, and the filtering guideline should reference that when saying "return a 400 for bad filter params".

Originally I took the stance that the bad param should just be ignored (be liberal in what you accept), but it was exactly the case in this bug that convinced me otherwise.

Changed in openstack-api-wg:
importance: Undecided → Medium
status: New → Confirmed
Steve Martinelli (stevemar) wrote :

Thanks Chris, Ed said the same thing on IRC:

edleafe: stevemar: just saw your question. We generally recommend a 400 with a message that explains the problem

I'll re-word this bug to be a general fix for all our list operations in keystone. Though this is backwards incompatible, I believe it is allowed since it fixes a bug.

summary: - Listing users with non-existent filter returns all users
+ Listing resources with invalid filters should result in a 400
description: updated
Ed Leafe (ed-leafe) on 2017-01-05
Changed in openstack-api-wg:
assignee: nobody → Ed Leafe (ed-leafe)
Tin Lam (lamt) on 2017-01-06
Changed in keystone:
assignee: nobody → Tin Lam (tl3438)

Fix proposed to branch: master
Review: https://review.openstack.org/417315

Changed in keystone:
status: Confirmed → In Progress

Fix proposed to branch: master
Review: https://review.openstack.org/417441

Changed in openstack-api-wg:
status: Confirmed → In Progress
Morgan Fainberg (mdrnstm) wrote :

As said in the meeting, this is an API behavior break that is not security related. We cannot have this bug fixed without microversions or API version rev.

See http://specs.openstack.org/openstack/api-wg/guidelines/evaluating_api_changes.html#guidance

I have marked the assosicated review with a -2 and a similar comment. I will remove the -2 once we have a way forward that is not an API contract break.

An idea floated was a strict query param that new clients can use to change behavior... there are ways to slice this, lets figure out a path forward and we can unblock it.

For the record, I like this being more explicit on non-supported params resulting in a 4xx.

Chris Dent (cdent) wrote :

In response to Morgan: As I said in response to a similar issue in glance, I think the rules in that 'evaluating' guideline are potentially too strict, especially with regard to changes just like this: making people's lives better.

Making people's lives better should be more important than following fairly arbitrary rules about versioning.

That, however, is just my personal opinion and not some kind of official api-wg statement (I wonder, however, if maybe we should change the rules to make them moderately sane?).

Steve Martinelli (stevemar) wrote :

Marking this as wishlist since we can't break API compatibility. The only way we could do this is by implementing microversions.

Changed in keystone:
importance: Low → Wishlist

Reviewed: https://review.openstack.org/417441
Committed: https://git.openstack.org/cgit/openstack/api-wg/commit/?id=b3e3b5f15344d130555f8f49f01336f7781d6ccb
Submitter: Jenkins
Branch: master

commit b3e3b5f15344d130555f8f49f01336f7781d6ccb
Author: EdLeafe <email address hidden>
Date: Fri Jan 6 16:31:00 2017 +0000

    Add guideline for invalid query parameters

    This is in response to the bug "Listing resources with invalid filters
    should result in a 400", which caused problems in Keystone. We had a
    similar guideline for handling invalid entries in a request body; this
    adds similar guidance for query strings.

    Change-Id: I92c1e0e25036c85398ffd0c40f9c16cb3e8a3029
    Partial-Bug: #1654084

Chris Dent (cdent) on 2017-03-05
Changed in openstack-api-wg:
status: In Progress → Fix Released
tags: added: fix-requires-microversion
Lance Bragstad (lbragstad) wrote :

Automatically unassigning due to inactivity.

Changed in keystone:
assignee: Tin Lam (lamt) → nobody
Morgan Fainberg (mdrnstm) wrote :

I want to point out I disagree with the guidance from the API-SIG. I rather have a server ignore (drop) query params than raise a 400.

Changed in keystone:
status: In Progress → Triaged
Chris Dent (cdent) wrote :

But but Morgan (said in a whiney voice) how would anyone know that they sent a bad query param, one that might have been a typo for a good one?

Change abandoned by Tin Lam (<email address hidden>) on branch: master
Review: https://review.opendev.org/417315

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers