diff --git a/doc/quality-report/README.adoc b/doc/quality-report/README.adoc new file mode 100644 index 00000000..e7d93738 --- /dev/null +++ b/doc/quality-report/README.adoc @@ -0,0 +1,136 @@ += Repository Quality Report +:toc: macro +:toclevels: 2 + +toc::[] + +== Scope and provenance + +Full-repository review of code and documentation at commit `b54eb12` (branch cut 2026-08-15), +covering correctness, edge-case handling, hidden traps / false security, fail-closed posture, +test substance, generator usage, and documentation accuracy (Javadoc and AsciiDoc). + +The review was produced by seven parallel automated deep-review passes (Claude Code), each +reading its slice in full: + +1. `config` / `asset` / `auth` / `events` production code (findings `CFG-*`) +2. `bff` production code — OIDC/BFF, cookies, sessions, CSRF (findings `BFF-*`) +3. Data plane — `edge` / `pipeline` / `routing` / `tls` / `forward` / `http` / `quarkus` (findings `DP-*`) +4. Cross-cutting security posture — claimed vs. implemented (findings `SEC-*`) +5. Test quality — unit, integration, benchmark suites (findings `TST-*`) +6. Documentation — AsciiDoc tree and Javadoc (findings `DOC-*`) +7. Peripherals — demo-client, benchmarks, scripts, Docker/compose, CI, POMs (findings `PER-*`) + +Every finding was then re-verified end-to-end by an independent adversarial verification pass +started with no prior context and instructed to refute each claim from the code. Outcome: of +the 98 findings drafted, *93 were confirmed as written, 5 were confirmed with detail +adjustments (now corrected in place: CFG-15, BFF-1, TST-1, TST-6, PER-8), and none were +refuted*. Four counting inaccuracies the verifier found in the "verified positives" sections +(harden-runner coverage, workflow count, `@since` count, log-record count) are likewise +corrected in place, and one additional observation the verifier surfaced was added afterwards +(DP-17), bringing the published inventory to *99 numbered findings*. Four of those are +cross-listed duplicates reported independently by two passes (SEC-1 = CFG-4, SEC-5 = DP-10, +SEC-11 = DP-8, PER-1 = DOC-2), so the deduplicated defect count is 95. This review changed no +production code, tests, or documentation outside `doc/quality-report/`. + +=== Currency + +This report is a *point-in-time snapshot at commit `b54eb12`*, not a living document. Findings +are not updated as the code moves; a finding may be fixed (or a positive invalidated) by any +later commit. Re-run the review — or retire this directory — after major architectural or +security-relevant changes, and treat the finding IDs (`CFG-*`, `SEC-*`, …) as stable handles +for triaging into tracking issues; where a finding relates to a recorded decision it already +cites the governing ADR inline. + +== Reports + +|=== +|Report |Contents + +|link:code-correctness.adoc[Code correctness] +|Production-code findings: `CFG-*` (config/asset/auth/events), `BFF-*` (BFF/OIDC), `DP-*` (data plane) + +|link:security-posture.adoc[Security posture] +|Claimed-vs-implemented verification table and `SEC-*` findings; fail-closed audit + +|link:test-quality.adoc[Test quality] +|`TST-*` findings, coverage map, generator-usage assessment + +|link:documentation.adoc[Documentation] +|`DOC-*` findings, structure assessment, Javadoc compliance table + +|link:build-and-peripherals.adoc[Build and peripherals] +|`PER-*` findings: demo-client, benchmarks, scripts, Docker/compose, CI, POMs +|=== + +== Severity and confidence conventions + +* *Severity* — CRITICAL (exploitable or data-loss), HIGH (functional contradiction or materially + wrong claim), MEDIUM (real defect, bounded impact), LOW (polish-level defect), INFO + (observation, accepted risk, or latent issue with no current trigger). +* *Confidence* — HIGH (proven from code), MEDIUM (probable, one step of inference), LOW + (plausible, not fully traced). + +== Executive summary + +*Overall verdict: the repository is in unusually strong shape for a pre-1.0 codebase.* The +review found *no critical finding, no exploitable authentication or routing bypass, no +fail-open security path, no crypto misuse, and no secret leakage*. The claimed fail-closed +posture is real at essentially every enforcement point traced (deny-by-default routing, boot +refusal on ambiguous auth or trust configuration, bounded stores, tamper-rejecting sealed +cookies, clean sensitive-data log sweep). Documentation splits into two layers with different +accuracy: the *checkable reference layer* is exemplary — zero broken links across 751 +cross-references, `LogMessages.adoc` in perfect two-way sync with the code, configuration docs +matching the JSON schemas field-for-field — while the *self-descriptive/status layer* carries +real staleness defects (DOC-1 HIGH, DOC-2 through DOC-8). The test corpus is substantive, with +matched positive/negative controls and a self-auditing doctrine whose claims check out against +HEAD. + +The findings that matter most, in priority order: + +1. *`token_validation` is effectively mandatory at boot* (CFG-1, HIGH): the validator producer + forces eager assembly and throws when the block is absent, contradicting the schema, the + config validator, `AuthenticationStage` Javadoc, and the readiness check — a validated-correct + public-only or BFF-only gateway cannot start. Fails closed, but a hard functional + contradiction between four in-repo contracts. Note: `GatewayReadinessCheck.java:164-192` + documents the eager-boot coupling as a deliberate fail-closed decision, so the producer side + is the recorded intent and the other three contracts are the stale ones. +2. *Stale self-description in the flagship architecture doc* (DOC-1, HIGH): + `architecture.adoc`'s "Implementation Deviations" still claims data-plane structured logging + is unimplemented; it shipped long ago across at least 10 classes. +3. *Session-WebSocket CSWSH gap* (SEC-1 = CFG-4, MEDIUM): a `require: session` WebSocket route + gets neither Origin-allowlist enforcement (boot rule covers bearer only) nor CSRF coverage + (GET-exempt). Doc-admitted in ADR-0015/threat model, but the largest genuinely open hole for + a BFF deployment using WebSockets. +4. *Parsed-but-inert configuration keys* (DP-2, DP-10, BFF-1, MEDIUM): per-upstream + `connect_timeout_ms` / `read_timeout_ms` / `retry.max_attempts` / `circuit_breaker.*` bind but + nothing consumes them (contradicting ADR-0008); `rate_limit` is documented-inert; the RFC 9470 + step-up coordinator is fully built and never invoked. Exactly the "key that parses is not a + key that acts" class the project's own CLAUDE.md warns about. +5. *Silent YAML config traps* (CFG-2, CFG-3, MEDIUM): duplicate keys last-win and trailing YAML + documents are silently dropped — on a gateway whose whole posture is this file, a duplicated + `require:` key can weaken auth with zero diagnostic. +6. *Security-headers stage weaknesses* (SEC-3, SEC-4, TST-1, MEDIUM): headers are entirely + opt-in, `hsts: {}` actively emits `max-age=0` (clears HSTS in browsers), and the + HSTS/nosniff/frame-deny half of the stage is assertion-free in the whole corpus and inactive + in the whole IT topology. +7. *Missing RFC 7230 Connection-nominated header stripping* (DP-1, MEDIUM) in both directions, + and *protocol verb semantics enforced only in documentation* (DP-7, MEDIUM — a non-upgrade + request on a WebSocket route leaves the client hanging with no response). +8. *Dead security observability* (BFF-2, MEDIUM): six catalogued session-lifecycle/CSRF/logout + LogRecords have no emission site; CSRF rejections log at DEBUG only — a SIEM keying on the + documented records never fires. +9. *Instruction-file drift* (PER-1 = DOC-2, DOC-3, DOC-4, MEDIUM): CLAUDE.md/AGENTS.md pin + Quarkus 3.37.4 (actual: 3.38.2), call the k6 benchmarks "WRK", and under-count the reactor's + modules. +10. *Integration stack exposes admin surfaces on all interfaces* (PER-3, MEDIUM): admin/admin + Keycloak console and unauthenticated Toxiproxy control API published on `0.0.0.0`, where the + sibling compose-sample deliberately binds the same class of ports to loopback. + +Of the 99 numbered findings, 2 are HIGH, 23 MEDIUM, and 74 LOW/INFO (counting the four +cross-listed duplicates once each: 95 distinct — 2 HIGH, 21 MEDIUM, 72 LOW/INFO). The +LOW/INFO tail covers attribution and normalization inconsistencies, stale point-in-time +comments, ADR status hygiene (20 of 37 still "Proposed" while shipped), Javadoc falling short +of the project's thread-safety-note/usage-example standard, generator under-adoption relative +to the stated preference, and a handful of accepted-risk items already documented by the +project itself. diff --git a/doc/quality-report/build-and-peripherals.adoc b/doc/quality-report/build-and-peripherals.adoc new file mode 100644 index 00000000..b0cc9319 --- /dev/null +++ b/doc/quality-report/build-and-peripherals.adoc @@ -0,0 +1,231 @@ += Build and Peripherals Report +:toc: macro +:toclevels: 2 + +Review of the peripheral surface: `demo-client/`, `benchmarks/`, the non-test-Java parts of +`integration-tests/` (scripts, nginx, compose, Dockerfiles), `deployment/compose-sample/`, +build and CI (POMs, workflows, Dockerfiles, `application.properties`), and repo hygiene. See +link:README.adoc[the index] for conventions. + +toc::[] + +== Verdict + +*The peripheral surface is in unusually strong shape.* The CI workflows are a near-model +implementation of supply-chain hygiene; the compose/docker material applies non-root, +cap-drop, read-only, tmpfs hardening consistently to every gateway container; the demo client +is XSS-safe by construction. The most consequential defects are documentation drift (PER-1/2) +and the integration stack's all-interfaces port exposure (PER-3). All credentials found are +unambiguous, clearly labeled test/sample fixtures. + +== Findings + +=== PER-1 — CLAUDE.md/AGENTS.md pin Quarkus 3.37.4; the build runs 3.38.2 + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: doc-mismatch +|Location |`CLAUDE.md:7`, `AGENTS.md:8` vs `pom.xml:59` +|=== + +An agent or developer reasoning about BOM alignment, CVE exposure, or upgrade scope from +CLAUDE.md works against the wrong platform version. Same as +link:documentation.adoc[DOC-2]. + +=== PER-2 — Root-POM BOM-ordering comment cites a verification at superseded versions + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: doc-mismatch +|Location |`pom.xml:89-96` +|=== + +The load-bearing comment justifying BOM import ordering says "Verified against the resolved +dependency graph at token-sheriff 0.9.3 / Quarkus 3.37.4 …"; the properties are now 0.9.4 / +3.38.2 (lines 59-60). The ordering is probably still correct, but the "verified" claim is +stale. + +=== PER-3 — Integration stack publishes admin surfaces on all host interfaces + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: docker +|Location |`integration-tests/docker-compose.yml:26-27,55,125-126,929-930` +|=== + +Published on `0.0.0.0`: Keycloak admin console `1443` with bootstrap `admin/admin`, +Toxiproxy's unauthenticated admin/control API `8474` (create/modify proxies at runtime), +Prometheus `9090`, go-httpbin `18080`, plus 7 gateway management ports. The repo itself +articulates why this is dangerous — `deployment/compose-sample/docker-compose.yml:54-66` binds +the same class of ports to `127.0.0.1` precisely because "publishing it on 0.0.0.0 would hand +full control of the stack's trust root to anything that can reach the host" — but the IT stack +does not apply the loopback binding. On a LAN-reachable developer or shared CI host this +exposes an admin/admin IdP and a remote traffic-manipulation API. Mitigating: ephemeral test +stack, throwaway realms. + +=== PER-4 — Committed test private-key material (deliberate fixture) + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: secret (inventory) +|Location |`integration-tests/src/main/docker/certificates/` (`localhost.key`, `*-truststore.p12`, `mtls-*.p12`) +|=== + +Deliberate, documented test-fixture material (passwords stated non-secrets; CN=localhost, +valid 2026-03 → 2028-03). The compose-sample explicitly does the opposite (generates and +gitignores). Residual risk is only copy-paste reuse. The `chmod 600` in the generate script is +nominal — git does not preserve it, as the sample script itself observes. + +=== PER-5 — One checkout missing `persist-credentials: false` + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: ci +|Location |`.github/workflows/benchmark.yml:104-112` +|=== + +The "Fetch Previous History" checkout of `cuioss/cuioss.github.io` omits +`persist-credentials: false`, unlike every other read-only checkout in the repo (same file +line 90 even carries the explaining comment). Token is `contents: read` only, so impact is +small — but it breaks the repo's own stated invariant. + +=== PER-6 — Older script generation still on bare `set -e` + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: script +|Location |`integration-tests/scripts/start-integration-container.sh:4`, `stop-integration-container.sh:4`, `build-native-if-needed.sh:2`, cert-generation scripts, `benchmarks/.../pre-benchmark-health-check.sh:11` +|=== + +Newer scripts standardize on `set -euo pipefail`; the older generation uses `set -e` only. +Concrete exposure: unset-variable typos and mid-pipeline failures +(`$COMPOSE_CMD config --format json | python3 …`) are not hard failures; +`stop-integration-container.sh:48` reads `"$1"` unguarded (benign only because `-u` is +absent). Quoting elsewhere is clean; no unsafe temp files (the IT temp keystore cleans only on +the success path — same generation gap). + +=== PER-7 — Stale port-inventory comment in verify script + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: doc-mismatch +|Location |`integration-tests/scripts/verify-invalid-config-fails.sh:47-49` +|=== + +Comment says "outside the 19000-19005 block … for the six gateway instances"; the compose file +now publishes 19000-19006 for seven instances. `MGMT_PROBE_PORT=19009` is still +collision-free — functionally harmless mirrored-list drift of exactly the kind the repo's other +scripts eliminated by derivation. + +=== PER-8 — One hardcoded banner target survived the derivation refactor + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: script +|Location |`integration-tests/scripts/start-integration-container.sh:369` +|=== + +The closing banner hardcodes `Keycloak: https://localhost:1443/auth` while the script's own +header rule derives every probe/banner target from the Compose model — and the derived +Keycloak health row sits a few lines below it (line 375). + +=== PER-9 — `@claude` workflow tool-allowlist breadth + +[cols="1,4"] +|=== +|Severity |INFO — Confidence MEDIUM — Category: ci +|Location |`.github/workflows/claude.yml:58` +|=== + +`allowed_tools` grants `Bash(./mvnw*)`, `Bash(git*)`, `Bash(gh*)`, `Bash(find*)`, +`Bash(java*)` to an LLM job triggerable by any `@claude` comment; the nine itemized `./mvnw` +entries are redundant under the wildcard. Mitigations are real: SHA-pinned action, +`persist-credentials: false` with rationale, read-only permissions. Residual: `gh*` is an +authenticated write-capable CLI surface driven by attacker-authorable comment text. + +=== PER-10 — k6 load generators run as root (documented rationale) + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: docker +|Location |`integration-tests/docker-compose.yml:965`; `docker-compose.apisix.yml:141` +|=== + +`user: "0:0"` with in-file rationale (bind-mount writability, no published ports, ephemeral). +All seven gateway instances, by contrast, are exemplary: distroless `USER nonroot`, +`cap_drop: ALL`, `no-new-privileges`, `read_only` + sized tmpfs, resource limits. + +=== PER-11 — Digest-pinning inconsistent in the IT stack + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: docker +|Location |`integration-tests/docker-compose.yml:53,118,137,928` +|=== + +Keycloak is digest-pinned; toxiproxy, go-httpbin, nginx, prometheus are tag-only. Where +reproducibility is load-bearing the repo does pin by digest with explicit reasoning (benchmark +fairness backend, apisix, k6, both production Dockerfiles) — drift, not policy absence. + +=== PER-12 — Dev/sample credential inventory (all clearly marked) + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: secret (inventory; no action implied) +|Location |repo-wide +|=== + +`admin/admin` Keycloak bootstrap (IT + sample, the latter with "SAMPLE CREDENTIALS. Change +both"); `OIDC_CLIENT_SECRET=integration-secret` (IT); fixed AES test key with "never a +production key" comment; `benchmark-secret`/`benchmark-password`; +`integration-user`/`integration-password`; `localhost-trust`; sample realm secret literally +`CHANGE-ME-BEFORE-PRODUCTION`. No real secret found anywhere; CI secrets referenced only via +`${{ secrets.* }}` with explicitly narrowed mapping (pr-agent.yml refuses `secrets: inherit` +with rationale). + +=== PER-13 — Sample IdP runs `start-dev` without a not-for-production callout + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: docker +|Location |`deployment/compose-sample/docker-compose.yml:35-40` +|=== + +The gateway side of the "production-shaped" sample genuinely is production-shaped, but +Keycloak runs `start-dev --import-realm`; an operator copying the stack shape may carry +`start-dev` forward. The credentials are called out as sample-only; the dev-mode IdP is not. + +== Verified positives + +* *demo-client (PER-14)*: no XSS sink exists — all rendering via `textContent` + (`spa/app.js:54-83`), no `innerHTML` anywhere, zero inline script/style (commented as + deliberate), login/logout are top-level navigations with `encodeURIComponent`-ed returnUrl. + ESLint runs `eslint-plugin-security` at `--max-warnings 0`; `npm ci` with integrity-pinned + committed lockfile; the POM documents why `npx` is banned (registry-fallback RCE). Docs match + implementation. TLS-verification bypasses confined to the self-signed test stack with in-file + rationale. +* *Build claims (PER-15)*: CLAUDE.md's `failOnWarning` claim is real (`pom.xml:163-179` + reactor-wide); the CI matrix "25 + 26" is real (`.github/project.yml:10`); parent + `cui-java-parent:1.5.4` matches; all six POMs at `0.2.0-SNAPSHOT` consistently; no + duplicate/conflicting dependency declarations; the two classifier deps bypassing BOM + management are explicitly version-pinned with comments; `.env` image pin `0.1.1` matches + `project.yml current-version`. +* *CI workflows (PER-16)*: every `uses:` pinned to a full commit SHA (10 workflow files; + comments carry the version, except the claude action's `# beta`); dependabot lanes for + actions/maven/docker; least-privilege `permissions:` blocks with commented, scoped + escalations; no `${{ github.event.* }}` interpolation inside any `run:` body + (env-indirection enforced by comments at each site); `harden-runner` leads the caller-local + jobs with one exception — `claude.yml`'s `claude` job runs without it (other jobs lacking it + delegate to SHA-pinned org reusable workflows); the release lane's Trivy gate (HIGH+CRITICAL + before `docker login`), digest-addressed push/smoke/sign/verify chain, and cosign identity + anchoring are unusually rigorous. +* *application.properties (PER-17)*: no insecure defaults found — HTTP 8080 + `insecure-requests=redirect`; management HTTPS-by-default with loudly-failing plain-HTTP + opt-out; file logging off by default (former root-path hazard removed and documented); log + level INFO; SecureRandom forced to runtime init in native. Both Dockerfiles digest-pin their + base and run non-root; the supply-chain scan job asserts the digest pin at CI time. +* *Hygiene (PER-18)*: zero TODO/FIXME/HACK markers in code (the only hits are the instruction + files describing the OpenRewrite marker convention); clean working tree; `.gitignore` covers + targets/node_modules/.plan (with marshal.json exception); all `curl -k` / + `insecureSkipTLSVerify` sites confined to the self-signed test bundle, each with in-file + justification — no TLS verification disabled against any real endpoint. diff --git a/doc/quality-report/code-correctness.adoc b/doc/quality-report/code-correctness.adoc new file mode 100644 index 00000000..d161ea5e --- /dev/null +++ b/doc/quality-report/code-correctness.adoc @@ -0,0 +1,825 @@ += Code Correctness Report +:toc: macro +:toclevels: 2 + +Production-code findings from three deep-review passes: `CFG-*` +(config/asset/auth/events), `BFF-*` (BFF/OIDC/session/cookie/CSRF), and `DP-*` (data plane: +edge/pipeline/routing/tls/forward/http/quarkus). Every file in scope was read in full, with +callers/callees and governing ADRs consulted. See link:README.adoc[the index] for severity and +confidence conventions. + +toc::[] + +== Summary + +No CRITICAL finding. One HIGH finding (CFG-1). No exploitable authentication or routing +bypass, no fail-open error path, and no crypto misuse were found. One resource-exhaustion +hazard exists in the default configuration — DP-6, where fault-tolerance timeouts against a +slow upstream strand pooled connections; the other resource-bound finding (DP-4) requires the +`tls.passthrough_sni` configuration opt-in. The dominant defect classes are: internal contract +contradictions (CFG-1, DP-2, DP-7, BFF-1), silent config-parsing hazards (CFG-2, CFG-3), and +consistency drift between validation-time and runtime semantics (CFG-9, DP-13). + +== config / asset / auth / events (CFG) + +=== CFG-1 — `token_validation` is effectively mandatory at every boot + +[cols="1,4"] +|=== +|Severity |*HIGH* — Confidence HIGH — Category: consistency / correctness +|Location |`api-sheriff/src/main/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducer.java:92-96,110-114` +|=== + +`onStartup` unconditionally forces validator assembly at every boot (`validator.toString()` on +the ArC proxy), and `gatewayTokenValidator()` throws `CONFIG_INVALID` when `tokenValidation == +null`. Consequence: any `gateway.yaml` without a `token_validation` block cannot boot, even +though four in-repo contracts promise otherwise: + +* the schema marks `token_validation` optional (`required: ['version']` only); +* `ConfigValidator.validateEffectiveAuth` requires it only when some route resolves + `require: bearer` (`ConfigValidator.java:782-789`); +* `AuthenticationStage`'s constructor Javadoc (`AuthenticationStage.java:77-84`) explicitly + promises "a gateway serving only `require: none` routes (with no `token_validation` block) + never triggers the validator producer and therefore never fails boot"; +* `GatewayReadinessCheck` (`quarkus/GatewayReadinessCheck.java:142-152`) carries a + reachable-looking `jwks: not-applicable` UP branch for exactly that state. + +A public-only or pure session/BFF gateway (which needs `oidc`, not `token_validation`) is +validated as correct and then aborted by the producer. The failure direction is closed (denial, +not bypass), but it is a hard functional contradiction between four in-repo contracts. + +Adversarial-verification note: `GatewayReadinessCheck.java:164-192` carries a comment +documenting the `not-applicable` branch as "NOT REACHED IN THE SHIPPED EAGER-BOOT TOPOLOGY, AND +DELIBERATELY RETAINED … the eager-boot coupling it depends on is a fail-CLOSED security +property". One side of the contradiction is therefore a recorded decision, not drift: the +producer's eager-boot behavior is intended, and the schema, `ConfigValidator`, and +`AuthenticationStage` Javadoc are the stale contracts to reconcile. + +=== CFG-2 — Duplicate YAML keys silently last-win + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: edge-case / false-security +|Location |`config/load/ConfigLoader.java:382-402,637-673` +|=== + +Neither the compose-only pre-pass (`LoaderOptions` default `allowDuplicateKeys=true`; the +Composer does not check duplicates) nor the Jackson bind (`STRICT_DUPLICATE_DETECTION` not +enabled on the `YAMLMapper`) rejects a document declaring the same key twice. A +`gateway.yaml`/endpoint file with, e.g., two `require:` keys or two `security_filter:` blocks +silently drops the first — on a security gateway whose whole posture is this file, a duplicated +key can silently weaken auth with zero diagnostic. `hardenedYamlFactory()` sets only +alias/nesting/code-point limits; `buildMapper()` enables only `ACCEPT_CASE_INSENSITIVE_ENUMS` +and snake-case. + +=== CFG-3 — Multi-document YAML is half-processed + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: edge-case +|Location |`config/load/ConfigLoader.java:392-396,328` +|=== + +`withinExpansionLimits` runs `composeAll` and walks every document, but `mapper.readTree +(snapshot)` binds only the first document (`FAIL_ON_TRAILING_TOKENS` not enabled). A file +containing `--- doc1 --- doc2` has doc2's routes/settings silently discarded while the +bomb-guard's own iteration proves the loader knows more documents exist. Silent config loss, no +`ConfigError`. + +=== CFG-4 — Session-WebSocket routes may carry an empty Origin allowlist + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: false-security +|Location |`config/validation/ConfigValidator.java:1364-1367` +|=== + +`validateWebSocketRoute` mandates a non-empty `allowed_origins` only when effective auth is +`BEARER`. A `require: session` WebSocket route — the case where the browser auto-attaches the +session cookie, i.e. the actual CSWSH vector — may carry an empty allowlist, and an empty +allowlist at the edge means no Origin enforcement. ADR-0015 explicitly records this as an open +GAP for session routes, so it is known and documented rather than an oversight — but it remains +live false-security: the rule reads "fail-closed WebSocket allowlist contract" while covering +only the leg that browsers cannot attack (JS cannot set `Authorization` on a WebSocket +handshake). Independently found by the security-posture pass as SEC-1 (see +link:security-posture.adoc[security posture]). + +=== CFG-5 — Validator fabricates endpoint file names in error locations + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: consistency +|Location |`config/validation/ConfigValidator.java:1468-1470` +|=== + +`endpointFile()` fabricates the error location as `"endpoints/" + endpoint.id() + ".yaml"`, but +nothing enforces that an endpoint's file name equals its `id` — `ConfigLoader.loadEndpoint` uses +the real `path.getFileName()`. For a file `orders-v2.yaml` declaring `id: orders`, validator +errors point at the nonexistent `endpoints/orders.yaml` while loader errors point at the real +file. Two attribution schemes for the same error channel. + +=== CFG-6 — `UpstreamAssetSource` never validates `maxBytes` + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: consistency / edge-case +|Location |`asset/UpstreamAssetSource.java:152-161,280` +|=== + +`DirectoryAssetSource` (lines 181-185) rejects negative values and values above +`MAX_ENFORCEABLE_BYTES` at construction with an extensively documented rationale; +`UpstreamAssetSource` performs no such validation. A negative `maxBytes` makes every fetch 413 +(fail-closed but silent); `maxBytes = Long.MAX_VALUE` makes `Math.min(maxBytes + 1, +body.length)` overflow negative (latent — `truncated` cannot be true at that cap). The +documented "unenforceable cap" reasoning of the directory source applies equally here and is +not enforced. + +=== CFG-7 — `AssetSource.Served` Javadoc contradicts the upstream source + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: javadoc +|Location |`asset/AssetSource.java:99-100` +|=== + +`Served` Javadoc: "body: empty for HEAD and every non-200 outcome". Contradicted by +`UpstreamAssetSource.serve` (line 210): `serveBody = method == GET && fetched.status() < 400` — +a governed 201/206/301/302 upstream response is relayed *with* its body. The contract as +documented matches only the directory source. + +=== CFG-8 — `RouteTable.lookup` is production dead code with divergent semantics + +[cols="1,4"] +|=== +|Severity |LOW — Confidence MEDIUM — Category: consistency +|Location |`config/model/RouteTable.java:59-81` +|=== + +The data plane selects routes via `RouteSelectionStage` iterating +`RouteRuntime.getMatcher().matches(...)`; no production caller of `lookup` exists. Its +semantics also differ from real selection (first-prefix-only; would mis-serve legal same-prefix +routes distinguished by host/method/header). A drift trap; under the project's Pre-1.0 +"remove unused API" rule it should be deleted rather than carried. + +=== CFG-9 — Trailing-slash and host-case asymmetry between validation and runtime + +[cols="1,4"] +|=== +|Severity |LOW — Confidence MEDIUM — Category: edge-case / consistency +|Location |`config/model/RouteTable.java:76-81`, `config/RouteTableBuilder.java:163-170`, `routing/RouteMatcher.java:85-91,113` +|=== + +Validation and sorting normalize trailing slashes (`/api` ≡ `/api/` collide in +`validateRouteDisjointness`; the sort key is normalized), but runtime matching uses the raw +prefix: a route declared `path_prefix: /api/` matches `/api/x` but never `/api` itself, while +the validator treats the two spellings as identical. An operator writing the trailing-slash +form silently loses the exact-path match. Related: `RouteMatcher.matches` compares `host` +case-sensitively (`host.equals(requestHost)`) while `ConfigValidator.hostsOverlap` (line 570) +compares case-insensitively — the boot model and the runtime disagree about whether +`Host: API.example.com` is the same host. + +=== CFG-10 — Topology property-name filter silently drops entries, then misleads + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: edge-case +|Location |`config/topology/TopologyResolver.java:172-178` +|=== + +`readProperties` silently drops any property whose name fails `[A-Z][A-Z0-9_]*`. An operator +defining `orders_api=http://...` (lowercase) gets "Unresolved topology alias 'orders_api' +referenced by enabled endpoint ..." with no hint the entry exists but was filtered. +`Properties.load` also last-wins on duplicate keys silently. Fail-closed, but the diagnostic +actively misleads. + +=== CFG-11 — Topology URL userinfo/query/fragment silently discarded + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: edge-case +|Location |`config/topology/TopologyResolver.java:181-201` +|=== + +`decompose` keeps scheme/host/port/path and silently drops userinfo, query, and fragment: +`http://user:pass@host/path?x=1#f` is partially honoured instead of refused (contrast: a +non-http scheme *is* refused). A credential embedded in a topology URL is silently thrown away +rather than flagged. + +=== CFG-12 — `require: none` plus `required_scopes` is accepted and inert + +[cols="1,4"] +|=== +|Severity |LOW — Confidence MEDIUM — Category: false-security +|Location |`auth/AuthenticationStage.java:118-127` + `config/validation/ConfigValidator.java` +|=== + +An `auth` block of `require: none` with a non-empty `required_scopes` is accepted by schema and +validator, and the `NONE` switch arm enforces nothing — the declared scopes are silently inert. +No cross-cutting rule refuses the incoherent pair (unlike the analogous refusals for +`websocket` blocks on non-websocket routes and `*_allow`+`*_deny`). An operator reading the +file sees a scope requirement that is never enforced. + +=== CFG-13 — Omitted `audience` silently disables audience validation + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: false-security (deliberate; flagged for posture) +|Location |`auth/TokenValidatorProducer.java:125-135` +|=== + +Omitting `audience` on an issuer silently sets `audienceValidationDisabled(true)`. Deliberate +and commented, but it is a security-relevant silent default on a security gateway: any token +minted by the issuer for any audience is accepted. Neither a validator rule nor a boot WARN +surfaces it (contrast: `management.tls.enabled: false` gets a loud WARN). + +=== CFG-14 — Stale rule inventory in `config.validation` package Javadoc + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: javadoc +|Location |`config/validation/package-info.java:24-31` +|=== + +Names a "whole-second timeouts" rule that does not exist in `DEFAULT_RULES`, and omits several +rules that do exist (terminal-action matrix, minimal-profile refusal, forward-mode exclusivity, +asset content-type rules, edge hardening). + +=== CFG-15 — `asset` package lacks `@NullMarked` unlike every sibling + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: consistency +|Location |`asset/package-info.java` +|=== + +Within the reviewed slice (config/asset/auth/events), `asset` is the only package without +`@NullMarked`, while its code uses jspecify annotations (`Flow.@Nullable Subscription` in +`UpstreamAssetSource.java:304`) and every sibling (auth, events, config.*) is NullMarked. +Repo-wide, `quarkus/package-info.java` and the root `gateway/package-info.java` also lack it +(24 of 27 package-infos carry the annotation). Nullness defaults silently differ across the +codebase. + +=== CFG-16 — `GatewayEventCounter` Javadoc overstates snapshot/reset semantics + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: javadoc +|Location |`events/GatewayEventCounter.java:59-72` +|=== + +`getCounters()` says "all non-zero counters" but returns every map entry; under the +`computeIfAbsent`→`incrementAndGet` race a transiently zero entry can appear. `reset()` says +"resets every counter to zero" but clears the map; a concurrent `increment` racing `clear()` +can be lost. Acceptable for metrics; worth one sentence of Javadoc honesty. + +=== CFG-17 — JSON-pointer construction does not escape `~`/`/` in keys + +[cols="1,4"] +|=== +|Severity |INFO — Confidence MEDIUM — Category: edge-case (latent) +|Location |`config/load/ConfigLoader.java:414-436,530-546` +|=== + +Pointer construction concatenates raw field names (`pointer + "/" + name`) with no `~0`/`~1` +escaping. A map key containing `/` (possible in `patternProperties`-style maps if ever widened) +would corrupt the error location and the `declaredScalarType` schema walk (falls back to shape +inference — benign but silent). No current key class carries `/`. + +=== CFG-18 — Third copy of YAML limit constants; pre-pass divergences + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: consistency +|Location |`config/NeutralTlsConfigSource.java:143-145,237` +|=== + +Third copy of the `MAX_YAML_*` limit constants (ADR-0010's Consequences section accounts only +for the two sites inside `ConfigLoader`); this pre-pass uses full `Yaml.load()` construction +(safe under SnakeYAML 2.x default TagInspector) with no 512 KiB byte cap (only the 1 MiB +code-point limit). Bounded and fail-safe, but exactly the drift ADR-0010 warns about. + +=== CFG-19 — Asset `HEAD` requests issue a full upstream `GET`; 405s lack `Allow` + +[cols="1,4"] +|=== +|Severity |INFO — Confidence MEDIUM — Category: edge-case +|Location |`asset/UpstreamAssetSource.java:261` +|=== + +A `HEAD` asset request issues a full upstream `GET` (method never propagated), buffering up to +`maxBytes` to discard it. Correct output, wasteful for an SSRF-guarded fetch seam. Both asset +sources' 405 responses omit the `Allow` header. + +=== CFG-20 — Case-colliding operator `content_types` keys collapse silently + +[cols="1,4"] +|=== +|Severity |INFO — Confidence LOW — Category: edge-case +|Location |`config/model/AssetDefaultsConfig.java:55-61` +|=== + +Two `content_types` keys differing only by case collapse silently in `lowercaseKeys` +(LinkedHashMap last-wins) before the add-only validator sees them — the shadowed entry gets no +diagnostic. Only operator-extension keys can silently collapse (built-in collisions are +boot-refused). + +=== CFG verified positives + +* *ADR-0010 genuinely implemented*: the compose-only pre-pass runs over the same byte snapshot + the bind consumes (no TOCTOU); the alias cap is armed there and correctly documented as inert + on the Jackson path; the 512 KiB byte cap is ordered below the 1 MiB code-point limit so it + can actually fire. +* *PathConfinement + DirectoryAssetSource* is a rigorous fail-closed stack: lexical confinement + (cui-http strict pipeline, `UrlSecurityException` → 404), real-path re-check closing the + in-root-symlink spelling, `SecureDirectoryStream` descent closing per-component TOCTOU with a + warned fallback, `NOFOLLOW` stat+read on one descriptor, FIFO refusal, bounded `maxBytes+1` + read with construction-enforced overflow ceiling. Nothing fails open. +* *UpstreamAssetSource*: fixed boot-resolved target, scheme allowlist, `Redirect.NEVER`, + mid-flight-abort capped subscriber, and an explicit `truncated` signal that turns a + fetch-cap/serve-cap mismatch into 413 instead of silently serving a prefix. +* *JwksTrustProfileResolver* refuses unbound profiles, `trust-all` buckets (checked before the + anchor-free guard, correctly ordered), and material-free buckets — no fallback-to-default-trust + path exists. +* *Secret hygiene*: bare-`${VAR}` enforcement pre-substitution, no value echo in + `MalformedPlaceholderException`, redacting `toString()` on secret-bearing records, + `EnvSecretResolver` non-recursive expansion (`quoteReplacement`) closing env-value injection. +* *ConnectionHeaders* strips `Content-Length` and HTTP/2 pseudo-headers on the response path; + the `-`/`_` separator fold closes the CGI-folding class on the request path. +* *AuthenticationStage* boot refusal and runtime governance resolve through the same shared + seams (`effectiveAccessLevel`, `globalProfile`, `normalizePrefix`), so they cannot drift. + +== BFF / OIDC (BFF) + +=== BFF-1 — RFC 9470 step-up is fully built but never invoked + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: false-security / correctness +|Location |`bff/refresh/StepUpCoordinator.java` (whole class); wiring `quarkus/BffRuntimeProducer.java:292-296` +|=== + +`StepUpCoordinator` is instantiated and exposed via `BffRuntime.stepUpCoordinator()`, and +`oidc.session.step_up.honor_upstream_challenge` exists in schema and model — but +`detectChallenge(...)`/`coordinate(...)` are never called anywhere in the edge or dispatch +path; the `SilentSatisfaction` seam is hardwired to `Optional.empty()`, and +`stepUpCoordinator()` is referenced only by tests. An upstream `401 +insufficient_user_authentication` on a `require: session` route is relayed to the browser +unhandled; no step-up re-drive ever happens in production. The wiring comment in +`BffRuntimeProducer.java:286-287` claims "the upstream-challenge edge integration is exercised +by the Keycloak integration tests" — edge integration that does not exist in `main/java`. (The +coordinator's own Javadoc, `StepUpCoordinator.java:66-67`, is honest: "It is exercised only +when `session.step_up.honor_upstream_challenge` is enabled; the enablement gate is the +runtime's concern.") + +=== BFF-2 — Catalogued session-lifecycle and CSRF LogRecords are never emitted + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: javadoc / false-security (observability) +|Location |`bff/BffLogMessages.java`; `bff/csrf/CsrfDefence.java:128`; `bff/logout/BackchannelLogoutReceiver.java:114` +|=== + +Only 6 of the catalogue's records are emitted. `INFO.SESSION_CREATED` (10), +`INFO.SESSION_DESTROYED` (11), `INFO.BACKCHANNEL_LOGOUT` (13), `INFO.RP_LOGOUT_COMPLETED` (14), +`WARN.CSRF_REJECTED` (110), and `WARN.LOGOUT_TOKEN_REJECTED` (112) have no production emission +site. `CsrfDefence.rejected` logs at DEBUG; the back-channel receiver logs rejections at DEBUG +only; session create/destroy/logout-complete are never logged at all. For a security gateway, +session establishment/destruction, accepted back-channel logouts, and CSRF rejections produce +no structured WARN/INFO audit line — a SIEM keying on the documented `ApiSheriff-xxx` records +never fires. (The `CSRF_REJECTED` *metric* counter does fire via the edge `GatewayException`; +only the log record is dead.) + +=== BFF-3 — Back-channel logout "replay guard" is freshness-window-only + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: oidc +|Location |`bff/logout/LogoutTokenValidator.java:142-149` +|=== + +The Javadoc calls the `iat` freshness window "the replay guard (BFF-09)", but a symmetric +±2-minute window admits unlimited replays of the same logout token; no `jti` tracking exists. +Impact is bounded (destruction is idempotent), but "replay guard" overstates what a freshness +window provides. + +=== BFF-4 — Logout initiation has no CSRF protection + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: false-security / oidc +|Location |`bff/reserved/LogoutEndpoint.java:94-125`; gating `edge/GatewayEdgeRoute.java:724-726` +|=== + +Reserved paths (including `LOGOUT`) are dispatched by `handleReservedPath` before route +selection and never pass through the CSRF gate; the logout leg is state-changing and the +`SameSite=Lax` session cookie is sent on top-level GET navigation, so an attacker page can +force-log-out a victim via a link/redirect to `oidc.logout.path`. Standard low-severity +logout-CSRF — annoyance, not data compromise — but unmitigated (unlike the logout *return* leg, +which is state-cookie-protected). + +=== BFF-5 — Cookie-mode cross-replica refresh can trigger token-family revocation + +[cols="1,4"] +|=== +|Severity |LOW — Confidence MEDIUM — Category: concurrency / session +|Location |`bff/refresh/TokenRefreshCoordinator.java:90,131-149`; `bff/cookie/CookieSessionBinding.java:56-63` +|=== + +Single-flight coalescing is per-instance. In cookie mode two near-simultaneous requests with +the same cookie landing on different replicas both present the same refresh token; an engine +with RFC 9700 reuse detection treats the second as replay and revokes the token family — +destroying the session and forcing re-login. `CookieSessionBinding` Javadoc frames this only as +"a cross-instance duplicate refresh cannot be prevented … documented, accepted trade-off", +understating that the consequence is family revocation → forced logout. + +=== BFF-6 — Session cookie name accepts non-`__Host-` values without validation + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: cookie +|Location |`quarkus/BffRuntimeProducer.java:194-197`; `config/validation/ConfigValidator.java` (no rule) +|=== + +`session.cookie_name` flows into the codecs with only a non-blank check; no validator rule +requires the `__Host-` prefix. A renamed cookie keeps `Secure; HttpOnly; SameSite=Lax; +Path=/` but loses the host-locked, `Domain`-less guarantee against subdomain cookie +injection/fixation. The binding and logout-state cookies are hardcoded `__Host-`; only the +session cookie is exposed. The default (`__Host-sheriff-session`) is safe. + +=== BFF-7 — 1-byte key id gives a 1/256 collision on key change (fail-closed) + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: crypto +|Location |`bff/cookie/CookieKeyMaterial.java:134-143` +|=== + +`keyIdOf` uses `SHA-256(label ‖ key)[0]` — one byte. On key change there is a 1/256 chance old +cookies pass the key-id gate and reach the cipher — where the GCM tag then fails and the value +unseals to "no session". Fail-closed regardless; only the rejection disposition attribution +changes. The Javadoc's "unknown key id refused before a Cipher is constructed" holds for +255/256 of key changes. + +=== BFF-8 — `events` claim membership check accepts nested keys + +[cols="1,4"] +|=== +|Severity |INFO — Confidence MEDIUM — Category: oidc +|Location |`bff/logout/LogoutTokenValidator.java:165-182` +|=== + +`hasBackchannelLogoutEventMember` confirms the value is an object then scans for the quoted +event URI followed by `:`; it rejects value-position occurrences but not a nested-key +occurrence such as `{"a":{"http://schemas.openid.net/event/backchannel-logout":1}}`. +Exploitation requires a validly IdP-signed token, so risk is minimal, but the check is looser +than a real JSON parse. + +=== BFF-9 — Pending-authorization FIFO eviction under login flood + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: session / correctness +|Location |`bff/pending/PendingAuthorizationStore.java:115-121` +|=== + +`InMemory.evictOldestBeyondCapacity` drops the oldest record beyond `maxEntries` (default +10 000). Unauthenticated clients create these records, so an attacker sustaining >10 000 login +initiations evicts genuine in-flight pending records before their 5-minute TTL; victims' +callbacks then fail 403. Memory is bounded (the intended guard); login availability is not. +Self-healing once the flood stops. + +=== BFF-10 — `SessionRecord`/`SealedSessionPayload` `toString()` exposes `sub`/`sid` (latent) + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: javadoc +|Location |`bff/session/SessionRecord.java:154-160`; `bff/cookie/SealedSessionPayload.java:184-189` +|=== + +Tokens, session id, and nonce are redacted, but `sub`, `sid`, `acr`, `authTime` print. +`BffLogMessages` class Javadoc asserts these "never appear" in logs. No current BFF logger +passes either record (verified), so latent — but a future `LOGGER.x("… %s", record)` would leak +`sub`/`sid`, contradicting the stated no-PII contract. + +=== BFF-11 — Logout token `typ` header not validated; ID-token validation seam + +[cols="1,4"] +|=== +|Severity |INFO — Confidence MEDIUM — Category: oidc +|Location |`bff/logout/LogoutTokenValidator.java:96-128`; `quarkus/BffRuntimeProducer.java:315` +|=== + +OIDC back-channel logout §2.4 recommends verifying `typ: logout+jwt`; the validator relies on +mandatory-`events`/prohibited-`nonce` instead (which does distinguish a logout token from an ID +token — defense-in-depth only). The signature seam is `idBridge::validateRefreshedIdToken`, +i.e. the logout token is signature-verified through the ID-token validation path; if that +engine method imposes ID-token-specific constraints it could reject spec-valid logout tokens. +Engine behavior, unverifiable from this repo; flagged for awareness. + +=== BFF verified positives + +* Sealed cookie: AES-256-GCM with a fresh 12-byte `SecureRandom` nonce per `seal()` (never + counter/payload-derived); `version ‖ key-id ‖ cookieName` bound into GCM AAD; unsealing + deterministically key-id-gated (never try-every-key); every decode/tag/version/key-id/payload + failure returns `Optional.empty()` fail-closed; key material must decode to exactly 32 bytes + or boot fails. +* Callback: browser-binding cookie *and* constant-time `state` compare + (`MessageDigest.isEqual`); raw-query HPP parse with duplicate rejection; pending store + single-use (remove-on-consume under the instance monitor). +* Open-redirect defense rejects `//`, backslash, and cross-origin targets + (`PendingAuthorizationRecord.sameOrigin`). +* Both session stores capacity-bounded; refresh coordinator single-flight via atomic + `putIfAbsent` keyed on stable session id; refresh failure destroys the session. +* Logout-token validation enforces iss/aud/iat-freshness/`events`-object/`nonce`-absent/ + `sub`-or-`sid`; cookie-mode back-channel destruction capability-gated to 404 before the body + is read. +* CSRF defence is fixed (no disable knob), checked before mediation, case-insensitive, + fail-closed. + +== Data plane (DP) + +=== DP-1 — Connection-nominated headers are not stripped in either direction + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: headers +|Location |`forward/ForwardPolicyStage.java:290-307`; `edge/ResponseStage.java:63-72` +|=== + +Only the fixed enumerated sets (`ConnectionHeaders.REQUEST_STRIP`/`RESPONSE_STRIP`) are +stripped; RFC 7230 §6.1 requires a proxy to also remove any header the `Connection` field +names. Request direction: `Connection: X-Session-Hint` + `X-Session-Hint: v` forwards the +hop-scoped header upstream as end-to-end (the `Connection` header itself is stripped, +converting a hop-scoped claim into an end-to-end one). Response direction: an upstream emitting +`Connection: X-Internal-Debug` + `X-Internal-Debug: ` has `Connection` stripped but the +nominated header relayed to the client — an internal, deliberately hop-scoped header leaks. +`FramingGate.rejectFramingHeaderStrip` rejects only `Connection` tokens naming the nine +`PROTECTED_HEADERS`; every other nominated name passes. + +=== DP-2 — Per-upstream resilience/transport configuration is parsed but inert + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: false-security / correctness +|Location |`edge/GatewayEdgeRoute.java:1518-1541` (guardFor), `:1291-1303` (clientFor); `config/model/UpstreamConfig.java:43-47,87` +|=== + +`UpstreamConfig` binds `connect_timeout_ms`, `read_timeout_ms`, `retry.max_attempts`, +`circuit_breaker.failures`, `circuit_breaker.reset_ms`; nothing consumes them. `guardFor` +hardcodes `requestVolumeThreshold(20)`, `failureRatio(0.5)`, `delay(5s)`, `withTimeout(30s)`, +`maxRetries(2)/delay(100ms)`; `clientFor` builds `vertx.createHttpClient()` with no options at +all (no connect timeout, no idle timeout). Only `retry.enabled` acts +(`RouteTableBuilder.resolveRetryEnabled`). This directly contradicts ADR-0008 ("Timeouts are +transport options on the shared Vert.x client (connect/read per upstream tuple)"; "one guard +per route, assembled at boot from `upstream.retry` / `upstream.circuit_breaker`"). An operator +declaring `circuit_breaker.failures: 3` believes a tighter breaker is in force; it is not — +the exact "key that parses is not a key that acts" class CLAUDE.md warns about. + +=== DP-3 — `Sec-WebSocket-*` headers leak into the upstream dial; subprotocol never relayed + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence MEDIUM — Category: websocket +|Location |`edge/WebSocketRelayStage.java:145-154`; `http/ConnectionHeaders.java:157-163` +|=== + +Inbound `Sec-WebSocket-Key/-Version/-Extensions/-Protocol` are absent from `REQUEST_STRIP`, so +under forward-all/negative-list they are copied into the upstream `WebSocketConnectOptions` +while the Vert.x/Netty dialer performs its own handshake: a leaked `Sec-WebSocket-Extensions: +permessage-deflate` can negotiate compression the dialer did not configure (RSV-bit protocol +errors / corrupted frames); a leaked `Sec-WebSocket-Protocol` offer can make the upstream +select a subprotocol the handshaker did not expect (failed upstream handshake). Independently, +subprotocol negotiation is never relayed to the client: `ctx.request().toWebSocket()` runs with +no server-side `webSocketSubProtocols`, so the client's 101 never carries a +`Sec-WebSocket-Protocol` selection — an RFC 6455 client that offered one must then fail the +connection. Net: WebSocket routes with subprotocol- or compression-using clients break in ways +that look like upstream faults. + +=== DP-4 — SNI front listener lacks every edge hardening bound + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: tls / dos +|Location |`tls/SniFrontListener.java:95-103,147-197`; `tls/PassthroughRelay.java` +|=== + +The front `NetServer` has no per-connection deadline for completing the ClientHello, no idle +timeout (`NetServerOptions` default 0 = disabled), and no cap on concurrent pre-decision +connections; established `PassthroughRelay` sessions are likewise unbounded in count and +lifetime. A client that connects and sends nothing — or trickles bytes below the 32 KiB parser +bound — holds a socket indefinitely. The L4 front reintroduces the slow-loris class the HTTP +listener's `IDLE_TIMEOUT_SECONDS = 60` exists to close, positioned in front of every other +defence whenever `tls.passthrough_sni` is configured. + +=== DP-5 — SNI split makes every terminated peer address loopback + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence MEDIUM — Category: headers / tls +|Location |`tls/TlsEdgeProducer.java:112`; `edge/GatewayEdgeRoute.java:1153`; `forward/TcpPeerGate.java:60-76` +|=== + +With the SNI split active, every terminated connection reaches the Quarkus HTTPS listener +through the loopback relay (no PROXY protocol), so `peerAddress()` is `127.0.0.1` for all +external clients. Consequences: (a) the regenerated `X-Forwarded-For` reports loopback — the +real client IP is unrecoverable by upstreams; (b) if an operator adds a loopback CIDR to +`forwarded.trusted_proxies` (the natural response), every external client becomes a "trusted +proxy" and can spoof `X-Forwarded-For`/`Forwarded` freely — the exact attack the ADR-0003 peer +gate exists to stop. Neither `tls-edge.adoc` nor ADR-0017/0003 mentions this interaction. + +=== DP-6 — Fault-tolerance timeout strands the upstream connection + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence MEDIUM — Category: correctness / resource leak +|Location |`edge/DispatchStage.java:246-266` with `GatewayEdgeRoute.guardFor` (30 s FT timeout) +|=== + +On a SmallRye FT timeout the virtual thread is interrupted and a 504 rendered, but the in-flight +Vert.x dispatch is never cancelled: no `request.reset()`, and a response head arriving later is +immediately `pause()`d with no consumer ever attached — the pooled connection is stranded +holding an unread, backpressured body. Default HTTP/1.1 pool is 5 connections per host with no +client idle timeout configured (DP-2), so a slow upstream (>30 s to headers) leaks a pool slot +per timeout; five timeouts can wedge every route sharing that upstream tuple into +queued-then-504 — a small self-inflicted DoS amplifier. + +=== DP-7 — Protocol verb semantics exist only in documentation + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: pipeline / javadoc +|Location |`routing/ProtocolProcessor.java` (+3 impls); `config/RouteTableBuilder.java:477-489`; `edge/GatewayEdgeRoute.java:743-747` +|=== + +`supports()`/`standardMethods()` have zero call sites outside the SPI; `effectiveAllowedMethods` +falls back to all `HttpMethod` values regardless of protocol. So a `protocol: grpc` route with +no explicit `allowed_methods` passes GET/DELETE/PATCH into `dispatchGrpc`, and a +`protocol: websocket` route passes POST/PUT into `dispatchWebSocket` — while both processors' +Javadoc and `protocol-processors.adoc` state POST-only / GET-only verb scope. Concrete +misbehaviour: a plain (non-upgrade) request reaching `dispatchWebSocket` dials the upstream WS, +then `ctx.request().toWebSocket()` fails and the failure branch releases admission and closes +the upstream without ever ending the client's HTTP response — the client hangs until the 60 s +idle reap, with no status and no metric. + +=== DP-8 — Multi-valued request headers collapse to their first value + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: headers / correctness +|Location |`forward/ForwardPolicyStage.java:245,263` +|=== + +All forward modes copy `request.firstHeader(name)` / `values.getFirst()` into a +`Map`; additional values are silently dropped (repeated `Accept`, `X-Trace-*`, +split HTTP/2 headers). Security-consistency is preserved — route header matchers also use the +first value and stage-1/3 validation validates all values — but upstreams receive a silently +truncated header set. (Query parameters, by contrast, correctly forward all values.) + +=== DP-9 — Fragmented text frames are decode/re-encode corrupted + +[cols="1,4"] +|=== +|Severity |LOW — Confidence MEDIUM — Category: websocket / correctness +|Location |`edge/WebSocketRelayStage.java:287-295` +|=== + +`dataFrame` relays the first fragment of a fragmented text message via +`WebSocketFrame.textFrame(frame.textData(), frame.isFinal())`; `textData()` decodes fragment +bytes as UTF-8, so a fragment boundary splitting a multi-byte codepoint yields U+FFFD +replacement characters that are re-encoded and relayed — silent payload corruption +(continuations correctly use `binaryData()`). + +=== DP-10 — No rate limiting exists anywhere in the data plane + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: ratelimit / false-security +|Location |`config/model/RateLimitConfig.java:21-35` +|=== + +The per-route `rate_limit` block (`requests_per_second`, `burst`) is accepted and ignored +(Javadoc: feature deferred). No limiting, no boot warning, no runtime signal. The only +admission controls are the global concurrency semaphores (2048 / 512 relay), which bound +concurrency, not rate. README/configuration.adoc do disclose this (see +link:security-posture.adoc[SEC-5]); a boot WARN when the block is present would close the trap. + +=== DP-11 — Proxy-path 413 leaves the connection pinned (asymmetric with reserved path) + +[cols="1,4"] +|=== +|Severity |LOW — Confidence MEDIUM — Category: smuggling / correctness +|Location |`edge/GatewayEdgeRoute.java` (`handleGatewayRejection` vs `closeAfterOversizedReservedBody:586-595`) +|=== + +The reserved-body 413 path deliberately retires the connection (the file's own comment explains +why); the proxy-path mid-stream 413 (`ByteCappedBodyStream` breach) and stage-3 declared-CL 413 +render `problem+json` without `Connection: close` — the paused, partially-read body pins the +connection until the 60 s idle timeout. No desync (Netty keeps framing the discarded +remainder), only per-connection resource pinning — the same half-effectiveness the +reserved-body comment identifies, unfixed on the higher-volume path. + +=== DP-12 — Terminated-listener loopback confinement asserted by docs, not enforced + +[cols="1,4"] +|=== +|Severity |LOW — Confidence MEDIUM — Category: tls +|Location |`doc/development/tls-edge.adoc:78` vs `api-sheriff/src/main/resources/application.properties:99-109` +|=== + +The doc states the internal terminated HTTPS listener (8444) is "bound to `localhost` only", +but nothing sets `quarkus.http.host` — Quarkus defaults to `0.0.0.0`, so in a passthrough +deployment the "internal" listener is reachable on all interfaces unless the deployment adds an +undocumented property. Direct access bypasses no gateway policy (same listener, same pipeline), +but it exposes an extra externally-reachable port whose confinement the docs assert and the +code does not enforce. + +=== DP-13 — Origin allowlist does not normalize default ports; two origin comparators disagree + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: websocket +|Location |`pipeline/OriginValidationStage.java:74-80`; `config/RouteTableBuilder.java:498-508` +|=== + +Origin matching lowercases and compares whole strings — no default-port normalization. An +operator listing `https://app.example.com:443` never matches a browser's +`https://app.example.com` (browsers omit default ports): all legitimate clients are locked out. +Fail-closed, so not a bypass, but an undetectable misconfiguration — and the BFF's own +`originOf` (`BffRuntimeProducer.java:410-427`) explicitly normalizes default ports on the CSRF +side, so the two origin comparators disagree. + +=== DP-14 — CORS reflection without `Vary: Origin`; preflight answers before route selection + +[cols="1,4"] +|=== +|Severity |LOW — Confidence MEDIUM — Category: headers / correctness +|Location |`pipeline/SecurityHeadersStage.java:93-111` +|=== + +The request `Origin` is reflected into `Access-Control-Allow-Origin` (also under the `"*"` +branch) without ever emitting `Vary: Origin` — a shared cache may serve one origin's ACAO to +another allowed origin, breaking CORS for it (availability, not a grant). The preflight +short-circuit (204) also answers before route selection, so any path — including nonexistent +routes — receives a positive preflight; minor surface-mapping aid, no access granted. + +=== DP-15 — 405s metered as ``; WebSocket relays unmetered + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: correctness (metrics) +|Location |`edge/GatewayEdgeRoute.java:716-719` +|=== + +`ctx.put(ROUTE_KEY, …)` happens only after the verb gate passes, so a 405 on a matched route is +metered as ``; established WebSocket relays never fire the end handler, so +`sheriff_requests_total`/duration record nothing for them. Observability gap only. + +=== DP-16 — Double-decode defence rests entirely on cui-http + +[cols="1,4"] +|=== +|Severity |INFO — Confidence LOW — Category: path +|Location |`pipeline/CanonicalPathGuard.java:53-69`; `pipeline/BasicChecksStage.java:164-170` +|=== + +The guard rejects literal `%2f`/`%5c`/`;` on the raw path only; defence against double-encoded +separators (`%252f`) and dot-segment/unicode tricks rests on the external cui-http +`urlPathPipeline`. That reliance appears sound — the pipeline is pinned against the OWASP +attack corpus in `BasicChecksStageTest`, and the TOCTOU property holds structurally (route +matching, `allowed_paths`, and the forwarded upstream path all consume the same +`canonicalPath`) — recorded as the one place where a cui-http regression would not be caught by +gateway-local code. + +=== DP-17 — `renderProblem` builds JSON by string concatenation (closed inputs, unpinned) + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: correctness (found during adversarial verification) +|Location |`edge/GatewayEdgeRoute.java:1116` +|=== + +The RFC 9457 problem body is assembled by string concatenation from enum-derived +`type`/`title` values. Safe today because the inputs come from a closed enum, but it is an +injection-shaped seam with no test pinning that closure — a future dynamic value flowing into +the template would need JSON escaping that does not exist. + +=== DP verified positives + +* Anti-smuggling `FramingGate`: rejects CL+TE, duplicate/comma-listed Content-Length, and + body-on-bodyless-method; the GET opt-in is cut exactly to the Content-Length leg per + ADR-0029. +* Single-canonical-path invariant holds structurally: authorization, `allowed_paths`, and the + upstream URI all consume the same stage-1 canonical string — no normalization TOCTOU. +* Route selection is deny-by-default over a longest-prefix-sorted table with segment-boundary + prefix matching. +* Body cap enforced both as pre-read declared-CL 413 and as a streaming counter aborting the + in-flight upstream request (ADR-0023), including the framework-floor boot validation. +* Inbound `X-Forwarded-*`/`Forwarded` (including underscore spellings and vendor IP aliases, + closed by separator folding) never propagate — only regeneration behind the boot-parsed CIDR + peer gate (trust-all CIDR union refused at boot, split ranges included). +* Gateway-owned header sets match ADR-0037 exactly, including `TE: trailers` and the + Authorization-readmission carve-outs. +* Auth runs strictly before forwarding; the auth-stage short-circuit gate prevents an + unauthenticated session request from falling through to the upstream. +* WebSocket admission-permit lifecycle is CAS-guarded on every teardown path (ADR-0020); the + SNI parser is fail-closed, bounds-checked, and buffer-bounded (ADR-0017). diff --git a/doc/quality-report/documentation.adoc b/doc/quality-report/documentation.adoc new file mode 100644 index 00000000..4ec6c8bf --- /dev/null +++ b/doc/quality-report/documentation.adoc @@ -0,0 +1,269 @@ += Documentation Quality Report +:toc: macro +:toclevels: 2 + +Review of the AsciiDoc tree (root README, `doc/` with 37 ADRs, `doc/user/`, +`doc/development/`, `doc/plan/`, `doc/variants/`, module docs) and the production Javadoc, +checked for structure, accuracy against code, completeness, ambiguity, duplication drift, and +currency. See link:README.adoc[the index] for conventions. + +toc::[] + +== Verdict + +*One of the most accurate documentation trees reviewed.* Every cheaply checkable technical +claim verified exactly: config keys against the bundled JSON schemas, a dozen numeric defaults +against code constants, the complete log-message catalogue including template strings, the +never-forward header set, the 21-entry content-type map, the six reserved paths. The link +graph of 751 local references contains zero broken targets. The `LogMessages.adoc`/code parity +is perfect in both directions. The needed fixes are a small number of stale self-descriptions, +not systemic quality problems. + +== Findings + +=== DOC-1 — architecture.adoc "Implementation Deviations" materially misstates the product + +[cols="1,4"] +|=== +|Severity |*HIGH* — Confidence HIGH — Category: inaccurate/stale +|Location |`doc/architecture.adoc:1039-1044` +|=== + +The section claims: "*Data-plane structured logging is not yet emitted*. The data-plane +LogRecord catalogue (`ApiSheriffLogMessages` …) is defined but no edge stage emits it yet. The +only catalogue currently emitted is the configuration subsystem's `ConfigLogMessages` … those, +and only those, are catalogued in `LogMessages.adoc`." Every part of this is false today: +`ApiSheriffLogMessages`/`BffLogMessages` records are emitted from at least 10 production +classes (`edge/GatewayEdgeRoute.java:317,796`, `edge/UpstreamFailureMapper.java:119-121`, +`edge/WebSocketRelayStage.java`, `pipeline/OriginValidationStage.java`, +`tls/SniFrontListener.java`, `tls/PassthroughRelay.java`, `asset/DirectoryAssetSource.java`, +`bff/cookie/*`, `bff/refresh/TokenRefreshCoordinator.java`), and `LogMessages.adoc` catalogues +all three gateway catalogues (33 gateway records: 13 ApiSheriff + 12 Bff + 8 Config), not only +Config. The bullet also +misattributes the "auth-weakened / route-load" records to `ApiSheriffLogMessages` (they live in +`ConfigLogMessages`, IDs 3/101). Drift in the flagship architecture doc that a reader would use +to judge observability maturity. + +=== DOC-2 — CLAUDE.md pins the wrong Quarkus version + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: stale +|Location |`CLAUDE.md:7` (and `AGENTS.md:8`) +|=== + +Both claim "Quarkus 3.37.4"; `pom.xml:59` declares `3.38.2`. +Same finding as link:build-and-peripherals.adoc[PER-1]. + +=== DOC-3 — CLAUDE.md describes the benchmarks as WRK; they are k6 + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: stale +|Location |`CLAUDE.md` (Project Structure) +|=== + +"`benchmarks/` — WRK HTTP load testing benchmarks" — the module is k6-based +(`benchmarks/README.adoc`, `Dockerfile.k6`, `K6BenchmarkLogMessages`; root `README.adoc:92` +correctly says k6). WRK survives only in historical plan text. + +=== DOC-4 — CLAUDE.md and root README under-count the reactor's modules + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: incomplete +|Location |`CLAUDE.md` (Project Structure); `README.adoc:80-92` (Modules) +|=== + +Both list 3 modules; the reactor has 5 (also `demo-client` and `deployment`). CLAUDE.md itself +later says "-Werror … fails the build in all six modules" (root + 5), contradicting its own +3-module structure section. `demo-client` has substantial docs (`demo-client/doc/*.adoc`) that +no root-level index reaches. + +=== DOC-5 — doc/user/README.adoc cites a Known Limitation that no longer exists + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: duplication-drift +|Location |`doc/user/README.adoc:26` +|=== + +Cites "the single-node in-memory session store and *the cookie-mode schema gap*" as examples +"listed in Known Limitations". The root README's Known Limitations contains no +cookie-mode-schema-gap item, and "schema gap" appears nowhere else in the repo — a limitation +evidently fixed/renamed in the root list but not in this pointer. + +=== DOC-6 — ADR status hygiene: 20 of 37 "Proposed" while shipped and enforced + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence MEDIUM — Category: ambiguous/stale +|Location |`doc/adr/*` +|=== + +Examples verified in code: ADR-0017 (SNI split) is Proposed, yet configuration.adoc's +`[[_tls_active]]` says "Status — active … enforced at runtime" and `tls/SniFrontListener.java` +exists; ADR-0036 Proposed but implemented in `forward/ForwardPolicyStage.java`; ADR-0037 +Proposed but `http/ConnectionHeaders.java` implements the exact sets; ADR-0025 Proposed but +`config/NeutralTlsConfigSource.java` ships. A reader cannot tell a genuinely pending proposal +from an accepted-in-practice decision. + +=== DOC-7 — Plan 09 premise outdated by the doc/user layer + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: stale +|Location |`doc/plan/09-release-readiness.adoc:8-9` +|=== + +Premise "an operator-facing configuration guide (*none exists today* — `doc/` is design +documentation)" is outdated: `doc/user/` exists with 10 documents. The plan README correctly +marks plan 09 undelivered, but the plan body's inventory no longer matches reality. + +=== DOC-8 — FAPI status not re-stamped for 0.1.1 + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: stale (version stamp) +|Location |`doc/fapi_status.adoc:1,8,15`; `doc/README.adoc:82` +|=== + +Titled and asserted "as of the 0.1.0 alpha release" while the current cut is 0.1.1. The root +README re-verifies the FAPI gap "at the 0.1.1 cut", but the FAPI doc and its doc-index row were +not re-stamped. + +=== DOC-9 — Historical version citations reading as current + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: stale (point-in-time) +|Location |`doc/adr/0001-java25-runtime-baseline.adoc:37`; `doc/plan/01-base-implementation.adoc:41` +|=== + +ADR-0001 cites "cui-java-parent 1.5.1 and Quarkus 3.37.2" (now 1.5.4 / 3.38.2) with "Today's … +support" phrasing that reads as current; plan 01 is explicitly frozen by its merge banner, so +its stale citations are acceptable. + +=== DOC-10 — Curated ADR index omits 16 of 37 ADRs (self-declared drift) + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: structure +|Location |`doc/README.adoc:286-288` +|=== + +The index table stops rows at ADR-0019 + 0036/0037; ADR-0020–0035 have no row. The doc admits +this ("a missing row is index drift"), so it is transparent — but 16 ADRs are undiscoverable +from the curated index. + +=== DOC-11 — ADR filename convention flips mid-series + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: structure +|Location |`doc/adr/` filenames +|=== + +ADR-0001–0009 and 0013–0016 use short kebab-case slugs; ADR-0010–0012 and 0017–0037 use +full-sentence underscore filenames up to ~150 chars. Cosmetic, but directory listings and +links are unwieldy. + +=== DOC-12 — Javadoc misses the thread-safety-note and usage-example pillars + +[cols="1,4"] +|=== +|Severity |LOW — Confidence MEDIUM — Category: javadoc-missing (vs project standard) +|Location |`api-sheriff/src/main/java` (corpus-wide) +|=== + +The CLAUDE.md standard demands thread-safety notes and usage examples. Measured: only 24/163 +production files mention thread-safety/concurrency; ~6 carry usage examples. Mitigating: most +classes are immutable records/final types where safety is implicit, and every other pillar is +met (see the compliance table). + +=== DOC-13 — benchmarks/doc contains a single mislabeled pointer stub + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: structure +|Location |`benchmarks/doc/performance-scoring.adoc` +|=== + +A 5-line pointer stub titled "JWT Performance Scoring System" — TokenSheriff branding in an +API-gateway benchmarks module; the sole doc under `benchmarks/doc/`. + +== Verified-clean checks (positives) + +* *Link check: 0 broken references.* All 751 local `link:`/`xref:`/`include::` targets across + all `.adoc` files resolve; all 214 cross-file anchored references resolve to real anchors. +* *`LogMessages.adoc` is 100% in sync with code* — every LogRecord identifier in all four + catalogues documented, no doc entry lacks code, every template string matches + character-for-character, ID ranges (INFO 1-99 / WARN 100-199 / ERROR 200-299) hold, no + cross-catalogue collision. (Note: six documented BFF records have no *emission site* — a code + finding, link:code-correctness.adoc[BFF-2], not a catalogue mismatch.) +* *configuration.adoc matches the bundled JSON schemas field-for-field* (all 14 gateway + top-level keys, the full oidc/tls/edge_hardening/forward/websocket/rate_limit trees, the + endpoint/route tree). +* *Documented defaults verified in code*: admission_cap 2048 + relay quarter-derivation, + websocket idle 300, max_cookie_size 4096, auth-header budget 8192, management port 9000, + ssl-port 8443, 64M framework body floor, refresh leeway 30, `final_redirect` "/", the + 21-entry content-type map (exact list match), the never-forward header set including + provenance aliases, the six reserved paths. +* ADR numbering 0001–0037 continuous, no gaps/duplicates; every ADR has a Status section; the + one Superseded ADR (0034) and its superseder (0035) record the supersession in both + directions. +* No TODO/TBD/WIP/FIXME markers anywhere in the doc tree; no empty/stub docs besides DOC-13. +* variants/ vs user/ duplication candidates are consistent where sampled (ttl 3600, leeway 30, + cookie budget, session-store constraints agree across `variants/02|03`, + `user/bff-session|cookie`, configuration.adoc, root README). +* All class names referenced in architecture/dev docs exist in code; the only unresolvable + names are genuinely external library types. +* README "rate limiting not implemented" verified true in code. + +== Structure assessment + +`doc/` follows an explicit, well-executed three-layer model: design/reference under `doc/` +(architecture, configuration, threat model, ADRs), operator layer under `doc/user/`, +contributor layer under `doc/development/`, each with its own README index, plus +`doc/variants/` for the three deployment postures and `doc/plan/` for surviving plan docs. +Cross-linking is exceptional. Multi-layer topics (TLS edge, compose sample, container image, +BFF variants) deliberately name their layer split instead of duplicating. +`configuration.adoc`'s declared dual role (design + operator reference) is unusual but +consciously documented in three places. Weak spots: the curated ADR index omits 16 ADRs +(DOC-10); the ADR filename convention flips mid-series (DOC-11); `demo-client/` documentation +is a well-built island unreachable from the root or `doc/` indexes (DOC-4); and the plan +layer's frozen historical statements sit next to living docs with only plan 01 carrying a +"body is frozen" banner. + +== Javadoc compliance + +All 27 packages containing classes have substantive `package-info.java` (21–60+ lines, with +class-by-class stage maps). `@since` present in 161/163 files — all 136 class files carry it; +`gateway/package-info.java` and `quarkus/package-info.java` do not. A heuristic scan found 0 +undocumented public/protected methods (excluding `@Override`) across 358 method declarations; +manual reads (ReservedPathRegistry, GatewayConfig, EdgeHardeningConfig, CsrfDefence, +ConnectionHeaders, AssetResponseEnvelope, RpInitiatedLogout) found Javadoc consistent with +implementation. Individual Javadoc-vs-behavior contradictions found by the code passes are +filed as code findings (CFG-7, CFG-14, CFG-16, BFF-2, BFF-10, SEC-12, DP-7). + +[cols="3,1,3"] +|=== +|Package (de.cuioss.sheriff.gateway…) |Rating |Note + +|(root), asset, auth, bff.*, edge, events, forward, http, pipeline, routing, tls +|Good +|Full class+method docs, security rationale in prose + +|config, config.load, config.topology, config.validation(.rule), config.model (36 files) +|Good +|Records with per-`@param` docs + +|quarkus +|Good +| + +|Cross-cutting gap +|— +|Thread-safety notes in ~15% of files, usage examples in ~4% (DOC-12) — below the letter of the +CLAUDE.md standard, mostly mitigated by immutability +|=== diff --git a/doc/quality-report/security-posture.adoc b/doc/quality-report/security-posture.adoc new file mode 100644 index 00000000..02c5f04a --- /dev/null +++ b/doc/quality-report/security-posture.adoc @@ -0,0 +1,344 @@ += Security Posture Report — Claimed vs. Implemented +:toc: macro +:toclevels: 2 + +Cross-cutting audit verifying that the security posture *claimed* by the documentation +(threat model, architecture, configuration, ADRs, user docs, FAPI status) matches the +*implemented* one, end-to-end. A configuration key that parses but never reaches an +enforcement point counts as not implemented, per the project's own rule. See +link:README.adoc[the index] for conventions. + +toc::[] + +== Verdict + +*"Fail closed" is real in this codebase, and unusually consistently so.* Every enforcement +point traced actually throws or refuses rather than defaulting open. No log-and-continue path +in security-decision code, no permissive `orElse`, no TODO markers in security code; the +sensitive-data sweep across all 104 production logger calls came back clean (dispositions, +lengths, and modes only, with `toString()` redaction on secret-bearing records). Several +threat-model statuses actually *understate* what is implemented (SEC-6). The false-security +zones are narrow but real and are listed below; the residual risk lives in the doc-admitted +GAP rows, not in hidden contradictions. + +== Claims-verification table + +Verdicts: FULLY = fully implemented; PARTIAL = implemented with a described gap; +CONFIRMED = doc claim of a limitation confirmed accurate. + +[cols="4,1,4"] +|=== +|Claim (source) |Verdict |Evidence / gap + +|Deny-by-default routing: no route → 404, never forwarded (architecture, configuration) +|FULLY +|`pipeline/RouteSelectionStage.java:60-72` throws `NO_ROUTE_MATCHED`; no fallback route exists + +|Auth posture always written out, never inferred; unresolvable auth fails boot (ADR-0007) +|FULLY +|`config/RouteTableBuilder.java:243-258`; `ConfigValidator.java:736-747`; no default `Require` + +|Anchor auth floor cannot be weakened to `none` (ADR-0007/0013) +|FULLY +|`ConfigValidator.java:754-772` + +|Fail-closed boot: invalid config never serves; violations aggregated +|FULLY +|`ConfigValidator` single-pass collection; `TopologyResolver.java:190-196`; `ConfigProducer.java:236-237` + +|GW-01: one canonical path consumed by route/verb/auth/allowed_paths/upstream; `%2f`/`%5c`/`;` rejected (ADR-0026) +|FULLY (doc says PARTIAL) +|`pipeline/CanonicalPathGuard.java:53-68` and consumers; threat-model status stale in the conservative direction (SEC-6) + +|GW-02: CL+TE, duplicate CL, body-on-bodyless rejected 400; GET-body opt-in Content-Length-leg-only, default false (ADR-0029) +|FULLY (structure); test corpus a doc-admitted GAP +|`pipeline/FramingGate.java:93-148`; default false via `SecurityDefaultsConfig`; see SEC-2 + +|GW-03/ADR-0037: never-forward set under every mode; `Cookie` absolute; `Authorization` positive-list-only; separator folding +|FULLY +|`http/ConnectionHeaders.java:157-216`; `forward/ForwardPolicyStage.java:290-307` + +|GW-04: TCP-peer gate before forwarding headers honored; regenerate never append; empty trust set trusts no one; trust-all CIDR (incl. split ranges) boot-refused (ADR-0003) +|FULLY +|`forward/TcpPeerGate.java:60-76`; `ForwardPolicyStage.java:376-389`; `ConfigValidator.java:1040-1100` + +|GW-05: fixed boot-resolved upstream; scheme allowlist; redirects never followed; bounded asset fetch +|FULLY +|`TopologyResolver.java:190-206`; `UpstreamAssetSource.java:257` (`Redirect.NEVER`); proxy path has no `setFollowRedirects` anywhere (rests on Vert.x default — 3xx relayed, not followed) + +|JWKS `tls_profile`: unbound / anchor-free / trust-all each refuse boot, trust-all first +|FULLY +|`auth/JwksTrustProfileResolver.java:120-156` — exact documented order + +|`allowed_egress_hosts` absent → secure default (GW-05/BFF-07) +|FULLY (wiring) +|`TokenValidatorProducer` per-issuer list; enforcement lives in the token-sheriff dependency (not re-audited here) + +|GW-06: missing/malformed SNI fails closed to terminated-strict (ADR-0017) +|PARTIAL (doc-admitted) +|`tls/SniFrontListener.java`; `PassthroughHostGuardStage` Host-vs-SNI smuggle 404; behaviour tests unpinned per threat model + +|GW-07/ADR-0023: `max_body_bytes` streaming counter; mid-stream breach → 413; declared cap vs transport floor boot-checked +|FULLY +|`edge/DispatchStage.java:52-56,253,330`; `ThoroughChecksStage.java:44-46`; `ConfigProducer.java:236` + +|GW-07: admission cap + WS relay sub-budget (ADR-0020); YAML bombs rejected pre-bind (ADR-0010) +|FULLY +|`EdgeHardeningConfig` (2048/512); `ConfigLoader.java:360-368` + +|GW-09/ADR-0015: WS Origin allowlist fail-closed on bearer routes; origin checked before upstream dial +|FULLY for bearer; session gap real and doc-admitted +|`OriginValidationStage.java:67-81`; `ConfigValidator.java:1364`; see SEC-1 + +|GW-10: no admin API, no scripting, no shipped credential; management on separate port +|FULLY +|No mutation endpoint exists; `application.properties:15-63` + +|GW-11: config paths confined; secrets rule on pre-substitution scalar; asset canonicalize-then-confine +|FULLY +|`EnvSecretResolver.java:98-100`; `ConfigLoader.java:152,228-231`; `PathConfinement` shared by both sources + +|GW-12: rejections are problem+json category-only; secrets redacted in `toString()` +|FULLY +|`GatewayEdgeRoute.java:1102-1116`; `OidcConfig.java:90-91`; unexpected exceptions → opaque 500 + +|Inbound-filter floor non-skippable; omitted profile → `strict`; `minimal` refused on authenticated/BFF routes (ADR-0024) +|FULLY +|`BasicChecksStage` pre-route floor; `SecurityProfile.DEFAULT_PROFILE = STRICT`; `ConfigValidator.java:860-902` + +|ADR-0019: reserved BFF paths bypass only the url-parameter value pipeline, post-floor +|FULLY +|`GatewayEdgeRoute.java:704-715` (reserved dispatch after basicChecks/canonicalGuard/framingGate) + +|BFF-02: pending record single-use; `__Host-` binding cookie; constant-time state compare +|FULLY (server mode) +|`PendingAuthorizationStore.java:98-106`; `CallbackEndpoint.java:182-198,277-282`; cookie-mode TTL replay window is the documented accepted risk + +|BFF-13: callback parses the raw query, duplicates → 400 +|FULLY +|`CallbackEndpoint.java:164-172`; fed `ctx.request().query()` raw + +|BFF-09: logout token iss/aud/iat/`events`-object/`sub|sid`, rejects `nonce`; cookie-mode back-channel → 404 +|FULLY +|`LogoutTokenValidator.java:96-182`; `BackchannelLogoutEndpoint.java:113-117` + +|BFF-10/12: sealed cookie AES-256-GCM, fresh 96-bit nonce, AAD binding, single key, ~4KB budget, tamper → no session never 500 +|FULLY +|`SealedSessionCookieCodec.java:110-272` — every listed property present + +|Session cookies `__Host-` + HttpOnly + Secure + SameSite=Lax; session id regenerated at login +|FULLY (defaults) +|`SessionCookieCodec.java:43`; `SealedSessionCookieCodec.java:283-299`; `CallbackEndpoint.java:220`; operator may rename off `__Host-` (code-correctness BFF-6) + +|BFF-11: fixed CSRF defence, no disable knob; Origin exact else `Sec-Fetch-Site: same-origin` else 403 +|FULLY +|`CsrfDefence.java:102-125`; enforced for every `require: session` route; does not cover WS handshakes (SEC-1) + +|Session store bounded; full store refuses logins after one sweep; refresh failure destroys session +|FULLY +|`InMemorySessionStore.java:76-102`; `TokenRefreshCoordinator.java:172-180` + +|Return URL same-origin validated; post-logout state cookie single-use +|FULLY +|`LoginFlow.java:99-102`; `LoginInitiationEndpoint.java:112-117`; `RpInitiatedLogout.java:59,142-148` + +|`response_mode=query` tradeoff accepted, bounded by PKCE-S256 + binding cookie + state +|FULLY as documented +|`QueryResponseModeAuthorizationRequestBuilder`; deliberate, honestly documented risk acceptance + +|Secrets never in logs; rejections log dispositions only +|FULLY +|Sweep of all 104 `LOGGER.*` calls in main: dispositions/lengths/modes only + +|Rate limiting reserved-only, never produces 429 (README, configuration) +|CONFIRMED +|No non-model reference to `rateLimit` in runtime code; honest disclosure, still a parse-but-not-act key (SEC-5) + +|Management: one port, HTTPS default, no plain listener alongside +|FULLY +|`application.properties:36-85`; `Dockerfile.native:34` exposes only 8443/9000 + +|FAPI 2.0: NOT conformant (PAR, sender-constraining, client auth unmet) +|CONFIRMED accurate +|`BffRuntimeProducer` (null senderConstraint, `ClientSecretBasicAuth`); no `ParClient` use + +|GW-02 smuggling corpus, GW-08 h2 abuse bounds, GW-06 behaviour tests: GAP/PARTIAL +|CONFIRMED as claimed +|Gaps admitted, not hidden; h2c non-forward covered structurally + +|Compose-sample credentials are placeholders with warnings +|CONFIRMED +|`docker-compose.yml:42-44`; `sample-realm.json:33` (`CHANGE-ME-BEFORE-PRODUCTION`) +|=== + +== Findings + +=== SEC-1 — Session-WebSocket routes have neither Origin gating nor CSRF coverage + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: claim-gap (doc-admitted) +|Location |`pipeline/OriginValidationStage.java:71-73`; `ConfigValidator.java:1364`; `bff/csrf/CsrfDefence.java:104-106` +|=== + +A `require: session` WebSocket route with no `allowed_origins` gets no origin gating at all: +the boot rule forcing a non-empty allowlist applies only to `require: bearer` WS routes, and +the CSRF defence exempts GET — which is what a WS handshake is. A session cookie rides +ambiently on a cross-site WS upgrade → CSWSH, the exact GW-09 class the threat model cites. +Scenario: attacker page opens `wss://gateway/ws-route`; the browser attaches +`__Host-sheriff-session`; the relay is established authenticated as the victim. The threat +model records this as a GAP, so it is not an overclaim — but it is the largest genuinely open +hole for a BFF deployment that adds a session WS route. Same finding independently reached by +the config review (link:code-correctness.adoc[CFG-4]). + +=== SEC-2 — `Connection`-strip rejection list narrower than the folded header sets + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: claim-gap +|Location |`pipeline/FramingGate.java:59-62,140-148` +|=== + +The `Connection`-strip rejection list omits `x-forwarded-prefix` and the vendor aliases, and +does not separator-fold tokens — `Connection: X_Forwarded_For` passes while +`Connection: X-Forwarded-For` is rejected. Impact bounded: the gateway never honors +Connection-driven stripping and regenerates forwarding headers into a fresh outbound map; the +check is defence-in-depth for other hops. Still asymmetric with the folded-equivalence-class +rigor claimed for ADR-0037. Related: link:code-correctness.adoc[DP-1] (nominated-header +stripping missing entirely). + +=== SEC-3 — `hsts: {}` emits `max-age=0`, actively clearing HSTS + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: insecure-default +|Location |`pipeline/SecurityHeadersStage.java:78-82`; `schema/gateway.schema.json` (`hsts` has no required `max_age`) +|=== + +An operator writing `security_headers: { hsts: {} }` gets `Strict-Transport-Security: +max-age=0`, which actively clears HSTS state in browsers — worse than omitting the header. A +boot refusal or a non-zero default would match the project's fail-loud philosophy. + +=== SEC-4 — With `security_headers` omitted, no security headers are emitted + +[cols="1,4"] +|=== +|Severity |INFO/LOW — Confidence HIGH — Category: insecure-default (doc wording) +|Location |`pipeline/SecurityHeadersStage.java:65-68`; `doc/configuration.adoc:1690` +|=== + +With the block omitted, no HSTS/nosniff/frame-deny headers are emitted at all, yet +configuration.adoc opens the section with "applied to every response the gateway returns" and +only the CORS row states its default — a reader can plausibly assume the headers are on by +default. Doc wording, not a code bug. Test coverage for this stage is also the weakest in the +corpus (link:test-quality.adoc[TST-1]). + +=== SEC-5 — `rate_limit` parses and is dead (disclosed) + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: claim-gap (fully documented) +|Location |`config/model/RateLimitConfig.java` +|=== + +By the project's own CLAUDE.md rule this is the canonical "key that parses but does not act" — +but README and configuration.adoc state it plainly, so it is disclosure, not overclaim. See +link:code-correctness.adoc[DP-10] for the boot-WARN recommendation. + +=== SEC-6 — Threat-model status lags the code in the conservative direction + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: doc-overclaim (inverse) +|Location |`doc/security-threat-model.adoc` (GW-01, GW-02 status text) +|=== + +GW-01 status says the single-canonical-path invariant is "not stated explicitly anywhere, nor +is the divergence tested", yet `CanonicalPathGuard` exists and every consumer demonstrably +consumes `canonicalPath()`. GW-02 is marked GAP while `FramingGate` implements the rejections. +Stale in the safe direction; worth refreshing so the release-gate matrix stays truthful. + +=== SEC-7 — Revocation failure during logout is DEBUG-only + +[cols="1,4"] +|=== +|Severity |INFO — Confidence MEDIUM — Category: fail-open (accepted direction) +|Location |`bff/logout/RpInitiatedLogout.java:121-124` +|=== + +Token-revocation failure during logout logs at DEBUG and proceeds with local logout. The local +session is destroyed (fail-safe locally), but IdP-side tokens stay live until natural expiry +and an operator cannot observe systematic revocation failures. Consider WARN + counter. + +=== SEC-8 — Cookie mode silently generates an ephemeral boot key (documented) + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: insecure-default (documented) +|Location |`bff/cookie/CookieKeyMaterial.java:91-97` +|=== + +Cookie mode with no `encryption_key` generates an ephemeral key (`Mode.GENERATED`, +INFO-logged). Sessions die on restart and cannot span replicas; a multi-replica operator who +misses the caveat gets random logouts, not a vulnerability. Documented in +`doc/user/bff-cookie.adoc`. + +=== SEC-9 — Sample credentials (documented with warnings) + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: insecure-default (sample) +|Location |`deployment/compose-sample/docker-compose.yml:43-44`; `sample-realm.json:33` +|=== + +Keycloak `admin`/`admin` and client secret `CHANGE-ME-BEFORE-PRODUCTION`, both with explicit +in-file and in-doc warnings. Acceptable for a sample; listed for completeness. See also +link:build-and-peripherals.adoc[PER-3/PER-13] for the integration-stack port exposure and the +`start-dev` IdP. + +=== SEC-10 — Upstream response header values are name-filtered, not character-revalidated + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: claim-gap (doc-honest) +|Location |`edge/ResponseStage.java:68`; `asset/AssetResponseEnvelope.java:223` +|=== + +Only name-filtering via `RESPONSE_STRIP`; a compromised configured upstream could attempt +response splitting toward clients. The threat model states this explicitly ("the honest +statement of the control's reach"); bounded to inside-trust-boundary sources, as documented. + +=== SEC-11 — Only the first value of multi-valued client headers crosses upstream + +[cols="1,4"] +|=== +|Severity |INFO — Confidence MEDIUM — Category: claim-gap (undocumented semantics) +|Location |`forward/ForwardPolicyStage.java:255-264` +|=== + +Fail-safe direction (drops rather than smuggles), but silently changes semantics for +legitimately repeated headers; nowhere documented. Same code as +link:code-correctness.adoc[DP-8]. + +=== SEC-12 — `FramingGate` "re-runnable" claim describes a capability, not a call site + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: claim-gap (javadoc) +|Location |`pipeline/FramingGate.java` (class Javadoc) vs `edge/GatewayEdgeRoute.java:707` +|=== + +Described as "re-runnable after any header mutation" with exactly one call site. Harmless +today — the outbound header map is built fresh and framing names are in `REQUEST_STRIP`. + +== Posture strengths (verified) + +* Configuration validator: trust-all CIDR union math (split ranges included), minimal-profile + refusal, secrets-on-raw-scalar rule, body-cap-vs-transport-floor check, aggregated boot + errors. +* ADR-0037 header-set machinery with separator folding, both directions. +* AES-GCM sealed cookie with AAD binding and deterministic key-id gating. +* Callback flow: raw-query HPP parse, binding cookie plus constant-time state, store-invariant + single-use. +* Documentation honesty: Known Limitations, the FAPI non-conformance verdict, the + response-direction caveat, and the accepted-risk entries all match the code. diff --git a/doc/quality-report/test-quality.adoc b/doc/quality-report/test-quality.adoc new file mode 100644 index 00000000..09c81130 --- /dev/null +++ b/doc/quality-report/test-quality.adoc @@ -0,0 +1,246 @@ += Test Quality Report +:toc: macro +:toclevels: 2 + +Review of all three test surfaces — `api-sheriff/src/test/java` (~101 files), +`integration-tests/src` (~50 files), `benchmarks/src` — against the project's own testing +doctrine (`doc/development/declared-limit-assertion-coverage.adoc`, +`doc/development/test-corpus-integrity.adoc`) and the CLAUDE.md testing standard. See +link:README.adoc[the index] for conventions. + +toc::[] + +== Verdict + +*The tests are overwhelmingly real content tests, not coverage theater.* The matched-control +discipline — every admission paired with a rejection differing in exactly the dimension under +test — is applied consistently enough that inverting most security controls would demonstrably +turn tests red. The project audits itself against its own doctrine, and the doctrine documents' +claims (18 strengthenings, the inversion audit, the WIRED/GAP matrix) all check out against +HEAD. The "if the key were deleted, would a test go red" rule is honored for the big controls: +body caps, header caps, CSRF, WebSocket origin, host-smuggle, mTLS, framing, and path +confinement are all deletion-detectable. Forbidden frameworks are fully absent; there are no +disabled tests, no order dependence, and no silent exception swallowing. The one materially +*unrecorded* weakness is `SecurityHeadersStage` (TST-1). The known-gap list the project keeps +is accurate rather than optimistic. + +== Findings + +=== TST-1 — `SecurityHeadersStage`: response-header half untested, CORS negatives missing, inactive in IT topology + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: missing-edge-case / missing-negative +|Location |`api-sheriff/src/test/java/.../pipeline/SecurityHeadersStageTest.java` vs `pipeline/SecurityHeadersStage.java:75-111` +|=== + +The stage's primary documented job — emitting `Strict-Transport-Security`, +`X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY` — has zero assertions on the +stage's *emission from configuration* anywhere in the corpus; the test covers only the CORS +half (4 tests). (The nosniff/frame-deny literals do appear in `WebSocketRelayStageTest:246-249` +and `GrpcStatusMapperTest:176,183`, but only as fixture-supplied stage-0 header maps proving +relay paths *carry* them — never as assertions that the stage produces them; +`Strict-Transport-Security` has zero test hits of any kind.) Also untested: +`allowCredentials=true` emitting `Access-Control-Allow-Credentials`; a disallowed-origin +preflight *not* short-circuiting; null-config no-op; the `Allow-Methods/Headers` values. The +CORS origin match is case-sensitive `contains()` (line 122) while `OriginValidationStage` and +`CsrfDefence` lower-case — no test pins the asymmetry in either direction. The IT topology +(`integration-tests/src/main/docker/sheriff-config/gateway.yaml`) declares no +`security_headers` block, so the stage is inactive in every booted IT instance — the nosniff +assertions in the asset ITs come from the asset envelope, not this stage. By the project's own +rule ("an opt-in runtime feature needs a deployment-activation test"), this is an unrecorded +gap. Cross-references: link:security-posture.adoc[SEC-3/SEC-4] (the same stage's `max-age=0` +hazard and opt-in default). + +=== TST-2 — Strict-preset parameter-value and path length caps (1024) unproven at the boundary + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: missing-edge-case (project-acknowledged G6/G7) +|Location |tests referencing `maxParameterValueLength`/`maxPathLength` (only `GatewayEdgeRouteTest.java:450,480` — config-plumbing equality) +|=== + +No test at unit or IT level exercises 1024-admitted/1025-rejected for parameter values or +paths. By the deletion discriminator, these two Layer-2 caps are enforced (by the cui-http +validators) but not test-proven at their boundary. Already recorded as G6/G7 in the project's +own coverage doctrine — independently verified as real. + +=== TST-3 — JWKS SSRF egress guard: no runtime refused-egress test + +[cols="1,4"] +|=== +|Severity |MEDIUM — Confidence HIGH — Category: missing-negative (project-acknowledged G2 runtime leg) +|Location |`EgressAllowlistActivationWiringTest` (descriptor-parsing, correctly self-labelled WIRED); `TokenValidatorProducerTest.allowlistSurvivesTheProducerPath` +|=== + +The producer-seam behavioural test exists with a matched no-allowlist control, but no test +anywhere exercises an actual refused JWKS egress through a running gateway. For the flagship +SSRF control of a security gateway this is the highest-value missing runtime negative; the +project records it as the standing remainder's rank-2 item. + +=== TST-4 — TLS runtime legs: no real below-floor handshake or ALPN assertion + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: missing-negative (project-acknowledged G8/G11) +|Location |`TlsServerCustomizerTest` (394 lines, options-level, excellent); `MtlsHandshakeIT` (covers mTLS handshake) +|=== + +Options-level coverage is thorough incl. fail-closed boot cases ("TLS 1.3-only allowlist under +a 1.2 floor fails the boot"); missing is only the handshake leg: no test performs a real +below-floor handshake or asserts negotiated ALPN. The doctrine doc itself recommends the wiring +shape over an unaffordable handshake matrix. + +=== TST-5 — Declared session TTL / refresh leeway values not deletion-detectable + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: missing-edge-case (deployment layer; project-acknowledged G9) +|Location |`InMemorySessionStoreTest`, `SealedSessionPayloadTest`, `TokenRefreshCoordinatorTest` (mechanisms well covered) +|=== + +Unit coverage of the mechanisms is strong and boundary-inclusive with injected clocks +(inclusive TTL boundary; within/outside leeway; single-flight concurrency with real threads). +The gap: no descriptor guard asserts the declared values (`ttl_seconds: 3600`, +`leeway_seconds: 30`) and no IT forces a refresh at the leeway boundary — deleting either key +would turn nothing red. + +=== TST-6 — `allowed_claims` negative leg missing at IT level only + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: missing-negative (IT wiring; project-acknowledged G10) +|Location |`BffUserInfoIT`; unit negative exists at `ClaimAllowlistFilterTest.java:113` +|=== + +The unit level has both the "claim outside allowlist silently dropped" negative and the "empty +allowlist discloses nothing" secure default. At IT level, `BffUserInfoIT.java:63-64` does +assert `assertNull(response.path("claims.client_secret"), …)` — but `client_secret` is not a +claim any IdP puts in an ID token, so the assertion passes even with the allowlist deleted: a +vacuous negative that cannot detect removal of the filter. The substantive gap (no IT negative +for a *present-but-disallowed* claim) stands. + +=== TST-7 — Generator adoption below the stated standard (no misuse found) + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: generator +|Location |corpus-wide; `@GeneratorsSource` in 4 of ~101 unit-test files +|=== + +Against CLAUDE.md's "@GeneratorsSource preferred", it appears in only 4 files (10 occurrences: +`RouteTableBuilderTest`, `ConfigValidatorTest`, `ConfigValidatorRouteDisjointnessTest`, +`JsonWriterTest`). Where used, it is used well: `@EnableGeneratorController` present in 13 +generator-touching files (seed reproducibility), and `RouteTableBuilderTest:1020-1102` proves +prefix-ordering/disjointness laws generically over generated segments. Auth tests use the CUI +`TestTokenGenerators` (real generated tokens over offline JWKS). One deviation: +`tls/PassthroughRelayTest.java:95` hand-rolls `new Random(42)` — seeded, trivial. Mitigating: +most fixed values in this corpus are deliberate boundary/attack values (the OWASP attack +database is used parameterized in `BasicChecksStageTest`/`ThoroughChecksStageTest`), where +generators would be wrong. Verdict: under-adoption relative to the stated standard; no +generator misuse. + +=== TST-8 — Fixed-sleep timing tests could flake under CI load + +[cols="1,4"] +|=== +|Severity |LOW — Confidence HIGH — Category: hygiene +|Location |`WebSocketRelayStageTest:324,462`; `TokenRefreshCoordinatorTest:281,429`; (bounded polling in `TlsEdgeProducerTest:213`, `GatewayEdgeRouteTest:761-786` is fine) +|=== + +Every `Thread.sleep` carries a NOSONAR rationale. The fixed sleeps (400 ms against a 250 ms +idle timeout; 100 ms for "no observable hook for the follower reaching the in-flight join") +have margins that could flake on a loaded runner; the coordinator ones paper over a missing +synchronization seam. No other flakiness vectors exist: no `@TestMethodOrder`, no shared +mutable static state in ITs, no exception-swallowing catch blocks. + +=== TST-9 — Cookie carve-out exact at-cap admission untested at unit level + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: missing-edge-case (compensated at IT level) +|Location |`BasicChecksStageTest.java:196-217` +|=== + +The at-budget test uses 4096 against a 4608 cap and over-cap at 4609 — exact at-cap admission +(4608) is never asserted at unit level; the Authorization within-cap probe is 2007 chars, not +8192. Fully compensated by `DeclaredLimitBoundaryIT` (8192/8193 and 1024/1025 exactly, with the +401-vs-400 discriminator). + +=== TST-10 — Vacuous-marker backlog: spot-check confirms low yield + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: coverage-theater (residual, project-counted) +|Location |`doc/development/test-corpus-integrity.adoc` backlog (43 files / 140 occurrences) +|=== + +Independent spot-reads of six backlog files (`RouteSelectionStageTest`, `SheriffMetricsTest`, +`SessionCookieCodecTest`, `MtlsServerCustomizerTest`, `FramingGateTest`, +`OriginValidationStageTest`) found every `assertNotNull`/`assertDoesNotThrow` paired with +substantive assertions or matched controls — consistent with the doc's 0%-yield control +sample. The doc's 18 claimed strengthenings verified as actually landed in HEAD. + +=== TST-11 — Production classes without dedicated tests + +[cols="1,4"] +|=== +|Severity |INFO — Confidence HIGH — Category: untested-class +|Location |`ApiSheriffApplication` (trivial Quarkus main); `quarkus/TokenClientDslJsonReflection` (zero test references; sibling `ConfigModelReflection` has one); `pipeline/PipelineRequest` (no dedicated test; exercised in every pipeline test, but `firstHeader` case handling and `shortCircuit` semantics only implicitly covered) +|=== + +Everything else without a dedicated file is an interface, annotation, enum, or record covered +by contract/catalogue tests (verified per-name). + +=== TST-12 — Literal `Origin: null` never named as a case + +[cols="1,4"] +|=== +|Severity |INFO — Confidence MEDIUM — Category: missing-edge-case +|Location |`OriginValidationStageTest`; `CsrfDefenceTest` +|=== + +The literal `Origin: null` a sandboxed browser context sends is untested in both gates. Both +fail closed on it by construction (exact-match miss), so behaviour is safe but unpinned. + +=== TST-13 — Forbidden frameworks: none found (positive) + +Zero Mockito/PowerMock/Hamcrest imports across all three modules (grep-verified). Zero +`@Disabled`. Zero `System.out` (one suppression-annotated `IO.println` boot print in +`BaseIntegrationTest:45`). Test doubles are hand-rolled fakes; throwing providers are used as +neutralised fixtures — a technique, not a smell. + +== Coverage map + +*Well-tested* (boundary values, matched positive/negative controls, attack corpora): +`pipeline` (OWASP Top-10 attack DB parameterized through both stages; carve-outs tested at both +baselines with reflection-based "only-the-cap-changes" sweeps), `bff/cookie` + `bff/csrf` + +`bff/session` + `bff/pending` + `bff/logout` + `bff/refresh` (AES-GCM byte-flip per region, AAD +binding, key-withdrawal, inclusive TTL boundaries, single-use consumption, no-secret-disclosure, +real-thread single-flight concurrency), `asset` (traversal/symlink/real-path escapes, +at-cap/one-over/stat-underreport), `tls` (byte-level ClientHello fixture incl. fragmentation +and overrun fail-closed; fail-closed boot matrix; port-probed edge producer), `forward` +(three-mode × two-dimension matrix with spoofed-header and smuggle-through-allowlist +negatives), `config` (validator: 128 tests; loader; route table with generated-segment law +tests), `edge` (body ceiling at/over with keep-alive retirement), `auth`, +`http/ConnectionHeaders`, `events`, `quarkus` producers. `benchmarks` asserts values against +known-number fixtures with explicit "absent never renders as 0" negatives. + +*Weak spots*: `SecurityHeadersStage` (TST-1 — the one genuinely weak security-relevant class); +`RouteSelectionStage`/`VerbGateStage` (small but two-legged; matcher depth lives in +`RouteRuntimeTest`). + +*Integration tests* are genuinely end-to-end: they assert status *and* bodies/headers (RFC 9457 +`type`/`title` envelope; echoed upstream bodies proving forwarding; header presence/absence on +the upstream side). Negatives are pervasive: tokenless 401, malformed-token 401, CSRF 403 with +a 401 control proving gate ordering, foreign/absent-Origin WS 403, smuggled-Host 404 with +benign-Host 200 control, 413 one-byte-over with one-byte-under control, 405 with `Allow`. The +`*ActivationWiringTest` family carries anti-vacuity floors (`COMMITTED_DESCRIPTOR_COUNT`, +doctrine-note parsing that fails loud when markers are missing) — machine-checked note-to-test +coupling that is genuinely unusual rigor. + +== Highest-value next assertions + +1. TST-1 — `SecurityHeadersStage` response-header assertions + a deployment-activation IT row. +2. TST-2 — G6/G7 1024/1025 boundary tests for parameter values and paths. +3. TST-3 — the runtime refused-JWKS-egress negative through a running gateway.