775208 - Clarify handling of reserved comma character in URIs

Status: RESOLVED FIXED

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[Gregory Szorc [:gps]]

We need to clarify how commas are handled in URIs to storage service 2.0.

Per https://tools.ietf.org/html/rfc3986#section-2.2, "," is a reserved character. This means some clients will percent encode it to "%2C" when it is seen in the query string to collection GET and DELETE URIs.

I can't remember exactly what RFCs take precedence here, but I /think/ there may be some flexibility in choosing to percent encode "," because it isn't being used as a delimiter in the query string. Still, a client might be conservative and percent encode *all* reserved characters, regardless of whether its not being used as a delimiter. JavaScript's encodeURIComponent (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/encodeURIComponent) does this, for example.

Anyway, there are no examples in the 2.0 spec, so it isn't clear what clients should do. The spec should clarify this with examples. I would vote for accepting both. If the server applies percent decoding on the value in the query string, it should be a noop. Not sure if it does this today.

My JS server implementation explicitly splits on comma without percent decoding and it chokes when it sees percent encoding. It passes the Python functional tests. So, I think we need additional functional tests ensuring the server handles percent encoding properly.

I will follow a separate bug to tackle the JS server changes to support percent encoding.

Comment 1

[Toby Elliott [:telliott]]

I think this is a wider bug, which is essentially "make sure servers do percent decoding always". That's basically free on modern frameworks, though - they do it before we even get to code we handle.

Comment 2

[Ryan Kelly [:rfkelly]]

Attachment: patch adding a test with percent-encoded commas

Adding python functional test case.  :gps, any other interesting corners this needs to cover?

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 643650 [details] [diff] [review]
patch adding a test with percent-encoded commas

I was first confused because the test was named "delete_ids" but it also contained a test for GET. Looks good though.

Comment 4

[Ryan Kelly [:rfkelly]]

(In reply to Gregory Szorc [:gps] from comment #3)
> I was first confused because the test was named "delete_ids" but it also
> contained a test for GET. Looks good though.

Right, good point, I will change this to "test_specifying_ids_with_percent_encoded_query_string"

Comment 5

[Ryan Kelly [:rfkelly]]

https://github.com/mozilla-services/server-syncstorage/commit/c8949ca082c847d543877bd1150f2581e0b12a75