627024 - Be more strict on query strings

Status: RESOLVED WONTFIX

(Cloud Services :: Server: Core, defect)

Description

[Tarek Ziadé (:tarek)]

Right now, APIs that get unknown options in query strings --besides metrics for info/collections-- will throw an error.

Let's harden the URL match definition and explicitly list allowed variables in query strings. For each definition, we'll add an optional list of options:

- ['*'] means any options.
- ['sort', 'asc'] means the API calls query strings option need to be in that list.
- no option provided means that the API does not take any query string.

When the URL matches but the query string contains non allowed variables, a 400 is returned.

The change will be done in the base application in server-core, and definitions of APIs will be adapted in server-reg and server-storage.

Comment 1

[Tarek Ziadé (:tarek)]

Attachment: adds the check_qs + controls the query string

Comment 2

[Tarek Ziadé (:tarek)]

Attachment: added check_qs options

Comment 3

[Tarek Ziadé (:tarek)]

Attachment: added check_qs options

Comment 4

[Toby Elliott [:telliott]]

Comment on attachment 505145 [details] [diff] [review]
adds the check_qs + controls the query string

The code on these is fine, but I'm not sold that it's a good idea. We don't know what parameters other applications may want to add (id strings, proxy information), and this now breaks them in unexpected ways. It also means that if we ever add a parameter, we're forced into a major version bump, since we can't ensure that all the other clients out there have updated the code.

Comment 5

[Tarek Ziadé (:tarek)]

(In reply to comment #4)
> Comment on attachment 505145 [details] [diff] [review]
> adds the check_qs + controls the query string
> 
> The code on these is fine, but I'm not sold that it's a good idea. 

I won't push it if you're not convinced about this :) e.g. that's helpers to build our apps, so..

> We don't
> know what parameters other applications may want to add (id strings, proxy
> information), and this now breaks them in unexpected ways.

Having a free list of arguments in a GET or DELETE is a very specific use case. e.g. like the metrics marker. Other than that, everything should happen in the headers.

I can change it, so by default everything is allowed. Then if we want to restrict an API to a specific set of options, we can do it.

> It also means that
> if we ever add a parameter, we're forced into a major version bump, since we
> can't ensure that all the other clients out there have updated the code.

Why ? adding a parameter does not force other clients to have that parameter, it just says that this new parameter, if encountered, is allowed.

Comment 6

[Toby Elliott [:telliott]]

(In reply to comment #5)
> (In reply to comment #4)
> > It also means that
> > if we ever add a parameter, we're forced into a major version bump, since we
> > can't ensure that all the other clients out there have updated the code.
> 
> Why ? adding a parameter does not force other clients to have that parameter,
> it just says that this new parameter, if encountered, is allowed.

You have the problem backwards. There will be old servers out there that will reject new clients because they have an unrecognized parameter. 

Odds are that this'll never be an issue, but both the benefits and the costs here seem somewhat minimal, so I'm having a hard time weighing it on balance.

Comment 7

[Tarek Ziadé (:tarek)]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > It also means that
> > > if we ever add a parameter, we're forced into a major version bump, since we
> > > can't ensure that all the other clients out there have updated the code.
> > 
> > Why ? adding a parameter does not force other clients to have that parameter,
> > it just says that this new parameter, if encountered, is allowed.
> 
> You have the problem backwards. There will be old servers out there that will
> reject new clients because they have an unrecognized parameter. 
> 
> Odds are that this'll never be an issue, but both the benefits and the costs
> here seem somewhat minimal, so I'm having a hard time weighing it on balance.

MMMmm but if the new client use a new parameter, it means that it expects the server to do something with it. --unless its a vague one ala metrics-- e.g. a new server-side behavior/feature.

Will think about it on my side and come up with precise examples.

Comment 8

[Toby Elliott [:telliott]]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > It also means that
> > > > if we ever add a parameter, we're forced into a major version bump, since we
> > > > can't ensure that all the other clients out there have updated the code.
> > > 
> > > Why ? adding a parameter does not force other clients to have that parameter,
> > > it just says that this new parameter, if encountered, is allowed.
> > 
> > You have the problem backwards. There will be old servers out there that will
> > reject new clients because they have an unrecognized parameter. 
> > 
> > Odds are that this'll never be an issue, but both the benefits and the costs
> > here seem somewhat minimal, so I'm having a hard time weighing it on balance.
> 
> MMMmm but if the new client use a new parameter, it means that it expects the
> server to do something with it. --unless its a vague one ala metrics-- e.g. a
> new server-side behavior/feature.

Right. It's the vague ones a la metrics that worry me (or some weird backwards-compatible corner). If it's a behavior changing parameter, then we need to version-bump, yes.