Skip to content

Conversation

@ItalyPaleAle
Copy link
Contributor

@ItalyPaleAle ItalyPaleAle commented Apr 26, 2025

Re-do of #3841 which had ended up in a bad state

The Go standard library includes a method http.MaxBytesReader that allows limiting the request body. For example, users can create a middleware like:

func MiddlewareMaxBodySize(c *gin.Context) {
	// Limit request body to 100 bytes
	c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, 100)
	c.Next()
}

When the body exceeds the limit, reading from the request body returns an error of type http.MaxBytesError.

This PR makes sure that when the error is of kind http.MaxBytesError, Gin returns the correct status code 413 (Request Entity Too Large) instead of a generic 400 (Bad Request).

@ItalyPaleAle ItalyPaleAle changed the title The Go standard library includes a method http.MaxBytesReader that allows limiting the request body. For example, users can create a middleware like: Bind: return 413 status code when error is http.MaxBytesError Apr 26, 2025
The Go standard library includes a method `http.MaxBytesReader` that allows limiting the request body. For example, users can create a middleware like:

```go
func MiddlewareMaxBodySize(c *gin.Context) {
	// Limit request body to 100 bytes
	c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, 100)
	c.Next()
}
```

When the body exceeds the limit, reading from the request body returns an error of type `http.MaxBytesError`.

This PR makes sure that when the error is of kind `http.MaxBytesError`, Gin returns the correct status code 413 (Request Entity Too Large) instead of a generic 400 (Bad Request).
@ItalyPaleAle ItalyPaleAle force-pushed the bind-request-entity-too-large-2 branch from 5dcdb08 to 2efd359 Compare April 26, 2025 05:26
@codecov
Copy link

codecov bot commented Apr 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.92%. Comparing base (3dc1cd6) to head (4de0ae4).
Report is 123 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4227      +/-   ##
==========================================
- Coverage   99.21%   98.92%   -0.30%     
==========================================
  Files          42       44       +2     
  Lines        3182     3432     +250     
==========================================
+ Hits         3157     3395     +238     
- Misses         17       26       +9     
- Partials        8       11       +3     
Flag Coverage Δ
?
--ldflags="-checklinkname=0" -tags sonic 98.85% <83.33%> (?)
-tags go_json 98.85% <83.33%> (?)
-tags nomsgpack 98.90% <100.00%> (?)
go-1.18 ?
go-1.19 ?
go-1.20 ?
go-1.21 ?
go-1.23 98.92% <100.00%> (?)
go-1.24 98.92% <100.00%> (?)
macos-latest 98.92% <100.00%> (-0.30%) ⬇️
ubuntu-latest 98.92% <100.00%> (-0.30%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ItalyPaleAle
Copy link
Contributor Author

The test is failing when using "sonic avx" as tags, but I can't repro the error locally... Any advice?

@appleboy
Copy link
Member

Please rebase the master branch

@appleboy appleboy requested a review from Copilot May 20, 2025 10:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that when a binding error is due to http.MaxBytesError, Gin responds with HTTP 413 instead of 400.
Key changes:

  • Update MustBindWith in context.go to detect http.MaxBytesError and return 413
  • Add TestContextBindRequestTooLarge and adjust related tests in context_test.go
  • Simplify URL concatenation in gin_test.go

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
context.go Detect http.MaxBytesError in MustBindWith and set 413
context_test.go Replace buffer usage, add test for oversized request binding
gin_test.go Simplify HTTP GET calls by string concatenation

@ItalyPaleAle
Copy link
Contributor Author

Please rebase the master branch

Done, thanks

@appleboy appleboy requested a review from Copilot May 21, 2025 00:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR changes Gin’s binding behavior to return a 413 (Request Entity Too Large) status code when the error from reading the request body is of type http.MaxBytesError. In addition, it updates several tests to use strings.NewReader for constructing request bodies and adds a new test (TestContextBindRequestTooLarge) to verify the new behavior.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
context_test.go Updates to use strings.NewReader for request bodies and addition of a new test for 413.
context.go Updates MustBindWith to handle http.MaxBytesError, returning HTTP 413 instead of 400.

@appleboy appleboy changed the title Bind: return 413 status code when error is http.MaxBytesError chore(bind): return 413 status code when error is http.MaxBytesError May 21, 2025
@appleboy appleboy added this to the v1.11 milestone May 21, 2025
@appleboy
Copy link
Member

@ItalyPaleAle testing error. Please help to take a look.

@ItalyPaleAle
Copy link
Contributor Author

@ItalyPaleAle testing error. Please help to take a look.

I cannot repro locally. Looks like tests pass on other platforms, but fail when the sonic tag is used. Maybe this is caused by an incompatibility with the sonic library?

@appleboy
Copy link
Member

image

@ItalyPaleAle
Copy link
Contributor Author

image

But the test passes when NOT using sonic: https://github.com/gin-gonic/gin/actions/runs/15143597508/job/42624844475?pr=4227 must be a bug in the sonic library

I can disable this test when sonic is used?

@ItalyPaleAle ItalyPaleAle force-pushed the bind-request-entity-too-large-2 branch from ecbeb4a to 666545f Compare May 23, 2025 14:37
@ItalyPaleAle
Copy link
Contributor Author

This feature won't work when using go-json or sonic due to bugs upstream:

goccy/go-json#485
bytedance/sonic#800

In those cases, gin will continue to return a 400 response. Tests should pass now

@appleboy appleboy merged commit 40725d8 into gin-gonic:master May 25, 2025
24 of 25 checks passed
@appleboy
Copy link
Member

@ItalyPaleAle Thanks for your effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants