feat(config): implement endpoint anchors (D1-D5)#74
Merged
Conversation
…-time hardening (D1-D5)
Implements Plan 03 D1-D5: anchor-scoped policy inheritance (ADR-0007, flipped to Accepted), the single ${VAR}/${VAR:-default} substitution engine replacing convention-named env overrides (EndpointEnablementResolver removed; TopologyResolver TOPOLOGY_ lookup dropped), and seven config-subsystem boot-time fixes (real-CIDR trusted_proxies with full-space rejection and broad-prefix WARN, same-prefix route-disjointness moved to ConfigValidator, http/https topology scheme allowlist, stray .yml rejection, secret-safe binding errors, YAML expansion limits). Integration configs gain api+bff anchors and an anchor-violation negative check; docs reconciled.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-102 Collapse the byte-identical normalizePrefix duplicate in ConfigValidator onto the shared RouteTableBuilder implementation, and add the missing ApiSheriff-102 (broad trusted_proxies prefix) row to doc/LogMessages.adoc. Co-Authored-By: Claude <noreply@anthropic.com>
TopologyResolver.decompose() echoed the fully substituted topology URL into TopologyResolutionException messages (malformed URI, missing scheme/host, disallowed scheme), which ConfigProducer logs at ERROR. Keep only the alias name / offending scheme, mirroring the redaction contract EnvSecretResolver and ConfigLoader already enforce (CWE-209). Co-Authored-By: Claude <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @OliverWolffGIP, 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 (5)
📝 WalkthroughWalkthroughThe configuration pipeline adds namespace-scoped policy anchors, richer placeholder substitution, anchor-aware validation, effective route posture materialization, structured logging, and expanded unit, integration, and fail-fast coverage. ChangesAnchor policy and configuration pipeline
Estimated code review effort: 5 (Critical) | ~120 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements policy anchors (ADR-0007) and visible in-file placeholder substitution (D4) across the configuration boot pipeline, while moving route disjointness checks to the validator (ADR-0009) and hardening YAML loading. The review feedback highlights two important issues: first, the global scalar coercion in ConfigLoader may incorrectly convert string-typed fields (like secrets or IDs) to booleans or integers if their resolved values resemble those types, causing strict schema validation to fail; second, the public static normalizePrefix method in RouteTableBuilder lacks a defensive null check, which could lead to unexpected NullPointerExceptions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
api-sheriff/src/test/java/de/cuioss/sheriff/api/config/validation/ConfigValidatorTest.java (1)
427-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that tightly scoped CIDRs emit no broad-range WARN.
The test name promises “without warning,” but the assertions only prove that validation succeeds. Add a negative WARN assertion so a threshold regression that warns for
10.0.0.0/8or2001:db8::/32is detected.🤖 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/api/config/validation/ConfigValidatorTest.java` around lines 427 - 440, Update shouldAcceptTightlyScopedCidrs in ConfigValidatorTest to assert that validation produces no broad-range WARN for either 10.0.0.0/8 or 2001:db8::/32. Keep the existing trusted_proxies validation assertion and use the returned ConfigError details to specifically reject the relevant warning.api-sheriff/src/test/java/de/cuioss/sheriff/api/quarkus/ConfigFailFastTest.java (1)
88-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin this failure to the squatter validation rule.
assertThrows(IllegalStateException.class)also passes if the fixture later fails for an unrelated loading, topology, or schema problem. Assert the ERROR log contains the namespace diagnostic, such asdoes not declare it.Proposed assertion
assertThrows(IllegalStateException.class, producer::gatewayConfig, "an undeclared squatter route inside an anchor namespace must refuse boot"); +LogAsserts.assertLogMessagePresentContaining(TestLogLevel.ERROR, "does not declare it");🤖 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/api/quarkus/ConfigFailFastTest.java` around lines 88 - 94, Strengthen shouldRefuseBootOnAnAnchorSquatterConfig by capturing the ERROR log produced during producer.gatewayConfig and asserting it contains the namespace diagnostic “does not declare it”. Keep the existing IllegalStateException assertion, but ensure the test specifically verifies the squatter validation rule rather than any unrelated boot failure.
🤖 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
`@api-sheriff/src/main/java/de/cuioss/sheriff/api/config/load/ConfigLoader.java`:
- Around line 310-329: Update ConfigLoader.coerce and its callers to be
destination-type aware: preserve whole-value placeholders as TextNode by
default, and only convert values such as “true” or numeric strings when the
target schema expects boolean or integer. Pass the expected schema type through
the placeholder-resolution path, while retaining existing integer range handling
for integer destinations.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/api/config/load/EnvSecretResolver.java`:
- Around line 56-58: Update EnvSecretResolver’s placeholder parsing and
assertNoMalformedPlaceholder validation to reject any nested “${” sequence
within a placeholder default, including `${A:-${B}}`, instead of treating the
inner expression as resolved text. Add a malformed-placeholder test covering
this nested-default case.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/api/config/validation/ConfigValidator.java`:
- Around line 95-106: Update validateEffectiveAuth and its related
auth-validation flow to resolve an Optional<AuthConfig> for every route,
including route-specific anchors and auth overrides, rather than checking only
the endpoint anchor. Report routes with missing auth configuration at
/endpoint/routes, then derive bearer and session requirements only from present
route values so both route-provided auth and auth-less overrides are validated
correctly.
- Around line 621-625: Update prefixContains so a normalized root container "/"
is treated as containing every normalized candidate path; preserve exact-match
and owner-prefix behavior for non-root containers, while avoiding the owner +
"/" double-slash check.
- Around line 215-217: Update hostsOverlap to compare present route hosts
case-insensitively, such as by normalizing both host values before equality
comparison, while preserving the existing behavior that empty hosts overlap with
any host.
In `@api-sheriff/src/main/resources/schema/gateway.schema.json`:
- Around line 138-152: The CORS validation in ConfigValidator.validateCors
currently checks only gateway.securityHeaders(); update it to also validate
security_headers from every configured anchor under anchors. Apply the same
allow_credentials with wildcard-origin rule to each anchor policy, while
preserving the existing gateway validation behavior.
In
`@api-sheriff/src/test/java/de/cuioss/sheriff/api/config/load/ConfigLoaderTest.java`:
- Around line 404-413: Update rejectsYamlExceedingAliasExpansionLimit to use a
schema-valid YAML document while retaining alias expansion beyond the configured
limit, then assert the thrown ConfigLoadException contains the parser’s
alias-expansion-limit diagnostic. Ensure the test cannot pass solely because
schema validation rejects unknown properties.
In `@doc/configuration.adoc`:
- Around line 408-412: Update the validation summary around the existing
differing-exact-header-values text to also state that opposite presence
assertions, present: true versus present: false for the same header at the same
prefix, are disjoint. Keep the clarification that a lone present: true does not
establish a conflict.
- Around line 89-101: Update the topology override passages around the earlier
descriptions to remove any implication of automatic environment-over-file
lookup. State that topology and enablement values can be overridden only through
explicit in-file `${VAR}` or `${VAR:-default}` placeholders, consistent with the
placeholder resolution rules already documented.
In `@integration-tests/scripts/verify-invalid-config-fails.sh`:
- Around line 72-79: Update verify-invalid-config-fails.sh so each test
explicitly asserts that the container logs contain the expected ${marker},
rather than merely printing matching lines. Preserve the existing non-zero exit
check, but fail with a clear error when ${marker} is absent, ensuring unrelated
startup failures cannot satisfy the validation test.
---
Nitpick comments:
In
`@api-sheriff/src/test/java/de/cuioss/sheriff/api/config/validation/ConfigValidatorTest.java`:
- Around line 427-440: Update shouldAcceptTightlyScopedCidrs in
ConfigValidatorTest to assert that validation produces no broad-range WARN for
either 10.0.0.0/8 or 2001:db8::/32. Keep the existing trusted_proxies validation
assertion and use the returned ConfigError details to specifically reject the
relevant warning.
In
`@api-sheriff/src/test/java/de/cuioss/sheriff/api/quarkus/ConfigFailFastTest.java`:
- Around line 88-94: Strengthen shouldRefuseBootOnAnAnchorSquatterConfig by
capturing the ERROR log produced during producer.gatewayConfig and asserting it
contains the namespace diagnostic “does not declare it”. Keep the existing
IllegalStateException assertion, but ensure the test specifically verifies the
squatter validation rule rather than any unrelated boot failure.
🪄 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: 73976a11-6005-4ba9-8360-da2682b4d250
📒 Files selected for processing (39)
api-sheriff/src/main/java/de/cuioss/sheriff/api/config/ConfigLogMessages.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/RouteTableBuilder.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/load/ConfigLoader.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/load/EnvSecretResolver.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/model/AnchorConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/model/EndpointConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/model/GatewayConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/model/ResolvedRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/model/RouteConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/topology/EndpointEnablementResolver.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/topology/TopologyResolver.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/topology/package-info.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/config/validation/ConfigValidator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/quarkus/ConfigModelReflection.javaapi-sheriff/src/main/java/de/cuioss/sheriff/api/quarkus/ConfigProducer.javaapi-sheriff/src/main/resources/schema/endpoint.schema.jsonapi-sheriff/src/main/resources/schema/gateway.schema.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/api/config/RouteTableBuilderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/api/config/load/ConfigLoaderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/api/config/load/EnvSecretResolverTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/api/config/model/ConfigModelContractTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/api/config/topology/EndpointEnablementResolverTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/api/config/topology/TopologyResolverTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/api/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/api/quarkus/ConfigFailFastTest.javaapi-sheriff/src/test/resources/config/anchor-squatter/endpoints/squatter.yamlapi-sheriff/src/test/resources/config/anchor-squatter/gateway.yamlapi-sheriff/src/test/resources/config/anchor-squatter/topology.propertiesapi-sheriff/src/test/resources/config/anchored/endpoints/api.yamlapi-sheriff/src/test/resources/config/anchored/endpoints/bff.yamlapi-sheriff/src/test/resources/config/anchored/gateway.yamlapi-sheriff/src/test/resources/config/anchored/topology.propertiesdoc/LogMessages.adocdoc/adr/0007-anchor-scoped-policy.adocdoc/configuration.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/api/integration/ConfigLoadedIntegrationIT.java
💤 Files with no reviewable changes (2)
- api-sheriff/src/main/java/de/cuioss/sheriff/api/config/topology/EndpointEnablementResolver.java
- api-sheriff/src/test/java/de/cuioss/sheriff/api/config/topology/EndpointEnablementResolverTest.java
Sonar java:S3655 flagged anchor.get() in validateAnchorAuthFloor as an unguarded Optional access (unprovable-though-safe via the derived anchorRequire). Restructure with an explicit isEmpty() continue so the access is guarded; behavior unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
… and YAML alias-bomb guard TASK-012: rewrite the ConfigValidator auth-mandatoriness rule to check per-route via the route->endpoint->anchor resolution chain (the same chain RouteTableBuilder uses). A config where every route self-declares auth is no longer falsely rejected, and a route overriding to an auth-less anchor is now caught in the ADR-0009 all-violations pass instead of escaping to a RouteTableException at boot. TASK-013: add a SnakeYAML-native compose-only pre-pass to ConfigLoader that actually enforces setMaxAliasesForCollections. Jackson's YAMLParser consumes SnakeYAML's event stream directly and never runs the Composer, so the alias-expansion cap was inert on the readTree path; the alias-bomb test now asserts the alias-specific diagnostic so it fails loudly if the guard regresses. Also folds in the unified-triage review fixes: RouteTableBuilder null-check, EnvSecretResolver nested-placeholder rejection, ConfigValidator host/prefix/anchor-CORS validation, ConfigValidatorTest/ConfigFailFastTest/EnvSecretResolverTest nitpicks, doc/configuration.adoc wording, and the verify-invalid-config-fails.sh marker assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Yaml.composeAll()'s iterator never yields null (verified against snakeyaml 2.6), so Objects.requireNonNull in withinExpansionLimits was dead code implying a false hazard. Drive the composer with a plain iterator loop instead; the alias/nesting caps still fire. Co-Authored-By: Claude <noreply@anthropic.com>
- hostsOverlap: hoist both host() Optionals into locals so the isEmpty guard provably covers the get() accesses (Sonar java:S3655 flagged the repeated accessor calls). - verify-invalid-config-fails.sh: assert on the unknown property KEY (unknown_key, carried in the ApiSheriff-200 pointer) instead of the rejected raw value, which the D5 binding-error redaction deliberately keeps out of the logs. Co-Authored-By: Claude <noreply@anthropic.com>
mktemp -d yields a 700-permission directory; the distroless image runs non-root, so the mounted gateway.yaml was unreadable and the container failed on "configuration file not found" instead of the validation under test (the old script passed for the wrong reason — any non-zero exit counted). chmod the temp dirs/files so the schema-invalid and anchor-violation configs are actually parsed and the asserted markers appear. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0010 (Proposed): YAML anchor/alias expansion limits are enforced in a SnakeYAML compose-only pre-pass before the Jackson bind — Jackson's event-stream path never runs the Composer, the only layer that counts collection aliases, so bind-side LoaderOptions caps are inert. Also persists the lessons-capture architecture hints (IT profile not covered by local pre-commit; package-info in deletion sweeps) into the per-module enriched.json descriptors. Co-Authored-By: Claude <noreply@anthropic.com>
OliverWolffGIP
enabled auto-merge
July 18, 2026 20:52
This was referenced Jul 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements
doc/plan/03-endpoint-anchors.adocin full — all five design decisions (D1-D5) — governed by ADR-0007 (endpoint anchors) and ADR-0009 (single-reporter disjointness move).gateway.schema.json/endpoint.schema.jsongain an optionalanchorsmap (name → path_prefix + policy blocks) and optionalanchorrefs on endpoint/route, with shared$defs; endpointauthbecomes optional.AnchorConfigrecord;GatewayConfig/EndpointConfig/RouteConfiggain anchor fields;RouteTableBuilderresolves gateway -> anchor -> endpoint -> route (wholesale replacement) intoResolvedRoute(effective auth/methods/security_filter/security_headers), preserving weakening WARNs and adding a per-route effective-posture INFOLogRecord.ConfigValidatorrules (prefix disjointness, anchor exists, route-inside-prefix, undeclared squatter, non-weakenable auth floor, endpoint-auth conditional mandatoriness, effective-auth completeness) in the existing all-violations convention, each with a negative test.${VAR}/${VAR:-default}pre-schema substitution pass acrossgateway.yaml,endpoints/*.yaml, andtopology.properties, with loud failure on a non-matching${and secrets classified pre-substitution; the convention-namedTOPOLOGY_<ALIAS>(TopologyResolver) andENDPOINT_<ID>_ENABLED(EndpointEnablementResolver) machinery is deleted outright (dead code per PR fix(config): close Plan-02/01 landing gaps from post-merge review #64 landing-gap note).trusted_proxiesCIDR parsing with an address-space-coverage trust-all guard,http/https-only upstream scheme allowlist inTopologyResolver, prefix normalization ahead of disjointness/containment checks (with the same-prefix route-disjointness check moved into the all-violationsConfigValidatorpass per ADR-0009), stray*.ymlboot failure underendpoints/, sanitized binding-error messages (no resolved scalar, including secrets, is ever logged), explicit YAMLStreamReadConstraints/ alias-expansion limits, and a doc-only fix reconciling the header-matcher disjointness description inconfiguration.adoc.Changes
api-sheriff/src/main/resources/schema/gateway.schema.json,endpoint.schema.json—anchorsmap andanchorrefs (D1)api-sheriff/src/main/java/de/cuioss/sheriff/api/config/model/AnchorConfig.java(new),GatewayConfig.java,EndpointConfig.java,RouteConfig.java,ResolvedRoute.java— anchor-aware config model (D2)api-sheriff/src/main/java/de/cuioss/sheriff/api/config/RouteTableBuilder.java— anchor resolution chain and effective-posture logging (D2)api-sheriff/src/main/java/de/cuioss/sheriff/api/config/validation/ConfigValidator.java— seven anchor validation rules (D3)api-sheriff/src/main/java/de/cuioss/sheriff/api/config/load/ConfigLoader.java,EnvSecretResolver.java(new) —${VAR}substitution pass and pre-substitution secrets classification (D4)api-sheriff/src/main/java/de/cuioss/sheriff/api/config/topology/TopologyResolver.java,EndpointEnablementResolver.java— env-override machinery deleted; scheme allowlist and CIDR trust-all guard added (D4, D5)api-sheriff/src/main/java/de/cuioss/sheriff/api/config/ConfigLogMessages.java— new effective-postureLogRecordapi-sheriff/src/main/java/de/cuioss/sheriff/api/quarkus/ConfigModelReflection.java,ConfigProducer.java— native-reflection and CDI wiring for the new modelapi-sheriff/src/test/**— unit tests for the above, includingConfigFailFastTest.java(squatter-route boot refusal)api-sheriff/src/test/resources/config/anchor-squatter/**,anchored/**— new anchor-violation and anchored fixturesintegration-tests/src/main/docker/sheriff-config/**,integration-tests/src/test/java/.../ConfigLoadedIntegrationIT.java,integration-tests/scripts/verify-invalid-config-fails.sh— anchor-block sample configs and anchor-violation IT variantdoc/adr/0007-anchor-scoped-policy.adoc— flipped toAccepteddoc/configuration.adoc,doc/LogMessages.adoc— Anchors/validation-rules documentation and the new LogRecord entryTest Plan
verify -Ppre-commit)Related Issues
None.
Generated by plan-finalize skill
Summary by CodeRabbit
${VAR}/${VAR:-default}placeholder support and tightened placeholder validation across config and topology..ymlfiles.