fix(middleware): reset ContentLength after gzip decompression#3000
fix(middleware): reset ContentLength after gzip decompression#3000shblue21 wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3000 +/- ##
=======================================
Coverage 93.17% 93.18%
=======================================
Files 43 43
Lines 4501 4504 +3
=======================================
+ Hits 4194 4197 +3
Misses 189 189
Partials 118 118 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3fe50ad to
0a25d94
Compare
| req.Header.Del(echo.HeaderContentEncoding) | ||
| req.Header.Del(echo.HeaderContentLength) | ||
| req.ContentLength = -1 | ||
| req.GetBody = nil |
There was a problem hiding this comment.
GetBody doc comments says that For server requests, it is unused.
|
The description is little bit cryptic on first clance. It would be better if the situation/use-case would be explained in a "caveman manner" I am using Decompress middleware along with the body limit middleware. This snippet func main() {
e := echo.New()
e.Use(middleware.Decompress())
e.Use(middleware.BodyLimit(4))
e.POST("/", func(c *echo.Context) error {
body, readErr := io.ReadAll(c.Request().Body)
if readErr != nil {
return readErr
}
return c.String(http.StatusOK, string(body))
})
if err := e.Start(":8080"); err != nil {
slog.Error("Failed to start server", "error", err)
}
}does not work because the Decompress middleware leaves At the moment and tested also with printf "ok" | gzip | curl -X POST -H "Content-Type: text/plain" -H "Content-Encoding: gzip" --data-binary @- http://localhost:8080/Currently the result is incorrect: Expected would be: |
|
just nitpicking here as it is not easy sometimes to understand what the actual use-case is, and fixing (accepting PRs) something that you do not understand can break things on some other place. |
| if c.Request().Header.Get(echo.HeaderContentEncoding) != GZIPEncoding { | ||
| req := c.Request() | ||
| contentEncoding := req.Header.Values(echo.HeaderContentEncoding) | ||
| if len(contentEncoding) != 1 || strings.TrimSpace(contentEncoding[0]) != GZIPEncoding { |
There was a problem hiding this comment.
is this change necessary? It does not help if the request has been compressed multiple times. For example:
Content-Encoding: gzip, br
or
Content-Encoding: gzip
Content-Encoding: br
meaning that decompression is in reverse order:
- decode
br - decode
gzip - obtain the original content
| c.Request().Body = gr | ||
| req.Body = gr | ||
| } | ||
| req.Header.Del(echo.HeaderContentEncoding) |
There was a problem hiding this comment.
same as above - if are to fix headers then the plain "deletion" is wrong approach as there could be multiple Content-Encoding values and we are deleting all of them now - not only gzip part.
Is this complexity necessary and valuable for any realworld use-case?
|
Please not to take this as me being mean. I am trying keep things simple and avoid unessesary complexity if possible. |
0a25d94 to
30b16e7
Compare
30b16e7 to
918b1a5
Compare
Summary
Fix
Decompressso middleware after it does not keep using the compressed request size after the body has been replaced with the decoded gzip stream.For example:
A gzipped request whose decoded body is
okshould pass:Before this change,
Decompressreplacedreq.Bodybut leftreq.ContentLengthset to the compressed size, soBodyLimitcould reject the request before reading it:With this change,
Decompresssetsreq.ContentLength = -1after gzip decompression is set up, so downstream middleware enforces limits while reading the decoded body:This intentionally does not change
Content-Encoding,GetBody, or multiple content-coding behavior.Test plan
go test -count=1 ./...go test -race -count=1 ./...go vet ./...staticcheck ./...okand HTTP 200