Allow 0 length JSON requests #1822
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @allanbomsft, As discussed here: #1767 and here: #1392. That may not be a good idea as the content type state that a JSON is expected. At very least, I would expected an empty JSON, which will be: What is your take on that? |
|
The backstory: we had a customer escalation where their application was posting 0 length requests. Probably it was just zero-parameter RPC calls, and their HTTP client library was probably set up globally to use content type json. It's kind of like passing null, and seems pretty harmless to me. We allow 0 length for xml and form/multipart, so I think we should align to this. |
|
Otherwise maybe we should align so xml also requires at least and form/multipart similar idea. |
|
Adding a bit more to this discussion, It seems like the request body parser for JSON and XML is behaving a bit differently when it comes to overall validation on v2. For instance using an invalid JSON (or XML) request like so: Results in the following: [Using Content-Type: application/json]: [Using Content-Type: application/xml]: This brings up the following on the error log and returns a 500 error from Apache by default: Notice that even though the parser failed for both, the JSON parser ends up being more forgiving and still allow the REQUEST_BODY phase (2) to start and process the rules on this phase before moving over to the next phase (RESPONSE_HEADERS). But for the 0 length requests, it's the opposite. For a request like so: Results in the following: [Using Content-Type: application/json]: This brings up a On the case of the JSON parsing, yajl_complete_parse() as is, will not return yajl_status_ok for yajl_status() when the JSON handle is empty. (As a reminder, this PR, adds a workaround to change the scenario above checking that the length is large enough for the JSON parser to run successfully and allowing for phase 2 to continue.) Moving on... [Using Content-Type: application/xml]: The scenario above happens due to xml_complete() not satisfying the following condition: Due to the expected XML content being empty, parsing_ctx is not set as normally would and it returns 1 allowing the phase to continue. |
|
All that being said, which behaviour is "correct" for both cases (invalid and empty data) and for both XML and JSON parsers? Should we always allow for the phase to go through regardless and and simply fill REQBODY_ERROR with a non zero value and only and let the rules to do the job of forbidding an empty or invalid request (e.g. rule 200002) or we should stop processing when such conditions are encountered and return an error to Apache (500 by default)? |
|
If you don't provide a way to accept this, we won't be able to support
some (buggy) applications.
If we know it's the (incorrect but) expected behaviour, we must have a
way to let the request in.
An internal error don't allow you to add an exception, so the
REQBODY_ERROR seems the only possibility for me (unless you add some
rules before the ctl:requestBodyProcessor one).
…On 16-11-18 01:37, Victor Hora wrote:
All that being said, which behaviour is "correct" for both cases
(invalid and empty data) and for both XML and JSON parsers?
Should we always allow for the phase to go through regardless and and
simply fill REQBODY_ERROR with a non zero value and only and let the
rules to do the job of forbidding an empty or invalid request (e.g.
rule 200002) or we should stop processing when such conditions are
encountered and return an error to Apache (500 by default)?
|
|
Work-around for all people having that problem (and the same with other parsers): This totally solves the problem, is totally generic & perene and should, imo, be the default - unless somebody sees a possible attack using an empty body. |
|
After long discussions here and out of band, I am convinced that @allanbomsft is the best approach. Among of other reasons:
This was merged into 2.x family and ported to v3. Here is the v3 commit for the reference: d2b14de Test cases for v3 are here: Thank you @allanbomsft, and everybody who participate in the discussion. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow 0 length JSON requests.
0 length XML and multipart already allowed.