fix(gateway): derive the HTTP body limit and reject oversize with 413 - #131
Conversation
Ship quarkus.http.limits.max-body-size=64M so a declared per-route max_body_bytes is reachable instead of being silently clipped by the Vert.x 10 MiB default, and add a fail-closed boot check in ConfigProducer that aborts when any declared cap exceeds the framework limit. Both body-cap raise sites now throw the new CONTENT_TOO_LARGE (413) event instead of PARAMETER_LIMIT_EXCEEDED (400); enforcement predicates, thresholds and abort paths are unchanged. GrpcStatusMapper gains RESOURCE_EXHAUSTED (8) with the 413 arm and its class-javadoc row, so a gRPC body-cap breach no longer degrades to UNKNOWN. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
topology.properties declared UPSTREAM as a bare literal, and EnvSecretResolver substitutes only explicit in-file placeholders — there is no TOPOLOGY_<ALIAS> precedence path — so the benchmark overlay's TOPOLOGY_UPSTREAM bound to nothing and every proxy aspect silently measured go-httpbin instead of nginx-static.
Declare UPSTREAM as ${TOPOLOGY_UPSTREAM:-http://go-httpbin:8080/anything}; the default keeps the IT stack on go-httpbin so the proxy ITs still observe its JSON echo. Two comments that asserted the non-existent mechanism are corrected.
Add an http-context nginx fragment for the static benchmark backend with lingering headroom sized for the 50 MB uploadLarge aspect, so the backend drains a large body rather than resetting it mid-transfer. Verified: nginx -t clean, and a 50 MB POST to location / returns 200 with the full body accepted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
Add BodyLimitActivationWiringTest, a fast no-Docker surefire guard that the committed descriptors activate the framework body floor — the unit-green / integration-red blind spot lesson 2026-07-25-15-001 names. It asserts application.properties declares quarkus.http.limits.max-body-size, that the floor covers the largest declared max_body_bytes in every committed sheriff-config*/gateway.yaml (glob-discovered, so a new instance directory is covered automatically and an empty glob cannot pass vacuously), and that the IT container override exceeds LargeBodyIT's negative-case body size. The third assertion asserts the QUARKUS_HTTP_LIMITS_MAX_BODY_SIZE override that the next deliverable adds; test-compile is green and the surefire run is covered at that deliverable's chain tail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
There was a problem hiding this comment.
Sorry @cuioss-oliver, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 2 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: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change standardizes oversized request handling on HTTP 413, validates framework body limits during startup, adds large-body integration wiring and coverage, and enables manifest-driven benchmark reporting with ChangesBody-size enforcement and error contract
Large-body integration and benchmark coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Reviewer Guide 🔍
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java (1)
73-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPrevent the error-contract table from drifting from
EventType.The
@CsvSourcenames a manually maintained subset of constants fromapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java. A future request-time failure added there can be omitted from this table while the suite still passes; adding the Line [81] row does not make coverage exhaustive. Derive the source or assert that the table’s keys equal everyEventTypewith an HTTP mapping.As per path instructions, hardcoded lists that mirror enum constants must be derived or guarded against drift; the authoritative definition is
EventType.🤖 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 `@api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java` around lines 73 - 90, Update the parameterized test around the `@CsvSource` table to derive its cases from EventType, or add an assertion that the table keys exactly match every EventType constant with an HTTP mapping. Keep the existing status-code and category assertions, but ensure newly added mapped EventType values cannot be omitted without failing the test.Source: Path instructions
🤖 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 @.github/workflows/benchmark.yml:
- Line 148: The benchmark coverage workflow must stop maintaining the hard-coded
expected list and instead consume a build-generated manifest produced from the
resolved Maven benchmark executions, including each benchmark’s enabled or
intentionally skipped state, property, and reason; update the coverage reporting
logic around the expected table and the Maven benchmark configuration that
generates the manifest. In benchmarks/README.adoc lines 199-204, retain the
documented skipped-goal reporting claim only if this manifest-driven behavior is
implemented; otherwise revise the documentation to state that suppressed goals
are not represented in the CI coverage table.
In `@doc/configuration.adoc`:
- Around line 944-949: Update the max_body_bytes documentation to make the
application/problem+json envelope conditional on the request reaching gateway
enforcement. Reconcile the related framework-floor and chunked-request
statements so they no longer promise the gateway envelope when the framework or
transport limit rejects the request first, while preserving the documented 413
behavior and enforcement order.
In
`@integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java`:
- Around line 155-165: Add an assertion in the LargeBodyIT response-validation
block to require the response status to equal HTTP 413 (CONTENT_TOO_LARGE),
while preserving the existing RFC 9457 envelope and upstream-rejection
assertions.
---
Outside diff comments:
In
`@api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java`:
- Around line 73-90: Update the parameterized test around the `@CsvSource` table
to derive its cases from EventType, or add an assertion that the table keys
exactly match every EventType constant with an HTTP mapping. Keep the existing
status-code and category assertions, but ensure newly added mapped EventType
values cannot be omitted without failing the test.
🪄 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: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d7dda21b-d0ea-4a73-b0ea-fb89835ccb9b
📒 Files selected for processing (28)
.github/workflows/benchmark.ymlapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.javaapi-sheriff/src/main/resources/application.propertiesapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.javabenchmarks/README.adocbenchmarks/pom.xmldoc/architecture.adocdoc/configuration.adocintegration-tests/docker-compose.benchmark.ymlintegration-tests/docker-compose.ymlintegration-tests/src/main/docker/nginx/body-limits.confintegration-tests/src/main/docker/sheriff-config/topology.propertiesintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffSessionMediationIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java
JsonPath.get is generic (<T> T get(String)). Inside String.valueOf(...) javac resolves against the most specific applicable overload, String.valueOf(char[]), inferring T = char[]; the JSON value is a String, so the negative case failed at runtime with "class java.lang.String cannot be cast to class [C". The adjacent assertions escaped it only because they infer Object. Switch the type and title reads to JsonPath.getString and hoist the value into a local, so the assertion message and the assertion share one already-typed read. Verified against the containerised stack: LargeBodyIT runs 2/2 green. Co-Authored-By: Claude <noreply@anthropic.com>
Triage dispositionsIn reply to comment_id:
|
…tional The security_filter table row promised an application/problem+json envelope unconditionally for every over-cap request, contradicting the "Shadowing at the boundary" paragraph in the same section: when the framework floor equals the largest declared cap, the framework's Content-Length pre-check answers with a bare 413 before gateway request handling runs. Qualify the envelope guarantee on the request reaching gateway enforcement and cross-reference the shadowing paragraph. The 413 behaviour and the enforcement order are unchanged. Addresses PR #131 review finding f53145. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
…ure drift The @CsvSource error-contract table was a hand-maintained mirror of the EventType constants carrying an HTTP mapping, and it had already drifted: seventeen constants satisfy hasHttpMapping() while the table carried sixteen rows, leaving WEBSOCKET_ORIGIN_REJECTED (AUTHORIZATION, 403) asserted by nothing. WEBSOCKET_IDLE_TIMEOUT was likewise unclaimed by any list, so its 1001 Going Away close code was unasserted. Close both gaps and make omission impossible: * Add the WEBSOCKET_ORIGIN_REJECTED row, and give WEBSOCKET_IDLE_TIMEOUT its own assertion rather than folding it into the success list — it carries a null category and httpStatus 0 but a non-zero wsCloseCode. * Move the four contract lists into shared constants driving @MethodSource, so each list has exactly one declaration. Expected statuses and categories stay stated as literals, never re-read from the enum under test. * Add three drift guards: the error-contract table's key set must equal the hasHttpMapping() set, the WebSocket-close list must equal the non-zero wsCloseCode set, and the four lists must partition the enum with no constant unclaimed or double-claimed. Addresses PR #131 review finding b21e8d. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
Re-enabling uploadLarge took the executing set from eleven to twelve. Two of the three count-prose sites in benchmark.yml were updated in that change; the comment block above the "Summarise benchmark coverage" step was missed and still read "3 of the 11 executing goals" / "all 11" while the expected list below it holds twelve names. Comment-only change; the step body, the expected list and the skip table are untouched. Addresses finding b4790e, found during ground-truth verification of PR #131 review finding f0867e. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
Finding 8525f1. The negative case asserted only the RFC 9457 envelope, on the reasoning that the status is incidental once the gateway hands over a rejection. It is not incidental: it is the mapping this PR changes. A regression that moved the event back to a 400-mapped constant would have left the envelope assertions green. Assert both halves, and say why neither replaces the other: the status locks the CONTENT_TOO_LARGE -> 413 mapping, while the envelope is what discriminates the gateway's rejection from the framework's own Content-Length pre-check, which answers with a bare 413 carrying no envelope. Verified against the containerised stack: LargeBodyIT 2/2 green. Co-Authored-By: Claude <noreply@anthropic.com>
…anifest Finding f0867e. The benchmark coverage step maintained a hard-coded expected goal list plus a hand-maintained skip table, both mirroring the resolved -Pbenchmark execution set in benchmarks/pom.xml with nothing enforcing agreement. The drift was not hypothetical: this PR moved the executing set from eleven to twelve and left prose in the same file saying eleven. Add benchmarks/scripts/benchmark-manifest.py, bound to initialize so the manifest exists before any goal runs and a suite truncated by a later failure still states what was expected. Maven hands over only the RESOLVED value of each skip property, so a -Dskip.benchmark.upload.large=true override is reflected; the generator exits non-zero when an execution's skip property was not handed over, so a new suppressible goal cannot silently report as executing. The workflow now derives both the gated set and the not-expected table from the manifest, and treats an absent manifest as a hard failure rather than an empty expected set that would vacuously report full coverage. The unwired sessionMediated script carries its own @unwiredReason tag, so that reason lives in one place instead of being duplicated into the workflow. Scope note: this exceeds the plan's D6 (re-enable uploadLarge). It was implemented at explicit operator direction after triage recommended deferring it to a follow-up. Verified: manifest generates via Maven initialize with 12 enabled / 1 unwired and uploadLarge enabled; both jq expressions checked against the real manifest; the missing-skip-property guard exits 1. The workflow step itself only runs in a post-merge Performance Benchmark run and is unproven until then. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/benchmark.yml (1)
139-217: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse one authoritative manifest coverage summary implementation.
.github/workflows/benchmark.ymlreimplements the CI coverage step in bash/jq, whilebenchmarks/scripts/benchmark-manifest.pyprovides asummarisesubcommand that is documented as the CI consumer but is not invoked. This creates a second copy of the same contract: the bash output uses### Not expected to run, adds a rawstatecolumn, and treats<skip>true</skip>entries as “not wired”; the Python implementation uses### Deliberately not runwithSkipped by/Reasoncolumns and labels inline skips correctly. Either callbenchmark-manifest.py summarisefrom the workflow, or remove the deadsummarise()CLI/subcommand if bash is intended to remain authoritative.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 11d08dc5-b818-47be-89cd-81ad1db0100a
📒 Files selected for processing (8)
.github/workflows/benchmark.ymlapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.javabenchmarks/README.adocbenchmarks/pom.xmlbenchmarks/scripts/benchmark-manifest.pybenchmarks/src/main/resources/k6-scripts/session_mediated.jsdoc/configuration.adocintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java
🚧 Files skipped from review as they are similar to previous changes (1)
- benchmarks/README.adoc
… in bash Review finding on the previous commit, and it is correct. benchmark-manifest.py already ships a summarise subcommand — its module docstring names it as the CI consumer — but the workflow reimplemented the same rendering in bash/jq. That recreated, one level up, the duplicated-contract defect the manifest was introduced to remove: two implementations of one contract, already diverged on the table heading, and the jq fallback mislabelled an inline <skip>true</skip> entry as "not wired" where the Python names the property. Delete the bash/jq block and call summarise, so the manifest has exactly one consumer. Verified all three exit semantics: complete coverage exits 0; a missing summary document with a successful benchmark step exits 1; an absent manifest exits 1 when the benchmark step succeeded and 0 when it failed, deferring to Maven's own error rather than masking it. Co-Authored-By: Claude <noreply@anthropic.com>
Review finding on EventTypeTest:114. The success, configuration and
WebSocket-close sets were three hand-maintained mirrors of EventType, and the
drift guards added earlier only detect an omission after that second registry has
already drifted. A derived set cannot drift at all.
EventType already exposes the classifying accessors, so no new production API was
needed: hasHttpMapping(), wsCloseCode() and category() partition the enum into the
four buckets the suite asserts, and membership is now selected by predicate over
allOf(EventType.class). A constant added to the enum lands in its bucket
automatically.
What stays literal is the part that carries independent information: ERROR_CONTRACT
still states each mapped event's expected status and category as written-out
values, so the mapping assertions cannot pass by re-reading the enum under test.
Derive the membership, state the mapping.
Two consequential follow-ons rather than a mechanical swap:
* the WebSocket-close case was a single hard-coded constant, so a second
close-code constant would have gone unasserted. It is now parameterized over
the derived set, with the literal 1001 kept as its own assertion since the
parameterized test derives membership from wsCloseCode() and therefore cannot
also be what pins the value.
* the ws-close coverage guard became tautological once its set was derived, and
is removed. The partition guard is retained and re-documented: with membership
derived it no longer guards a forgotten list entry, it guards the PREDICATES —
a constant claimed by none or by two shapes means they have stopped
partitioning the enum.
Verified: derived counts match the previously hand-listed sets exactly (7 success,
2 configuration, 1 WebSocket-close, 18 error-contract), the partition guard passes,
and no bucket derives empty.
Co-Authored-By: Claude <noreply@anthropic.com>
Triage dispositionsIn reply to comment_id:
|
The plan settled a real architectural question rather than fixing a local bug: whether a declared max_body_bytes is the effective ceiling, and which component owns an over-cap rejection. The spec asked for the choice to be stated with its reasoning, and the alternatives were weighed and discarded on their merits, so the decision belongs in an ADR rather than only in a commit message. Records the derivation plus fail-closed boot validation, the 413 rendering on both the proxy and gRPC paths, and — as consequences of the resulting architecture rather than incident notes — the shadowing behaviour at floor == cap, the differing enforcement path for chunked requests, and the invariant that raising the framework floor must never become the only ceiling for a route. Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Derives the framework body-size limit from the configured
security_filteranchors soquarkus.http.limits.max-body-sizeis fail-closed-checked against the largest declaredmax_body_bytes, making the gateway (not Vert.x) the component that rejects an oversizedbody. A breach now surfaces as a
CONTENT_TOO_LARGE(INPUT_VALIDATION, 413)RFC 9457problem response instead of a bare framework 400/413 with no envelope. Adds a containerised
IT proving both the positive (accepted) and negative (gateway-rejected, envelope-discriminated)
cases, reconciles
doc/configuration.adocanddoc/architecture.adoc, fixes theTOPOLOGY_UPSTREAMbenchmark-overlay no-op so the placeholder actually binds, andre-enables the previously-quarantined
uploadLargebenchmark goal now that its defect isfixed and it runs against the intended static backend.
Changes
Body-limit derivation & 413 event contract
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java— fail-closed boot check deriving/validating the framework body limit against configured anchor maximaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java— newCONTENT_TOO_LARGE(INPUT_VALIDATION, 413)eventapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java,api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java— repoint the two body-cap raise sites at the new 413 eventapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java— status-mapping follow-through for the new eventapi-sheriff/src/main/resources/application.properties— derived/operator-set Quarkus limit configurationConfigProducerTest.java,ConfigFailFastTest.java,ThoroughChecksStageTest.java,DispatchStageTest.java,GrpcDispatchStageTest.java,GrpcStatusMapperTest.java,EventTypeTest.javaDocumentation reconciliation
doc/configuration.adoc— themax_body_bytesrow now states the declared cap is the effective onedoc/architecture.adoc— RFC 9457 error-contract table gains the new 413 rowLarge-body regression IT
integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java— positive (above framework default, below declared cap → accepted) and negative (above declared cap → gateway-rejected, envelope-discriminated via content-type + category + nullmethod) casesintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java— activation-wiring coverageintegration-tests/src/main/docker/nginx/body-limits.conf,integration-tests/docker-compose.yml— supporting IT topologyTOPOLOGY_UPSTREAMno-op fix (findingacf3e3)integration-tests/src/main/docker/sheriff-config/topology.properties— placeholder now binds${TOPOLOGY_UPSTREAM}instead of a bare literal, default keeps the IT stack on go-httpbinintegration-tests/docker-compose.benchmark.yml— corrected precedence-mechanism commentsRe-enable
uploadLargebenchmarkbenchmarks/pom.xml—skip.benchmark.upload.largenow defaults tofalse.github/workflows/benchmark.yml— moved out of the skipped table into the coverageexpectedlist, updated count prosebenchmarks/README.adoc— removed stale "skipped" proseUnrelated-surface follow-through swept in under
pr_strategy: rideintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java,BffCookieStatelessnessIT.java,BffKeycloakLoginFlow.java,BffSessionMediationIT.java— in-plan test follow-up riding this PR per the compactpr_strategydecisionTest Plan
verify -Ppre-commit)verify)LargeBodyITpositive/negative cases passuploadLargeproduces a summary documentRelated Issues
None — no
issue_urlrecorded for this plan.Generated by plan-finalize skill
Summary by CodeRabbit
New Features
RESOURCE_EXHAUSTEDfor body-cap breaches.Bug Fixes
Documentation