feat(config): allow a single env var to supply an array-of-string key - #254
Conversation
ConfigLoader.coerce gains one `case "array"` arm, so a resolved ${VAR} at a
schema-declared array pointer splits on `,` into the whole list rather than
reaching the binder as a single scalar. Each element is stripped and typed from
the destination's declared item type, so `forwarded.trusted_proxies` — the CIDR
allow-list ADR-0003 requires before any forwarding header is believed — is
supplyable in full from the environment, and its cardinality is no longer fixed
in the mounted file.
The rule is un-gated: the arm is selected by the declared `array` type alone,
with no pointer test and no item-type predicate, so it reaches every array-typed
pointer the bundled schema declares. At the one array-of-object pointer
(token_validation.issuers) it is reached and refused loudly by schema
validation, never silently skipped.
An empty resolution is refused at the substitution site rather than by later
validation. The schema declares no minItems on these pointers and an explicit
`[]` stays legal, so a validator reading the bound list cannot distinguish "the
operator wrote []" from "a variable was unset and swallowed"; substitution can.
The refusal is conditioned on an array destination — a placeholder resolving to
empty at a scalar destination is unchanged. The recorded ConfigError names the
pointer and the rule and never echoes the resolved value, which at an allow-list
pointer is topology intelligence.
The private declaredScalarType is renamed to declaredType, since it now
legitimately returns "array"; its javadoc and the one documentation consumer
(doc/quality-report/code-correctness.adoc, finding CFG-17) are corrected in the
same change, with CFG-17's cited line ranges re-derived against the new file.
Tests: six loader cases covering the list substitution, element stripping, the
un-gated radius at a pointer this change never names, the empty and defaulted-
empty refusals, and the no-echo discipline; plus a full-boot effect nest in
GatewayEdgeRouteTest that loads a ${VAR}-bearing gateway.yaml through the real
loader, builds the real edge, and asserts behaviourally through the TcpPeerGate
that a peer inside the supplied CIDR set has its X-Forwarded-For honoured while
a matched control differing only in the environment value does not.
Co-Authored-By: plan-marshall <noreply@cuioss.de>
…environment
The sample's gateway.yaml gains a `forwarded` block declaring trusted_proxies as
the bare ${SHERIFF_TRUSTED_PROXIES}, and docker-compose.yml supplies the value in
the same literal KEY=value form its neighbouring deployment-door entries use. An
operator can now see the whole story in two files: what to set, where to set it,
and that setting nothing fails the boot loudly rather than starting with an empty
allow-list.
The bare form carries no `:-` default on purpose — the same rule
oidc.client_secret follows. A committed default for a trust allow-list is a trust
decision written into a document operators copy, and it would reintroduce exactly
the silent fallback the empty-resolution boot failure refuses.
The committed sample value is Docker's default bridge address pool, and the
comment says plainly that it is a shape rather than a recommendation: this stack
publishes only 8443 off-host and puts no proxy in front of the gateway, so a real
deployment lists its proxies' exact addresses instead.
ComposeSampleForwardedTrustWiringTest is what makes the pairing an executing
assertion rather than a review item. `deployment` is a packaging=pom module that
builds and tests nothing, so a sample that stopped loading would ship green; this
guard follows the module's four established no-Docker surefire wiring guards and
drives the real ConfigLoader over the shipped configuration directory end to end —
parse, substitute, schema-validate, bind. Three assertions, each derived from the
committed files and fronted by a vacuity guard: the loader binds
forwarded().trustedProxies() to the list the compose file supplies; every ${VAR}
the sample references is a key of that same block, walked over the parsed
document's values so a placeholder written in prose is documentation and not a
reference; and the trusted_proxies text is the bare placeholder with no default.
The .env pin is deliberately NOT bumped — it names an image that exists, which is
the rule that file states about itself. The 0.1.1 release predates the loader's
list-valued substitution, so the compose comment documents the version skew and
the API_SHERIFF_IMAGE=api-sheriff:distroless local-build override needed to
exercise the sample until 0.2.0 ships.
Co-Authored-By: plan-marshall <noreply@cuioss.de>
…e radius
Three touch points in doc/configuration.adoc, all beside existing
trusted_proxies text so the contract is found where an operator already looks.
The annotated YAML sample shows the ${VAR} form alongside the literal list: one
variable carries the whole list, the bare form is required, an empty resolution
fails the boot, and the presence of the forwarded block itself still needs a file
edit. The `forwarded` field-reference row states the substitution as a property
of the field — comma-separated, elements stripped, an empty resolution refused at
the substitution site rather than by CIDR validation, and an explicit [] still
legal and still meaning "no proxy is trusted". The boot-rules list gains the
empty-substitution rule as a peer boot rule.
The radius is stated once, and at the radius the code actually has rather than
the narrower one it would be comfortable to claim. The rule keys on the declared
`array` type alone, so it reaches all 18 array-typed keys the bundled schema
declares. The 17 array-of-string ones are enumerated. The eighteenth,
token_validation.issuers, is an array of objects: it is named as reached and
refused by schema validation rather than silently skipped, because a reader told
the rule covers "array-of-string keys" would reasonably conclude it is skipped,
and it is not. tls.passthrough_sni is named explicitly as NOT a member — it is an
object map, not an array.
A closing bullet keeps ADR-0025's classification intact: trusted_proxies is
deployment-bound and the intended user, while tls.cipher_suites and tls.alpn stay
policy — mechanically reachable by the uniform rule, and that reachability is
stated as not an endorsement, since silence there would read as permission.
doc/user/environment-variable-overrides.adoc is deliberately NOT edited. It is
being reworked concurrently by its owner; the contract text it owes is specified
in the plan's outline for them to land, and this commit's footprint is this one
file.
Co-Authored-By: plan-marshall <noreply@cuioss.de>
…ng test The quality gate's formatter normalizes the import block in the forwarded-trust wiring test that this branch introduced. Keeping the rewrite rather than reverting it, since the branch authored the file. Co-Authored-By: plan-marshall <noreply@cuioss.de>
… schemas
The self-review found one root cause behind three statements: the
reachable-array surface was enumerated from gateway.schema.json alone.
ConfigLoader runs the same substitute() pass over gateway.yaml with
gatewaySchemaTree (:217) and over each endpoints/*.yaml with
endpointSchemaTree (:281), so the endpoint schema is equally in radius.
doc/configuration.adoc claimed closure over 'the 18 array-typed keys the
bundled schema declares'. 18 is exactly gateway.schema.json's array-node
count, which is the evidence the second schema was never walked. The
endpoint schema adds six further string-item keys reachable from an
endpoint document -- routes[].match.methods, the routes[].forward
headers_allow/headers_deny/query_allow/query_deny lists, and
routes[].websocket.allowed_origins -- plus two more object-item keys,
endpoint.routes and routes[].match.headers.
This mattered beyond bookkeeping: the omitted keys are header allow/deny
lists and a WebSocket origin allow-list, and the adjacent 'Reachable is
not recommended' guidance is written about the reachable surface. An
operator reading it did not learn those controls are environment-
supplyable. That guidance now names them as security controls.
The bullet also now points at the two schema files as the authoritative
source rather than standing alone as a closure claim, and corrects 'the
eighteenth' to the three object-item members.
coerce()'s javadoc carried the same claim in production ('the bundled
schema', 'the one whose items are objects') while line 150 of the same
file already said 'schemas' correctly; both halves are corrected.
ComposeSampleForwardedTrustWiringTest's DisplayName claimed 'every VAR
the sample references' while the scan read gateway.yaml only. Widened the
scan to every document the loader substitutes rather than narrowing the
name -- endpoints/demo-api.yaml carries no placeholder today, so
narrowing would have left a future endpoint placeholder unchecked while
the test stayed green, which is the boot failure this test exists to
prevent. The new substitutedDocuments() helper mirrors
ConfigLoader.listEndpointFiles's .yaml filter and name ordering.
Co-Authored-By: plan-marshall <noreply@cuioss.de>
…ing test's scope claim Second self-review round. Four findings, two causes. Three were stale ConfigLoader.java line anchors in the code-correctness report, all broken by this branch: 90c42f1 shifted the file by +9 and +93, and 4f58816 added a further +2 that re-broke the very range 90c42f1 had just corrected. CFG-2 and CFG-3 were never re-derived at all -- 90c42f1's message called CFG-17 'the one documentation consumer', but these two cite the same file. Re-deriving the numbers would only reset the clock, so all three Location fields now name the methods instead: withinExpansionLimits, buildMapper, hardenedYamlFactory (CFG-2); withinExpansionLimits/composeAll and readYaml/readTree (CFG-3); substitute and declaredType (CFG-17). That removes the drift class at these sites rather than deferring it. Scoped to exactly the three entries this branch invalidated -- the other ~30 entries cite files this branch does not touch and keep the existing line-number convention. The fourth was mine, introduced by the previous round. Widening the scan to endpoints/*.yaml, I also widened the assertion message from 'derived from GATEWAY_YAML' (narrow, true) to 'any document under CONFIG_DIR' (universal, false): topology.properties sits there too. The right resolution is deleting the over-claim, not widening again -- topology.properties is substituted by TopologyResolver under a different rule where a ${VAR:-default} is legitimate, so the sample's ${TOPOLOGY_UPSTREAM:-http://demo-api:8080} is correctly absent from the compose environment block, and pulling it in would demand a variable the sample is right not to supply and turn the test red. The DisplayName and both assertion messages now say 'YAML config documents', and substitutedDocuments() carries a javadoc paragraph recording why topology.properties is excluded, so the boundary reads as deliberate rather than as an oversight. Co-Authored-By: plan-marshall <noreply@cuioss.de>
… it did not check
VARIABLE_REFERENCE matches ${VAR:-default} as well as the bare ${VAR}
-- the trailing [^}]* swallows the ':-default' -- so the captured name
set includes defaulted placeholders. The failure message nonetheless
asserted 'The placeholder carries no default, so this stack fails its
boot', which is true only for the bare form. A defaulted placeholder
added later would fire this message with a claim that is false for that
input, and would misdirect the reader toward a boot failure that would
not occur.
The message now states the bare-form consequence as the bare-form
consequence and gives the actual reason the sample supplies its
defaulted placeholders too: so every externally-bound value is readable
from the compose file rather than only from the document. The remedy
sentence is unchanged -- adding the variable, not a default, is right
either way.
Round 3 of the self-review declined to file this under the
blocking-only bar I set for that round, and correctly so. Fixed here
because it is a branch-authored false claim in a diagnostic, the fix is
one message, and it needs no further review round to verify.
Co-Authored-By: plan-marshall <noreply@cuioss.de>
… to unreachable addresses
The sample shipped SHERIFF_TRUSTED_PROXIES=172.16.0.0/12 — Docker's whole
default address pool for bridge networks — while the comment two lines
above forbade exactly that: 'never a whole bridge range, and never a
range wide enough to include the clients themselves'. A sample is a
template, and the copied artifact is the value, not the paragraph.
Three routes put a caller inside that trusted set, and only one of them
is platform-specific. On the Linux docker-proxy path a host-originated
connection reaches the container from the bridge gateway address; on
Docker Desktop every client, remote ones included, arrives from the VM
gateway; and unconditionally, on every platform, the co-located keycloak
and demo-api containers hold 172.x addresses on the same bridge, so
either one — if compromised — could state its own address via
X-Forwarded-For. Port 8443 is published on 0.0.0.0.
There was no runtime signal either: ConfigValidator's BROAD_PREFIX_IPV4
is 8, so a /12 passes the breadth check without even a warning.
Ships 192.0.2.10/32,192.0.2.11/32 instead — RFC 5737 TEST-NET-1,
reserved for documentation, never routable, and never allocated by
Docker's default bridge pools. Empty was not an option: the bare ${VAR}
form refuses an empty resolution by design, so the sample must name
something, and naming something unreachable is the only value that is
both loadable and correct. The two /32 host routes also demonstrate the
shape the comment prescribes and keep the multi-element list that
exercises this plan's comma-split.
The comment now explains why the addresses are deliberately unreachable
and names the three leak routes concretely, rather than stating a rule
the shipped value broke.
Nothing depended on the old value: the wiring test derives its
expectation from the committed compose file rather than mirroring a
constant, .env does not set the variable, and release.yml's smoke test
mounts a different config directory.
Co-Authored-By: plan-marshall <noreply@cuioss.de>
Tier-0 deterministic refresh against the committed origin/main baseline. discover --force folded the per-module enriched descriptions for api-sheriff-build-parent and my-gateway into _project.json, where the committed baseline carried empty strings, and updated both tree_shas. descriptor-regression-check reports regressive: false — no curated name or description was blanked. Co-Authored-By: plan-marshall <noreply@cuioss.de>
There was a problem hiding this comment.
Sorry @cuioss-oliver, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 3 days and 3 hours by commenting @sourcery-ai review. Upgrade to get a review now.
PR Reviewer Guide 🔍
|
📝 WalkthroughWalkthroughChangesArray substitution and forwarded trust
Architecture metadata refresh
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: 🟡 Moderate · up to The standard compose sample cannot boot with its default image, so the sample should be made compatible or deferred before merge. 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.plan/project-architecture/_project.json (1)
55-55: 📐 Maintainability & Code Quality | 🔵 TrivialKeep the current reactor and CI exclusions.
build-parent/example/pom.xmlis outside the reactor, CI does not build it automatically, anddoc/user/downstream-parent.adocdocuments the manual JVM and native build procedures.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Team
Run ID: 8b9dcbfb-95a3-404b-8557-9a8721fea98b
📒 Files selected for processing (9)
.plan/project-architecture/_project.jsonapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/load/ConfigLoader.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/load/ConfigLoaderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javadeployment/compose-sample/docker-compose.ymldeployment/compose-sample/docker/sheriff-config/gateway.yamldoc/configuration.adocdoc/quality-report/code-correctness.adocintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/ComposeSampleForwardedTrustWiringTest.java
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Triage dispositionsIn reply to comment_id:
|
PR #254 added ForwardedTrustFromEnvironment after this branch forked, carrying two bare `listen(0)` binds and a local LOOPBACK = "127.0.0.1" constant. Both are the exposure class this branch exists to remove: the listeners bind the dual-stack wildcard while the fixture dials loopback. Found by this branch's own ArchUnit guard, which CI ran against the merge result. That is the guard working as intended on code the branch never saw — the sites are invisible from the branch tip and appear only once the two histories are combined. Routes the local constant through LoopbackHost.ADDRESS rather than leaving a second spelling of the same address in the tree. Co-Authored-By: plan-marshall <noreply@cuioss.de>
The build-gate page said the fix converted "All 20 such sites across 9 files". It converted 22: PR #254 landed two more bare listen(0) sites in GatewayEdgeRouteTest after this branch forked, and rebasing brought them in. Re-derived from the tree rather than trusted — 22 host-bound listen calls across the nine fixture files, excluding the specimens this branch added and the two occurrences that are Javadoc text rather than bind sites. This one matters more than the earlier stale counts: it is the claim a reader checks the deliverable against, and it UNDERSTATED the work, so a reader verifying it would have found more sites than the page admits to and had no way to tell whether the surplus was intended. Second, from the simplify re-run: the wrapped-line assertion's message said joining the specimen "leaves five violations" while the count assertion three lines above asserts four. A leftover from the round that briefly had five specimens. No coverage change. Co-Authored-By: plan-marshall <noreply@cuioss.de>
Intent
ConfigLoader.coercegains onecase "array"arm so a single resolved${VAR}splits into thelist its destination pointer declares. That makes
forwarded.trusted_proxies— the CIDR allow-listADR-0003 requires before any forwarding header is believed — supplyable in full from the
environment rather than one fixed slot at a time.
The rule is uniform: it keys on the declared
arraytype alone, with no pointer gate and noitem-type predicate. An empty resolution is refused at the substitution site
(
ConfigLoader.substituteChild), because validation provably cannot distinguish an emptysubstitution from the explicit
[]the schema still permits — an operator who wires a variable andmistypes its name gets a loud boot failure rather than a silently disarmed control.
The mechanism is proven twice: a substitution test with its empty-resolution negative control, and
a full-boot effect test that loads a
gateway.yamlcarrying${VAR}, constructs the realGatewayEdgeRoute, and asserts behaviourally through theTcpPeerGate— because a configurationkey that parses is not a configuration key that acts.
Deliverables
ConfigLoader— the list-valued${VAR}substitution arm plus the empty-resolution refusal.doc/configuration.adoc— the uniform contract documented.Accepted trade-off — the compose sample's default path is broken until 0.2.0
CodeRabbit raised this as Major and it is correct on the facts, so it is recorded here rather
than argued away. This PR adds the
forwarded:block to the sample'sgateway.yaml— it did notexist on
main— and.envpins the0.1.1release, whose loader predates the array arm. Afterthis merges, the standard
./scripts/start-sample.shpath fails its boot until0.2.0ships. Thenew wiring test drives the source-tree
ConfigLoader, so by construction it cannot catch areleased-image mismatch.
It is accepted rather than deferred because the alternatives each break something else: bumping
.envahead of the release would violate the rule that file states about itself (it names an imagethat exists), and keeping the sample compatible with
0.1.1would mean not demonstrating thefeature the sample exists to demonstrate. The skew is documented in place at
docker-compose.yml:159-169with the exact override command(
API_SHERIFF_IMAGE=api-sheriff:distroless ./scripts/start-sample.sh), and the trade-off wassurfaced and recorded at outline time rather than discovered at review.
This is the one item in this PR worth a second opinion before merge.
Notes from finalize
Three things surfaced in the finalize gates, each correcting a claim that was wrong rather than
merely absent.
The documented blast radius was derived from one schema of two. The contract originally claimed
closure over "the 18 array-typed keys the bundled schema declares". 18 is exactly
gateway.schema.json's array-node count — which is the evidence the second schema was neverwalked.
ConfigLoaderruns the samesubstitute()pass over eachendpoints/*.yamlagainstendpoint.schema.json, which adds six further string-item keys (routes[].match.methods, theroutes[].forwardheaders_allow/headers_deny/query_allow/query_denylists, androutes[].websocket.allowed_origins) plus two more object-item keys. Those omitted keys are headerallow/deny lists and a WebSocket origin allow-list, and the adjacent "Reachable is not recommended"
guidance is written about the reachable surface — so an operator reading it would not have learned
those security controls are environment-supplyable. The bullet now covers both schemas, names the
security controls, and points at the two schema files as authoritative rather than standing alone as
a closure claim.
The compose sample trusted the local host and every co-located container. It shipped
SHERIFF_TRUSTED_PROXIES=172.16.0.0/12— Docker's whole default bridge pool — while the comment twolines above forbade exactly that. Three routes put a caller inside that trusted set, and only one is
platform-specific: on Linux the docker-proxy path presents host connections from the bridge gateway;
on Docker Desktop every client arrives from the VM gateway; and unconditionally, the co-located
keycloakanddemo-apicontainers hold172.xaddresses on the same bridge.ConfigValidator'sBROAD_PREFIX_IPV4is 8, so a/12passed the breadth check without even a warning. The sample nowships
192.0.2.10/32,192.0.2.11/32— RFC 5737 TEST-NET-1, reserved for documentation and neverallocated by Docker's pools. Empty was not an option: the bare
${VAR}form refuses an emptyresolution by design, so the sample must name something, and naming something unreachable is the
only value that is both loadable and correct.
Two clarifications on where the refusal actually matters. For
trusted_proxiesan empty list isnot fail-open —
TcpPeerGate.isTrustedPeerreturnsfalseon an empty set, so an emptiedallow-list trusts nobody; the refusal's value there is intent-preservation. The genuine fail-open
case is at the endpoint deny-lists, where
ForwardPolicyStagetreats an empty deny set asforward-all. The refusal covers both on the same branch.
Two structural properties bound the radius: substitution runs before validation, so every schema
constraint and post-bind guard applies to environment-supplied values identically; and substitution
can only re-value a key the committed document already declares — an environment variable cannot
introduce
headers_denyor flip a route to forward-all.Follow-ups filed, not fixed here
ConfigValidator.BROAD_PREFIX_IPV4 = 8lets a/12trust range boot with no warning. Outside thisplan's footprint, and changing a guard's threshold inside a security-audit pass is the wrong place
— but this plan changed its risk profile, since the value it governs now lives in an environment
variable no code review reads.
GatewayEdgeRouteTestuses wildcardlisten(0)binds, matching four pre-existing sites in thesame file. This lines up with the recorded macOS loopback-stall diagnosis whose fix is
listen(0, "127.0.0.1"); correcting it is a six-site sweep beyond this change.Verification
Whole-tree
verify -Ppre-commitand fullverifyboth green at the final HEAD — 202 tests, cleantree, zero warnings under
-Werror. Coverage run green. CI green. Sonar reports 0 new-code issues(
count_status: confirmed). The pre-submission self-review ran three rounds (7 findings, allresolved) and converged clean; the simplification sweep found no surplus; the security audit found
and fixed the compose-sample trust-range defect above.