Skip to content
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

Conversation

acolombier
Copy link
Contributor

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

  • The basics
    • I tested these changes manually in my local or dev environment (Running + Docker)
  • Tests
    • Added or updated
    • N/A
  • Event Tracking
    • I added event tracking and followed the event tracking guidelines
    • N/A
  • Error Reporting
    • I reported errors and followed the error reporting guidelines
    • N/A

Screenshots

Before
image

After
image

Additional context

N/A

@@ -0,0 +1 @@
16
Copy link
Contributor Author

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

Copy link
Contributor

@chohmann chohmann left a 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',
Copy link
Contributor

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.

Copy link
Contributor Author

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:

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.

Copy link
Contributor

@chohmann chohmann left a 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!

@chohmann chohmann merged commit c172f42 into stoplightio:master Aug 11, 2022
@chohmann chohmann mentioned this pull request Aug 11, 2022
@chohmann
Copy link
Contributor

This has been released in version 4.10.2 🎉

@childish-sambino
Copy link

childish-sambino commented Aug 24, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants