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

[bug] cannot deny transfer-encoding requests #6628

Open
shyim opened this issue Oct 15, 2024 · 8 comments · May be fixed by #6629
Open

[bug] cannot deny transfer-encoding requests #6628

shyim opened this issue Oct 15, 2024 · 8 comments · May be fixed by #6629

Comments

@shyim
Copy link
Contributor

shyim commented Oct 15, 2024

Hey,

I am trying to block Transfer-Encoding: chunked requests. But this seems not possible right now in Caddy:

I tried following:

@length {
   header Transfer-Encoding chunked
}

respond @length 411

I think the header Transfer-Encoding is missing in the available headers: as the logs does not mention it too:

{"level":"info","ts":1729001178.8945065,"logger":"http.log.access","msg":"NOP","request":{"remote_ip":"192.168.117.1","remote_port":"49353","client_ip":"192.168.117.1","proto":"HTTP/1.1","method":"POST","host":"127.0.0.1:8000","uri":"/","headers":{"Content-Type":["application/json"],"User-Agent":["curl/8.7.1"],"Accept":["*/*"]}},"bytes_read":0,"user_id":"","duration":0.000008292,"size":0,"status":0,"resp_headers":{"Server":["Caddy"]}}

When i change the header to X-Foo everything works

@francislavoie
Copy link
Member

Interestingly, Go doesn't keep Transfer-Encoding in the headers map, it moves it to a separate field on the request: https://pkg.go.dev/net/http#Request.TransferEncoding

We have special handling for the header matcher for Host which is also moved separately, we could probably do the same for TransferEncoding. Would make sense to allow matching it I think.

@steffenbusch
Copy link
Contributor

steffenbusch commented Oct 15, 2024

@francislavoie Wouldn't it make sense to add the special handling for the Transfer-Encoding header also to the json logging area to have Transfer-Encoding be logged in request>headers>Transfer-Encoding[]?

@francislavoie
Copy link
Member

Hm, maybe yeah. Lemme take a look.

@shyim
Copy link
Contributor Author

shyim commented Oct 15, 2024

It should be here right?

func getHeaderFieldVals(input http.Header, fieldName, host string) []string {

@francislavoie
Copy link
Member

@shyim did you see #6629?

@steffenbusch It's tricky actually. I think right now we're logging with no copying of headers, but if we try to manipulate the request header list to log it then we do need to copy it to avoid the inserted header field from affecting anything else outside of logging. I think I can add it as a new field beside headers though.

@WeidiDeng
Copy link
Member

@shyim Simply matching by transfer-encoding isn't enough, http2/3 doesn't have this header. The problem here is that the content-length is unknown in advance, the php-fpm needs this info to proceed.

I'll retry to figure out how to buffer requests in this case. Might take a few day.

@dallyger
Copy link

@WeidiDeng If I understand correctly, php-fpm will read the content-length header. But if Transfer-Encoding: chunked is used, that header is not given by the client. Instead each chunk will be prefixed with the content length.

php-fpm hangs if there is no content-length header. This header is only missing if transfer chunked is used. Therefore, it should be enough to match transfer-encoding. Anything else should work as expected. Am I missing something?

Or would it be a better idea to match against a missing content-length header?

@WeidiDeng
Copy link
Member

@dallyger Yes, it's better to match against the missing content-length header.

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 a pull request may close this issue.

5 participants