Skip to content

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

Merged
mholt merged 5 commits intomasterfrom
unbuffer-chunked
Feb 12, 2023
Merged

reverseproxy: Don't buffer chunked requests (fix #5366)#5367
mholt merged 5 commits intomasterfrom
unbuffer-chunked

Conversation

@mholt
Copy link
Copy Markdown
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.

@mholt
reverseproxy: Don't buffer chunked requests (fix #5366)
4921928 
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 mholt mentioned this pull request Feb 10, 2023
Closed
@mholt
Copy link
Copy Markdown
Member Author

mholt commented Feb 10, 2023 •
edited
Loading

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!

@francislavoie francislavoie mentioned this pull request Feb 10, 2023
Closed
francislavoie
francislavoie approved these changes Feb 10, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

u5surf commented Feb 11, 2023 •
edited
Loading

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

u5surf
u5surf reviewed Feb 11, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread modules/caddyhttp/reverseproxy/caddyfile.go Outdated
Comment thread modules/caddyhttp/reverseproxy/reverseproxy.go Outdated
Comment thread modules/caddyhttp/reverseproxy/reverseproxy.go Outdated
@mholt
Copy link
Copy Markdown
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
@Jimbolino Jimbolino mentioned this pull request Mar 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@francislavoie francislavoie francislavoie approved these changes

+1 more reviewer

@u5surf u5surf u5surf left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug 🐞 Something isn't working

Projects

None yet

Milestone

v2.6.4

Development

Successfully merging this pull request may close these issues.

3 participants

@mholt @u5surf @francislavoie