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.

Sign up for GitHub

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

Jump to bottom

[bug] cannot deny transfer-encoding requests #6628

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

[bug] cannot deny transfer-encoding requests #6628

shyim opened this issue Oct 15, 2024 · 6 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
@shyim shyim mentioned this issue Oct 15, 2024
Open
@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.

@francislavoie francislavoie linked a pull request Oct 15, 2024 that will close this issue
Open
@steffenbusch
Copy link
Contributor

steffenbusch commented Oct 15, 2024 •
edited by francislavoie
Loading

@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?

caddy/modules/caddyhttp/matchers.go

Line 972 in a211c65

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

caddyhttp: Allow matching Transfer-Encoding caddyserver/caddy
4 participants
@francislavoie @shyim @WeidiDeng @steffenbusch