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

reverseproxy: Don't buffer chunked requests (fix #5366) #5367

Merged
merged 5 commits into from
Feb 12, 2023
Merged

Conversation

mholt
Copy link
Member

@mholt mholt commented Feb 10, 2023

Mostly reverts 845bc4d (#5289) -- that was my bad, totally an oversight on my part of the code review.

Adds warning for unsafe config. The buffer_requests and buffer_responses config params, together with a separate max_buffer_size parameter, were not safely designed: very prone to configuring unbounded buffers. I'm not sure why I ever thought that was fine.

Deprecates unsafe properties in favor of simpler, safer designed ones.

We'll need a better fix for #5236 -- even if it's the same kind of fix (buffering, and resetting the Content-Length) as long as it's opt-in this time.

/cc @u5surf What do you think?

I think this patch merits a quick release of v2.6.4.

Mostly reverts 845bc4d (#5289)

Adds warning for unsafe config.

Deprecates unsafe properties in favor of simpler, safer designed ones.
@mholt mholt added the bug 🐞 Something isn't working label Feb 10, 2023
@mholt mholt added this to the v2.6.4 milestone Feb 10, 2023
@mholt
Copy link
Member Author

mholt commented Feb 10, 2023

I could remove isChunkedRequest() to satisfy the linter, but I left it in there because I want to use it to apply @u5surf's earlier patch, just in a more careful/opt-in way. I am just not sure how to go about that. Would love any ideas/insights!

Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Seems good to me.

I think we can just deleted the chunked encoding code. If users who that fix helped find that they need buffering, they can enable it for themselves (and now, set limits).

@u5surf
Copy link
Contributor

u5surf commented Feb 11, 2023

@mholt
Sorry for late.
I also agree with not to buffering such a large chunked contents. I lost my sight at this point because I misunderstood that it has already taken this problem in h.bufferedBody or somewhere.
Your patch looks good to me. Thank
you.

@mholt
Copy link
Member Author

mholt commented Feb 11, 2023

@u5surf No worries, thank you for checking it out! And thank you for the original patch. The mistake was my oversight. Hope you'll contribute again.

Copy link
Contributor

@u5surf u5surf left a comment

Choose a reason for hiding this comment

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

@mholt
I have left a few comment, I still might have confused for me the new configurations.

modules/caddyhttp/reverseproxy/caddyfile.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/reverseproxy.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/reverseproxy.go Outdated Show resolved Hide resolved
@mholt
Copy link
Member Author

mholt commented Feb 11, 2023

@u5surf Ah wow, thank you for your review -- I had a lot going on yesterday and looks like I got distracted during a search-replace and mixed them up 🙈

Thank you for catching those errors! I've pushed what I hope is the final change for this patch.

@mholt mholt merged commit 4b119a4 into master Feb 12, 2023
@mholt mholt deleted the unbuffer-chunked branch February 12, 2023 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants