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

fastcgi: check for CONTENT_LENGTH when sending requests #6661

Merged
merged 4 commits into from
Dec 18, 2024

Conversation

WeidiDeng
Copy link
Member

@WeidiDeng WeidiDeng commented Oct 24, 2024

For #6637, I think previous pr is a bit too ambitious. Requests buffering is hard to do right without considering a multitude of factors. This patch only requires CONTENT_LENGTH to be set for fastcgi requests.

@francislavoie
Copy link
Member

So we just throw 411 for any chunked requests? Makes sense that it'll prevent hanging. But long term, shouldn't we have support for streaming chunked requests?

@WeidiDeng
Copy link
Member Author

php-fpm doesn't support streaming chunked requests. That's the thing. Golang fastcgi supports streaming chunked requests, but does anyone use that with caddy?

@francislavoie
Copy link
Member

https://bz.apache.org/bugzilla/show_bug.cgi?id=57087 looks like Apache fixed their fpm support with buffering

@lutoma
Copy link

lutoma commented Nov 14, 2024

Are there any updates on this (or the other patch for the issue)? Seeing these PRs just lying around for a month is pretty discouraging when this is essentially a DoS that seems to affect a lot of people

@mohammed90
Copy link
Member

Are there any updates on this (or the other patch for the issue)? Seeing these PRs just lying around for a month is pretty discouraging when this is essentially a DoS that seems to affect a lot of people

The patch only been up for 3 weeks, and it's under review. We've been releasing 2.9.0 betas. You can build Caddy with this patch using xcaddy.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks Weidi. I like the more conservative approach. Maybe in the future we can figure out how to fix it with buffering to disk, but for now, this will do. :) Really appreciate it!

@mholt mholt added this to the v2.9.0 milestone Dec 10, 2024
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
@mholt mholt enabled auto-merge (squash) December 18, 2024 00:13
@mholt mholt merged commit 6790c0e into master Dec 18, 2024
33 checks passed
@mholt mholt deleted the fastcgi-cl-header branch December 18, 2024 00:22
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.

6 participants