-
Notifications
You must be signed in to change notification settings - Fork 360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(http-server): discard request body if the content-length header i… #2103
fix(http-server): discard request body if the content-length header i… #2103
Conversation
…s explicitly set to zero
@@ -0,0 +1 @@ | |||
16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have taken the freedom to add a NVM RC file. Let me know if you would like this out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acolombier thanks so much for the PR! I think we need to make just one small change to keep the spirit of the test the same.
severity: 'Error', | ||
code: 'required', | ||
message: "must have required property 'status'", | ||
message: 'Body parameter is required', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to update this test to have one of the required properties (either id or status) so that the spirit of the test is still the same: throws error if there's a missing required property in the body. Otherwise this test now changes to testing the behavior if the body is missing completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is a problem:
- The body used to be optional (thus change at line 689)
- There
body required
check and the content assertion happens at two different places
My understanding of the describe
/it
is that this unit test might not have been covering the part it says it does.
I have made a change which I think should reflect a bit better the usecase this test was meant to test. The body required
check seems to already be covered by this test, but happy to add an other one if you think it should have thorough testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the test! This looks good to me. Thank you for your contribution!
This has been released in version 4.10.2 🎉 |
@chohmann This looks to be causing a breakage where requests are accepted when no body is provided, but the body is defined as having one or more required parameters and should thus be rejected. |
Addresses #2102
Summary
This PR changes the condition of the HTTP server resolving the request body. It assumes that the body is null in case of an explicit
Content-Length: 0
.Since in #1990 the decision was made to assume no body in case of no explicit
Content-Length
other than zero (omitting will assume no body - which seems matching the HTTP/1.1 spec), however I didn't follow the same approach as it could be seen as a minor changes (as apposite to a patch).Let me know if you would like me to default it to 0 in case if missing as well!
Checklist
Screenshots
Before
![image](https://user-images.githubusercontent.com/7086688/183255399-8e13a3fd-507a-4a09-a5d6-96fc6d31298f.png)
After
![image](https://user-images.githubusercontent.com/7086688/183255463-a38f6795-5fdf-4fba-976e-b619d6d2f168.png)
Additional context
N/A