Code review · Round 11 · Remediation check and fresh pass

The fixes landed. One tier is missing.

Every round-10 finding that was targeted is closed, several more thoroughly than asked. The fresh pass over what changed found a cross-tenant problem that predates both rounds: the SaaS has one flat admin role doing the work of two, and role administration never consults the tenant scoping the data model already carries.

2026-09-05 Closed 6 of 6 targeted Carried forward 11 New 3 Method: static analysis, no build executed
trustweave
7.6/ 10+0.6

All four Verifiable Intent findings closed, with tests, and stricter than requested. What remains is engineering hygiene — untested plugins, no API-surface control, no artifact provenance — none of it touched this round.

Security
9.0
Correctness
8.5
Architecture
7.5
Testing
7.0
Supply chain & CI
6.0
trustweave-saas
7.0/ 10+0.1

Real gains in correctness and testing — the migration fix caught three tables that would have failed a first production deploy. Offset by a HIGH finding neither round had opened, which the new operations endpoint made visible.

Security
6.5
Correctness
8.5
Architecture
6.5
Testing
8.0
Supply chain & CI
5.5
Why the SaaS score barely moved

Correctness and testing both rose by a point and a half. Security fell by the same amount, because SA-R11-01 is worse than anything round 10 raised on this repo. It is not a regression — the code has been this way throughout — but the new /api/operations/** endpoint gave a cross-tenant reader one more thing to read, and reviewing that endpoint is what surfaced the role model behind it.

New this round

Findings

High

One flat admin role serves two tiers, and role administration ignores tenancy

SA-R11-01

Authorities come from Keycloak realm roles — KeycloakRealmRoleConverter maps realm_access.roles to ROLE_*, and both Spring's hasRole("admin") and TenantContext.requireRole("admin") read that one claim. There is no platform-administrator concept anywhere in the codebase; a search for one returns nothing.

That single role is required for ordinary tenant work: UserController calls requireRole("admin") to let a customer manage their own organization's users. It also gates operations that are realm-wide. So the administrator of any customer organization can:

Grant any realm role to any user in the realm. RoleService.assignRoleToUser is a bare passthrough to Keycloak. It never checks that the target Keycloak user belongs to the caller's organization, and never checks that roleName corresponds to a Role row the caller's organization owns — including granting admin itself. createRole carefully stamps the caller's organizationId on the database row, with a comment saying the org is never trusted from the body, but the Keycloak role it creates is realm-global and the assign/remove paths never read that column back.

Read every tenant's users and roles. getAllUsers() is userRepository.findAll(). getAllRoles() is roleRepository.findAll(). GET /api/roles/keycloak lists every realm role. searchUsers(query) searches the whole Keycloak realm and then writes local User rows for whoever it finds, so a cross-tenant read has a cross-tenant write as a side effect.

Read platform operations. /api/operations/** and /actuator/metrics/** both resolve to the same role. The new recovery panel's own error copy says “Check your platform administrator access” — naming a tier the authorization model does not have.

KeycloakJwtAuthenticationConverter.kt realm_access.roles → ROLE_*, one flat set TenantContext.kt:42-51 hasRole/requireRole read the same claim RoleService.kt:86-93 assignRoleToUser / removeRoleFromUser → straight to Keycloak RoleService.kt:23-25 getAllRoles() = roleRepository.findAll() UserService.kt:29-31 getAllUsers() = userRepository.findAll() UserService.kt:80-85 searchUsers() = realm-wide search, then syncs rows locally UserController.kt:33,40,72,109,116 requireRole("admin") for tenant-level work KeycloakSecurityConfig.kt:81,88 /actuator/metrics/**, /api/operations/** on the same role grep platform-admin|superadmin|global-admin → 0 hits
Fix: split the tier. Introduce a distinct realm role for platform operators and move /api/operations/** and /actuator/metrics/** onto it. Then scope role administration: assignRoleToUser should reject a target user outside the caller's organization and a roleName whose Role.organizationId is not the caller's, and getAllRoles/getAllUsers should filter by the caller's organization. The Role.organizationId column already exists — nothing reads it on the paths that matter.
Medium

The Keycloak admin credential kept the default the database credential just lost

SA-R11-02

SA-R10-06 removed postgres/postgres from the default profile so an unset DATABASE_PASSWORD fails at startup. Six lines further down the same file, the identity provider still carries admin-password: ${KEYCLOAK_ADMIN_PASSWORD:admin} alongside a matching username default and a localhost server URL. Both deployment profiles correctly declare it with no fallback.

This is the higher-value credential of the two, and it compounds SA-R11-01: the Keycloak realm administrator can mint users and grant realm roles directly, which is the same capability that finding is about reaching indirectly.

application.yml:41 admin-username: ${KEYCLOAK_ADMIN_USERNAME:admin} application.yml:42 admin-password: ${KEYCLOAK_ADMIN_PASSWORD:admin} application.yml:37 server-url: ${KEYCLOAK_SERVER_URL:http://localhost:8090} application-fly.yml:22 / application-staging.yml:35 no default ✓
Fix: drop the :admin fallbacks the same way the datasource ones went, and move the localhost convenience values into application-dev.yml where the database URL now lives.
Low

Ellipses became question marks in nine shipped UI strings

SA-R11-03

A lossy character conversion replaced … with a literal ASCII ? in user-facing text across the console. The bytes are plain 0x3F, not mojibake, so this survives any encoding fix and has to be corrected in the source. Loading states now read as questions.

App.tsx:46 "Loading page?" WebhookRecoveryPanel.tsx:15 "Loading recovery events?" CheckCredential.tsx:209 "Checking final status?" UserAssignmentsModal.tsx:76,106, AnchorToBlockchainModal.tsx:73, Explain.tsx:72, Blockchains.tsx:50, StarterKits.tsx:169 "Loading ?"
Fix: replace with a literal … or drop the punctuation. Worth checking whatever wrote these files — the same substitution shows up in round-9/findings.json, where an en dash became 1?128.
Remediation

Verified closed

Each re-read in the current source. Two went further than the recommendation.

✓ TW-R10-01 — constraint enforcement is now a shared checkConstraints helper called from both L3 branches with isOpenMandate = true. It also hardened beyond the ask: a missing or non-array constraints claim, and any malformed entry, now fail closed rather than resolving to an empty list. ChainVerifier.kt:245, 264, 286
✓ TW-R10-02 — requireReplayProtection: Boolean = true rejects the chain at entry when either expected audience or nonce is absent, and records the opt-out in checksSkipped when a caller deliberately disables it. ChainVerifier.kt:58, 64-67
✓ TW-R10-03 — a new temporalShape check requires iat and exp to be non-negative integers and rejects exp ≤ iat. It also catches string-encoded numerics, which I had not raised. L3 always requires both: the audit escape hatch is hardcoded off there and can only relax L1/L2. ChainVerifier.kt:68-73, 305-323, 380
✓ TW-R10-04 — the cross-check now fails when the L2 mandate carries no authorized payment_instrument, instead of returning a pass. ChainVerifier.kt:440
✓ SA-R10-01 — the default profile now runs ddl-auto: validate with Flyway enabled, and the dev conveniences moved to application-dev.yml. The stronger half is the new test: it runs the whole migration set against an empty PostgreSQL in its own container and derives the expected table set by reading @Table annotations off the source tree, so an entity added without a migration fails without anyone maintaining a list. It has already caught three tables that had no CREATE anywhere and a later ALTER TABLE against one of them — invisible in development, fatal on first deploy. application.yml:16,24 · application-dev.yml · FlywayMigrationTest.kt
✓ SA-R10-06 — the datasource credential defaults are gone from the default profile, so an unset DATABASE_PASSWORD now fails at startup. Partially: the Keycloak admin credential in the same file kept its default (SA-R11-02). application.yml:6-8
✓ Not requested — the SDK source verification in CI now pins the reviewed revision explicitly (--revision) rather than checking the hash alone, tightening a control that was already the best supply-chain practice in either repository. .github/workflows/ci.yml:96-101
✓ Consequence, correctly accepted — because ConstraintChecker fails closed on line_items for open mandates, enforcing checkout constraints means an autonomous L3 checkout cannot currently verify at all. That is the right trade, and it is pinned by two tests rather than left to be discovered. Worth tracking as a product limitation: the autonomous checkout path stays non-functional until line-item matching lands. ChainVerifierKnownAnswerTest.kt:60-77 · IssuanceRoundTripTest.kt:390-391
Unchanged

Carried forward

Eleven engineering-hygiene findings from round 10, each re-measured rather than assumed. The remediation was surgical; none of these were in scope.

IDFindingRe-measured
TW-R10-05Modules with production code and no tests21 modules
TW-R10-06No explicitApi, no binary-compatibility validatorstill absent
TW-R10-07Exceptions swallowed with no log line108
TW-R10-08Modules with coverage measurement3 of 106
TW-R10-09Baselined ktlint violations32,692
TW-R10-10No artifact signing, SBOM, or provenancestill absent
TW-R10-11Test dependencies invert module layering2 modules
SA-R10-02No ktlint, detekt, or spotless on the backendstill absent
SA-R10-03No Dependabot, no npm audit in CIstill absent
SA-R10-04Tenant isolation by convention, not constructionsee SA-R11-01
SA-R10-05Unused repository methods, several unscoped11 of 63

SA-R10-04 deserves a note: round 10 said isolation held by convention and nothing enforced it. SA-R11-01 is what that looks like when the convention is not followed. The suggested ArchUnit-style gate would not have caught it either — RoleController does reference TenantContext; it just does not use it on the paths that matter.

Scoring

Movement since round 10

Dimension SDK Δ SaaS Δ What moved it
Security 9.0+1.5 6.5−1.5 SDK: both HIGH findings closed and the fixes go further than asked. SaaS: SA-R11-01 is a cross-tenant privilege and data-exposure path.
Correctness 8.5+0.5 8.5+1.5 SDK: temporal handling now rejects what it used to pass. SaaS: one schema authority, and the migration set proven to build from empty.
Architecture 7.5+0.5 6.5−0.5 SDK: the shared constraint helper removed the asymmetry rather than patching it. SaaS: one role serving two tiers is a modelling gap, not a missing check.
Testing 7.0+0.5 8.0+1.0 SDK: substantial new adversarial VI cases; 21 modules still have none. SaaS: the migration test derives its own guard from source, which is the durable kind.
Supply chain & CI 6.0— 5.5+0.5 SDK: unchanged. SaaS: explicit revision pinning; still no static analysis, Dependabot, or audit.
Overall 7.6+0.6 7.0+0.1 Equal weight across the five dimensions.
Sequence

What I would do first

  1. SA-R11-01, part one — scope assignRoleToUser and removeRoleFromUser to the caller's organization, for both the target user and the role. This is the privilege-escalation half and it is a few lines against a column that already exists.
  2. SA-R11-01, part two — filter getAllUsers, getAllRoles and searchUsers by organization. searchUsers first: it is the one that writes.
  3. SA-R11-01, part three — add a platform-operator role and move /api/operations/** and /actuator/metrics/** onto it, so the recovery panel's copy becomes true.
  4. SA-R10-02 — ktlint on the SaaS backend. The library has had it on 106 modules throughout; this repo holds the tenancy logic and has none.
  5. TW-R10-06 — the binary-compatibility validator and committed .api dumps, still the cheapest thing to do before 1.0 freezes the surface.