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(server): skip automatic Content-Length headers when not allowed #2216

Merged
merged 4 commits into from
Mar 5, 2021

Conversation

psmit
Copy link
Contributor

@psmit psmit commented Jun 3, 2020

Closes #2215

@psmit
Copy link
Contributor Author

psmit commented Jun 3, 2020

Note that some old tests fail at this moment. I'll fix those after getting feedback whether this is the right way to fix this issue.

@seanmonstar
Copy link
Member

It's probably somewhere in that code, for sure! I'd suggest starting by modifying this test case to check there is no content-length header, which should fail without your changes.

@psmit psmit force-pushed the content_length_fixes branch 2 times, most recently from f7ceff8 to 4570076 Compare June 3, 2020 18:11
@psmit
Copy link
Contributor Author

psmit commented Jun 3, 2020

Thanks for the hint. I adjusted 2 test cases and now the PR is ready for review.

tests/server.rs Outdated Show resolved Hide resolved
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep the existing HEAD behavior.

@psmit
Copy link
Contributor Author

psmit commented Jul 29, 2020

@seanmonstar I have modified the code accordingly. Could you please review it again?

src/proto/h1/role.rs Outdated Show resolved Hide resolved
@krallin
Copy link

krallin commented Jan 25, 2021

@psmit just curious: are you still interested in driving this? We're affected by this issue as well and would love to see the fix go in. Thanks! :)

krallin added a commit to krallin/hyper that referenced this pull request Jan 25, 2021
krallin added a commit to krallin/hyper that referenced this pull request Feb 11, 2021
@psmit
Copy link
Contributor Author

psmit commented Feb 15, 2021

Hi @krallin, my apologies, I had not realized that this PR wasn't merged yet and your reminder got lost in my mail box. I'll be working now to get this PR finished.

@krallin
Copy link

krallin commented Mar 3, 2021

Is anything blocking this PR?

krallin added a commit to krallin/hyper that referenced this pull request Mar 3, 2021
@psmit
Copy link
Contributor Author

psmit commented Mar 4, 2021

Is anything blocking this PR?

Not to my knowledge, we just have to wait for the review to be done.

@seanmonstar
Copy link
Member

Sorry, I got distracted. This looks good to me, let's go! Thanks for sticking with it <3

@seanmonstar seanmonstar merged commit 8cbf952 into hyperium:master Mar 5, 2021
@krallin
Copy link

krallin commented Mar 25, 2021

@seanmonstar any plans to cut a new release? We'd love to stop having to use a fork of Hyper just to get this :)

@seanmonstar
Copy link
Member

I was hoping to get #2473 merged today or tomorrow, and then do a release. If it gets delayed, I'll release anyways. So either way, there should be a 0.14.5 today or tomorrow.

@krallin
Copy link

krallin commented Mar 25, 2021 via email

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.

Hyper should skip automatic Content-Length header for HTTP 1xx responses
3 participants