fix(security): settle carried security debt before the 0.1.0 cut - #154
Conversation
ConfigLoader opened every configuration file twice: once in the compose-only alias/nesting-bomb pre-pass and again for the Jackson bind. The two reads were not guaranteed to observe the same bytes, so the document that passed the bomb check was not provably the document that got bound (a check-then-act window). Neither read had a total-byte cap — MAX_YAML_STRING_LENGTH bounds a scalar's code points, not the file. Each file is now read exactly once into a byte-bounded snapshot that both passes consume, so the bomb check and the bind observe identical bytes and no window exists between them. withinExpansionLimits takes the snapshot instead of a Path. A file exceeding the new MAX_CONFIG_FILE_BYTES cap is refused as a collected ConfigError, never a fail-fast throw, preserving the loader's collect-all-errors contract. Applies to gateway.yaml and endpoints/*.yaml alike. The cap sits deliberately below MAX_YAML_STRING_LENGTH, SnakeYAML's code-point budget over the whole stream: were it the larger value, the code-point budget would always trip first and an over-sized file would be reported as a bomb rather than as an over-sized file. ADR-10 is preserved — the compose-only pre-pass stays (Jackson's YAMLParser never runs the Composer that counts collection aliases); only the source the two passes read changes. Tests cover the inclusive boundary (a document exactly at the cap binds, one byte over is refused), that the size refusal is the only error reported for the file (no partial bind), that the cap applies to endpoint files too, and that an over-cap alias bomb is refused by size alone — the absence of the alias diagnostic is what pins the single-read contract against a silent reintroduction of the second, unbounded read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RW7E6V8jaYCEssSgqmo2gy
coerce(String) received no destination type, so it inferred a substituted
scalar's JSON type from the resolved string's shape: "true"/"false" became a
BooleanNode and an integer literal an IntNode/LongNode, whatever the field's
declared type. A schema-declared string field whose ${VAR} happened to resolve
to "true" or "123" was therefore silently retyped — and then refused by schema
validation as a type mismatch, so an operator whose client id was literally
"123" could not boot, for no reason but how the value looked.
The coercion now consults the type the bundled JSON Schema declares at the
value's own JSON pointer. The pointer that substitute/substituteChild already
compute is threaded into the decision, and the schema resource is parsed a
second time into a plain tree that the resolver walks through properties,
patternProperties, additionalProperties, items, and local $refs (bounded by
MAX_SCHEMA_REF_HOPS). A schema-declared string stays a TextNode whatever it
resolves to; shape inference survives only where the schema pins no single
scalar type — an absent type keyword, a union, or a pointer it does not
describe.
A value that cannot carry its declared type is deliberately left as text rather
than force-fitted: schema validation then refuses it with a diagnostic naming
the expected and actual types, never the value. That is what keeps a resolved
scalar — which may be a secret — out of every collected ConfigError, and it is
pinned by a test asserting the refusal never echoes the resolved value.
Tests cover the core case (a schema-string field resolving to true/false/TRUE/
123/-7 binds as a string), the matched positive control (a schema-boolean field
still coerces, so the fix cannot regress into disabling coercion outright), the
fail-closed integer leg, and an array-item pointer so the walk is pinned to
descend `items` and not just object properties.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW7E6V8jaYCEssSgqmo2gy
On a validator-resolution failure the readiness probe wrote String.valueOf(failure.getMessage()) into the payload's `error` datum, placing a raw internal cause — which can name issuer URLs, internal hostnames, TLS/trust material and filesystem paths — onto an endpoint served on the management port. That port is not a safe place to assume: Quarkus' management interface declares no ssl-port and no insecure-requests key, so it has exactly ONE port, and the documented opt-out can legitimately make it plain HTTP (ADR-0025). The payload must therefore be treated as reachable by anything that can reach the port. Readiness owes the caller a state, not a cause. The datum is now the fixed token `validation-unavailable` (the `jwks` datum already carries `unavailable`), and the full cause reaches the operator through a new WARN ApiSheriff-116 instead. The raw message is removed outright — no deprecation marker, no transitional alias — per the plan's breaking compatibility. The new LogRecord's template deliberately takes no parameters: the cause travels as the logged exception, so no formatted fragment can drift into carrying the detail itself. Documented in doc/LogMessages.adoc, and doc/architecture.adoc now states the state-never-cause contract for the readiness payload. Two read-only consults backed the change: ManagementPlainHttpAudit confirms the single-port plain-HTTP reachability premise, and DefaultProfileReadinessTest asserts probe state only, so it carries no assertion on the error datum. SheriffMetricsTest.downWhenValidatorFails was the one pre-existing assertion expecting the raw message; it now pins the fixed token. A negative control is added alongside it, sweeping EVERY datum on the payload — not just `error` — for fragments of a deliberately disclosive failure message, so a future change that moved the cause onto another datum would still be caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RW7E6V8jaYCEssSgqmo2gy
verify-invalid-config-fails.sh covered six invalid configurations, all of them refused by ConfigProducer while the route table is produced. It carried no case for the JWKS seam, so the boot-time constructibility contract that TokenValidatorProducer.onStartup forces into existence (ADR-0027) had no end-to-end regression coverage: an issuer whose `source: file` names a path that does not exist must abort startup rather than defer the failure to the first bearer request. Add case 7 for exactly that, and give it the negative leg the other six do not need. Cases 1-6 are refused before any bean that could open a port exists, so "exited non-zero" is already conclusive for them. Case 7 is refused later, from a StartupEvent observer, which is late enough that "did a port open first?" is a real question — a change that moved JWKS assembly behind the listener bind would keep the non-zero exit, keep the marker, and still ship the partial-config serving this script exists to forbid. The negative leg therefore publishes the management port and asserts two things: that the logs never announce a management listener (emitted the instant it binds, so its absence is race-free), and that the published port never answered on either scheme across the whole boot window (the direct on-the-wire observation). Both predicates were checked against a deliberately booting instance and both fire there, so the leg is capable of failing rather than merely green. The marker is the fixed "Cannot read JWKS file" sentence without the path token-sheriff appends, holding this case to the same discipline as the others: assert on the fixed diagnostic, never on the rejected scalar. The probe port sits outside the 19000-19005 block docker-compose.yml publishes, so the script still runs against a live integration stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RW7E6V8jaYCEssSgqmo2gy
BackchannelLogoutEndpoint.extractLogoutToken (java:S135): fold the two continue guards into a single positive match test so the parameter scan has one loop exit. Behaviour is unchanged and still fails closed - an unnamed pair, a malformed percent-encoded name and a non-logout_token name are all simply unmatched, so the scan falls through to an absent value and the caller answers 400. The unauthenticated reserved path still logs at DEBUG. RouteTableBuilder (java:S6539): suppress in code with a recorded rationale rather than extract. The dependencies are data records from one config.model package, not behavioural collaborators - this is the single assembly point mapping that record model onto ResolvedRoute. Extraction was rejected on ADR-0007 (inheritance chains are centralised here, once) and ADR-0009 (normalizePrefix, effectiveAccessLevel and globalProfile are shared seams ConfigValidator resolves through).
Bounds the directory asset read against the cap actually read rather than a pre-sampled size, closing a TOCTOU where a file growing between Files.size and readAllBytes was materialized whole; the directory path now matches the upstream path's mid-flight guarantee. Hoists the duplicated 10 MiB asset cap onto AssetSource as the single derivation seam both implementations read (ADR-0026), and records the fetch-cap/serve-cap pairing invariant with its silent-truncation consequence. Records the disposition for the readiness validation-unavailable branch: retained as the redaction guard and the seam for ADR-0027's open live-JWKS gap, with the fail-closed eager-boot coupling deliberately kept. CI hardening: adds the missing github-actions and docker Dependabot lanes so SHA-pinned actions and the release-gated base image can be updated; stops claude.yml persisting the token for an agent holding Bash(git*); narrows benchmark.yml to contents:read and routes github.sha through env; pins the benchmark fairness backend. Corrects the falsified no-literal-secret claim in bearer_proxied.js, settling it against the realm import that provisions the same throwaway credentials.
…le API TASK-14 closed the T3 coverage gap TASK-012 self-reported (finding b8767f) by reading all nine shipped and infra files in full rather than sampled. Two of the observations were real and are fixed here; the rest are closed with a recorded rationale (findings 0eb9bb, 69222f, f9bce6). The compose sample published the gateway management interface as "9000:9000", which binds every host interface. That port serves /q/health and /q/metrics with no authentication in front of them, so a copied sample — and the file tells the operator it is meant to be copied — exposed an internal-state and metrics view to anything able to reach the host. It is now bound to 127.0.0.1. Both consumers were checked before the change rather than after: wait-for-ready.sh derives its probe against localhost and filters on port.target/port.published, neither of which a host_ip prefix perturbs, and the user guide's documented https://localhost:9000/q/health/ready still resolves. The release workflow drives a bare docker run and never touches this file, so no CI path is affected. The integration harness ran Prometheus with --web.enable-lifecycle while publishing 9090, which exposes unauthenticated POST /-/reload and POST /-/quit — a remote-shutdown control on a service that needs none. Nothing calls either endpoint, and the scrape config is a read-only mount that cannot change while the stack is up, so the flag bought nothing to offset the surface. Both edits carry an inline rationale so the next reader does not restore them. Also records, in the sample's header, why resource limits are set on the gateway only: an unvalidated memory cap on a third-party image is a crash loop rather than a hardening win, and the file's completeness claim now scopes itself to the security directives it genuinely sets on all three services. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RW7E6V8jaYCEssSgqmo2gy
…sweep Two behaviour-preserving cleanups surfaced by the finalize simplify pass. ConfigLoader.coerce carried a `declaredType == null` guard clause whose body was byte-identical to the switch's `default` arm, so the same call was written twice and a future edit to one arm could silently diverge from the other. Both are now collapsed into `case null, default -> inferFromShape(value)`. The private `cappedSource` test helper in DirectoryAssetSourceTest carried javadoc whose `@param`/`@return` restated the signature verbatim without adding intent; it is removed rather than left as noise a reader must check against the code. Four further observations were recorded rather than acted on, each because the fix is wider than a finalize pass should take unreviewed: the UpstreamAssetSource fetcher/maxBytes parameter pair whose agreement cannot be structurally enforced, the assert_fails_to_boot helper's two wait strategies (only executable under -Pintegration-tests, so a refactor could not be verified against any runnable gate), the ADR-0027 rationale block duplicated into GatewayReadinessCheck, and the repeated cooldown block in dependabot.yml. Quality gate and full verify are green on the resulting tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RW7E6V8jaYCEssSgqmo2gy
…sole to loopback Four findings from the finalize security-audit sweep over this branch's own diff. Two are substantive. DirectoryAssetSource re-walked the requested path after confining it. The branch had already added readWithinCap to close a check-then-act window on the asset SIZE, and that javadoc puts a volume-write attacker in model — but the identical window stayed open on a stronger control. realPathWithinRoot resolved the symlink chain, returned a boolean, and discarded the resolved path; Files.size and Files.newInputStream then each re-walked the REQUESTED path and followed the chain again. A chain re-pointed between the check and the open is followed by the read, and the process can reach /app/certificates/localhost.key, so the payoff is the TLS private key. realPathWithinRoot now returns Optional<Path>, serve() drives both the stat and the read from the resolved path, and the open carries LinkOption.NOFOLLOW_LINKS. The resolved path holds no symlink by construction, so legitimate in-root symlinked assets still serve; a post-resolution swap becomes a 500 instead of a silent out-of-root read. The compose sample published the Keycloak admin console as "1443:8443" on every interface while setting KC_BOOTSTRAP_ADMIN_PASSWORD=admin four lines above. That is a strictly worse exposure than the unauthenticated management port this plan had just narrowed to loopback in the same file — it is full control of the IdP that mints every token the gateway trusts. doc/user/compose-sample.adoc documents the console only as https://localhost:1443, so the narrower bind costs nothing. Also narrows the integration-test probe port to loopback, and corrects a ConfigLoader javadoc that asserted a boot-wide byte bound the per-file cap does not actually provide. Two observations are recorded rather than acted on. claude.yml grants id-token: write to a job any commenter can trigger, with Bash(./mvnw*) and Bash(gh*) in allowed_tools — pre-existing, gated upstream by the action's author-association check, and an operator decision rather than a finalize-time edit. And endpoints/*.yaml carries no file-count cap; that would be an operator-visible ceiling on how many endpoints a deployment may declare, so the javadoc was made precise instead of inventing a product constraint. Quality gate and full verify are green on the resulting tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RW7E6V8jaYCEssSgqmo2gy
![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, 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: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 PR adds bounded configuration and asset processing, schema-aware substitutions, redacted readiness failures, shared size limits, and log-catalogue validation. It also tightens workflow permissions, container exposure, benchmark reproducibility, credential consistency, and startup-failure checks. ChangesGateway behavior
Operational controls
Estimated code review effort: 4 (Complex) | ~75 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 🔍(Review updated until commit 6189349)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/main/java/de/cuioss/sheriff/gateway/asset/UpstreamAssetSource.java (1)
115-143: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce the fetch-cap and serve-cap relation.
UpstreamAssetSourcedocuments that the fetch-seam cap cannot be lower thanmaxBytes, but the public six-argument constructor is open to all callers and does not enforce that invariant. A fetcher capped belowmaxBytescan return a truncated body within theserve()size check, so the gateway serves corrupt content as200. Restrict this constructor to package/test scope or add an explicit overflow signal fromUpstreamFetcherthatserve()rejects, after verifying every non-test construction path.Source: Path instructions
🧹 Nitpick comments (2)
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/load/ConfigLoaderTest.java (1)
861-879: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
$refbranch of the destination-type walk.
declaredScalarTypefollowsproperties,patternProperties,additionalProperties,items, and local$refhops, but the schema-string tests only coveroidc.client_id,oidc.client_secret, and theitemscase. Add a placeholder for a schema-string field reached through a local$ref, such asendpoint.auth.require, so that this branch is pinned to destination typing.api-sheriff/src/test/java/de/cuioss/sheriff/gateway/asset/DirectoryAssetSourceTest.java (1)
348-368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReflect the sealed implementation set from
AssetSourceto the assertion loop.
AssetSourceis the declared authority for permitted asset-source implementations. If a new source is added toAssetSource permits ..., this test should cover it automatically rather than requiring another hard-coded assertion.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4708da21-1182-4e26-905e-b1d74863009f
📒 Files selected for processing (23)
.github/dependabot.yml.github/workflows/benchmark.yml.github/workflows/claude.ymlapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/ApiSheriffLogMessages.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/asset/AssetSource.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/asset/DirectoryAssetSource.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/asset/UpstreamAssetSource.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/BackchannelLogoutEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/ConfigLogMessages.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/RouteTableBuilder.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/load/ConfigLoader.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/GatewayReadinessCheck.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/asset/DirectoryAssetSourceTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/asset/UpstreamAssetSourceTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/load/ConfigLoaderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/SheriffMetricsTest.javabenchmarks/src/main/resources/k6-scripts/bearer_proxied.jsdeployment/compose-sample/docker-compose.ymldoc/LogMessages.adocdoc/architecture.adocintegration-tests/docker-compose.benchmark.ymlintegration-tests/docker-compose.ymlintegration-tests/scripts/verify-invalid-config-fails.sh
Triage dispositionsIn reply to comment_id:
|
Close the ancestor-component symlink window on the directory asset read (CWE-367). NOFOLLOW_LINKS constrains only the final path component, so an ancestor directory of a proven-in-root path could still be re-pointed after the check and traversed by the read. DirectoryAssetSource now descends from the real root one confined name at a time through a SecureDirectoryStream and issues both the stat and the bounded read against that descriptor, so no component is looked up by name a second time. Where the platform yields no SecureDirectoryStream the source falls back to the resolved-path read and says so: a once-per-source WARN (ApiSheriff-117) and the javadoc both state that the ancestor window remains open there. No closure is claimed that the code does not deliver. Repair the vacuous shouldBoundTheReadRatherThanTheSampledSize: it wrote 8192 bytes against a 16-byte cap, so serve() returned at the Files.size fast path and the post-read cap check was never entered -- the test passed unchanged against an unbounded readAllBytes. The stat and the bounded read are now one package-visible seam, so a test can drive them apart and reach the check. Enforce the upstream fetch-cap / serve-cap relation structurally rather than by prose: Fetched carries an explicit truncated signal and serve() refuses a truncated fetch with 413, so a fetcher capped below maxBytes can no longer yield a 200 carrying a silent prefix of the asset. Also drop the redundant long cast in the read-limit clamp (java:S1905).
…erage gap Close the ConfigLoader new-code coverage gap that put the PR Sonar gate at new_coverage 74.6 < 80. The destination-type walk had no coverage at all: no test resolved a local $ref, none drove a key described only by patternProperties, and neither numeric arm past the int range was reached, so the whole indirection machinery could have been deleted unnoticed. Add cases for the $ref walk, the patternProperties anchor key and the key no pattern describes, both coerceNumber arms, the unpinned-type shape inference, and the two file-read failure arms. Every one asserts the observable consequence -- a bound value or a collected ConfigError -- never a private method's return. Replace the hand-maintained LogRecord identifier inventory with LogMessagesCatalogueTest, which discovers the catalogues from the compiled output and asserts per-prefix identifier uniqueness and band membership. The inventory was demonstrably unsafe: it named two of the three catalogues, and following it faithfully still lands on 110, which BffLogMessages already owns. The new WARN is 117 as a result. The test was mutation-checked -- colliding an identifier fails it, reverting passes. Add BenchmarkRealmCredentialConsistencyTest so the k6 benchmark credentials and the Keycloak realm import that provisions them can no longer drift into an all-401 run. It fails loudly when a file moves or a value is extracted empty, rather than passing quietly with nothing compared; also mutation-checked. Pin the benchmark fairness backend by content digest. The comment claimed the 1.27 minor tag prevented an upstream image from changing between runs; a minor tag stays mutable, so it did not. The digest is the multi-platform index digest resolved with buildx imagetools and verified by pull. Correct two prose claims this branch authored that the code does not deliver: the Prometheus lifecycle rationale rested on a false read of :ro, and architecture.adoc described readiness as live issuer reachability when the probe reports boot-time constructibility with a lazy JWKS fetch.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/asset/DirectoryAssetSource.java (1)
374-388: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider suppressing a close failure so it cannot replace the walk failure.
Two narrow paths in
descendbehave differently from the rest of the method:
- Line 380: if
dir.close()throws, the already-openedchildstream leaks, becausediris not yet reassigned.- Line 384: if
dir.close()throws, thatIOExceptionpropagates instead ofwalkFailure, so the refusal reason a swapped component produced is lost.Both require a filesystem that is already failing, so this is hardening rather than a defect.
♻️ Proposed hardening
private static ConfinedAsset descend(SecureDirectoryStream<Path> top, Path relative) throws IOException { SecureDirectoryStream<Path> dir = top; try { for (int i = 0; i < relative.getNameCount() - 1; i++) { SecureDirectoryStream<Path> child = dir.newDirectoryStream(relative.getName(i), LinkOption.NOFOLLOW_LINKS); - dir.close(); + closeQuietly(dir); dir = child; } } catch (IOException walkFailure) { - dir.close(); + closeQuietly(dir); throw walkFailure; } return new DescriptorAsset(dir, relative.getFileName()); } + + /** Closes a descent stream without letting its failure replace the reason the descent ended. */ + private static void closeQuietly(SecureDirectoryStream<Path> dir) { + try { + dir.close(); + } catch (IOException closeFailure) { + LOGGER.debug("closing a descent descriptor failed", closeFailure); + } + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e4163e8-bd97-4900-9f3d-f1f333219394
📒 Files selected for processing (16)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/ApiSheriffLogMessages.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/asset/DirectoryAssetSource.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/asset/UpstreamAssetSource.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/BffLogMessages.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/ConfigLogMessages.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/LogMessagesCatalogueTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/asset/DirectoryAssetSourceTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/asset/UpstreamAssetSourceTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/load/ConfigLoaderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.javabenchmarks/src/main/resources/k6-scripts/bearer_proxied.jsdoc/LogMessages.adocdoc/architecture.adocintegration-tests/docker-compose.benchmark.ymlintegration-tests/docker-compose.ymlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BenchmarkRealmCredentialConsistencyTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- benchmarks/src/main/resources/k6-scripts/bearer_proxied.js
- integration-tests/docker-compose.yml
- doc/LogMessages.adoc
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/ConfigLogMessages.java
Sonar java:S2095 (BLOCKER) on the descriptor-relative descent added for the ancestor-symlink fix. The leak was genuine and had two arms: the child SecureDirectoryStream was stranded whenever closing its parent threw, because nothing owned it between the open and the assignment; and the catch handler then called close() a second time on the very stream whose close had just failed, while the child stayed unreachable. Cleanup moves to a finally, and the child is adopted into `dir` before the parent is closed, so exactly one stream is owned at a time and ownership transfers to the returned handle only once that handle exists. The syscall order is byte-for-byte unchanged — open the child from the parent's descriptor with NOFOLLOW_LINKS, then close the parent — so no component is ever re-looked-up by name from the root and the walk's confinement guarantee is untouched. What changed is only who closes what on which path. The descend javadoc now states the one-stream-at-a-time ownership invariant rather than the wrong claim it carried. Also clears java:S1130 (an unthrowable `throws Exception` on shouldServeAssetBehindInRootAncestorSymlink) and java:S6213 (a local named `record`, a Java restricted identifier, in LogMessagesCatalogueTest). The fourth reported issue, java:S1905, needed no edit: the redundant `(long)` cast was already removed by commit 131aa06, so the Sonar snapshot that reported it predates HEAD. No edit was fabricated to satisfy it. Quality gate and full verify are green on the resulting tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RW7E6V8jaYCEssSgqmo2gy
…tests The read-limit clamp in DirectoryAssetSource prevented the overflow it documented and created a second, undocumented outcome in its place. With a maxBytes above Integer.MAX_VALUE - 1 the clamp made readLimit() return Integer.MAX_VALUE, so the read could stop before the asset ended while body.length stayed at or under maxBytes -- the post-read check passed and serveConfined answered 200 carrying a prefix of a larger asset. That is the same silent-truncation failure UpstreamAssetSource refuses through Fetched.truncated(), and the five-argument public constructor is open to any caller, so the value is reachable rather than theoretical. A byte[] cannot exceed Integer.MAX_VALUE, so such a cap is unenforceable by construction and is now refused where it is supplied instead of degrading to a prefix; the narrowing in readLimit() is exact because the invariant holds by construction. Three tests asserted less than they documented. The catalogue-discovery guard derived both sides of its equality from the same walk, so it compared the walk against itself, and its only independent anchor was a hand-maintained count of a set defined elsewhere -- after a fourth catalogue is added, a rename that drops one out of the walk still leaves three and every check passes over the shrunken set. The expected set is now derived from the module's own source tree at run time, so a catalogue the walk stops seeing fails the test and a new one is expected the moment its file lands. Two ConfigLoader tests matched only on the file name, a predicate any collected error satisfies, and are now pinned to the pointer and the message of the arm each one documents. The benchmark-credential guard promised that every compared value is proven present before the comparison, but implemented that on the k6-script side only: the realm side ran through String.valueOf, which turns an absent field into the ordinary-looking literal "null". The realm-side extractions now go through an accessor that refuses an absent or blank field by name, and the two remaining call sites were match predicates rather than extractions, so they compare the raw value instead. Also: an abandoned descent descriptor is now released quietly, so a failing close cannot supersede -- and thereby discard -- the refusal a re-pointed path component produced; and two readiness claims in architecture.adoc that overstated the code are corrected, since the issuer_reachability datum is emitted only in BFF server mode and onStartup forces contextual-instance creation through a proxy method call rather than an Instance.get(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RW7E6V8jaYCEssSgqmo2gy
Triage dispositionsIn reply to comment_id:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/asset/DirectoryAssetSource.java (1)
465-473: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDenial of Service (CWE-400): Uncontrolled Resource Consumption
Reachability: External
Use a non-blocking, descriptor-bound open for asset reads.
serve(),serveConfined(), andDescriptorAsset.read()do not re-check the final component type between theisRegularFile()check and the read channel open. A writeable asset volume can replace that component with a FIFO after the check;DIR_NOFOLLOW/NOFOLLOW_LINKSstop final symlinks but accept FIFOs, and the standard Java NIO read path can block until the writer closes the FIFO. Use a non-blocking descriptor-bound open with file-type rejection on that descriptor, or restrict assets to an immutable trusted writer model.Source: Path instructions
integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BenchmarkRealmCredentialConsistencyTest.java (1)
136-161: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDerive the mirrored credential set from the source.
mirroredcontains exactly four locally specified values, so Line 155 checks only the test's own list. It cannot detect a new mirrored credential added tobenchmarks/src/main/resources/k6-scripts/bearer_proxied.jsorintegration-tests/src/main/docker/keycloak/benchmark-realm.json.Discover the fallback declarations from the k6 source, or compare the discovered source set with the realm set. Do not maintain a separate fixed list and count.
As per path instructions: a hardcoded list that must mirror a set defined elsewhere must be derived from that source to prevent drift.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a941d07a-95ac-4aa5-b8b1-d614acfe863e
📒 Files selected for processing (6)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/asset/DirectoryAssetSource.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/LogMessagesCatalogueTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/asset/DirectoryAssetSourceTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/load/ConfigLoaderTest.javadoc/architecture.adocintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BenchmarkRealmCredentialConsistencyTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- doc/architecture.adoc
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/asset/DirectoryAssetSourceTest.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/load/ConfigLoaderTest.java
The type test in serve() is taken on a path, from the filesystem root, before the descent even begins, so it reports what the entry was rather than what the read is about to open. NOFOLLOW_LINKS narrows that gap for symlinks only: it accepts a FIFO, a FIFO stats as size 0 and so clears the cap pre-check, and opening one for reading blocks until a writer appears -- a stalled request thread instead of a served asset, reachable by anything with write access to the asset volume. Both asset handles now take their stat through one helper that refuses a non-regular entry, which moves the authoritative type decision onto the descriptor the read will use and immediately before it. That narrows the window to the stat-to-open interval; it does not close it, because closing it needs a non-blocking open the JDK's NIO surface does not expose, so the residual is recorded as the deployment-posture matter it is rather than left implied by the presence of a check. The byte cap validated only its upper bound. A negative cap constructed successfully and answered 413 to every file including an empty one, which points the operator at the assets rather than at the configuration that is actually wrong; it is now refused alongside the unenforceable upper values, with zero left valid as the degenerate but coherent serve-nothing cap. Two guards compared less than they claimed to. The catalogue comparison matched on simple names, which erases the package identity it exists to check -- a catalogue the walk lost would pass whenever another package held a compiled *LogMessages of the same simple name -- so both sides are now binary names. And the mirrored-credential guard checked a fixed list of four against itself while the k6 script already declared five such constants, so a newly mirrored credential was covered by nobody; the set is now discovered from the script and the count assertion is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RW7E6V8jaYCEssSgqmo2gy
Triage dispositionsIn reply to comment_id:
|
|
/review |
1 participant
Summary
Settles the carried security debt ahead of the 0.1.0 cut. Each deferred finding either lands as a fix or is closed with a rationale recorded against the actual code, tied together by a T3 security sweep of the shipped gateway surface. Per the plan's disposition ruling, the sweep is not report-only — every finding it surfaced is fixed in this PR rather than deferred to a follow-up issue.
Two outcomes are stated plainly below rather than glossed: deliverable 4 was re-scoped by operator decision after its target proved structurally unreachable, and two carried
java:S5738rows were not fixed because the migration does not compile on the pinned Quarkus version.Changes
Config loading —
ConfigLoader.javaReadiness probe —
GatewayReadinessCheck.java, withApiSheriffLogMessages.java,ConfigLogMessages.java,SheriffMetricsTest.java,doc/LogMessages.adoc,doc/architecture.adocBoot fail-fast coverage —
integration-tests/scripts/verify-invalid-config-fails.shAsset path handling —
DirectoryAssetSource.java,UpstreamAssetSource.java,AssetSource.java, withDirectoryAssetSourceTest.java,UpstreamAssetSourceTest.javaDirectoryAssetSource: the path was resolved and validated, then re-opened for the read. The finalize-stage security audit caught a second instance on the same read path, fixed in the final commit.Sonar carried findings —
BackchannelLogoutEndpoint.java,RouteTableBuilder.javajava:S135fixed outright;java:S6539suppressed in-code with a recorded rationale.Infrastructure surface —
deployment/compose-sample/docker-compose.yml,integration-tests/docker-compose.yml,integration-tests/docker-compose.benchmark.yml,.github/dependabot.yml,.github/workflows/benchmark.yml,.github/workflows/claude.yml,benchmarks/src/main/resources/k6-scripts/bearer_proxied.js0.0.0.0with default credentials in the compose sample; it is now bound to loopback.Deliverable 4 was re-scoped — read this before reviewing the redaction
The originally-planned deliverable was an integration test asserting the readiness DOWN payload carries no raw failure detail. That payload is structurally unreachable in the shipped configuration:
TokenValidatorProducer.onStartupforces the sameInstance.get()the probe performs, so a JWKS that fails construction aborts boot outright, and one that is merely lazily unreachable reports UP. There is no shipped path that reaches the DOWN branch. This was verified empirically against the distroless image, not inferred.The deliverable was therefore re-scoped by operator decision to the JWKS fail-fast boot regression coverage listed above, which asserts the behaviour that is reachable. The redaction change in
GatewayReadinessCheck.javais retained as defence-in-depth against a future relaxation of eager boot assembly — it is not closing a live exposure, and should not be reviewed as though it were.Two carried
java:S5738rows are NOT fixedConfigFailFastTest:59andConfigProducerTest:179remain open on the live gate. This is a deliberate, evidenced deferral rather than an oversight:javapagainst the pinned quarkus-core 3.37.4 showsMemorySize(BigInteger)carries noDeprecatedattribute, and it is the sole public construction path on that version.MemorySize.of(...)as the replacement.of(...)does not exist on 3.37.4, so the swap would not compile.Successor finding
b59ed6carries the item.Achieved-thoroughness self-report
The T3 sweep covered
api-sheriff/src/main/java/**plus the CI, compose and YAML legs. Graded to the floor, the sweep initially self-reported T2, and named that floor explicitly rather than claiming its declared level. The operator ordered a top-up of the shipped and infrastructure legs, which closed the gap to T3 on the shipped surface.api-sheriff/src/test/resources/config/**is accepted at sampled coverage and is the residual — it is test fixture data, not shipped surface.Test Plan
verify -Ppre-commit)verify)verify-invalid-config-fails.shGenerated by plan-finalize skill
Intent
The problem. Five security findings had been deferred across earlier plans, and four Sonar rows were sitting open on the live gate. Carrying unsettled security debt into a 0.1.0 cut is what this change exists to prevent — for a security-focused gateway, "we know about it and will get to it" is not an acceptable release posture.
The chosen approach. Every carried item ends in one of exactly two states: a landed fix, or a rationale recorded against the actual code. Nothing is deferred to a follow-up issue, and nothing is closed on assertion alone — an item found already closed is reported closed with its evidence. The carried items were then tied together with a T3 sweep of the shipped surface (
api-sheriff/src/main/java/**plus the CI, compose and YAML legs), whose findings are fixed in this same PR rather than filed. Coverage is graded to the floor and self-reported, so a sweep that fell short of its declared level says so instead of claiming the target.Explicit non-goals — please do not report these as gaps.
.github/workflows/release.ymland.github/project.ymlbelong to the successor plan, deliberately.[Intent truncated — 1394 of 1871 characters shown; full outline in the plan workspace]
Summary by CodeRabbit
New Features
Bug Fixes
Security