Respond with 412 Precondition Failed when required request headers are missing/invalid

[bhstahl]

Currently, a 412 Precondition Failed is used when the client is not supported by the server.

But, this could also be used if the request headers don't satisfy the protocol, such as if the Upload-Offset header is missing inPATCH requests

PATCH /files/17f44dbe1c4bace0e18ab850cf2b3a83 HTTP/1.1
Content-Length: 11
Tus-Resumable: 1.0.0

hello world

HTTP/1.1 412 Precondition Failed

or the Upload-Length, Upload-Defer-Length headers with POST requests.

POST /files HTTP/1.1
Tus-Resumable: 1.0.0

HTTP/1.1 12 Precondition Failed

Thoughts?

[bhstahl]

Providing f22a9268b9a6567979336711b0723cc83a24436b as an example.

[kvz]

I would be cool with this change, you @Acconut? If we merge, I think we'll have to bump the minor as part of this PR. So this will become tus 1.1.0 then. In addition, also as part of this PR, I think you should creditor yourself as a protocol author @bhstahl; so far we have added anyone who made a contribution like this.

[bhstahl]

Great! Will add that in a separate commit.

[Acconut]

I apologize for not responding there yet. I agree that the protocol is not always specific about how to handle each error case but I am afraid of giving to much possibilities to a single status code, in this case the 412.

As alternatives I propose, the good old 400 Bad Request or alternatively 422 Unprocessable Entity or 428 Precondition Required (although this may be appropriate when the Tus-Resumable header is missing). (see http://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml).

[bhstahl]

Sticking to 400 Bad Request completely works, request header fields are well within its prowess.

422 Unprocessable Entity worries me about giving too much possibilities to a single status code. It already encompasses properties, collections, locking, namespace operations, etc.

If we're going head to head between 412 Precondition Failed and 428 Precondition Required, I would favor 412. Unless I'm wrong, which is most likely the case, 428 is primarily for when something has changed serverside and this new request needs to be reformatted. By definition:

4.2. 412 Precondition Failed

The 412 (Precondition Failed) status code indicates that one or more conditions given in the request header fields evaluated to false when tested on the server.

3. 428 Precondition Required

The 428 status code indicates that the origin server requires the request to be conditional.

[ruv]

@bhstahl, it is not good idea to mix the cases of missed and invalid headers in response with status code 412.

By HTTP spec, when some required precondition header is missed, the response should be 428 Precondition Required.

When precondition header is not matched the server side state, the response should be 412 Precondition Failed. See also RFC4918, section 12.1 (HTTP extension for WebDAV)

if the client did not include a conditional header in the request, then the server MUST NOT use this [412] status code.

So, 412 is not suitable for case of missing header.

In case of some header has invalid syntax — 400 Bad Request is good choice.

What the headers are precondition is up to the certain protocol extension. In basic HTTP the precondition headers are If-* headers (like If-Match, If-Modified-Since, If-Range, etc), in WebDAV the Overwrite header is also precondition.

In sum:

• missed precondition header — 428
• mismatch resource state — 412
• invalid syntax — 400

PS side note regarding the protocol versioning (inspired by: 412 "is used when the client is not supported by the server"):

Using linear, numeric minor versions for negotiating new features is really, really limiting; complex APIs will find this especially impractical — Mark Nottingham, Evolving HTTP APIs.

[bhstahl]

Good points @ruv, thanks for opining. I agree that it makes sense to create a distinction between missing/invalid.

@Acconut @kvz would you guys be ok with:

Invalid header responds 400 Bad Request

Example: A non-numeric Upload-Length in POST request

Missing required header responds 428 Precondition Required

Example: Upload-Offset missing in PATCH request

If so, I can update #80.

[ruv]

@bhstahl, the following reasoning can be also considered.

If some request can be responded with 428, it also should be responded with 412 in some cases. Otherwise (if there is no 412 cases at all by design) this request doesn't have precondition headers at all (i.e. non of its headers can be considered as precondition). In such case it may be better to avoid using 428 for this request as well (although I don't sure that it is better in all the cases ;)

Applying this criterion to PATCH request: if Upload-Offset is precondition header then PATCH request should be responded with 412 in some cases by design.