Be more strict on query strings

RESOLVED WONTFIX

Status

Cloud Services
Server: Core
--
major
RESOLVED WONTFIX
7 years ago
7 years ago

People

(Reporter: tarek, Assigned: tarek)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 505145 [details] [diff] [review]
adds the check_qs + controls the query string
Attachment #505145 - Flags: review?(telliott)
(Assignee)

Comment 2

7 years ago
Created attachment 505146 [details] [diff] [review]
added check_qs options
Attachment #505146 - Flags: review?(telliott)
(Assignee)

Comment 3

7 years ago
Created attachment 505148 [details] [diff] [review]
added check_qs options
Attachment #505148 - Flags: review?(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.
Attachment #505145 - Flags: review?(telliott) → review+
Attachment #505146 - Flags: review?(telliott) → review+
Attachment #505148 - Flags: review?(telliott) → review+
(Assignee)

Comment 5

7 years ago
(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.
(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.
(Assignee)

Comment 7

7 years ago
(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.
(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.
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.