feat(security): canonicalize filter modes to strict/lenient/none - #133
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, your pull request is larger than the review limit of 150000 diff characters
|
Warning Review limit reached
Next review available in: 13 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 (1)
📝 WalkthroughWalkthroughChangesInbound filter profiles and staged validation
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 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 |
![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
🧹 Nitpick comments (3)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.java (1)
286-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate to
RouteTableBuilder.globalProfile(gatewayConfig)instead of re-implementing it.This is byte-for-byte the same resolution as
RouteTableBuilder.globalProfile(whichConfigValidator.effectiveProfilealready uses). Two copies of the gateway-wide fallback can drift, and a drift here silently changes the effective posture of every block-less route.♻️ Proposed refactor
- SecurityProfile globalProfile = gatewayConfig.securityDefaults() - .flatMap(SecurityDefaultsConfig::profile) - .flatMap(SecurityProfile::parse) - .orElse(SecurityProfile.DEFAULT_PROFILE); + SecurityProfile globalProfile = RouteTableBuilder.globalProfile(gatewayConfig);api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/RouteTableBuilder.java (1)
312-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne resolution site still hand-rolls this chain.
The Javadoc names
ConfigValidatoras the shared consumer, butGatewayEdgeRoute's constructor re-implements the identicalsecurityDefaults → profile → parse → DEFAULT_PROFILEchain inline rather than calling this method. That is the runtime seam that actually seedsBasicChecksStageanddefaultMaxBodySize, so it is the one most costly to let drift from the boot refusal.Routing it through
RouteTableBuilder.globalProfile(gatewayConfig)keeps all three sites on one definition.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/routing/RouteRuntime.java (1)
77-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
securityConfigurationis the onlyOptionalfield without a@Builder.Default.Every other
Optional/collection field here (upstream,httpClient,resilienceGuard,assetSource,effectiveWebSocketIdleTimeoutSeconds) declares one, so a builder call site that omits this field gets anullrather thanOptional.empty()—RouteRuntimeTest.runtimeBuilder()is exactly such a site. The new Javadoc says it "staysOptionalonly for the builder's test call sites", which only holds if omitting it is safe.♻️ Give the field the same default as its siblings
+ `@Builder.Default` private final Optional<SecurityConfiguration> securityConfiguration;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4760ee88-b659-4b15-bca1-073c808e0f3c
📒 Files selected for processing (35)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/RouteTableBuilder.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/SecurityDefaultsConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/SecurityFilterConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/SecurityProfile.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssembler.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/routing/RouteRuntime.javaapi-sheriff/src/main/resources/schema/endpoint.schema.jsonapi-sheriff/src/main/resources/schema/gateway.schema.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/RouteTableBuilderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/load/ConfigLoaderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/model/SecurityProfileTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgePipelineTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/routing/RouteRuntimeTest.javadoc/adr/0019-Reserved_BFF_paths_bypass_the_url-parameter_value_pipeline_and_the_cookie-header_length_relaxation_is_gateway-wide.adocdoc/adr/0023-Inbound_filter_modes_none_is_a_partial_disable_retaining_the_body_cap_and_path_allowlist_and_is_refused_on_authenticated_and_BFF_routes.adocdoc/architecture.adocdoc/configuration.adocdoc/development/README.adocdoc/user/README.adocintegration-tests/scripts/verify-invalid-config-fails.shintegration-tests/src/main/docker/sheriff-config/endpoints/httpbin.yamlintegration-tests/src/main/docker/sheriff-config/gateway.yamlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/MetricsIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/SecurityProfileModeIT.java
…tial disable Move url-parameter validation out of the pre-route BasicChecksStage into the post-route ThoroughChecksStage so it can be keyed on the route's resolved security profile. The `none` mode is a PARTIAL disable, not a bypass: it keeps the max_body_bytes cap and the allowed_paths check, and disables only the relocated url-parameter validation and the reRunPipelines re-run. The pre-route floor (parameter-count cap, encoded-separator rejection, header size limits) still applies to every route regardless of mode, and the reserved path bypass now holds structurally — a reserved path returns before route selection and never reaches ThoroughChecksStage. Tests updated for the none-mode partial-disable matrix across ThoroughChecksStageTest, BasicChecksStageTest, GatewayEdgePipelineTest and GatewayEdgeRouteBffWiringTest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…routes Add a fail-closed ConfigValidator rule (ADR-0023): a route whose effective inbound-filter profile resolves to `none` must be neither effectively authenticated nor anchored under a `type: bff` anchor. The refusal covers the gateway-wide case for free — a `security_defaults.profile: none` reaching such a route through the fallback resolves to `none` and is refused identically. The access level is derived through the promoted RouteTableBuilder.effectiveAccessLevel seam rather than read off AnchorConfig.access, so a route that strengthens a public anchor's auth floor (permitted by ADR-0007) is still refused — reading the anchor's static access would under-refuse exactly those routes. RouteTableBuilder.globalProfile is promoted alongside it so the boot refusal resolves the gateway-wide fallback through the same chain the route table uses. Errors collect into the shared list and never fail fast (ADR-0009); the message names the route, the refusing dimension and the remedy, and echoes no configured scalar value. No profile-value range rule is added — the JSON Schema owns that gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… in the Docker tier
Boot half — three new cases in verify-invalid-config-fails.sh (now six total), each
asserting a fail-fast non-zero exit plus a marker that is a config key or a fixed
message fragment, never a rejected scalar:
4. security_defaults.profile: default — the dropped preset, refused by the JSON
Schema's enum (the value-range gate);
5. profile: none on an effectively-authenticated route — refused by the ADR-0023
ConfigValidator rule on the effective-access-level dimension;
6. profile: none on a type: bff route — refused on the anchor-type dimension.
Splitting 4 from 5/6 keeps the schema's range gate and the validator's posture gate
independently regression-visible.
Runtime half — SecurityProfileModeIT proves `none` is a PARTIAL disable: a
url-parameter value the strict /proxy route rejects is forwarded on the none route,
while the pre-route floor (encoded separator, query-parameter count cap), the route
body cap and the allowed_paths allowlist all still reject. Without those
still-rejects cases the suite would pass equally against a total bypass.
The mounted config now declares security_defaults.profile: strict explicitly rather
than relying on the omitted-block default, and adds one none-mode route under the
existing public, effectively-unauthenticated `api` anchor, carrying both a
max_body_bytes cap and an allowed_paths allowlist.
Behavioural consequence of the tightened baseline: the pre-route floor moved from the
`defaults()` preset to `strict()` — 1 MiB body (was 5 MiB), 20 query parameters (was
100), 1024-character header values, parameter values and paths (was 2048/2048/4096),
and failOnSuspiciousPatterns now on. The mandated fixture sweep over the mounted
endpoints and the existing IT suites found no fixture that depends on the looser caps:
inbound header values are short (the BFF bearer is injected outbound, not sent by the
client), no IT sends over 20 parameters or a body above 1 MiB, and the upload anchor's
deliberate 64 MiB benchmark cap is an explicit per-anchor override that survives
unchanged. The full 84-test IT suite is green, so this run is also the blast-radius
probe for the tightened baseline — no fixture was loosened to accommodate it.
Also corrects a MetricsIT comment that deliverable 2 invalidated: the url-parameter
rejection it drives now happens post-route in ThoroughChecksStage, not before route
selection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two auto-fixes the quality gate produced over the new SecurityProfileNoneRefusal cases: restore the missing space after a comma in the assertHasError calls, and replace assertTrue(!x) with assertFalse(x). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ADR-0019
ADR-0023 records four coupled decisions: the canonical mode set strict/lenient/none
with `default` removed and an omitted profile resolving to strict (the value range
staying in the JSON Schema rather than being duplicated into ConfigValidator); `none`
as a partial disable bounded both structurally (the pre-route floor cannot be skipped
because route selection hard-requires the canonical path) and by policy (max_body_bytes
and allowed_paths are deliberately retained); the closed, normative list of what `none`
disables — the relocated url-parameter name/value validation and the pipeline re-run,
nothing else; and the fail-closed boot refusal on effectively-authenticated and BFF
routes, keyed on the derived effective access level per ADR-0007's strengthening case.
The wholesale-skip alternative is recorded as considered and rejected. ADR-0007,
ADR-0013 and ADR-0019 are cross-referenced and explicitly NOT superseded.
ADR-0019 gains an Amendment section: its reserved-BFF-path relaxation is now enforced
structurally — a reserved path terminates before route selection and so never reaches
the relocated parameter pipeline — instead of by the reserved-path predicate, which is
removed. The observable relaxation is unchanged, and the cookie-header relaxation is
untouched.
architecture.adoc's Component Model no longer describes inbound validation as one
undifferentiated step: it now describes the non-skippable pre-route floor, route
selection, and the always-dispatched post-route stage whose validation half is
profile-gated, plus a per-check table naming which half owns each check and whether
`profile` can switch it. `none`'s exact blast radius is stated without describing it as
skipping the thorough checks. The embedded request-pipeline.svg still renders the old
single-box model, so a NOTE marks the prose as authoritative for the pipeline's shape
and the diagram as accurate only for the surrounding stage order — the two no longer
contradict each other silently.
Deviation from the task's acceptance wording: the ADR-0019 amendment carries no literal
date. The manage-adr authoring discipline forbids dates in an ADR body ("no PR numbers,
commit SHAs, dates, or lesson IDs"), and git already carries the chronology.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oc layers
configuration.adoc — five stale or now-incomplete sites corrected:
(a) the security_defaults example comment now reads strict | lenient | none and points
at the closed definition instead of glossing `none` inline;
(b) the when-omitted table row now states the resolution precisely, naming the
route -> anchor -> security_defaults wholesale-replacement order (and that the
endpoint level carries no security_filter block, so it is not in the chain);
(c) the security_filter mapping table drops defaults() and adds `none` as a dispatch
mode with no cui-http preset of its own, inheriting its limits from the nearest
non-`none` profile in the chain;
(d) the security_defaults section is rewritten for the new set — an omitted block
resolves to strict, the removed `default` is a hard boot failure with a one-line
migration, and the fail-closed refusal on effective-authenticated and BFF routes is
stated, including why it keys on the effective rather than the declared access
level;
(e) the forwarded-header sanitization paragraph is verified and kept, now noting the
pipeline is configured gateway-wide so no per-route profile — `none` included —
relaxes it, alongside the fixed fail-secure field guards.
A new "The `none` mode" subsection reproduces the closed two-list definition as a
normative closed list: everything still enforced (the pre-route floor, max_body_bytes,
allowed_paths, verb gate, auth, CSRF, forward policy) and the exactly two things disabled
(the relocated url-parameter name+value validation, and the pipeline re-run). The
path-safety row now distinguishes the floor's run from the divergent post-route re-run,
since only the latter is switchable.
user/README.adoc (operator-facing) — a decision guide for the three modes with a
when-to-choose-each table, the partial-disable correction spelled out as what a `none`
route still rejects, the boot refusal on authenticated/BFF routes including the indirect
global-fallback case and the strengthened-public-anchor case, the hard boot failure on an
unrecognized value with the `default` -> `strict` migration, and the omitted-block
tightening. The tightening carries its diagnosable symptom — previously-passing requests
returning 400 — plus a before/after cap table (body 5 MiB -> 1 MiB, parameters 100 -> 20,
header/parameter values 2048 -> 1024, path 4096 -> 1024, suspicious-pattern rejection
off -> on) and two remedies ordered to prefer a per-route cap raise over a gateway-wide
loosening.
development/README.adoc (contributor-facing) — the four structural constraints that look
arbitrary without their reasons: why the url-parameter validation moved post-route (the
profile is per-route, so a pre-route check can be gated on neither the profile nor the
route's own configuration) and that the reservedPathMatcher predicate is gone and must not
return; why the pre-route floor cannot be made skippable (route selection consumes the
canonical path the floor produces — a circular dependency, not a preference); why the
`none` branch lives inside ThoroughChecksStage rather than at the dispatch site (the body
cap and path allowlist are enforced inside the stage and must run under `none`, so
skipping the dispatch would silently drop both); where the value-range gate lives (the
JSON Schema's three enum sites, never a second copy in ConfigValidator); and why the boot
refusal keys on the effective access level.
All cap figures are verified against the cui-http strict/lenient/default presets rather
than restated from memory. No occurrence of the `default` preset survives in doc/ as a
valid profile value — the only remaining mention is the migration note that names it as
rejected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ody cap on 413 Post-rebase reconciliation with PLAN-33 (29f1dac), which landed the derived body-cap plus its 413 CONTENT_TOO_LARGE rejection while this branch was in its finalize wait region. ADR number collision: PLAN-33 landed doc/adr/0023-The_declared_max_body_bytes_cap_is_the_effective_one... and owns 0023. This plan's inbound-filter-modes ADR is renumbered to 0024 — filename, internal title heading, and every cross-reference in doc/, doc/adr/0019 (the amendment this plan added), the JSON Schema description, javadoc and code comments, the IT config descriptors and the boot-refusal verification script. The renumbered record now also references ADR-0023 as the definition of the cap it retains under 'none'. Body-cap status alignment: three assertions and one operator-doc claim still described the retained cap as a 400. The cap now raises 413, unchanged by the mode, so GatewayEdgePipelineTest, SecurityProfileModeIT and doc/user/README.adoc are corrected. The 'none' semantics are untouched: the cap and allowed_paths remain enforced under 'none', which still disables only the relocated url-parameter validation and the pipeline re-run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2c0244c to
30ab701
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/RouteTableBuilder.java (1)
312-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute
GatewayEdgeRoute's profile fallback throughRouteTableBuilder.globalProfile().ConfigValidatorandRouteTableBuilder.logPosture()both use the helper, butGatewayEdgeRouteconstructor lines 286-291 still inlines the same chain, keeping the runtime default baseline on independent code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 177b194a-3eda-46e9-8971-e4abed69b2f4
📒 Files selected for processing (35)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/RouteTableBuilder.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/SecurityDefaultsConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/SecurityFilterConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/SecurityProfile.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssembler.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/routing/RouteRuntime.javaapi-sheriff/src/main/resources/schema/endpoint.schema.jsonapi-sheriff/src/main/resources/schema/gateway.schema.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/RouteTableBuilderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/load/ConfigLoaderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/model/SecurityProfileTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgePipelineTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/routing/RouteRuntimeTest.javadoc/adr/0019-Reserved_BFF_paths_bypass_the_url-parameter_value_pipeline_and_the_cookie-header_length_relaxation_is_gateway-wide.adocdoc/adr/0024-Inbound_filter_modes_none_is_a_partial_disable_retaining_the_body_cap_and_path_allowlist_and_is_refused_on_authenticated_and_BFF_routes.adocdoc/architecture.adocdoc/configuration.adocdoc/development/README.adocdoc/user/README.adocintegration-tests/scripts/verify-invalid-config-fails.shintegration-tests/src/main/docker/sheriff-config/endpoints/httpbin.yamlintegration-tests/src/main/docker/sheriff-config/gateway.yamlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/MetricsIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/SecurityProfileModeIT.java
🚧 Files skipped from review as they are similar to previous changes (7)
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/SecurityDefaultsConfig.java
- integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/MetricsIT.java
- doc/adr/0019-Reserved_BFF_paths_bypass_the_url-parameter_value_pipeline_and_the_cookie-header_length_relaxation_is_gateway-wide.adoc
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/SecurityFilterConfig.java
- doc/user/README.adoc
- doc/configuration.adoc
- doc/development/README.adoc
|
/review |
PR Reviewer Guide 🔍
|
Triage dispositionsIn reply to comment_id:
|
…untime.securityConfiguration GatewayEdgeRoute inlined a second copy of the fail-closed security_defaults profile resolution that RouteTableBuilder.globalProfile already owns; two copies of a security-critical default-resolution rule can silently diverge and change the effective posture of every block-less route. Route the constructor through the shared helper and drop the now-unused SecurityDefaultsConfig import. RouteRuntime.securityConfiguration was the only Optional field in the class without a @Builder.Default, so an omitting builder call site handed a raw null to ThoroughChecksStage's getSecurityConfiguration().orElse(...) on the hot path. Apply the same fail-closed remedy already accepted for securityProfile. Tests: a semantic round-trip guard over builderSeededFrom (seeding from a preset and restating one dimension at its own value must rebuild that preset, for both non-none presets) plus a 24-component tripwire so a cui-http upgrade surfaces the review question, and a RouteRuntime assertion that an omitted securityConfiguration is Optional.empty() rather than null. Addresses PR #133 review findings 10985c, 9cc397, 8a03df, cf8372. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e configuration ThoroughChecksStage.reRunPipelines re-ran the url-path and header-VALUE pipelines under the route's divergent SecurityConfiguration but never the header-NAME pipeline, while BasicChecksStage validates header names under the gateway baseline only. A route whose declared security_filter carried a stricter header-name policy -- allowed_header_names, blocked_header_names or a lower maxHeaderNameLength -- therefore never had that policy applied, so it accepted header names its own resolved configuration should reject. The method's javadoc already claimed the header pipelines were re-run, so the code under-delivered its documented contract. Iterate header entries and validate the key before the values. This completes the internals of reRunPipelines; it does not change WHAT profile: none disables -- the whole method is item two of the closed two-item list, and the skippableValidationEnabled() gate in process() is untouched. Header-name validation has no ADR-0019 cookie carve-out (that relaxation is value-only and lives in the pre-route floor), so the value loop's pipeline selection is unchanged. Tests: the stricter-header-name case is red without the validate call (verified locally: exactly the two STRICT/LENIENT cases fail), plus two boundary guards -- a none route with the same policy is still not rejected on the name, and a baseline-equal route is still not re-run. Addresses PR #133 review finding b35d66. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…refusal leg ConfigValidator.effectiveProfile resolves through three legs -- route-declared, anchor-declared, gateway-wide fallback -- but the SecurityProfileNoneRefusal suite covered only the first and third. The middle leg (an anchor declaring security_filter.profile: none, inherited by a block-less route) had zero coverage on a fail-closed security rule. Add the refusal on an authenticated anchor, its mirror-image acceptance on a genuinely public unauthenticated route, and the wholesale-replacement boundary the effectiveProfile javadoc states but nothing pinned: a route declaring a security_filter block WITHOUT a profile falls back to the gateway-wide value and never to its anchor's profile, so such a route is accepted under a gateway-wide strict even when its anchor says none. That last case is the one most likely to regress silently if the chain were ever turned into a merge. Test-only; ConfigValidator is unchanged. Addresses PR #133 review finding 9adc51. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… JSON pointer verify-invalid-config-fails.sh asserted Case 4 with the bare marker 'profile'. assert_fails_to_boot only needs a non-zero exit plus a grep -Fq hit, and the bare word can appear in unrelated startup output, so the case could pass green even if the D2 schema enum gate on security_defaults.profile regressed entirely -- which is the one thing the case exists to prove. Established the real message shape first rather than adopting a plausible fragment: ConfigLoader files the schema violation with the validator's instance location, ConfigLoadException renders it as 'gateway.yaml [<pointer>]', and the live container log reads ApiSheriff-200: Invalid configuration in 'gateway.yaml' at '/security_defaults/profile': does not have a value in the enumeration ["strict", "lenient", "none"] so /security_defaults/profile is the marker. It is a config key path, never the rejected scalar, so the D5 redaction contract holds; it is also preferred over any slice of the enum sentence, which the schema validator localises (the same assertion renders in German on a de_DE JVM) and which would make a text marker locale-fragile. Verified by running the script against the built distroless image: all six cases still fail-fast non-zero, Case 4 on the tightened marker. Addresses PR #133 review finding 35ec23. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nonical contract Three prose defects, all fixed by aligning the docs with the already-implemented and already-tested contract; no settled semantic changes. architecture.adoc: the SecurityConfiguration wiring bullet left a reader wondering what a none route gets, since none has no preset. Add one clause noting it inherits the nearest non-none preset and cross-reference the Profiles paragraph, which stays the single authoritative statement of the rule. doc/user/README.adoc, none row: 'Use it per route, never gateway-wide' contradicted the implemented contract -- a gateway-wide security_defaults.profile: none IS accepted while no authenticated or BFF route inherits it, which ConfigValidatorTest.shouldAcceptGlobalNoneOnPublicRoutesOnly proves. Rewritten as a recommendation plus the real boundary. The same cell named only one of the two disabled behaviours, contradicting the section directly below it; the per-route pipeline re-run is now named too. doc/user/README.adoc, strict row: the flat 1024-character header-value claim is the preset cap, not universal. Qualified, with the Cookie / Set-Cookie carve-out named and cross-referenced to the configuration reference. Figure verification: the review comment's 2048-character figure is cui-http's defaults() baseline (MAX_HEADER_VALUE_LENGTH_DEFAULT), not a cap any documented mode produces -- BasicChecksStage is seeded from the resolved profile preset and an omitted security_defaults block resolves to strict, so 1024 is the correct strict figure and 2048 is deliberately NOT written into the strict row. The cookie cap is oidc.session.max_cookie_size (default 4096, range 40-8192) plus the fixed 512-byte COOKIE_HEADER_OVERHEAD_BYTES headroom; the README states the derivation rather than a literal number so it cannot drift from the key. Addresses PR #133 review findings 170b58 and d7525c. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doc/architecture.adoc (1)
143-148: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the profile-validation mechanism description.
This says the range is enforced by JSON Schema “not by the configuration validator,” but this PR adds
ConfigValidatorhard-failure paths for unrecognized profile values. Document both layers rather than excluding the validator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0b0da30-6fd8-4841-93fd-a33d090f2053
📒 Files selected for processing (10)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/routing/RouteRuntime.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/routing/RouteRuntimeTest.javadoc/architecture.adocdoc/user/README.adocintegration-tests/scripts/verify-invalid-config-fails.sh
Triage dispositionsIn reply to comment_id:
|
The passage claimed profile-range validation is enforced "by the bundled JSON Schema, not by the configuration validator". The range half is accurate, but the trailing clause read as excluding ConfigValidator from profile validation entirely — which this change makes false, since ConfigValidator now owns the `none` fail-closed posture refusal via validateSecurityProfileNone. The paragraph now names both layers and cross-references the existing "What `profile: none` costs, exactly." paragraph, which documented the boot refusal without naming the enforcing layer. No gate moved: the value range stays owned by the three JSON Schema enum sites, matching ConfigValidator's own class javadoc. Resolves CodeRabbit finding 14ae54 on PR #133. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
Introduce a
SecurityProfilemode set{strict, lenient, none}and wire the previously-inertprofileknob end-to-end: the gateway-wide
SecurityConfigurationis seeded from the resolved global profileinstead of the hard-coded
SecurityConfiguration.defaults(), and each route resolves an effectiveprofile through the
route/endpoint/anchor security_filter → security_defaultsfallback chain onto acui-httppreset, with an omittedsecurity_defaults.profileresolving tostrict. Thenonemode isa partial disable: it turns off only the per-route injection/validation checks — the URL-parameter
name/value pipeline (relocated out of the pre-route
BasicChecksStageintoThoroughChecksStage) andthe per-route divergent-pipeline re-run — while
max_body_bytes,allowed_paths, and the entirepre-route floor (canonical-path production,
CanonicalPathGuard, collection limits, header validation)keep running.
noneis refused at boot for any route whose effectiveAccessLevelisAUTHENTICATEDor whose anchorAnchorTypeisBFF, and thedefaultpreset is dropped outright fromthe three JSON-Schema enum sites that already gate the value.
Changes
SecurityProfile— new mode enum{strict, lenient, none}replacing the droppeddefaultpreset.SecurityDefaultsConfig/SecurityFilterConfig— canonicalizedprofilefield, range-checked atconfig-validation time with a hard boot-failure error for unrecognized values.
RouteTableBuilder— resolves the effective per-routeprofilethrough thesecurity_filter → security_defaultsfallback chain and refusesnoneat boot when the route'seffective
AccessLevelisAUTHENTICATEDor its anchorAnchorTypeisBFF.GatewayEdgeRoute—securityConfigurationFornow maps the resolvedprofileonto the correspondingcui-httppreset instead of ignoring the knob.BasicChecksStage/ThoroughChecksStage— stage-1 inbound pipeline split three ways: an always-onpre-route part (canonical-path production, URL path pipeline, collection limits, header validation)
and a per-route-skippable post-route part (URL-parameter value validation) plus
ThoroughChecksStagewholesale, both skippable undernone.CanonicalPathGuardkeeps its position.RouteRuntimeAssembler/RouteRuntime— wired the skip decision through to pipeline assembly.ConfigValidator— added thenone-on-authenticated /none-on-BFF boot refusal and the unrecognizedprofilehard-failure path.endpoint.schema.json,gateway.schema.json) —profileenum updated to{strict, lenient, none},defaultpreset removed.nonepartial-disable semantics; ADR-0019 amended for the relocatedURL-parameter validation boundary.
doc/configuration.adoc,doc/architecture.adoc,doc/development/README.adoc,doc/user/README.adoc— corrected stale claims and documented the new mode set (three-layer docs).integration-tests/scripts/verify-invalid-config-fails.shand the newSecurityProfileModeIT— Docker IT coverage for strict-applies-when-omitted,none-skips-the-skippable-stages, and the
none-on-authenticated /none-on-BFF boot refusals.RouteTableBuilderTest,ConfigLoaderTest,SecurityProfileTest,ConfigValidatorTest,GatewayEdgePipelineTest,GatewayEdgeRouteBffWiringTest,GatewayEdgeRouteTest,BasicChecksStageTest,ThoroughChecksStageTest,RouteRuntimeTest,RouteRuntimeAssemblerTest.Test Plan
verify -Ppre-commitandverify)test -pl api-sheriff -am)verify -Pintegration-tests -pl integration-tests -am)Related Issues
None
Generated by plan-finalize skill
Intent
The problem. The
profileknob onsecurity_filter/security_defaultswas inert at HEAD: noread site mapped it to a
cui-httppreset, so operators believed they could dial security strictnessper route and could not. There was also no way to fully disable the per-route injection/validation
pipeline for routes that genuinely don't need it, and the existing docs described behaviour the code
did not implement.
The chosen approach. Canonicalize the mode set to
{strict, lenient, none}(dropping the unuseddefaultpreset outright — pre-1.0, no deprecation shim) and wire it end-to-end: boot-timerange-checking with a hard-failure error on unrecognized values, per-route effective-profile resolution
through the
security_filter → security_defaultsfallback chain (defaulting tostrictwhen omitted),and a real mapping onto the route's
cui-httpSecurityConfigurationpreset.noneis implemented asa narrow, closed carve-out rather than a full bypass: the stage-1 inbound pipeline is split so only the
per-route injection/validation checks (relocated URL-parameter value validation plus the
ThoroughChecksStagere-run) are skippable, while the pre-route floor (canonical path production,CanonicalPathGuard, collection limits, header validation) and the body-cap / allowlist checks insideThoroughChecksStagealways run.noneis refused at boot when the[Intent truncated — 1391 of 2361 characters shown; full outline in the plan workspace]
Summary by CodeRabbit
New Features
strict,lenient, andnone.nonemode now selectively disables URL-parameter validation while retaining key safety limits, body-size caps, and allowed-path checks.Bug Fixes
noneusage on authenticated or BFF routes.413 Content Too Large.Documentation