feat(server): configurable body limit + group-404 guard hardening for echo v5.3.0 - #711
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughEcho is upgraded to v5.3.0. Routing metadata, 405 error mapping, JSON binding, and request body limits are updated, with configuration validation, middleware tests, migration guidance, and observability documentation. ChangesEcho routing and binding behavior
Configurable request body limit
Upgrade and documentation guidance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant EchoRouter
participant HandlerContext
participant ErrorEnvelope
Client->>EchoRouter: request unmatched path or method
EchoRouter->>HandlerContext: classify route metadata and parameters
HandlerContext-->>EchoRouter: empty or preserved route context
EchoRouter->>ErrorEnvelope: map 405 to METHOD_NOT_ALLOWED
ErrorEnvelope-->>Client: return JSON error envelope
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wiki/migrations.md`:
- Line 608: Update the migration detection command to search all tracked
configuration sources for SERVER_BODYLIMIT, including nested paths and both
.yaml and .yml files, while retaining the existing bodylimit pattern matching.
Ensure the no-match gate is based on the complete tracked configuration set
rather than only root-level config*.yaml files.
In `@wiki/observability.md`:
- Line 13: The documentation in “Group-Scoped 404 Route Labels (echo v5.3.0)”
incorrectly implies wrong-method spans are always named GET. Update the
span-name example to use a method placeholder, or explicitly state GET is only
an example, while preserving the route-label and metric guidance.
In `@wiki/startup_defaults.md`:
- Around line 38-40: Update the “Server Request Body Limit” documentation to
distinguish known Content-Length requests, which are rejected before the
handler, from chunked or unknown-length requests, which may exceed the cap while
being read by the handler. Preserve the 413 behavior and server.bodylimit
configuration details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a7ae1c23-36ea-490c-a8af-45a5674f72f6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
config.example.yamlconfig/config.goconfig/config_test.goconfig/types.goconfig/validation.goconfig/validation_test.gogo.modrenovate.jsonserver/constants.goserver/handler.goserver/handler_test.goserver/middleware.goserver/middleware_test.goserver/server.goserver/server_test.gowiki/migrations.mdwiki/observability.mdwiki/startup_defaults.md
4272af6 to
1469675
Compare
… echo v5.3.0 Bumps github.com/labstack/echo/v5 v5.2.1 -> v5.3.0 (supersedes #692) and its OTel companion echo-opentelemetry v0.0.2 -> v0.0.3, then hardens go-bricks against v5.3.0's behavior changes: - echo v5.3.0 restored v4 behavior where a middleware-bearing group auto-registers an implicit "/*" RouteNotFound catch-all. go-bricks KEEPS the new default (the /_sys CIDR gate and debug auth gate now cover unmatched sub-paths — a defense-in-depth win) and hardens HandlerContext.PathParams() and RouteTemplate() to still report "unmatched" for the catch-all: the guard now keys on RouteInfo().Method == echo.RouteNotFound (empty Name), not the Name sentinel alone. - server.bodylimit (int64 bytes, default 10 MB, env SERVER_BODYLIMIT) makes the request body cap configurable; a non-positive value falls back to the default so it can't silently disable the limit. - statusToErrorCode now maps 405 -> METHOD_NOT_ALLOWED (was INTERNAL_ERROR). - Renovate packageRule groups echo/v5 with echo-opentelemetry so the engine and its instrumentation always update together. - Docs: migrations.md E51 hop (adopt-only), observability.md route-label note, startup_defaults.md body-limit section. No exported go-bricks signature changes; JSON bind is now stricter (trailing bytes after the top-level value are rejected) via echo's pooled Deserialize. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1469675 to
83f2449
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wiki/startup_defaults.md (1)
40-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQualify the request-body limit enforcement timing.
Known
Content-Lengthrequests above the cap can be rejected before handler execution, but chunked or unknown-length bodies may exceed the cap while the handler reads them. Update both descriptions to reflect this distinction.
wiki/startup_defaults.md#L40-L40: qualify the 413/pre-handler statement.wiki/migrations.md#L638-L638: qualify the default and configured-limit verification guidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wiki/startup_defaults.md` at line 40, Qualify the request-body limit documentation in wiki/startup_defaults.md at line 40 to state that known Content-Length requests over the cap may be rejected with 413 before handler execution, while chunked or unknown-length bodies can exceed the limit as the handler reads them. Apply the same distinction to the default and configured-limit verification guidance in wiki/migrations.md at line 638.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/config_test.go`:
- Line 54: Update clearEnvironmentVariables in config/config_test.go to unset
SERVER_BODYLIMIT before the test calls Load(), ensuring the default
cfg.Server.BodyLimit assertion is isolated from the external environment.
In `@wiki/migrations.md`:
- Around line 627-630: Update the C51.2 migration note to specify that only
trailing non-whitespace content after the top-level JSON value is rejected,
while trailing whitespace remains accepted; retain examples such as concatenated
JSON values or stray bytes.
---
Duplicate comments:
In `@wiki/startup_defaults.md`:
- Line 40: Qualify the request-body limit documentation in
wiki/startup_defaults.md at line 40 to state that known Content-Length requests
over the cap may be rejected with 413 before handler execution, while chunked or
unknown-length bodies can exceed the limit as the handler reads them. Apply the
same distinction to the default and configured-limit verification guidance in
wiki/migrations.md at line 638.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 174320ba-5e1a-4852-a85c-83ad60412973
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
config.example.yamlconfig/config.goconfig/config_test.goconfig/types.goconfig/validation.goconfig/validation_test.gogo.modrenovate.jsonserver/constants.goserver/handler.goserver/handler_test.goserver/middleware.goserver/middleware_test.goserver/server.goserver/server_test.gowiki/migrations.mdwiki/observability.mdwiki/startup_defaults.md
- migrations.md C51.3 detect: use `git grep` to scan all tracked config sources (nested paths, .yml, env), not just root config*.yaml. - migrations.md C51.2 (gist/gate/verify): narrow "trailing bytes" to trailing NON-whitespace — trailing whitespace still binds. - observability.md: span name keeps the request method (<METHOD> placeholder), not always GET (a wrong-method request is POST /<prefix>/*). - startup_defaults.md: distinguish known-Content-Length rejection (before the handler) from chunked/unknown-length (limited reader trips during the read). - config_test.go: clearEnvironmentVariables now unsets SERVER_BODYLIMIT so the default assertion is isolated from the ambient environment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
Supersedes #692 (the Renovate
echo/v5v5.2.1 → v5.3.0 bump). Multi-lens analysis showed that bump is behavior-affecting, not a routine version bump — the existing test suite is green but structurally blind to what v5.3.0 changes. This PR carries the bump (plus its OTel companionecho-opentelemetryv0.0.2 → v0.0.3) and the hardening + tests + docs that make the behavior changes safe, reviewed, and pinned.What v5.3.0 changes (and how this PR handles it)
1. Group implicit-404 revert (the headline). echo restored v4 behavior: a middleware-bearing group auto-registers an implicit
/*catch-all. Verified reachable in go-bricks via the scheduler/_sysCIDR gate and the debug auth group. Two effects:NoGroupAutoRegister404Routes.RouteInfo().Name == ""(not the sentinel) withMethod == echo.RouteNotFound, which defeatedHandlerContext.PathParams()/RouteTemplate()'s name-only guard (would surface a phantom*param //group/*template). The guard now keys onMethod == echo.RouteNotFound. A wrong-method request under such a group now returns 404, not 405 (the catch-all shadows echo's automatic 405); top-level routes are unaffected.2. Stricter JSON bind. echo's
Deserializeswitched from a streamingjson.Decodertojson.Unmarshalover a pooled buffer (a small per-bind allocation win). Trailing non-whitespace bytes after the top-level JSON value are now rejected with 400 (v5.2.1 silently accepted them); trailing whitespace still binds. Both sides pinned by a test.3. New
server.bodylimitconfig (int64bytes, default 10 MB, envSERVER_BODYLIMIT) makes the request body cap configurable. A negative value is rejected at config validation (mirroringserver.gzip.minlength); a<=0value resolves to the 10 MB default at wire-up (defense-in-depth for directSetupMiddlewarescallers) — the cap can never be silently disabled.Also:
statusToErrorCodenow maps 405 →METHOD_NOT_ALLOWED(wasINTERNAL_ERROR); a RenovatepackageRulegroupsecho/v5withecho-opentelemetryso the engine and its instrumentation always update together.Tests & docs
New/expanded tests: group + nested-group catch-all guard (with mutation-checked assertions), global-404/405
RouteTemplateasymmetry, JSON trailing-content boundary, body-limit enforcement + non-positive fallback (0 and negative), 405→METHOD_NOT_ALLOWEDend-to-end, and config default/validation. Docs:migrations.mdE51 hop (adopt-only),observability.md(group-404http.routelabel shift),startup_defaults.md(server.bodylimit).No exported go-bricks signature changes; additive/adopt-only for consumers.
Note on versioning
The
migrations.mdE51 hop assumes the release cuts as v0.51.0 (this carries observable behavior changes → minor bump). If release-please lands a different version, the hop's version labels need a one-line reconciliation.Verification
make checkgreen (fmt + lint/gosec +-racetests + alloc guards + govulncheck 0 vulns). Pre-push gates run in order:/simplify→/security-audit→/code-review(CodeRabbit, converged to 0 findings). The design and the guard-fix predicate were established by direct empirical probing of echo v5.3.0, and the implementation was adversarially challenged (mutation checks confirm each fix's test fails without it; a completeness sweep confirmed no sibling site shares the guard's blind spot).🤖 Generated with Claude Code
Summary by CodeRabbit
server.bodylimitto configure the maximum request body size (default 10 MB);0uses the default, and negative values are rejected.METHOD_NOT_ALLOWED.