1397357 - Don't reject empty POST payloads

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

We return error code 6, "JSON parse failure", when an empty application/newlines payload is sent.

We should not: an empty payload is valid application/newlines.

This is particularly problematic when this is a request to commit a batch. The commit request might well be empty, as it is in this case.


Request:

Host:                   sync-542-us-west-2.sync.services.mozilla.com
Content-Type:           application/newlines
X-If-Unmodified-Since:  1504709019.14
Content-Length:         0
Connection:             keep-alive
Accept:                 */*
User-Agent:             Firefox-iOS-Sync/8.0b1 (iPhone; iPhone OS 10.3.3) (Fennec (sarentz))
Accept-Language:        en-CA;q=1.0, nl-CA;q=0.9, de-CA;q=0.8, fr-CA;q=0.7
Authorization:          ... redacted ...
Accept-Encoding:        gzip;q=1.0, compress;q=0.5

batch:  MTUwNDcxMzg4MjU0Nw==
commit: true

Response:

Server:             openresty/1.9.7.3
Date:               Wed, 06 Sep 2017 16:04:42 GMT
Content-Type:       application/json; charset=UTF-8
Content-Length:     1
Connection:         keep-alive
X-Weave-Timestamp:  1504713882.79

6

Comment 1

[:Grisha Kruglov]

In the Android uploader, we make sure to send an empty payload to commit a batch, in rare circumstances when it's necessary.

Comment 2

[:Grisha Kruglov]

"Empty" payload on Android means Content-Length=2, Content-Type=application/json, body="[]".

Comment 3

[Richard Newman [:rnewman]]

I believe iOS is the only platform that *uploads* application/newlines.

Comment 4

[Richard Newman [:rnewman]]

Ryan, I don't see where in the server codebase this might be rejected. Could you opine?

Comment 5

[Richard Newman [:rnewman]]

If it's true that the server doesn't reject `[]` JSON, but does reject `` newlines, then this is the smallest possible client-side workaround:

https://github.com/mozilla-mobile/firefox-ios/pull/3133

Comment 6

[Ryan Kelly [:rfkelly]]

Nice catch, this will fail out if request.body is an empty string:

  https://github.com/mozilla-services/server-syncstorage/blob/master/syncstorage/views/validators.py#L254

  bso_datas = [json_loads(ln) for ln in request.body.split("\n")]


The result of `"".split("\n")` is `[""]` and trying to `json_loads("")` will throw an error.

Should be a simple patch, and Benson says he has some testcases already, so let's fix this on the server side.  ni? Benson for the tests :-)

Comment 7

[Benson Wong [:mostlygeek]]

Test cases: https://github.com/mozilla-services/server-syncstorage/pull/71/

Comment 8

[Ryan Kelly [:rfkelly]]

> We should not: an empty payload is valid application/newlines.

I agree with this, but to clarify expectations: is an empty payload also valid application/json?  I don't think it should be, but the tests above treat it as valid.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Ryan Kelly [:rfkelly] from comment #8)

> I agree with this, but to clarify expectations: is an empty payload also
> valid application/json?  I don't think it should be, but the tests above
> treat it as valid.

I don't have strong feelings, but I would err on the side of "" being invalid JSON. "" isn't the representation of an empty JSON POST; that would be "[]".

Comment 10

[Benson Wong [:mostlygeek]]

I think we should only allow these for empty payloads:

application/json     -- "[]"
application/newlines -- ""

Anything else should return a 400 Bad Request

Comment 11

[Ryan Kelly [:rfkelly]]

Sold; PR here: https://github.com/mozilla-services/server-syncstorage/pull/72

Comment 12

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

What is the timeline to get this in production?

Asking for a friend who is shipping a product around the 21st.

Comment 13

[Ryan Kelly [:rfkelly]]

O(days), we should comfortably beat your friend's deadline

Comment 14

[Ryan Kelly [:rfkelly]]

Patch landed and going out in v1.6.10, you can follow the deploy at Bug 1397553