[go] net/http: add Server.MaxHeaderValueCount setting
Nicholas Husin (Gerrit)
Message from Nicholas Husin![]( "Open in Gerrit")
Set Ready For Review
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 3 comments![]( "Open in Gerrit")
MaxHeaderValueCount int0-means-default means the Transport will pick up the default of 500 as well, which probably isn't intended.
var regularHeaderCount intI think count pseudo-headers as regular headers?
In the current implementation, we parse pseudos into the Headers map just like any other header and don't validate until after all the headers have been read, so they're a vector for sending many headers.
Plus, pseudo-headers map onto regular HTTP/1 headers, so treating the same for accounting makes sense.
func TestRequestHeaderValueCountLimit(t *testing.T) {Include tests for 1xx headers and trailers as well?
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin added 3 comments![]( "Open in Gerrit")
0-means-default means the Transport will pick up the default of 500 as well, which probably isn't intended.
Oops, indeed. Changed 0 to be no limit, and made the child CL where `Transport.MaxResponseHeaderValueCount` is added revert this back to 500 (technically 0 value should never reach this point, but might as well for consistency and to be defensive).
I think count pseudo-headers as regular headers?
In the current implementation, we parse pseudos into the Headers map just like any other header and don't validate until after all the headers have been read, so they're a vector for sending many headers.
Plus, pseudo-headers map onto regular HTTP/1 headers, so treating the same for accounting makes sense.
Good catch, thanks! Changed to count pseudo-headers too.
func TestRequestHeaderValueCountLimit(t *testing.T) {Include tests for 1xx headers and trailers as well?
I don't think 1xx headers are relevant here right? This CL is only for the client sending headers to the server (I do have the 1xx headers test in the child CL where the server sends headers to the client).
Added test for trailers. Note that the header values count limit is only applied to the trailer headers in HTTP/2 currently in this CL. Since [`MaxHeaderBytes` is not applied to trailers in HTTP/1](https://cs.opensource.google/go/go/+/master:src/net/http/transfer.go;l=932-942;drc=cabdf7fdd04d48d283de41e93dcb36d0da21fb10), I figured I should follow the same pattern (plus, since this is a freeze exception, keeping the diff minimal is probably a good idea?). Let me know if you think otherwise.
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Damien Neil
| Code-Review | +2 |
1 comment
func TestRequestHeaderValueCountLimit(t *testing.T) {Nicholas HusinInclude tests for 1xx headers and trailers as well?
I don't think 1xx headers are relevant here right? This CL is only for the client sending headers to the server (I do have the 1xx headers test in the child CL where the server sends headers to the client).
Added test for trailers. Note that the header values count limit is only applied to the trailer headers in HTTP/2 currently in this CL. Since [`MaxHeaderBytes` is not applied to trailers in HTTP/1](https://cs.opensource.google/go/go/+/master:src/net/http/transfer.go;l=932-942;drc=cabdf7fdd04d48d283de41e93dcb36d0da21fb10), I figured I should follow the same pattern (plus, since this is a freeze exception, keeping the diff minimal is probably a good idea?). Let me know if you think otherwise.
Oh, sorry, right. 1xx doesn't make any sense here of course.
Let's go with the current CL to start with but have a followup that applies the limit to HTTP/1 trailers as well. I don't really want to put in a fix that still allows however many trailers you can pack into 4096 bytes, even after the user set MaxHeaderBytes=16 or something.
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |