Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 136 additions & 0 deletions doc/quality-report/README.adoc
Original file line number Diff line number Diff line change
@@ -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.
231 changes: 231 additions & 0 deletions doc/quality-report/build-and-peripherals.adoc
Original file line number Diff line number Diff line change
@@ -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.
Loading
Loading