Core: Empty body on custom operations

[soyuka]

1. Keep 400 status code on standard operations when an empty body is sent (not valid json)
2. Allow empty body on custom operations (POST/PUT/DELETE)

Ref: #1274 #782 and #805. #1786

[dunglas]

It looks weird to me to not have a different behavior for custom and built in operations.

What about this one: allow empty requests for a all operations IF Content-Type is not set?

[soyuka]

Only thing I know for sure is that sending an empty body on our operations should return a 400 status code because it's invalid (i.e. empty string is not valid json ; Content-Type is indeed a json-like one).

Now, there are users who may want to use empty contents (on POST/PUT operations). Forcing them to not use a Content-Type to be able to do so looks bad for DX no?

[gorghoa]

At least, it should be consistent between PUT and POST (not the case right know I reckon)

[gorghoa]

Now, there are users who may want to use empty contents (on POST/PUT operations). Forcing them to not use a Content-Type to be able to do so looks bad for DX no?

I have to admit that from my point of view, it rings a bell :+1:

[gorghoa]

What about this one: allow empty requests for a all operations IF Content-Type is not set?

~@dunglas, does it even work regarding content negotiation?~

Sorry, was thinking about this while reading bedtime stories, seems now totally irrelevant ;)

[dunglas]

I'm 👎 to allow empty bodies if the content-type is set to JSON or JSON-LD. An empty string is not a valid JSON document (you can try with https://jsonlint.com/) but the client is advertising that the payload is valid JSON. To me it should generate a 400.

Validating the content-type is an OWASP best practice.

[sroze]

I'm 👎 to allow empty bodies if the content-type is set to JSON or JSON-LD.

Agree. Though I would 👍 very very much without a Content-Type.

[gorghoa]

Wokay! I have to verify to be sure that PUT currently allow empty body while content-type is set, but if so, shouldn't it be the same for PUT then? (BC breaking…)

[dunglas]

I don't think we allow it for PUT neither.

[dunglas]

It seems to be OK if there is no body and no content-type set: https://stackoverflow.com/a/29784642/1352334

[gorghoa]

I don't think we allow it for PUT neither.

@dunglas , I was assuming PUT was allowed to skip deserialization of empty content based on https://github.com/api-platform/core/blob/master/src/EventListener/DeserializeListener.php#L55

I may very well be missing something though, I am not very comfortable with the codebase yet :)

[gorghoa]

Some facts I experiment with my setup (api-p v2.2.2):

note: all requests have been made with curl -X POST|PUT http://my-endpoint.tcho/api [-H 'Content-Type: application/json'] [--data '{}'])

| method | content-type | body content | resulting http code | notes |
|-----------|----------------|-------------|------------|--------|
|PUT | not set | empty | 200 | |
|PUT | application/json | empty | 200 | |
|PUT | application/json | {} | 200 | |
|POST | not set | empty | 406 | not acceptable: content-type mandatory |
|POST | application/json | empty | 400 | Bad request, syntax error |
|POST | application/json |{} | 200 | Ok |

[dunglas]

Ok we need to fix this inconsistency...

[gorghoa]

To recap, should the expected behavior for POST|PUT be:

| content-type | body content | result | notes |
|-----------------|-----------------|---------|--------|
| not set | empty | process the request | |
| not set | with data | 406 | throw not acceptable: content-type is mandatory |
| set | empty | may throw 400 if deserializer fails | an empty string is valid text but invalid json |
| set | with data | process the request ||

?

Should this behavior be extended to GET|PATCH|DELETE ? While passing a body to a GET request is technically ok with the HTTP spec, it’s not recommended to do it. Still, the choice of passing GET body should be in the hands of the end user IMO.

From elasticsearch (which does use body in GET request) doc:

A GET Request with a Body?

The HTTP libraries of certain languages (notably JavaScript) don’t allow GET requests to have a request body. In fact, some users are suprised that GET requests are ever allowed to have a body.

The truth is that RFC 7231—the RFC that deals with HTTP semantics and content—does not define what should happen to a GET request with a body! As a result, some HTTP servers allow it, and some—especially caching proxies—don’t.

The authors of Elasticsearch prefer using GET for a search request because they feel that it describes the action—retrieving information—better than the POST verb. However, because GET with a request body is not universally supported, the search API also accepts POST requests:

POST /_search
{
  "from": 30,
  "size": 10
}

The same rule applies to any other GET API that requires a request body.

https://www.elastic.co/guide/en/elasticsearch/guide/current/_empty_search.html

[teohhanhui]

See https://github.com/api-platform/core/pull/2757