From e29428b729ae3d63f14cd4b1e1f8c047c29d42ca Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:37:20 +0200 Subject: [PATCH 01/10] test(corpus): classify the test corpus and strengthen the capped subset Census the 135-file / 1364-method API Sheriff test corpus for assertions that cannot support the behaviour their names claim, publish the full classification as a checked-in note, and strengthen a capped subset highest-risk-first. Note (new): doc/development/test-corpus-integrity.adoc, indexed from doc/development/README.adoc. It carries the marker censuses with their complete-coverage evidence, the four vacuity shapes and three verdicts, the two selection gates and every higher-density file they excluded, one verdict row per flagged method with a file:line citation, and the standing backlog counted as 43 files carrying 140 marker occurrences. Strengthened: 18 methods (8 mandatory + 10 discretionary) against a cap of 20. The six BenignPassThrough tests in PassthroughHostGuardStageTest are both parameterized (closing java:S5976) and strengthened, since parameterizing alone would turn Sonar green while leaving all six vacuous. One test is deleted as strictly subsumed by a stronger sibling. Findings recorded in the note: marker density is a screen, not a verdict (three high-density files yielded zero strengthenings); EgressPolicy equality does not carry the host allowlist; TokenValidator exposes no view of its issuer configs. No production file is modified by this deliverable. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8 --- .../gateway/auth/AuthenticationStageTest.java | 11 - .../auth/TokenValidatorProducerTest.java | 180 +++- .../edge/RouteRuntimeAssemblerTest.java | 28 +- .../pipeline/BasicChecksStageTest.java | 7 +- .../PassthroughHostGuardStageTest.java | 116 ++- .../pipeline/ThoroughChecksStageTest.java | 55 +- .../quarkus/BffRuntimeProducerTest.java | 41 +- .../gateway/quarkus/ConfigProducerTest.java | 14 +- .../gateway/tls/TlsEdgeProducerTest.java | 137 ++- doc/development/README.adoc | 10 + doc/development/test-corpus-integrity.adoc | 829 ++++++++++++++++++ 11 files changed, 1277 insertions(+), 151 deletions(-) create mode 100644 doc/development/test-corpus-integrity.adoc diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.java index cc4af8d1..78441e05 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.java @@ -61,17 +61,6 @@ class AuthenticationStageTest { private static final String SESSION_ID = "opaque-session-id"; private static final String MEDIATED_TOKEN = "mediated-access-token"; - @Test - @DisplayName("passes a require:none route without inspecting any token") - void passesRequireNone() { - // Arrange - AuthenticationStage stage = stageFor(TestTokenGenerators.accessTokens().next()); - PipelineRequest request = request(authConfig("none", List.of()), Map.of()); - - // Act + Assert - assertDoesNotThrow(() -> stage.process(request)); - } - @Test @DisplayName("passes a require:none route without ever resolving the lazy validator") void passesRequireNoneWithoutResolvingValidator() { diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java index aacda9dd..c3cb08f7 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java @@ -17,12 +17,14 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; +import java.io.IOException; import java.net.URI; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.List; @@ -35,13 +37,23 @@ import de.cuioss.sheriff.token.commons.transport.EgressPolicy; import de.cuioss.sheriff.token.commons.transport.HttpJwksLoaderConfig; import de.cuioss.sheriff.token.validation.TokenValidator; - +import de.cuioss.sheriff.token.validation.domain.context.AccessTokenRequest; +import de.cuioss.sheriff.token.validation.domain.token.AccessTokenContent; +import de.cuioss.sheriff.token.validation.exception.TokenValidationException; +import de.cuioss.sheriff.token.validation.test.InMemoryKeyMaterialHandler; +import de.cuioss.sheriff.token.validation.test.TestTokenHolder; +import de.cuioss.sheriff.token.validation.test.generator.TestTokenGenerators; +import de.cuioss.test.generator.junit.EnableGeneratorController; + +import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; +@EnableGeneratorController @DisplayName("TokenValidatorProducer — builds the shared gateway validator from token_validation") class TokenValidatorProducerTest { @@ -62,39 +74,89 @@ void failsWhenTokenValidationAbsent() { assertEquals(EventType.CONFIG_INVALID, thrown.getEventType()); } - @Test - @DisplayName("builds a validator from an http-JWKS issuer with an explicit expected audience") - void buildsFromHttpIssuerWithAudience() { - // Arrange - TokenValidatorProducer producer = producerFor(IssuerConfig.builder() - .name("primary") - .issuer(ISSUER) - .audience("api-sheriff") - .jwks(IssuerConfig.Jwks.builder().source("http").url(JWKS_URL).build()) - .build()); + /** + * The {@code audience} posture the producer resolves, asserted behaviourally. + *

+ * The gateway's {@code audience} key is optional while token-sheriff requires an explicit choice + * at build time, so {@code toValidationIssuer} either sets an expected audience or sets the + * explicit opt-out. Neither choice is visible on the built {@link TokenValidator}, which exposes + * no view of its issuer configs — so the posture is asserted by *validating a real token* through + * the produced validator. That needs key material the validator can load without an IdP, which is + * why these two cases use a {@code file} JWKS source seeded from the in-memory test key material + * rather than the {@code http} source the surrounding cases use. + */ + @Nested + @DisplayName("audience posture — expected audience vs the explicit opt-out") + class AudiencePosture { - // Act - TokenValidator validator = producer.gatewayTokenValidator(); + @TempDir + Path jwksDir; - // Assert - assertNotNull(validator, "an http issuer with a jwks url yields a built validator"); - } + @Test + @DisplayName("a declared audience reaches the validator and refuses a token that does not carry it") + void declaredAudienceIsEnforced() throws IOException { + // Arrange — the declared audience is deliberately not one the generated token carries + TestTokenHolder holder = TestTokenGenerators.accessTokens().next(); + TokenValidator declaringForeignAudience = fileIssuerValidator(holder, "an-audience-the-token-lacks"); + TokenValidator declaringOwnAudience = + fileIssuerValidator(holder, holder.getAudience().iterator().next()); + AccessTokenRequest request = AccessTokenRequest.of(holder.getRawToken()); + + // Act + Assert — the declared audience is genuinely applied ... + assertThrows(TokenValidationException.class, + () -> declaringForeignAudience.createAccessToken(request), + "a declared audience must reach the built validator and refuse a token without it"); + + // ... and the matched control: the same token, key material and issuer pass once the + // declared audience is one the token actually carries, so the refusal above is + // attributable to the audience decision and not to the token or the key set. + AccessTokenContent accepted = declaringOwnAudience.createAccessToken(request); + assertEquals(holder.getAudience(), accepted.getAudience(), + "the accepted token carries exactly the audience the issuer declared"); + } - @Test - @DisplayName("builds a validator from an http-JWKS issuer that configures no audience (validation disabled)") - void buildsFromHttpIssuerWithoutAudience() { - // Arrange — no audience means audience validation is explicitly disabled at build time - TokenValidatorProducer producer = producerFor(IssuerConfig.builder() - .name("primary") - .issuer(ISSUER) - .jwks(IssuerConfig.Jwks.builder().source("http").url(JWKS_URL).build()) - .build()); + @Test + @DisplayName("an issuer configuring no audience disables audience validation rather than refusing the token") + void audienceLessIssuerDisablesAudienceValidation() throws IOException { + // Arrange — the same token and key material, this time with no audience declared at all + TestTokenHolder holder = TestTokenGenerators.accessTokens().next(); + TokenValidator validator = fileIssuerValidator(holder, null); - // Act - TokenValidator validator = producer.gatewayTokenValidator(); + // Act + AccessTokenContent content = + validator.createAccessToken(AccessTokenRequest.of(holder.getRawToken())); + + // Assert — the token validates although the producer declared no expected audience, which + // is only possible because the audience-less branch sets the explicit opt-out. Without it + // token-sheriff's IssuerConfig.build() refuses to build the issuer at all, so this is the + // exact contrast with declaredAudienceIsEnforced above: swap the two arrange blocks and + // both tests fail. + assertEquals(holder.getAudience(), content.getAudience(), + "the token is admitted unchanged, audience claim included, with validation disabled"); + } - // Assert - assertNotNull(validator, "an audience-less issuer still yields a built validator"); + /** + * A producer whose single issuer loads its key set from an on-disk JWKS file, so validation + * runs fully offline. + * + * @param holder the generated token whose issuer identifier and key material are mirrored + * @param audience the {@code audience} to declare, or {@code null} to declare none at all + * @return the produced gateway validator + * @throws IOException when the JWKS fixture cannot be written + */ + private TokenValidator fileIssuerValidator(TestTokenHolder holder, @Nullable String audience) + throws IOException { + Path jwks = Files.writeString(jwksDir.resolve("jwks-%s.json".formatted(audience)), + InMemoryKeyMaterialHandler.createDefaultJwks()); + IssuerConfig.IssuerConfigBuilder issuer = IssuerConfig.builder() + .name("primary") + .issuer(holder.getIssuer()) + .jwks(IssuerConfig.Jwks.builder().source("file").file(jwks.toString()).build()); + if (audience != null) { + issuer.audience(audience); + } + return producerFor(issuer.build()).gatewayTokenValidator(); + } } @Test @@ -242,25 +304,61 @@ void severalHostsAreAllowlisted() { "every entry in allowed_egress_hosts must be applied, not just the first"); } + /** + * The built {@link TokenValidator} exposes no view of its issuer configs, so the last hop of + * the assembly is asserted at the producer's own {@code toHttpJwksLoaderConfig} seam driven + * with the very issuer the public entry point consumed. Driving + * {@link TokenValidatorProducer#gatewayTokenValidator()} first is what proves the allowlist + * does not abort the whole-graph assembly; the policy assertions are what prove it survived + * rather than being silently dropped back to the secure default. + */ @Test @DisplayName("the allowlist is carried through the full producer path, not only the seam") void allowlistSurvivesTheProducerPath() { // Arrange — drive the public producer entry point rather than the helper - TokenValidatorProducer producer = producerFor(IssuerConfig.builder() + IssuerConfig.Jwks jwks = IssuerConfig.Jwks.builder() + .source("http") + .url(JWKS_URL) + .allowedEgressHosts(List.of(BLOCKED_HOST)) + .build(); + IssuerConfig issuer = IssuerConfig.builder() .name("benchmark-keycloak") .issuer(ISSUER) - .jwks(IssuerConfig.Jwks.builder() - .source("http") - .url(JWKS_URL) - .allowedEgressHosts(List.of(BLOCKED_HOST)) - .build()) - .build()); + .jwks(jwks) + .build(); + TokenValidatorProducer producer = producerFor(issuer); - // Act - TokenValidator validator = producer.gatewayTokenValidator(); + // Act — the public entry point assembles the whole issuer graph + producer.gatewayTokenValidator(); + EgressPolicy withAllowlist = producer.toHttpJwksLoaderConfig(issuer, jwks).getEgressPolicy(); - // Assert - assertNotNull(validator, "an issuer carrying an egress allowlist still builds a validator"); + // Arrange the matched control — an otherwise identical issuer declaring no allowlist, + // driven through the same public entry point + IssuerConfig.Jwks plainJwks = IssuerConfig.Jwks.builder() + .source("http") + .url(JWKS_URL) + .build(); + IssuerConfig plainIssuer = IssuerConfig.builder() + .name("benchmark-keycloak") + .issuer(ISSUER) + .jwks(plainJwks) + .build(); + TokenValidatorProducer plainProducer = producerFor(plainIssuer); + plainProducer.gatewayTokenValidator(); + EgressPolicy withoutAllowlist = + plainProducer.toHttpJwksLoaderConfig(plainIssuer, plainJwks).getEgressPolicy(); + + // Assert — the declared allowlist survived the assembly ... + assertDoesNotThrow(() -> withAllowlist.check(BLOCKED_JWKS_URI), + "the allowlisted host must be reachable through the policy the producer path builds"); + + // ... and the admission is attributable to the allowlist rather than to an inert guard: + // the same path over the same host refuses it once the allowlist is gone. Asserting the + // policy is not EgressPolicy.secureDefault() would NOT do this job — EgressPolicy's + // equality does not carry the host allowlist, so an allowlisted policy compares equal to + // the secure default. + assertThrows(TransportException.class, () -> withoutAllowlist.check(BLOCKED_JWKS_URI), + "without the declared allowlist the same producer path must still refuse the host"); } private static EgressPolicy egressPolicyFor(IssuerConfig.Jwks jwks) { diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java index b29c5d4e..0797b094 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java @@ -172,27 +172,35 @@ void shouldAssembleSessionRoutes() { "the assembled route keeps its require:session posture for the stage-4 runtime to dispatch on"); // A session-auth WebSocket route likewise assembles — session auth no longer gates boot, so - // it is treated exactly like any other WebSocket route. + // it is treated exactly like any other WebSocket route. Each remaining leg asserts on the + // runtime it produced rather than on the absence of a throw: an assemble() that quietly + // dropped the route would return an empty list and satisfy a bare no-throw assertion. RouteTable webSocketSessionTable = new RouteTable(List.of( route("sw", Protocol.WEBSOCKET, "session", null, upstream("a.example")))); - assertDoesNotThrow( - () -> assembler.assemble(webSocketSessionTable, securityConfigFactory, clientFactory, guardFactory, assetSourceFactory), - "a session-auth WebSocket route assembles — session auth no longer fails boot"); + RouteRuntime webSocketSession = assembler.assemble(webSocketSessionTable, securityConfigFactory, + clientFactory, guardFactory, assetSourceFactory).getFirst(); + assertEquals("sw", webSocketSession.getId(), + "the session-auth WebSocket route reaches the assembled table"); + assertEquals("session", webSocketSession.getEffectiveAuth().require(), + "and keeps its require:session posture for the stage-4 runtime to dispatch on"); // A gRPC route with non-session auth assembles cleanly — the forced-h2 upstream client is // built by the injected client factory. RouteTable grpcTable = new RouteTable(List.of( route("g", Protocol.GRPC, "none", null, upstream("a.example")))); - assertDoesNotThrow( - () -> assembler.assemble(grpcTable, securityConfigFactory, clientFactory, guardFactory, assetSourceFactory), - "a gRPC route with non-session auth assembles cleanly"); + RouteRuntime grpc = assembler.assemble(grpcTable, securityConfigFactory, clientFactory, + guardFactory, assetSourceFactory).getFirst(); + assertEquals("g", grpc.getId(), "the gRPC route reaches the assembled table"); + assertNotNull(grpc.getHttpClient(), "a gRPC route carries the forced-h2 upstream client"); // A WebSocket route with non-session auth likewise assembles cleanly. RouteTable webSocketNoneTable = new RouteTable(List.of( route("w", Protocol.WEBSOCKET, "none", null, upstream("a.example")))); - assertDoesNotThrow( - () -> assembler.assemble(webSocketNoneTable, securityConfigFactory, clientFactory, guardFactory, assetSourceFactory), - "a WebSocket route with non-session auth assembles cleanly"); + RouteRuntime webSocketNone = assembler.assemble(webSocketNoneTable, securityConfigFactory, + clientFactory, guardFactory, assetSourceFactory).getFirst(); + assertEquals("w", webSocketNone.getId(), "the WebSocket route reaches the assembled table"); + assertEquals("none", webSocketNone.getEffectiveAuth().require(), + "and carries its declared require:none posture"); } @Test diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.java index 9eb54264..6b5711ad 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.java @@ -115,8 +115,11 @@ void acceptsLegitimatePath() { // Act defaultStage.process(request); - // Assert - assertNotNull(request.canonicalPath()); + // Assert — the recorded value is what route selection matches on, so its presence is not the + // claim: a canonicalizer that emitted "/" or echoed a half-decoded path would satisfy a + // non-null check while breaking every route match downstream. + assertEquals("/api/v1/users", request.canonicalPath(), + "the floor records the canonical form of the request path for route selection"); } @Test diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/PassthroughHostGuardStageTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/PassthroughHostGuardStageTest.java index b01535b5..e40734e3 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/PassthroughHostGuardStageTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/PassthroughHostGuardStageTest.java @@ -20,9 +20,11 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.params.provider.Arguments.arguments; import java.util.List; import java.util.Set; +import java.util.stream.Stream; import de.cuioss.sheriff.gateway.config.model.HttpMethod; @@ -34,6 +36,8 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; @DisplayName("PassthroughHostGuardStage — runtime Host-vs-SNI smuggle 404 guard") @@ -111,65 +115,83 @@ void rejectsBeforeRouteSelection() { @DisplayName("Benign pass-through (a request the guard must let flow to route selection)") class BenignPassThrough { - @Test - @DisplayName("passes a benign Host through to route selection") - void passesBenignHost() { - // Arrange - PipelineRequest request = requestWithHost("edge.public.example"); - - // Act + Assert — a benign Host is a no-op (the guard never touches it) - assertDoesNotThrow(() -> guardedStage.process(request)); + /** + * The five benign {@code Host} shapes, each paired with the reason it does not match the + * reserved SNI. Every row is driven twice: once through the guard configured with + * {@link #PASSTHROUGH_SNI} (it must pass, untouched), and once through a guard that reserves + * the row's own {@code Host} verbatim (it must be rejected). + *

+ * The second pass is what makes the first one evidence. An {@code assertDoesNotThrow} alone + * cannot distinguish "the normalized Host genuinely does not match" from an inert guard, a + * swallowed exception, or a {@code process()} that stopped inspecting the {@code Host} at all + * — every one of which leaves a bare no-throw assertion green. + * + * @return one row per benign Host shape + */ + static Stream benignHosts() { + return Stream.of( + arguments("a benign Host", "edge.public.example"), + arguments("a Host sharing only a suffix with the passthrough SNI", + "evil-backend.internal.example"), + arguments("a non-numeric :suffix, which is not stripped", PASSTHROUGH_SNI + ":notaport"), + arguments("an empty :suffix, which is not stripped", PASSTHROUGH_SNI + ":"), + arguments("a multi-colon suffix, which is not stripped", PASSTHROUGH_SNI + ":80:80")); } - @Test - @DisplayName("treats an absent Host header as a no-op") - void passesNullHost() { - // Arrange — the edge may build a request without a Host authority - PipelineRequest request = requestWithHost(null); - - // Act + Assert - assertDoesNotThrow(() -> guardedStage.process(request)); - } + @ParameterizedTest(name = "passes {0}") + @MethodSource("benignHosts") + @DisplayName("passes a benign Host through to route selection with the request untouched") + void passesBenignHostToRouteSelection(String shape, String host) { + // Arrange + PipelineRequest request = requestWithHost(host); - @Test - @DisplayName("passes a Host that only shares a suffix with the passthrough SNI") - void passesSuffixLookalikeHost() { - // Arrange — "evil-backend.internal.example" must NOT match "backend.internal.example" - PipelineRequest request = requestWithHost("evil-backend.internal.example"); + // Act + guardedStage.process(request); - // Act + Assert - assertDoesNotThrow(() -> guardedStage.process(request)); + // Assert — the guard is a pass-through pre-check: it neither rewrites the authority it + // inspected nor advances any routing state, so stage 2 receives the request as it arrived. + assertAll("benign pass-through: " + shape, + () -> assertEquals(host, request.host(), + "The guard must not rewrite the Host it inspected"), + () -> assertNull(request.selectedRoute(), + "The guard runs before route selection and must select no route"), + () -> assertNull(request.canonicalPath(), + "The guard must not canonicalize the request it passes through")); } - @Test - @DisplayName("does not strip a non-numeric :suffix, so the Host no longer matches the SNI") - void passesHostWithNonNumericPort() { - // Arrange — only a purely-numeric :port is stripped; "notaport" is kept, so the normalized - // Host retains the colon suffix and can no longer match the reserved SNI. - PipelineRequest request = requestWithHost(PASSTHROUGH_SNI + ":notaport"); + @ParameterizedTest(name = "reserving {0} rejects it") + @MethodSource("benignHosts") + @DisplayName("each benign pass is attributable to the Host not matching, never to an inert guard") + void benignPassIsAttributableToTheHostNotMatching(String shape, String host) { + // Arrange — reserve the very Host under test, so it normalizes onto itself and matches + PassthroughHostGuardStage attracting = new PassthroughHostGuardStage(List.of(host)); + PipelineRequest request = requestWithHost(host); - // Act + Assert - assertDoesNotThrow(() -> guardedStage.process(request)); - } - - @Test - @DisplayName("does not strip an empty :suffix, so the Host no longer matches the SNI") - void passesHostWithEmptyPort() { - // Arrange — a trailing colon with no port digits is not a strippable :port suffix. - PipelineRequest request = requestWithHost(PASSTHROUGH_SNI + ":"); + // Act + GatewayException thrown = assertThrows(GatewayException.class, () -> attracting.process(request)); - // Act + Assert - assertDoesNotThrow(() -> guardedStage.process(request)); + // Assert + assertEquals(EventType.PASSTHROUGH_HOST_SMUGGLED, thrown.getEventType(), + "A guard reserving this exact Host must reject it — otherwise the pass above is " + + "explained by the guard being inert rather than by the Host not matching"); } @Test - @DisplayName("does not strip a multi-colon suffix, so the Host no longer matches the SNI") - void passesHostWithMultipleColons() { - // Arrange — the :port strip only fires for a single colon; two colons leave the Host intact. - PipelineRequest request = requestWithHost(PASSTHROUGH_SNI + ":80:80"); + @DisplayName("treats an absent Host header as a no-op, leaving routing state untouched") + void passesNullHost() { + // Arrange — the edge may build a request without a Host authority + PipelineRequest request = requestWithHost(null); - // Act + Assert - assertDoesNotThrow(() -> guardedStage.process(request)); + // Act + guardedStage.process(request); + + // Assert — an absent authority is nothing to match, and nothing to invent either + assertAll("absent Host", + () -> assertNull(request.host(), "The guard must not synthesize a Host"), + () -> assertNull(request.selectedRoute(), + "The guard runs before route selection and must select no route"), + () -> assertNull(request.canonicalPath(), + "The guard must not canonicalize the request it passes through")); } } diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.java index 6c78880d..afdc9caf 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.java @@ -110,23 +110,62 @@ void acceptsLegitimateRequestUnderDivergentFilter() { @Test @DisplayName("skips the pipeline re-run when the route config equals the stage-1 default") void skipsReRunWhenRouteConfigEqualsDefault() { - // Arrange — a route whose config equals the default was already covered by stage 1 - PipelineRequest request = requestFor("/api/orders", - routeWithConfig(defaultConfiguration)); + // Arrange — a stage whose OWN baseline refuses this path, and a route carrying exactly that + // baseline. Only the skip-if-equal guard can admit the request: a stage that re-ran the path + // pipeline under the identical configuration would reject it here, one stage too late. + SecurityConfiguration baseline = SecurityConfiguration.strict(); + ThoroughChecksStage baselineEqualStage = stageWith(baseline, null); + PipelineRequest request = requestFor("/api/../etc/passwd", routeWithConfig(baseline)); // Act + Assert - assertDoesNotThrow(() -> stage.process(request, List.of())); + assertDoesNotThrow(() -> baselineEqualStage.process(request, List.of()), + "a baseline-equal route is not re-validated here — BasicChecksStage already did it"); + + // Matched control — the same path through the same stage, on a route that DIVERGES from the + // baseline by one dimension, IS re-run and rejected. Without it the admission above could + // not distinguish "the re-run was skipped" from "the re-run ran and found nothing". + PipelineRequest divergent = requestFor("/api/../etc/passwd", routeWithConfig( + SecurityConfigurations.builderSeededFrom(baseline).maxBodySize(64L * 1024 * 1024).build())); + GatewayException thrown = assertThrows(GatewayException.class, + () -> baselineEqualStage.process(divergent, List.of())); + assertEquals(EventType.SECURITY_FILTER_VIOLATION, thrown.getEventType(), + "a divergent route re-runs the path pipeline and rejects the very path the " + + "baseline-equal route was admitted with"); } @Test @DisplayName("falls back to the stage-1 baseline when a route carries no resolved configuration") void fallsBackToBaselineWhenRouteDeclaresNoConfig() { // Arrange — the posture resolver leaves every assembler-produced route with a configuration, - // so this covers only a RouteRuntime built without one. - PipelineRequest request = requestFor("/api/orders", routeWithConfig(null)); + // so this covers only a RouteRuntime built without one. The fallback is observable through the + // unconditional body cap: with no route policy, the baseline's cap is the effective one. + long baselineCap = defaultConfiguration.maxBodySize(); + PipelineRequest overCap = bodyRequest(baselineCap + 1, routeWithConfig(null)); + PipelineRequest atCap = bodyRequest(baselineCap, routeWithConfig(null)); - // Act + Assert - assertDoesNotThrow(() -> stage.process(request, List.of())); + // Act + GatewayException thrown = assertThrows(GatewayException.class, + () -> stage.process(overCap, List.of())); + + // Assert — the baseline's cap governs a route that declared none ... + assertEquals(EventType.CONTENT_TOO_LARGE, thrown.getEventType(), + "a config-less route falls back to the stage-1 baseline's body cap"); + + // ... and the boundary control: a body exactly AT that cap is admitted, so the rejection is + // attributable to the fallen-back cap rather than to a config-less route being refused wholesale. + assertDoesNotThrow(() -> stage.process(atCap, List.of()), + "a body at the baseline cap is within it — the fallback applies the cap, not a refusal"); + } + + private static PipelineRequest bodyRequest(long declaredContentLength, RouteRuntime route) { + PipelineRequest request = PipelineRequest.builder() + .method(HttpMethod.POST) + .requestPath("/api/orders") + .declaredContentLength(declaredContentLength) + .build(); + request.canonicalPath("/api/orders"); + request.selectedRoute(route); + return request; } @Test diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducerTest.java index eeb99941..9a44bbce 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducerTest.java @@ -16,18 +16,19 @@ package de.cuioss.sheriff.gateway.quarkus; import static org.junit.jupiter.api.Assertions.assertAll; -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.junit.jupiter.api.Assertions.assertTrue; import java.lang.annotation.Annotation; import java.lang.reflect.Field; import java.lang.reflect.InaccessibleObjectException; import java.lang.reflect.Modifier; +import java.time.Duration; import java.time.Instant; import java.util.ArrayDeque; import java.util.ArrayList; @@ -92,10 +93,46 @@ void shouldActivate() { assertNotNull(runtime.stepUpCoordinator()); } + /** + * Assembly must not perform an OIDC discovery round-trip — that is deferred to first engine + * use, which is what lets the gateway boot without a live IdP. A bare + * {@code assertDoesNotThrow} cannot say this: a producer that did resolve discovery + * against a reachable IdP would complete just as quietly. The issuer here is therefore on + * {@code 192.0.2.0/24} (RFC 5737 TEST-NET-1, guaranteed unroutable), so a discovery attempt + * would burn the connect timeout instead of returning — which the preemptive bound catches. + */ @Test @DisplayName("Should assemble without resolving OIDC discovery (no live IdP required)") void shouldAssembleWithoutDiscovery() { - assertDoesNotThrow(() -> producer(serverModeOidc()).bffRuntime()); + // Arrange — a well-formed server-mode configuration whose issuer nothing can reach + OidcConfig unreachableIssuer = OidcConfig.builder() + .issuer("https://192.0.2.1:9999/realms/nowhere") + .clientId("gateway-client") + .clientSecret("secret") + .scopes(List.of("openid")) + .redirectUri(REDIRECT_URI) + .session(OidcConfig.Session.builder().mode("server").ttlSeconds(3600).build()) + .userInfo(OidcConfig.UserInfo.builder() + .path("/auth/userinfo") + .allowedClaims(List.of("sub", "name")) + .defaultView(List.of("sub")) + .build()) + .login(OidcConfig.Login.builder().path("/auth/login").build()) + .build(); + + // Act + BffRuntime assembled = assertTimeoutPreemptively(Duration.ofSeconds(10), + () -> producer(unreachableIssuer).bffRuntime(), + "assembly must not reach the IdP — a discovery round-trip against an unroutable " + + "issuer would exhaust the connect timeout instead of returning"); + + // Assert — and what came back is a fully wired runtime, not a degraded or inert one + assertTrue(assembled.isActive(), + "an unreachable issuer still yields an active runtime, because discovery is deferred"); + assertEquals(401, assembled.dispatch(ReservedEndpoint.USER_INFO, + new BffRuntime.ReservedHttpRequest("", null, null, null, null, null, "GET"), + Instant.parse("2026-07-25T10:00:00Z")).status(), + "the reserved endpoints are wired although no discovery ever ran"); } @Test diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java index a5b2c59e..bb0aa072 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java @@ -283,13 +283,21 @@ void shouldBuildOnceAndCacheTheResult() throws Exception { assertSame(first, second, "the pipeline should be assembled once and cached"); } + /** + * The eager half of the startup contract: assembly happens at the startup event, not + * lazily on the first bean accessor. The distinction is load-bearing — a lazily-assembled pipeline + * surfaces a misconfiguration as a runtime failure on the first request instead of refusing to + * boot — and it is invisible to "onStartup did not throw", which a no-op body satisfies equally. + * The {@code CONFIG_LOADED} record is emitted by {@code buildOnce}, so observing it while no + * accessor has been called yet is what pins the assembly to the startup event. + */ @Test void shouldAssembleEagerlyOnStartupForValidConfig() throws Exception { ConfigProducer producer = producerForValidConfig(); - assertDoesNotThrow(() -> producer.onStartup(null), - "a valid configuration should assemble without failing startup"); - assertNotNull(producer.gatewayConfig(), "beans should be available after startup assembly"); + producer.onStartup(null); + + LogAsserts.assertLogMessagePresentContaining(TestLogLevel.INFO, "Configuration loaded successfully"); } @Test diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java index 1dddd75c..a5622792 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java @@ -15,8 +15,13 @@ */ package de.cuioss.sheriff.gateway.tls; -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import java.io.IOException; +import java.net.InetSocketAddress; +import java.net.ServerSocket; +import java.net.Socket; import java.util.Map; @@ -38,14 +43,19 @@ * Boot-wiring contract of {@link TlsEdgeProducer}: the relay map is built from {@code * tls.passthrough_sni} against the resolved topology, the accept-time front listener is started only * when at least one passthrough SNI resolves, and shutdown is a clean no-op when nothing was started. - * An unresolved passthrough alias is defensively skipped rather than aborting boot (ADR-0009). The - * front binds an ephemeral port so the test never contends for a fixed public port. + * An unresolved passthrough alias is defensively skipped rather than aborting boot (ADR-0009). + *

+ * Every case is asserted against the public port itself rather than against the absence of an + * exception: each test allocates a currently-free port, hands it to the producer, and probes whether + * anything accepts a connection on it. A quiet {@code onStartup} is not evidence that a front listener + * started, and is equally not evidence that one was deliberately skipped — the port is. The port is + * allocated per test rather than fixed, so the suite never contends for a well-known public port. */ @DisplayName("TlsEdgeProducer — accept-time front listener boot wiring") class TlsEdgeProducerTest { - private static final int EPHEMERAL_PORT = 0; private static final int INTERNAL_HTTPS_PORT = 8444; + private static final int CONNECT_TIMEOUT_MILLIS = 2000; private static final String RESOLVED_ALIAS = "backend-alias"; private static final String UNRESOLVED_ALIAS = "missing-alias"; @@ -67,10 +77,12 @@ class PassthroughConfigured { @Test @DisplayName("starts the front listener and shuts it down cleanly when a passthrough SNI resolves") - void startsAndStopsFrontListener() { + void startsAndStopsFrontListener() throws IOException { // Arrange — one SNI maps to a resolvable alias, one to an alias absent from the topology. // The resolvable entry makes the relay map non-empty (front started); the unresolved entry - // exercises the defensive skip branch. + // exercises the defensive skip branch. A concrete free port is used rather than the + // ephemeral 0 so that "is the front actually bound?" is an observable fact. + int port = freePort(); TlsConfig tls = TlsConfig.builder() .passthroughSni(Map.of( "sni.resolved.example", RESOLVED_ALIAS, @@ -80,15 +92,24 @@ void startsAndStopsFrontListener() { .tls(tls).build(); ResolvedTopology topology = new ResolvedTopology(Map.of( RESOLVED_ALIAS, new ResolvedUpstream("https", "backend.local", 9443, ""))); - TlsEdgeProducer producer = new TlsEdgeProducer(vertx, config, topology, EPHEMERAL_PORT, + TlsEdgeProducer producer = new TlsEdgeProducer(vertx, config, topology, port, INTERNAL_HTTPS_PORT); + assertFalse(isListening(port), "control precondition: nothing owns the port before startup"); - // Act + Assert — the front binds the ephemeral port on startup, then shutdown stops the - // started listener and closes the backend client without error. - assertDoesNotThrow(() -> producer.onStartup(new StartupEvent()), - "a resolvable passthrough SNI starts the front listener on an ephemeral port"); - assertDoesNotThrow(() -> producer.onShutdown(new ShutdownEvent()), - "shutdown stops the started listener and closes the backend client"); + // Act + producer.onStartup(new StartupEvent()); + + // Assert — the front is genuinely accepting connections on the public port. Absence of an + // exception would not say this: a startup that silently skipped the front (an empty relay + // map, a swallowed bind failure) completes just as quietly. + assertTrue(isListening(port), + "a resolvable passthrough SNI starts the front listener on the public port"); + + // Act — and shutdown releases it + producer.onShutdown(new ShutdownEvent()); + + // Assert + awaitNotListening(port, "shutdown stops the started listener and releases the public port"); } } @@ -98,40 +119,102 @@ class PassthroughUnconfigured { @Test @DisplayName("never starts the front listener when passthrough_sni is empty") - void noFrontListenerWhenPassthroughEmpty() { + void noFrontListenerWhenPassthroughEmpty() throws IOException { // Arrange — no tls block at all, so the relay map is empty. + int port = freePort(); GatewayConfig config = GatewayConfig.builder().version(1).build(); ResolvedTopology topology = new ResolvedTopology(Map.of()); - TlsEdgeProducer producer = new TlsEdgeProducer(vertx, config, topology, EPHEMERAL_PORT, + TlsEdgeProducer producer = new TlsEdgeProducer(vertx, config, topology, port, INTERNAL_HTTPS_PORT); - // Act + Assert — an empty relay map short-circuits startup; the later shutdown is a clean - // no-op because neither the listener nor the backend client was ever created. - assertDoesNotThrow(() -> producer.onStartup(new StartupEvent()), + // Act + producer.onStartup(new StartupEvent()); + + // Assert — the public port is left untouched. The matched control for this negative claim + // is startsAndStopsFrontListener above, which binds the same kind of port from a resolvable + // configuration: without it, "no exception" could not distinguish a deliberate short-circuit + // from a front that started perfectly well. + assertFalse(isListening(port), "an empty passthrough map never starts the front listener"); - assertDoesNotThrow(() -> producer.onShutdown(new ShutdownEvent()), - "shutdown is a no-op when nothing was started"); + + // Act + Assert — shutdown is a clean no-op because nothing was ever created + producer.onShutdown(new ShutdownEvent()); + assertFalse(isListening(port), "shutdown leaves the unbound port unbound"); } @Test @DisplayName("skips a passthrough SNI whose alias does not resolve, leaving the map empty") - void skipsUnresolvedAlias() { + void skipsUnresolvedAlias() throws IOException { // Arrange — the only passthrough SNI maps to an alias absent from the resolved topology, so - // the defensive skip leaves the relay map empty and no front listener is started. + // the defensive skip leaves the relay map empty and no front listener is started. This + // differs from noFrontListenerWhenPassthroughEmpty in the arrange that matters: a + // passthrough entry IS declared here, and only the alias lookup empties the map. + int port = freePort(); TlsConfig tls = TlsConfig.builder() .passthroughSni(Map.of("sni.unresolved.example", UNRESOLVED_ALIAS)) .build(); GatewayConfig config = GatewayConfig.builder().version(1) .tls(tls).build(); ResolvedTopology topology = new ResolvedTopology(Map.of()); - TlsEdgeProducer producer = new TlsEdgeProducer(vertx, config, topology, EPHEMERAL_PORT, + TlsEdgeProducer producer = new TlsEdgeProducer(vertx, config, topology, port, INTERNAL_HTTPS_PORT); - // Act + Assert - assertDoesNotThrow(() -> producer.onStartup(new StartupEvent()), + // Act + producer.onStartup(new StartupEvent()); + + // Assert — the declared-but-unresolvable entry contributed no relay target, so the map + // stayed empty and the front was never started + assertFalse(isListening(port), "an unresolved alias is skipped, so no front listener is started"); - assertDoesNotThrow(() -> producer.onShutdown(new ShutdownEvent()), - "shutdown is a no-op when the only alias was skipped"); + + // Act + Assert + producer.onShutdown(new ShutdownEvent()); + assertFalse(isListening(port), "shutdown is a no-op when the only alias was skipped"); + } + } + + /** + * A port no process owns at the moment of the call. The socket is closed before the port is + * handed back, which is what makes the pre-startup {@code assertFalse(isListening(port))} control + * meaningful. + * + * @return a currently-free localhost port + * @throws IOException when no ephemeral port can be allocated + */ + private static int freePort() throws IOException { + try (ServerSocket socket = new ServerSocket(0)) { + return socket.getLocalPort(); + } + } + + /** + * Whether something accepts a TCP connection on {@code port}. A refused connection is the + * observation, not an error, so it is reported as {@code false} rather than raised. + * + * @param port the localhost port to probe + * @return {@code true} when the connection is accepted + */ + private static boolean isListening(int port) { + try (Socket probe = new Socket()) { + probe.connect(new InetSocketAddress("localhost", port), CONNECT_TIMEOUT_MILLIS); + return true; + } catch (IOException refused) { + return false; + } + } + + // NOSONAR java:S2925 - Thread.sleep is load-bearing: SniFrontListener.stop() completes on the + // Vert.x event loop, so the unbind is a real asynchronous release with no virtual clock to advance. + @SuppressWarnings("java:S2925") + private static void awaitNotListening(int port, String message) { + for (int attempt = 0; attempt < 100 && isListening(port); attempt++) { + try { + Thread.sleep(20); + } catch (InterruptedException interrupted) { + Thread.currentThread().interrupt(); + break; + } } + assertFalse(isListening(port), message); } } diff --git a/doc/development/README.adoc b/doc/development/README.adoc index 4a3ce734..63e0eda9 100644 --- a/doc/development/README.adoc +++ b/doc/development/README.adoc @@ -83,6 +83,16 @@ This tree is seeded here and grows as contributor-facing material lands. report-only findings -- that the body-cap wiring guard's glob never reaches the `endpoints/` tree, and that the inventory guard partitions files rather than keys. +| link:test-corpus-integrity.adoc[Test-Corpus Integrity -- Assertions That Assert Nothing] +| Which test methods carry assertions that cannot support the behaviour their names claim -- the + 135-file / 1368-method declared surface (`@Test` union `@ParameterizedTest`, both counted), the + `assertDoesNotThrow` / `assertNotNull` shape censuses with their complete-coverage evidence, the + four vacuity shapes and the three verdicts, the two selection gates that bound the strengthening + pool and every higher-density file they excluded, one verdict row per flagged method with a + `file:line` citation, and the standing backlog counted as 43 files carrying 140 marker occurrences. + Also records that marker density is a screen and not a verdict: three high-density files yielded + zero strengthenings. + | link:../../demo-client/doc/playwright-suite.adoc[Demo Client and the Playwright End-to-End Suite] | The `demo-client` module -- why the demo SPA is served by the gateway rather than a side-car static server, the opt-in `skipPlaywrightTests` / `e2e-demo` build mechanism that keeps it out of diff --git a/doc/development/test-corpus-integrity.adoc b/doc/development/test-corpus-integrity.adoc new file mode 100644 index 00000000..c035addd --- /dev/null +++ b/doc/development/test-corpus-integrity.adoc @@ -0,0 +1,829 @@ += Test-Corpus Integrity -- Assertions That Assert Nothing +:toc: +:toclevels: 3 +:sectnums: + +Which test methods in the API Sheriff corpus carry assertions that *cannot support the behaviour +their names claim*, what verdict each one received, which twenty were strengthened, and -- the part +that makes this note auditable rather than anecdotal -- exactly how much of the surface was left +un-classified, counted. + +The companion note link:declared-limit-assertion-coverage.adoc[Declared-Limit Assertion Coverage] +does the same job for declared gateway limits; this one does it for assertion *strength*. Both follow +the same discipline: enumerate the whole surface first, credit what already exists by name, derive +the cap against the real gap count, and record the remainder as a *countable* standing backlog. + +== Why This Note Exists + +A test that calls production code and asserts only `assertDoesNotThrow(...)` or +`assertNotNull(result)` reports as coverage while proving almost nothing. It stays green when the +behaviour its `@DisplayName` promises is deleted, inverted, or never implemented. The corpus is +large enough that "we read everything and it looked fine" is not a claim anyone can check -- so this +note is built from *mechanical censuses whose counts are reproducible* plus a *targeted read of the +ranked candidates*, and it says plainly which files were read and which were not. + +== Method + +Census-then-targeted-read, deliberately -- not a linear 135-file pass. A linear pass over a corpus +this size degrades into spot-checks, which is precisely the apparent-vs-real-coverage failure this +sweep exists to catch. The three steps: + +. *Mechanical census* over the module-attributed inventory (`architecture search --content`), + recording occurrence counts and complete-coverage evidence. +. *Targeted read* of the ranked candidate files (100% of them) plus a *control sample* of the + un-ranked remainder, to measure whether the ranking actually selected the vacuous ones. +. *Classify* every flagged method with exactly one verdict, cite `file:line`, then strengthen a + capped subset highest-risk-first and count what is left. + +[IMPORTANT] +==== +*Coverage claims in this note cite census counts.* Where a file was not read, this note says so and +counts it into the backlog. There is no "the whole corpus was reviewed" assertion anywhere below, +because no such claim could be verified. +==== + +=== Search-form constraint (load-bearing) + +Every symbol-reachability census in this sweep searched *both* the call form `name(` *and* the +method-reference form `::name` before recording any "unused" / "unreached" verdict. + +This is not a formality. During outline a literal `unseal(` search returned *no production consumer* +for `SealedSessionCookieCodec.unseal`, while the real production call site is `codec::unseal` inside +`CookieSessionBinding.resolve()`. A call-form-only census would have produced a false "dead API" +verdict and an unsafe removal -- the exact apparent-vs-real failure this note is about, turned on the +note's own method. + +== Census + +Measured at `ec6f7e4` (`main`) over the D1 surface: `api-sheriff/src/test/**` (93 files) plus +`integration-tests/src/test/**` (42 files). `benchmarks/src/test/**` (3 files) is outside the surveyed +surface by deliberate scope decision; `demo-client/**` carries Playwright specs, not JUnit. + +=== Declared surface -- the test-method denominator + +[cols="2,1,1,3",options="header"] +|=== +| Marker | Files | Occurrences | Role + +| `@Test` +| 135 +| 1271 +| test-method denominator, part 1 + +| `@ParameterizedTest` +| 35 +| 97 +| test-method denominator, part 2 + +| *Declared surface (union)* +| *135* +| *1368* +| *the full test-method population* +|=== + +*Both annotations are part of the denominator.* A `@Test`-only count under-states the real test-method +population by 97 methods (7.1%). Of the 35 `@ParameterizedTest`-carrying files, 34 are under +`api-sheriff/src/test/**` (95 occurrences) and one is +`integration-tests/.../grpc/GrpcEchoServiceTest.java` (2). + +[NOTE] +==== +*The `~1273` figure carried into this plan resolves to exactly 1271.* The earlier count used the +loose pattern `@Test`, which also matches `@Test`-prefixed annotations. The strict pattern +`@Test(?![A-Za-z0-9_])` yields 1271, and a separate census for `@Test[A-Za-z0-9_]` returns exactly +two hits -- the Quarkus `@TestProfile` annotations in +`api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/MountedTlsMapKeyTest.java` and +`api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/CookieModeBootTest.java`. 1271 + 2 = 1273. +The approximation is now an exact number with its two false positives named. +==== + +=== Vacuous-shape markers + +[cols="2,1,1,1,3",options="header"] +|=== +| Marker | Files | Occurrences | Call sites | Role + +| `assertDoesNotThrow` +| 28 +| 120 +| 92 +| primary vacuous-shape marker -- asserts nothing whatsoever about an outcome + +| `assertNotNull` +| 40 +| 147 +| 107 +| secondary (weaker) marker -- a construction test can be legitimately scoped around it +|=== + +*Occurrences vs call sites.* The occurrence count is the raw marker census and includes the +`import static` line in each file; the call-site count uses the pattern `marker\(`. For +`assertDoesNotThrow` the delta is exactly 28 -- one import line per file. For `assertNotNull` the +delta is 40, made up of the import lines plus one prose occurrence in +`api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/JwksTrustProfileResolverTest.java`, which +carries no `assertNotNull` import at all (see <>). + +*The published gates below are stated against the raw occurrence counts*, because that is the census +the selection was reproduced from. Both numbers are given so nothing is hidden behind a single figure. + +*Union of the two marker sets over the D1 surface: 58 files, 267 marker occurrences.* + +=== Complete-coverage evidence + +Every census above ran under complete coverage. The inventory reports, per run: + +[cols="3,1,1,1,1",options="header"] +|=== +| Census | Files scanned | Unreadable | Truncated | Elided + +| `@Test(?![A-Za-z0-9_])` | 300 | 0 | no | 0 +| `@Test[A-Za-z0-9_]` | 300 | 0 | no | 0 +| `@ParameterizedTest` | 300 | 0 | no | 0 +| `assertDoesNotThrow` | 300 | 0 | no | 0 +| `assertDoesNotThrow\(` | 300 | 0 | no | 0 +| `assertNotNull` | 300 | 0 | no | 0 +| `assertNotNull\(` | 300 | 0 | no | 0 +|=== + +`files_scanned: 300` is the whole `test`-category inventory (the D1 surface plus `benchmarks` and the +duplicate `api-sheriff-parent` module attribution, both filtered out when the per-module rows are +de-duplicated by path). + +=== Re-census rule (mandatory, both annotations) + +*Any post-change re-census MUST count both annotations.* This sweep converts the six +`BenignPassThrough` `@Test` methods in `PassthroughHostGuardStageTest` into parameterized form, so a +`@Test`-only re-census would report a phantom six-method drop *in the very file the sweep +strengthens* -- a measurement artefact read as a regression. That file already mixes both forms (it +carries one `@ParameterizedTest` before this change and after it). + +== Taxonomy + +=== What counts as flagged + +A test method is *flagged* when its assertions cannot support what it claims. Two classes: + +F1:: *Wholly vacuous* -- the method's entire assert-block consists only of `assertDoesNotThrow` +and/or `assertNotNull`. +F2:: *Partially vacuous* -- the method mixes a substantive assertion with one or more additional +*distinct scenarios* whose only assertion is a vacuous marker. + +=== The four shapes + +[cols="1,3",options="header"] +|=== +| Shape | Description + +| (a) | construction / wiring-only evidence -- the object was built, nothing about it was inspected +| (b) | call returned without inspecting the return value +| (c) | tautological given the arrange block -- the assertion is entailed by the setup +| (d) | *highest risk* -- named or documented for a behaviour its assertions do not exercise +|=== + +Shape (d) is ranked highest because it is the only shape that actively *misleads*: the +`@DisplayName` reads as a specification and the green result reads as its proof. + +=== The three verdicts + +strengthen:: The claim is not supported. Applies when the name promises a specific post-condition the +assertion cannot distinguish (d), when a value-returning call discards its return (b), or when the +assertion is entailed by the arrange block (c). +keep:: *Correctly scoped.* The named behaviour genuinely *is* "completes without throwing" or "is +constructible", AND the admission is *attributable* -- either a matched negative control exists (a +sibling test whose arrange differs in exactly the dimension under test and which fails), or the +fixture itself is neutralised so a wrong implementation would throw (e.g. a provider that raises when +resolved, proving a code path never reached it). +delete:: The test is fully subsumed by a strictly stronger sibling and carries no distinct arrange. + +The *matched-control* requirement is what keeps `keep` from becoming a rubber stamp. It is the same +standard +link:../adr/0030-Comment-only_invariants_become_positively-phrased_fitness_functions_shipped_with_a_four-leg_control_set.adoc[ADR-0030] +sets for machine-checked invariants -- that ADR names the *always-passing assertion* as one of four +vacuity sources ("a rule that asserts only the absence of something passes when the subject is +renamed, moved, or deleted") and requires a *negative control* leg before a gate may be trusted as +enforcement. This note applies the same two ideas to ordinary unit tests. + +== Selection + +=== The cap + +*20 methods maximum: 8 mandatory + up to 12 discretionary.* The cap is a deliberate bound on this +sweep, not a quota to fill. Everything above it is a countable backlog, not an invitation. + +*Actually strengthened: 18 (8 mandatory + 10 discretionary).* The discretionary pool yielded ten +`strengthen` verdicts, not twelve -- the remaining flagged methods in the pool earned `keep` on the +matched-control test. Padding to twelve would have meant strengthening tests that were already +correctly scoped, which is churn, not integrity. + +=== The mandatory eight + +[cols="3,2,1",options="header"] +|=== +| Method | File | Shape + +| `buildsFromHttpIssuerWithAudience`, `buildsFromHttpIssuerWithoutAudience` +| `TokenValidatorProducerTest` +| (d) + +| `passesBenignHost`, `passesNullHost`, `passesSuffixLookalikeHost`, + `passesHostWithNonNumericPort`, `passesHostWithEmptyPort`, `passesHostWithMultipleColons` +| `PassthroughHostGuardStageTest` +| (d) +|=== + +The `TokenValidatorProducerTest` pair is the archetype of shape (d): the two `@DisplayName` s promise +a *contrast* -- "with an explicit expected audience" vs "configures no audience (validation +disabled)" -- while both assert only `assertNotNull(validator, ...)`. Swap the arrange blocks and both +still pass. + +The six `PassthroughHostGuardStageTest` methods are the two `java:S5976` triples. They require +*parameterize AND strengthen* -- both operations. Parameterizing alone closes the Sonar rule while +leaving all six vacuous under this note's own taxonomy: the gate would go green and these rows would +*look* handled, which is exactly the failure mode this note exists to catch. + +=== The two discretionary gates (applied in order) + +The discretionary pool is *not* a raw density ranking over the whole surface. Its membership is not +reproducible from density alone, so both gates are stated here. + +==== Gate 1 -- verification reachability + +*The mutation pool is restricted to `api-sheriff/src/test/**`.* This sweep's verification command is +`test -pl api-sheriff -am`, which does not run the `integration-tests` module. Strengthening an IT +assertion would leave it *unverifiable under this sweep's own gate* -- a strengthened assertion that +nothing runs is precisely the vacuity being eliminated. + +Three IT files meet or exceed the pool's density maximum and are excluded by this gate alone. Their +exclusion is a *verification-scope decision, not a risk judgement*: + +[cols="3,1,3",options="header"] +|=== +| File | `assertNotNull` | Note + +| `integration-tests/.../BffCookieSessionIT.java` +| 11 +| the single highest `assertNotNull` density in the entire D1 surface + +| `integration-tests/.../BffCookieActivationWiringTest.java` +| 8 +| clears gate 2's weaker leg; blocked by gate 1 only + +| `integration-tests/.../TlsEdgeActivationWiringTest.java` +| 8 +| clears gate 2's weaker leg; blocked by gate 1 only +|=== + +All three are routed to the <>. + +==== Gate 2 -- density threshold within the reachable set + +Admit a file when `assertDoesNotThrow >= 5` *OR* `assertNotNull >= 8` (raw occurrence counts). + +The two legs carry different thresholds deliberately. `assertDoesNotThrow` asserts nothing whatsoever +about an outcome and is the primary vacuity marker; `assertNotNull` is weaker evidence -- a +construction test can be legitimately scoped around it -- so the weaker marker must clear a higher +bar. + +*Reachable files excluded by gate 2, named explicitly* so the boundary is auditable rather than +implicit: + +[cols="3,1,1,3",options="header"] +|=== +| File | `assertDoesNotThrow` | `assertNotNull` | Why excluded + +| `api-sheriff/.../quarkus/SheriffMetricsTest.java` +| 0 +| 7 +| below the `assertNotNull >= 8` leg; carries no `assertDoesNotThrow` at all + +| `api-sheriff/.../config/model/ConfigModelContractTest.java` +| 0 +| 6 +| below the `assertNotNull >= 8` leg; carries no `assertDoesNotThrow` at all +|=== + +Both are survey-only and enter the <>. Their exclusion is reproducible arithmetic from the +published census -- no read is required to justify it. + +==== The resulting mutation pool (12 files, all read in full) + +[cols="3,1,1,1,1",options="header"] +|=== +| File | `aDNT` | `aNN` | Admitted by | Flagged + +| `pipeline/ThoroughChecksStageTest.java` | 15 | 0 | gate 2, leg A | 9 +| `pipeline/PassthroughHostGuardStageTest.java` | 8 | 0 | mandatory | 7 +| `edge/GatewayEdgeRouteTest.java` | 8 | 3 | gate 2, leg A | 5 +| `pipeline/BasicChecksStageTest.java` | 7 | 2 | gate 2, leg A | 7 +| `tls/TlsEdgeProducerTest.java` | 7 | 0 | gate 2, leg A | 3 +| `quarkus/ConfigProducerTest.java` | 7 | 8 | gate 2, both | 3 +| `auth/TokenValidatorProducerTest.java` | 6 | 4 | mandatory | 7 +| `bff/csrf/CsrfDefenceTest.java` | 6 | 0 | gate 2, leg A | 4 +| `edge/RouteRuntimeAssemblerTest.java` | 6 | 6 | gate 2, leg A | 1 +| `auth/AuthenticationStageTest.java` | 5 | 0 | gate 2, leg A | 3 +| `bff/runtime/SessionAuthenticationStageTest.java` | 5 | 0 | gate 2, leg A | 0 +| `quarkus/BffRuntimeProducerTest.java` | 2 | 8 | gate 2, leg B | 1 +| *Total* | *82* | *31* | | *50* +|=== + +Both mandatory files also clear gate 2 independently (`TokenValidatorProducerTest` at 6 `aDNT`, +`PassthroughHostGuardStageTest` at 8) -- they are listed as `mandatory` because that is the route by +which they entered the mutation scope, not because the gate would have missed them. + +=== Did the ranking work? -- the control sample + +Three files from the un-ranked remainder were read in full as a control, spanning both markers and +both modules (3 of 46 un-ranked marker-carrying files, a 6.5% sample): + +[cols="3,1,1,1,1",options="header"] +|=== +| Control file | `aDNT` | `aNN` | Flagged | `strengthen` + +| `api-sheriff/.../pipeline/FramingGateTest.java` | 4 | 0 | 3 | 0 +| `api-sheriff/.../pipeline/OriginValidationStageTest.java` | 4 | 0 | 3 | 0 +| `integration-tests/.../EgressAllowlistActivationWiringTest.java` | 0 | 6 | 0 | 0 +| *Total* | *8* | *6* | *6* | *0* +|=== + +*The ranking selected correctly.* The ranked pool yielded a 36% strengthen rate over its flagged +methods (18 of 50); the control sample yielded 0% (0 of 6). Every flagged method in the control +sample carried a matched negative control in the same file. This is evidence that the density gate is +a working screen -- and equally, evidence for the finding below that density alone is not a verdict. + +== Classification + +One row per flagged method, with a `file:line` citation and exactly one verdict. + +=== `auth/TokenValidatorProducerTest.java` -- 7 flagged + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `buildsFromHttpIssuerWithAudience` | 67 | *strengthen* | (d) name promises an audience contrast the assertion cannot distinguish +| `buildsFromHttpIssuerWithoutAudience` | 85 | *strengthen* | (d) the mirror of the above; swapping the arrange blocks leaves both green +| `listedHostIsExempted` | 193 | keep | matched control `exemptionIsScopedToTheListedHost`:211 +| `severalHostsAreAllowlisted` | 229 | keep | matched control `exemptionIsScopedToTheListedHost`:211 +| `allowlistSurvivesTheProducerPath` | 247 | *strengthen* | (d) claims the allowlist "is carried through the full producer path"; `assertNotNull(validator)` shows only that a validator was built +| `omittedProfileNeverConsultsTheResolver`| 302 | keep | neutralised fixture -- the registry throws on any lookup, so completing IS the proof +| `completesForAValidlyConfiguredValidator`| 413 | keep | matched control `forcesEagerAssemblyByInvokingTheValidator`:398 +|=== + +=== `pipeline/PassthroughHostGuardStageTest.java` -- 7 flagged + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `passesBenignHost` | 116 | *strengthen* | (d)/(c) mandatory -- `java:S5976` triple 1; a Host sharing nothing with the reserved SNI cannot match under any normalization, so the assertion is entailed by the arrange +| `passesNullHost` | 126 | *strengthen* | (d) mandatory -- `java:S5976` triple 1 +| `passesSuffixLookalikeHost` | 136 | *strengthen* | (d) mandatory -- `java:S5976` triple 1 +| `passesHostWithNonNumericPort` | 146 | *strengthen* | (d) mandatory -- `java:S5976` triple 2 +| `passesHostWithEmptyPort` | 157 | *strengthen* | (d) mandatory -- `java:S5976` triple 2 +| `passesHostWithMultipleColons` | 167 | *strengthen* | (d) mandatory -- `java:S5976` triple 2 +| `inertWhenPassthroughSetEmpty` | 185 | keep | matched control `rejectsSmuggledHost`:54 -- same Host, non-empty set, rejects +|=== + +The precise gap across all six: each `@DisplayName` names a *positive* post-condition -- "passes … *through to route selection*", "so the Host *no longer matches the SNI*" -- and the assertion checks neither. `assertDoesNotThrow` cannot distinguish "the normalized Host genuinely does not match" from an inert guard, a swallowed exception, or a `process()` that stopped inspecting the `Host` at all, and it says nothing about the routing state the request is supposed to reach stage 2 in. + +=== `pipeline/ThoroughChecksStageTest.java` -- 9 flagged + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `acceptsLegitimateRequestUnderDivergentFilter` | 95 | keep | matched control `rejectsDivergentFilterViolation`:80 +| `skipsReRunWhenRouteConfigEqualsDefault` | 112 | *strengthen* | (d) cannot distinguish "the re-run was skipped" from "the re-run ran and passed" +| `fallsBackToBaselineWhenRouteDeclaresNoConfig` | 123 | *strengthen* | (d) cannot distinguish "fell back to the baseline" from "did nothing" +| `admitsWildcardWhitelistMatch` | 170 | keep | matched control `rejectsWildcardSegmentCountMismatch`:180 +| `acceptsParameterValidationTheModeDisables` | 329 | keep | matched control `strictRouteStillRejectsWhatMinimalAccepts`:355 +| `skipsPipelineReRun` | 343 | keep | matched control `strictRouteStillRejectsWhatMinimalAccepts`:355 +| `skipsHeaderNameValidationUnderMinimal` | 405 | keep | matched control `rejectsHeaderNameUnderDivergentConfig`:387 +| `skipsHeaderNameReRunWhenConfigEqualsBaseline` | 418 | keep | neutralised fixture -- the stage's own baseline refuses the name, so a re-run would throw +| `admitsLongCookieOnCookieModeGateway` | 525 | keep | matched control `bearerOnlyGatewayStillRejectsLongCookie`:539 +|=== + +=== `tls/TlsEdgeProducerTest.java` -- 3 flagged + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `startsAndStopsFrontListener` | 70 | *strengthen* | (d) cannot distinguish "the front listener started" from "it was never started" +| `noFrontListenerWhenPassthroughEmpty` | 101 | *strengthen* | (d) cannot show the listener was *not* started +| `skipsUnresolvedAlias` | 118 | *strengthen* | (d) cannot show the relay map is empty +|=== + +This file is the *sharpest* instance in the corpus: all three of its test methods are wholly vacuous, +none has a control, and swapping the arrange blocks of `noFrontListenerWhenPassthroughEmpty` and +`skipsUnresolvedAlias` leaves both green. + +=== `pipeline/BasicChecksStageTest.java` -- 7 flagged + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `acceptsLegitimatePath` | 111 | *strengthen* | (b)/(d) name claims it "records the single canonical path"; `assertNotNull` never says *which* +| `doesNotValidateParameterNamesOrValues` | 124 | keep | the arrange is a value the post-route stage rejects, so the admission is discriminating +| `acceptsCookieHeaderAtBudget` | 191 | keep | matched control `rejectsCookieHeaderBeyondCap`:213 +| `strictBaselineAcceptsCookieHeaderAtBudget` | 202 | keep | matched control `strictBaselineRejectsCookieHeaderBeyondCap`:226 +| `admitsBearerTokenAboveBaselineCap` | 300 | keep | matched control `rejectsAuthorizationBeyondConfiguredCap`:315 +| `matchesHeaderNameCaseInsensitively` | 328 | keep | a case-sensitive implementation rejects, so the test can fail +| `extendedAsciiIsAdmittedUnderAPermissiveBaseline` | 389 | keep | *is itself* the declared matched control for :356 and :372 +|=== + +=== `quarkus/ConfigProducerTest.java` -- 3 flagged + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `shouldBootWhenTheDeclaredBodyCapEqualsTheFrameworkLimit` | 259 | keep | matched control `shouldRefuseBootWhenADeclaredBodyCapExceedsTheFrameworkLimit`:239 -- a boundary pair, the PR #148 precedent +| `shouldAssembleEagerlyOnStartupForValidConfig` | 287 | *strengthen* | (d) cannot distinguish eager assembly at `onStartup` from lazy assembly at the later `gatewayConfig()` call +| `shouldResolvePassthroughSniAliasValueRatherThanHostKey` | 335 | keep | matched control `shouldFailBootWhenPassthroughSniAliasIsUnresolvable`:346; a `keySet()` swap fails this test +|=== + +=== `quarkus/BffRuntimeProducerTest.java` -- 1 flagged + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `shouldAssembleWithoutDiscovery` | 97 | *strengthen* | (d) claims assembly resolves *no OIDC discovery*; not throwing cannot show discovery was skipped +|=== + +=== `edge/RouteRuntimeAssemblerTest.java` -- 1 flagged (F2) + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `shouldAssembleSessionRoutes` | 163 | *strengthen* | (b) F2 -- three further scenarios (WebSocket+session, gRPC, WebSocket+none) each assemble a route table and discard the result, asserted only by `assertDoesNotThrow` +|=== + +=== `auth/AuthenticationStageTest.java` -- 3 flagged + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `passesRequireNone` | 66 | *delete* | strictly subsumed by `passesRequireNoneWithoutResolvingValidator`:77, which has the same arrange plus a throwing validator provider -- a strictly stronger assertion over the same path +| `passesRequireNoneWithoutResolvingValidator`| 77 | keep | neutralised fixture -- the provider raises when resolved, so completing proves the validator was never resolved +| `acceptsValidBearerToken` | 91 | keep | matched controls at :103 (missing), :118 (malformed), :133 (missing scope) +|=== + +=== `bff/csrf/CsrfDefenceTest.java` -- 4 flagged, all `keep` + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `safeMethodsBypass` | 73 | keep | matched control `bothHeadersAbsentRejected`:126 -- same absent headers, unsafe method, rejects +| `unsafeMethodTrustedOriginAccepted` | 83 | keep | matched control `unsafeMethodUntrustedOriginRejected`:92 +| `trustedOriginMatchIsCaseInsensitive`| 100 | keep | matched control :92; a case-sensitive match rejects this input +| `absentOriginSameOriginFetchAccepted`| 109 | keep | matched control `absentOriginCrossSiteFetchRejected`:118 +|=== + +=== `edge/GatewayEdgeRouteTest.java` -- 5 flagged, all `keep` + +[cols="3,1,1,4",options="header"] +|=== +| Method | Line | Verdict | Evidence + +| `bootsCleanlyOverEmptyRouteTable` | 121 | keep | (a) construction test, correctly scoped -- the claim IS "assembles without error"; `registersCatchAllRoute`:132 asserts on the assembled edge +| `bootsSessionAuthRoute` | 147 | keep | regression guard for a *removed* boot rejection -- restoring the rejection fails this test +| `bootsGrpcProtocol` | 161 | keep | same shape, gRPC +| `bootsWebSocketProtocol` | 174 | keep | same shape, WebSocket +| `bootsSessionAuthWebSocketRoute` | 187 | keep | same shape, WebSocket + session +|=== + +=== `bff/runtime/SessionAuthenticationStageTest.java` -- 0 flagged + +Five `assertDoesNotThrow` occurrences, and *every one* is followed by a substantive `assertEquals` on +the mediated bearer, the short-circuit status, or the emitted `Set-Cookie` list. The file clears gate +2 on raw density and contributes nothing to the vacuity population. See <>. + +=== Control-sample rows + +[cols="3,1,1,1,3",options="header"] +|=== +| Method | File | Line | Verdict | Evidence + +| `acceptsWellFramedPost` | `FramingGateTest` | 139 | keep | seven matched rejection controls in the same class +| `onAdmitsGetWithContentLengthBody` | `FramingGateTest` | 187 | keep | eight "STILL rejects" controls pin the single relaxed leg +| `onAdmitsCleanGet` | `FramingGateTest` | 271 | keep | same control block +| `emptyAllowlistEnforcesNothing` | `OriginValidationStageTest` | 66 | keep | matched control `foreignOriginRejected`:92 +| `allowlistedOriginProceeds` | `OriginValidationStageTest` | 75 | keep | matched control `foreignOriginRejected`:92 +| `matchingIsCaseInsensitiveOnHost` | `OriginValidationStageTest` | 83 | keep | matched control :92; a case-sensitive match rejects this input +|=== + +`EgressAllowlistActivationWiringTest` contributes no rows: its six `assertNotNull` occurrences sit +inside parsing helpers, each paired with `assertInstanceOf` / `assertEquals`, and both of its `@Test` +methods carry explicit anti-vacuity issuer counters. + +=== Classification totals + +[cols="3,1,1",options="header"] +|=== +| Population | Methods | Note + +| Flagged in the mutation pool (12 files, read in full) | 50 | +| Flagged in the control sample (3 files, read in full) | 6 | +| *Total classified* | *56* | every row carries a `file:line` and exactly one verdict +| -- verdict `strengthen` | *18* | 8 mandatory + 10 discretionary; cap 20, not exceeded +| -- verdict `keep` | *37* | 31 in the pool + 6 in the control sample +| -- verdict `delete` | *1* | `AuthenticationStageTest.passesRequireNone`:66 +|=== + +*Every classified row has a terminal verdict.* The un-strengthened remainder is therefore not made of +classified rows -- it is the un-classified surface, counted in the next section. + +== What Was Strengthened + +Eighteen methods, one `delete`. Each row names the observable the strengthened assertion reaches -- +the thing that was previously unchecked and now has to hold. + +[cols="3,1,4",options="header"] +|=== +| Method (file:line, pre-change) | Kind | The observable the strengthening now asserts + +| `TokenValidatorProducerTest`:67, :85 +| mandatory +| The audience posture, *behaviourally*: a real generated token is validated through the produced + validator over an offline `file` JWKS source. A declared audience the token lacks now *refuses* it; + an audience-less issuer *admits* it and the admitted token's audience claim is asserted. Swap the + two arrange blocks and both tests fail. + +| `PassthroughHostGuardStageTest`:116, :126, :136, :146, :157, :167 +| mandatory +| Parameterized *and* strengthened. Two assertions per benign Host shape: the request reaches route + selection *untouched* (`host()` unrewritten, `selectedRoute()` and `canonicalPath()` still unset), + and -- the attribution control -- a guard reserving that very Host *rejects* it, so the pass cannot + be explained by an inert guard. + +| `TokenValidatorProducerTest`:247 +| discretionary +| The egress allowlist reaches the policy the producer path builds, with a matched control: the same + path over an otherwise-identical issuer declaring no allowlist still refuses the host. + +| `ThoroughChecksStageTest`:112 +| discretionary +| A stage whose *own* baseline refuses the path admits a baseline-equal route (the skip fired), while + a route diverging in one dimension is re-run and rejected. + +| `ThoroughChecksStageTest`:123 +| discretionary +| The fallback is observed through the unconditional body cap: a config-less route is governed by the + baseline's cap -- one byte over rejects, exactly at the cap passes. + +| `TlsEdgeProducerTest`:70, :101, :118 +| discretionary +| The *public port itself*. Each case allocates a free port and probes it: the resolvable + configuration binds it and releases it on shutdown; the empty map and the unresolved alias leave it + unbound. "onStartup did not throw" is evidence for neither claim. + +| `BasicChecksStageTest`:111 +| discretionary +| The canonical path's *value*, not its presence -- a canonicalizer emitting `/` satisfied the old + non-null check while breaking every route match downstream. + +| `ConfigProducerTest`:287 +| discretionary +| The `CONFIG_LOADED` record is observed while *no bean accessor has been called*, which is what pins + assembly to the startup event rather than to first use. + +| `BffRuntimeProducerTest`:97 +| discretionary +| The issuer is moved to RFC 5737 TEST-NET-1 (unroutable) under a preemptive time bound, so a + discovery round-trip would exhaust the connect timeout; the assembled runtime is then asserted + active and its reserved endpoints reachable. + +| `RouteRuntimeAssemblerTest`:163 +| discretionary +| Each of the three previously result-discarding legs now asserts on the runtime it produced -- + an `assemble()` that quietly dropped a route returns an empty list and satisfied the old no-throw + assertion. + +| `AuthenticationStageTest`:66 +| *delete* +| Removed. `passesRequireNoneWithoutResolvingValidator`:77 has the same arrange plus a throwing + validator provider, so it is a strictly stronger assertion over the same path. +|=== + +Count: *8 mandatory + 10 discretionary = 18 strengthened*, against a cap of 20 (8 + 12). The +discretionary budget was not filled to twelve because the pool yielded ten `strengthen` verdicts; +the remaining flagged methods there earned `keep` on the matched-control test. + +=== Post-change re-census (both annotations) + +Run under the same complete coverage as the baseline (300 files scanned, 0 unreadable, no elision, +not truncated): + +[cols="2,1,1,1,3",options="header"] +|=== +| Marker | Before | After | Delta | Attribution + +| `@Test` +| 1271 +| 1265 +| -6 +| -5 in `PassthroughHostGuardStageTest` (converted to parameterized form), -1 in + `AuthenticationStageTest` (the `delete` verdict) + +| `@ParameterizedTest` +| 97 +| 99 +| +2 +| the two new parameterized methods in `PassthroughHostGuardStageTest` + +| *Declared surface (union)* +| *1368* +| *1364* +| *-4* +| net of the conversion (-5 +2) and the one deletion (-1); the file count is unchanged at 135 +|=== + +*This is what the re-census rule exists for.* A `@Test`-only re-census would have reported +`PassthroughHostGuardStageTest` dropping five methods and read it as a regression, when five `@Test` +methods became two `@ParameterizedTest` methods covering the same five Host shapes plus a new +attribution control. Counting both annotations shows the file's real movement: 12 methods before, 9 +after, with strictly more asserted per shape. + +[[backlog]] +== The Countable Backlog + +*43 files carrying 140 vacuous-shape marker occurrences were censused but not individually +classified.* That is the standing backlog, and it is the honest measure of what this sweep did not +cover. + +Derivation, reproducible from the censuses above: + +[cols="4,1,1",options="header"] +|=== +| Population | Files | Marker occurrences + +| Marker-carrying files in the D1 surface (`assertDoesNotThrow` union `assertNotNull`) | 58 | 267 +| -- read in full: the mutation pool | 12 | 113 +| -- read in full: the control sample | 3 | 14 +| *-- censused, not individually classified (the backlog)* | *43* | *140* +|=== + +=== Backlog rows -- `api-sheriff/src/test/**` (24 files, 73 occurrences) + +[cols="4,1,1,2",options="header"] +|=== +| File | `aDNT` | `aNN` | Note + +| `arch/NativeRuntimeInitRegistrationArchTest.java` | 2 | 3 | +| `arch/NoStoredOptionalArchTest.java` | 2 | 0 | +| `asset/UpstreamAssetSourceTest.java` | 2 | 2 | +| `auth/JwksTrustProfileResolverTest.java` | 0 | 1 | prose occurrence only -- see <> +| `auth/MountedTlsMapKeyTest.java` | 0 | 3 | +| `bff/cookie/SealedSessionCookieCodecTest.java` | 2 | 0 | +| `bff/refresh/TokenRefreshCoordinatorTest.java` | 0 | 2 | +| `bff/reserved/CallbackEndpointTest.java` | 2 | 0 | +| `config/RouteTableBuilderTest.java` | 2 | 3 | +| `config/load/ConfigLoaderTest.java` | 0 | 4 | +| `config/model/ConfigModelContractTest.java` | 0 | 6 | *named gate-2 exclusion* +| `config/topology/TopologyResolverTest.java` | 2 | 0 | +| `edge/GatewayEdgePipelineTest.java` | 0 | 3 | +| `edge/GatewayEdgeRouteBffWiringTest.java` | 2 | 0 | +| `edge/GrpcDispatchStageTest.java` | 2 | 0 | +| `edge/ReservedBodyCeilingTest.java` | 4 | 0 | +| `pipeline/CanonicalPathGuardTest.java` | 2 | 0 | +| `pipeline/RouteSelectionStageTest.java` | 0 | 2 | +| `pipeline/VerbGateStageTest.java` | 2 | 0 | +| `quarkus/ConfigFailFastTest.java` | 2 | 2 | +| `quarkus/ConfigModelReflectionTest.java` | 0 | 2 | +| `quarkus/CookieModeBootTest.java` | 0 | 3 | +| `quarkus/SheriffMetricsTest.java` | 0 | 7 | *named gate-2 exclusion* +| `tls/MtlsServerCustomizerTest.java` | 0 | 2 | +| *Total* | *28* | *45* | +|=== + +=== Backlog rows -- `integration-tests/src/test/**` (19 files, 67 occurrences) + +Every file below is additionally blocked from the mutation pool by *gate 1* (verification +reachability), regardless of density. + +[cols="4,1,1,2",options="header"] +|=== +| File | `aDNT` | `aNN` | Note + +| `BearerValidationIT.java` | 0 | 2 | +| `BffCookieActivationWiringTest.java` | 0 | 8 | *named gate-1 exclusion* +| `BffCookieSessionIT.java` | 0 | 11 | *named gate-1 exclusion* -- highest `assertNotNull` density in the surface +| `BffCookieStatelessnessIT.java` | 0 | 4 | +| `BffLoginInitiationIT.java` | 0 | 3 | +| `BffLogoutIT.java` | 0 | 2 | +| `BffSessionMediationIT.java` | 0 | 3 | +| `BodyLimitActivationWiringTest.java` | 0 | 4 | +| `CipherSuiteFixtureWiringTest.java` | 0 | 2 | +| `DeclaredLimitBoundaryIT.java` | 0 | 3 | +| `DescriptorInventoryWiringTest.java` | 0 | 2 | +| `GetWithBodyActivationWiringTest.java` | 0 | 2 | +| `ItProfileConfigBindingWiringTest.java` | 0 | 2 | +| `LargeBodyIT.java` | 0 | 2 | +| `ManagementPlainHttpActivationWiringTest.java` | 0 | 2 | +| `MtlsHandshakeIT.java` | 2 | 0 | +| `TlsEdgeActivationWiringTest.java` | 0 | 8 | *named gate-1 exclusion* +| `TlsPassthroughIT.java` | 0 | 2 | +| `WsAdmissionActivationWiringTest.java` | 0 | 3 | +| *Total* | *2* | *65* | +|=== + +=== What the backlog is not + +It is *not* "various remaining tests". Every entry is a named file with its measured marker density +and the gate that excluded it. Draining it means reading those 43 files and classifying their flagged +methods -- work that is bounded and countable, not open-ended. + +The control sample suggests the backlog's *yield* is low (0 strengthen verdicts from 6 flagged methods +across 3 sampled files), but a 6.5% sample is a signal, not a proof, and this note does not claim +otherwise. + +== Findings + +[[density-finding]] +=== Density is a screen, not a verdict + +Three files cleared gate 2 on raw density and produced *zero* strengthen verdicts: + +[cols="3,1,1,1",options="header"] +|=== +| File | Density admitting it | Flagged | `strengthen` + +| `bff/runtime/SessionAuthenticationStageTest.java` | 5 `aDNT` | 0 | 0 +| `bff/csrf/CsrfDefenceTest.java` | 6 `aDNT` | 4 | 0 +| `edge/GatewayEdgeRouteTest.java` | 8 `aDNT` | 5 | 0 +|=== + +In each case the marker count is high because the *subject* is an admit/reject gate, and every +admission is paired with a matched rejection control in the same class. Counting markers finds +candidates; only reading finds vacuity. A future sweep that ranks by density and then acts *without +reading* would have rewritten seventeen correctly-scoped tests in these three files alone. + +=== The `assertNotNull` companion pattern + +Three of the strengthen verdicts (`ConfigProducerTest`:287, `BffRuntimeProducerTest`:97, +`TokenValidatorProducerTest`:247) share a shape worth naming: `assertDoesNotThrow(() -> act())` +followed by `assertNotNull(someAccessor())`. Two vacuous markers stacked read as *two* assertions and +still prove only that an object exists. Under the F1 rule such a method is flagged exactly as a +single-marker method is. + +[[report-only]] +=== Report-only observations + +Per this sweep's standing boundary, defects found outside its write scope are *reported here, never +absorbed*. + +`JwksTrustProfileResolverTest` -- `assertNotNull` prose occurrence:: +`api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/JwksTrustProfileResolverTest.java` matches +the raw `assertNotNull` census once while carrying *no* `assertNotNull` static import and *no* call +site. The occurrence is textual (prose/identifier context), not an assertion. Recorded so the +147-vs-107 delta is fully accounted for and so a future census does not read it as a call site. + +`EgressPolicy` equality does not carry the host allowlist:: +While strengthening `TokenValidatorProducerTest`:247, an `EgressPolicy` built with +`allowedEgressHost("localhost")` was found to compare *equal* to `EgressPolicy.secureDefault()` -- +both render as `EgressPolicy(allowLoopback=false)`. The allowlist is therefore invisible to +`equals` / `toString`. Consequence for this codebase: `assertEquals(EgressPolicy.secureDefault(), …)` +is a weak assertion that cannot detect an unintended widening, and the *inverse* assertion cannot +detect a dropped allowlist at all. The existing tests survive because each pairs the equality check +with a behavioural `policy.check(uri)` assertion; the strengthened row uses the behavioural form +exclusively. The type is third-party (`de.cuioss.sheriff.token.commons.transport`) and outside this +sweep's write boundary, so this is reported, not changed. + +`TokenValidator` exposes no view of its issuer configs:: +`de.cuioss.sheriff.token.validation.TokenValidator`'s public surface is `createAccessToken` / +`createIdToken` / `createRefreshToken`, `close`, `getSecurityEventCounter` and +`getPerformanceMonitor` -- there is no accessor for the `IssuerConfig` list it was built from. The +producer's audience decision (expected audience vs the explicit opt-out) is therefore *structurally +unobservable* on the built validator, which is why the strengthened pair asserts it behaviourally by +validating a real token over offline key material. The same limit is why +`allowlistSurvivesTheProducerPath` asserts its last hop at the producer's own +`toHttpJwksLoaderConfig` seam rather than on the validator: the public entry point is driven first to +prove the whole graph assembles, and the policy assertions prove the allowlist survived. + +Production write boundary held:: +This sweep modified *no* production file. Its production write boundary is exactly three files, none +of which belongs to this deliverable: +`api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java`, +`api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.java` and +`api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/FramingGate.java`. A production edit +anywhere else would be a scope breach, not a judgement call. + +== Standard for a Strengthened Assertion + +Every strengthened method must satisfy the anti-vacuity contract +link:../adr/0030-Comment-only_invariants_become_positively-phrased_fitness_functions_shipped_with_a_four-leg_control_set.adoc[ADR-0030] +sets for fitness functions, applied to an ordinary unit test: + +* the assertion must be *capable of failing* on the behaviour the method's name claims; +* it must not degrade to an always-passing form when the subject is renamed or moved; +* where the claim is an *admission*, the strengthening either inspects the post-state directly or is + paired with a matched negative control that makes the admission attributable. + +Inverting the asserted behaviour locally must turn the strengthened test red. A strengthening that +survives its own inversion has not strengthened anything. From be7516d7349435f79f25c773111e34824a493e87 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:39:59 +0200 Subject: [PATCH 02/10] docs(corpus): record the inversion audit for the strengthened assertions Three transient production mutations, one per strengthening technique, were applied together and reverted after the run. Each broke exactly the strengthened assertions it targeted, and none is visible to the pre-change form of the same test - which is the contrast the sweep exists to establish. The audit ran after the deliverable was committed so the revert is a checkout against committed content rather than a destructive discard of uncommitted work. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8 --- doc/development/test-corpus-integrity.adoc | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/doc/development/test-corpus-integrity.adoc b/doc/development/test-corpus-integrity.adoc index c035addd..51e977a5 100644 --- a/doc/development/test-corpus-integrity.adoc +++ b/doc/development/test-corpus-integrity.adoc @@ -615,6 +615,42 @@ Count: *8 mandatory + 10 discretionary = 18 strengthened*, against a cap of 20 ( discretionary budget was not filled to twelve because the pool yielded ten `strengthen` verdicts; the remaining flagged methods there earned `keep` on the matched-control test. +=== The inversion audit + +A strengthened assertion is only worth the diff if it *fails* when the behaviour it claims is broken. +Three transient production mutations were applied together, the affected suites were run, and the +mutations were reverted. Each mutation targets a different strengthening technique, so the audit +covers the argument for all three rather than only one instance of it: + +[cols="2,2,3",options="header"] +|=== +| Transient mutation | Expected to break | Observed + +| `PassthroughHostGuardStage.normalize` strips *any* `:suffix` rather than only a purely-numeric port +| the three colon rows of `passesBenignHostToRouteSelection` +| rows [3], [4], [5] failed with `PASSTHROUGH_HOST_SMUGGLED`; rows [1], [2], `passesNullHost` and + every attribution-control row correctly stayed green + +| `TlsEdgeProducer.onStartup` returns before creating the front listener +| `startsAndStopsFrontListener` +| failed: `a resolvable passthrough SNI starts the front listener on the public port ==> expected: + but was: ` + +| `TokenValidatorProducer.toValidationIssuer` drops the `audienceValidationDisabled(true)` opt-out +| `audienceLessIssuerDisablesAudienceValidation` +| failed: `expectedAudience must be non-empty for issuer 'Token-Test-testIssuer'` -- the exact + library refusal the opt-out branch exists to prevent +|=== + +Each mutation is invisible to the *pre-change* form of the same test: the six `assertDoesNotThrow` +bodies, the three `TlsEdgeProducerTest` no-throw bodies and the two `assertNotNull(validator)` bodies +all stay green under all three mutations. That contrast is the whole point of the sweep. + +The audit was run *after* the deliverable was committed rather than before, deliberately: with the +strengthened files already in `HEAD`, reverting the transient mutations is a `git checkout` against +committed content rather than a destructive discard of uncommitted work. The mutations touched only +production files, which this deliverable does not modify, so the revert restored them exactly. + === Post-change re-census (both annotations) Run under the same complete coverage as the baseline (300 files scanned, 0 unreadable, no elision, From b85d347cf2d6027bb74daa05e56f736fc44267ed Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:48:55 +0200 Subject: [PATCH 03/10] fix(tls): close java:S3776 and java:S7467 in ClientHelloSniParser Read the authoritative cuioss SonarCloud gate BEFORE any code change. All four PLAN-05 findings were genuinely open, with the re-anchored line numbers exact: java:S3776 at ClientHelloSniParser.java:80 (cognitive complexity 21 > 15), java:S7467 at :171, and the two java:S5976 rows at PassthroughHostGuardStageTest.java:116 and :146. None had been resolved without a code change, so there is no silent-dismissal finding to report. java:S3776 - the while(true) reassembly loop interleaved four concerns. Each is now a named private helper returning the terminal Result it implies, or null to consume the record: recordHeaderVerdict, recordBodyVerdict, reassembledVerdict. parse() reads as the reassembly loop it is. The duplicated tail check after an INCOMPLETE span is removed because it restates the loop head's own first check, so looping back is byte-for-byte equivalent. Every fail-closed Result.parsed(null) path survives unchanged. java:S7467 - the unused catch binding becomes the unnamed variable. Structure-only: ClientHelloSniParserTest passes with ZERO edits, which is the behaviour-preservation harness this deliverable is verified by. The two S5976 rows were closed by deliverable 1's parameterize-and-strengthen pass. Four further live-gate findings are open and outside this plan's scope; they are reported in decision.log, not absorbed: java:S6539 RouteTableBuilder.java:80, java:S5738 ConfigFailFastTest.java:59, java:S5738 ConfigProducerTest.java:179, java:S135 BackchannelLogoutEndpoint.java:137. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8 --- .../gateway/tls/ClientHelloSniParser.java | 93 ++++++++++++++----- .../auth/TokenValidatorProducerTest.java | 4 +- .../pipeline/BasicChecksStageTest.java | 1 - .../gateway/tls/TlsEdgeProducerTest.java | 6 +- 4 files changed, 75 insertions(+), 29 deletions(-) diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java index 1336bb53..44b04304 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java @@ -81,37 +81,84 @@ public Result parse(byte[] bytes) { ByteArrayOutputStream handshake = new ByteArrayOutputStream(); int pos = 0; while (true) { - if (bytes.length - pos < RECORD_HEADER_LENGTH) { - return overBound(pos) ? Result.parsed(null) : Result.needMoreData(); - } - if ((bytes[pos] & UINT8_MASK) != RECORD_TYPE_HANDSHAKE) { - // Not a TLS handshake record — fail closed to the terminated-strict path. - return Result.parsed(null); + Result headerVerdict = recordHeaderVerdict(bytes, pos); + if (headerVerdict != null) { + return headerVerdict; } int recordLength = uint16(bytes, pos + 3); int recordEnd = pos + RECORD_HEADER_LENGTH + recordLength; - if (recordEnd > MAX_CLIENT_HELLO_BYTES) { - return Result.parsed(null); - } - if (bytes.length < recordEnd) { - return Result.needMoreData(); + Result bodyVerdict = recordBodyVerdict(bytes, recordEnd); + if (bodyVerdict != null) { + return bodyVerdict; } handshake.write(bytes, pos + RECORD_HEADER_LENGTH, recordLength); pos = recordEnd; - byte[] handshakeBytes = handshake.toByteArray(); - HandshakeSpan span = completeHandshake(handshakeBytes); - if (span == HandshakeSpan.MALFORMED) { - return Result.parsed(null); - } - if (span == HandshakeSpan.COMPLETE) { - return Result.parsed(extractServerName(handshakeBytes)); - } - // span == INCOMPLETE: keep reading records if any remain, else ask for more bytes. - if (bytes.length - pos < RECORD_HEADER_LENGTH) { - return overBound(pos) ? Result.parsed(null) : Result.needMoreData(); + Result reassembledVerdict = reassembledVerdict(handshake.toByteArray()); + if (reassembledVerdict != null) { + return reassembledVerdict; } + // The handshake is still incomplete: loop back and consume the next record. The + // "is another record header even present?" question is the loop head's own first check, + // so it is asked there rather than repeated here. + } + } + + /** + * The terminal verdict, if any, implied by the record header at {@code pos}: too few bytes for a + * header (keep buffering, or fail closed once the bound is passed), or a record that is not a TLS + * handshake at all. + * + * @param bytes the accumulated connection bytes + * @param pos the offset of the record header being examined + * @return the verdict to return from {@code parse}, or {@code null} to consume this record + */ + private static @Nullable Result recordHeaderVerdict(byte[] bytes, int pos) { + if (bytes.length - pos < RECORD_HEADER_LENGTH) { + return overBound(pos) ? Result.parsed(null) : Result.needMoreData(); + } + if ((bytes[pos] & UINT8_MASK) != RECORD_TYPE_HANDSHAKE) { + // Not a TLS handshake record — fail closed to the terminated-strict path. + return Result.parsed(null); + } + return null; + } + + /** + * The terminal verdict, if any, implied by the declared record body: a record running past the + * size bound fails closed, and a body that has not fully arrived means keep buffering. + * + * @param bytes the accumulated connection bytes + * @param recordEnd the offset one past the declared end of this record + * @return the verdict to return from {@code parse}, or {@code null} to consume this record + */ + private static @Nullable Result recordBodyVerdict(byte[] bytes, int recordEnd) { + if (recordEnd > MAX_CLIENT_HELLO_BYTES) { + return Result.parsed(null); + } + if (bytes.length < recordEnd) { + return Result.needMoreData(); + } + return null; + } + + /** + * The terminal verdict, if any, implied by the handshake bytes reassembled so far: a wrong + * handshake type or an over-bound body fails closed, a complete {@code client_hello} yields the + * extracted SNI. + * + * @param handshakeBytes the handshake bytes reassembled across the records consumed so far + * @return the verdict to return from {@code parse}, or {@code null} while the body is incomplete + */ + private static @Nullable Result reassembledVerdict(byte[] handshakeBytes) { + HandshakeSpan span = completeHandshake(handshakeBytes); + if (span == HandshakeSpan.MALFORMED) { + return Result.parsed(null); + } + if (span == HandshakeSpan.COMPLETE) { + return Result.parsed(extractServerName(handshakeBytes)); } + return null; } private static boolean overBound(int pos) { @@ -168,7 +215,7 @@ private static HandshakeSpan completeHandshake(byte[] handshake) { cursor.seek(next); } return null; - } catch (MalformedHelloException e) { + } catch (MalformedHelloException _) { return null; } } diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java index c3cb08f7..49e3c991 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java @@ -94,7 +94,7 @@ class AudiencePosture { @Test @DisplayName("a declared audience reaches the validator and refuses a token that does not carry it") - void declaredAudienceIsEnforced() throws IOException { + void declaredAudienceIsEnforced() throws Exception { // Arrange — the declared audience is deliberately not one the generated token carries TestTokenHolder holder = TestTokenGenerators.accessTokens().next(); TokenValidator declaringForeignAudience = fileIssuerValidator(holder, "an-audience-the-token-lacks"); @@ -117,7 +117,7 @@ void declaredAudienceIsEnforced() throws IOException { @Test @DisplayName("an issuer configuring no audience disables audience validation rather than refusing the token") - void audienceLessIssuerDisablesAudienceValidation() throws IOException { + void audienceLessIssuerDisablesAudienceValidation() throws Exception { // Arrange — the same token and key material, this time with no audience declared at all TestTokenHolder holder = TestTokenGenerators.accessTokens().next(); TokenValidator validator = fileIssuerValidator(holder, null); diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.java index 6b5711ad..14e4cf4f 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.java @@ -17,7 +17,6 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import java.util.LinkedHashMap; diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java index a5622792..b802ed7d 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java @@ -77,7 +77,7 @@ class PassthroughConfigured { @Test @DisplayName("starts the front listener and shuts it down cleanly when a passthrough SNI resolves") - void startsAndStopsFrontListener() throws IOException { + void startsAndStopsFrontListener() throws Exception { // Arrange — one SNI maps to a resolvable alias, one to an alias absent from the topology. // The resolvable entry makes the relay map non-empty (front started); the unresolved entry // exercises the defensive skip branch. A concrete free port is used rather than the @@ -119,7 +119,7 @@ class PassthroughUnconfigured { @Test @DisplayName("never starts the front listener when passthrough_sni is empty") - void noFrontListenerWhenPassthroughEmpty() throws IOException { + void noFrontListenerWhenPassthroughEmpty() throws Exception { // Arrange — no tls block at all, so the relay map is empty. int port = freePort(); GatewayConfig config = GatewayConfig.builder().version(1).build(); @@ -144,7 +144,7 @@ void noFrontListenerWhenPassthroughEmpty() throws IOException { @Test @DisplayName("skips a passthrough SNI whose alias does not resolve, leaving the map empty") - void skipsUnresolvedAlias() throws IOException { + void skipsUnresolvedAlias() throws Exception { // Arrange — the only passthrough SNI maps to an alias absent from the resolved topology, so // the defensive skip leaves the relay map empty and no front listener is started. This // differs from noFrontListenerWhenPassthroughEmpty in the arrange that matters: a From 54356c42916a2bd9feffd52856c8f5052ec6478a Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:51:44 +0200 Subject: [PATCH 04/10] refactor(api): settle the two undetermined public API elements Both consumer sweeps were re-confirmed against the current tree searching BOTH the call form and the ::name method-reference form, which is what decides them in opposite directions. no-arg FramingGate() - REMOVED. Sweeping `new FramingGate()` together with `FramingGate::` over 926 files finds exactly one consumer, FramingGateTest:40, and no production one; the only production instantiation is the boolean constructor at GatewayEdgeRoute. All four clean-break conditions hold, so under the plan-level breaking compatibility and the pre-1.0 remove-never-deprecate rule it goes directly, with no deprecation marker and no transitionary comment. The single call site becomes new FramingGate(false), which is value-identical: the removed constructor delegated to this(false), so the strict default posture is preserved exactly and no assertion changes. SealedSessionCookieCodec.Unsealed - KEPT, with the reason recorded in its Javadoc. It is the payload wrapper of unseal()'s return, consumed in production by the readSealedValue(...).flatMap(codec::unseal).filter(...).map(...) chain in CookieSessionBinding.resolve. That site is the METHOD-REFERENCE form, so a call-form-only search reports this API as unused - a false verdict that would have justified an unsafe removal. The Javadoc now records both that consumer chain and the ADR-0033 computed-vs-stored Optional distinction, so a future sweep does not re-flag Optional as PLAN-36 residue. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8 --- .../bff/cookie/SealedSessionCookieCodec.java | 14 ++++++++++++++ .../sheriff/gateway/pipeline/FramingGate.java | 7 ------- .../sheriff/gateway/pipeline/FramingGateTest.java | 2 +- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.java index 11b9e833..da408159 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.java @@ -400,6 +400,20 @@ private static String requireNonBlank(String cookieName) { /** * The successful outcome of {@link #unseal(String)}: the authenticated session payload. + *

+ * Load-bearing — do not remove. This record is the payload wrapper of + * {@link #unseal(String)}'s return type, and its production consumer is the + * {@code readSealedValue(…).flatMap(codec::unseal).filter(…).map(…)} chain in + * {@code CookieSessionBinding.resolve}. That call site uses the method-reference form + * {@code codec::unseal}, so a reachability search for the call form {@code unseal(} alone reports + * this API as having no production consumer — a false "unused" verdict that would justify an + * unsafe removal. Search both forms before re-opening the question. + *

+ * The {@code Optional} return type is not a residue of the PLAN-36 sweep: + * {@link de.cuioss.sheriff.gateway.bff.cookie.SealedSessionCookieCodec#unseal(String)} is a + * computed method return, and ADR-0033 retires {@code Optional} only from stored + * positions — fields, declared parameters and record components. A computed return is explicitly + * sanctioned, so a future sweep should not re-flag it. * * @param payload the authenticated session payload * @author API Sheriff Team diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/FramingGate.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/FramingGate.java index bc38f5be..d692de72 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/FramingGate.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/FramingGate.java @@ -63,13 +63,6 @@ public final class FramingGate { private final boolean allowGetWithContentLengthBody; - /** - * Creates a gate with the strict default posture — a body on {@code GET} is rejected. - */ - public FramingGate() { - this(false); - } - /** * Creates a gate with the boot-resolved {@code GET}-body posture. * diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/FramingGateTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/FramingGateTest.java index ac688226..eeeb5a7c 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/FramingGateTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/FramingGateTest.java @@ -37,7 +37,7 @@ class FramingGateTest { /** The strict default posture — the opt-in absent, i.e. every leg enforced as before it existed. */ - private final FramingGate gate = new FramingGate(); + private final FramingGate gate = new FramingGate(false); /** The same gate with {@code allow_get_with_content_length_body} enabled. */ private final FramingGate permissiveGate = new FramingGate(true); From 9907dc027ddde4bf63d2914c63e0b6e42b0ddee5 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:54:06 +0200 Subject: [PATCH 05/10] chore(config): carry the pending marshal.json change into this branch The phase-5 worktree is cut from main's committed HEAD, so it does not inherit main's uncommitted working-tree changes. This re-applies the pending .plan/marshal.json edit inside the worktree so it ships rather than being silently lost. Carried as a patch (git diff --output, then git apply) from the main checkout, never hand-retyped, and the main checkout was not written to. Three hunks: - default:lessons-capture and default:finalize-step-preference-emitter are relocated to sit after default:branch-cleanup. A canonical key re-ordering with byte-identical values, semantically a no-op. - provisioned_version 0.1.1286 -> 0.1.1288. REPORTED, not corrected: provisioned_version 0.1.1288 is already stale against the installed plan-marshall bundle 0.1.1292. Re-deriving it would mean running marshall-steward, which is outside this plan's scope, so it is carried verbatim. REPORTED, needs operator confirmation: the operator-supplied backup of this diff carried default:lessons-capture at lane "off", while the main checkout's current working tree carries it at lane "minimal" - main drifted after that backup was taken. D4's success criteria name the main checkout as the source of truth, so "minimal" is what shipped here. If "off" was intended, this hunk is wrong and must be corrected before merge. See decision.log. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8 --- .plan/marshal.json | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/.plan/marshal.json b/.plan/marshal.json index 474e4ccb..a84f1404 100644 --- a/.plan/marshal.json +++ b/.plan/marshal.json @@ -122,13 +122,6 @@ "ce_wait_timeout_seconds": 600, "lane": "standard" }, - "default:lessons-capture": { - "lane": "minimal" - }, - "default:finalize-step-preference-emitter": { - "preference_min_recurrence": 2, - "lane": "minimal" - }, "default:adr-propose": { "lane": "off" }, @@ -144,6 +137,13 @@ "pre_merge_comment_barrier": "fail_into_loopback", "lane": "minimal" }, + "default:lessons-capture": { + "lane": "minimal" + }, + "default:finalize-step-preference-emitter": { + "preference_min_recurrence": 2, + "lane": "minimal" + }, "default:record-metrics": { "lane": "minimal" }, @@ -324,7 +324,7 @@ "no_plan_body_days": 7, "build_results_days": 5 }, - "provisioned_version": "0.1.1286", + "provisioned_version": "0.1.1288", "config_seed_fingerprint": "714f8058" } } From 5a9a6cf416155ede5c9be36ea9a4d21e6d13a76a Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 4 Aug 2026 01:13:21 +0200 Subject: [PATCH 06/10] fix(config): ship lessons-capture lane "off" as the operator's pending change Corrects the one hunk of the D4 carry that was taken from a rewritten source. The carry in 9907dc0 read .plan/marshal.json from the main checkout, which D4's success criteria name as the source of truth, and shipped default:lessons-capture at lane "minimal". Main's working tree had been REWRITTEN mid-run: the orchestrator's init-time capture of the same pending change is blob 474e4cc..60ed916 and carries lane "off", while main now shows 474e4cc..a84f140 with "minimal". The plan's own handshake rows corroborate a mid-run config write (config_hash 93acf2ec -> 6951111f -> 577b902 -> e8e8b3e). "Matches main" was therefore not the same as "matches the operator's change". The operator's init-time intent is authoritative, so lane "off" is what ships. Verification is exact rather than asserted: after this change .plan/marshal.json hashes to blob 60ed916 - byte-identical to the post-image of the init-time capture. Applied through the manage-config step setter, so no JSON was hand-retyped and no other key was touched; the diff is one line. Everything else in the D4 carry is unchanged: the block relocation, finalize-step-preference-emitter at preference_min_recurrence 2 / lane minimal, and provisioned_version verbatim at 0.1.1288. No marshall-steward run. REPORTED, unchanged: provisioned_version 0.1.1288 is stale against the installed plan-marshall bundle 0.1.1292; re-deriving it is out of scope. REPORTED, new: main's .plan/marshal.json was mutated mid-run, so this branch deliberately differs from main's CURRENT on-disk value for lessons-capture.lane. Anyone diffing the branch against main's working tree later would otherwise read that difference as an error in this PR. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8 --- .plan/marshal.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.plan/marshal.json b/.plan/marshal.json index a84f1404..60ed916b 100644 --- a/.plan/marshal.json +++ b/.plan/marshal.json @@ -138,7 +138,7 @@ "lane": "minimal" }, "default:lessons-capture": { - "lane": "minimal" + "lane": "off" }, "default:finalize-step-preference-emitter": { "preference_min_recurrence": 2, From f8020439056bbbc76b60415d7a54c72b8b6520d5 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 4 Aug 2026 01:47:28 +0200 Subject: [PATCH 07/10] chore(simplify): collapse duplicated egress-policy arrange and drop inert NOSONAR marker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-implementation simplification sweep over the plan's changeset. TokenValidatorProducerTest: allowlistSurvivesTheProducerPath carried two near-identical ~13-line builder blocks differing only by the .allowedEgressHosts(...) call. Collapsed into a producerPathEgressPolicy helper alongside the existing egressPolicyFor. Both assertions and every explanatory comment are preserved verbatim; no assertion was weakened. TlsEdgeProducerTest: awaitNotListening carried two suppression mechanisms for java:S2925 — an inert "// NOSONAR java:S2925" on a standalone comment line (NOSONAR only suppresses on the line it sits on, and the Thread.sleep is six lines below) plus the @SuppressWarnings that actually suppresses. Dropped the inert marker, kept the rationale prose and the annotation. Two anti-patterns were reported rather than fixed, both out of scope: a naming shadow at ClientHelloSniParser.java:113 (a rename would re-open D2's behaviour-preserving, zero-test-edit constraint for cosmetic churn), and pre-existing five-argument call duplication in RouteRuntimeAssemblerTest that predates this plan. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8 --- .../auth/TokenValidatorProducerTest.java | 53 ++++++++++--------- .../gateway/tls/TlsEdgeProducerTest.java | 4 +- 2 files changed, 30 insertions(+), 27 deletions(-) diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java index 49e3c991..c7719b5d 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java @@ -315,38 +315,21 @@ void severalHostsAreAllowlisted() { @Test @DisplayName("the allowlist is carried through the full producer path, not only the seam") void allowlistSurvivesTheProducerPath() { - // Arrange — drive the public producer entry point rather than the helper - IssuerConfig.Jwks jwks = IssuerConfig.Jwks.builder() + // Arrange — a declared allowlist, and the matched control that differs from it in exactly + // one respect: the absence of that declaration + IssuerConfig.Jwks declaringAllowlist = IssuerConfig.Jwks.builder() .source("http") .url(JWKS_URL) .allowedEgressHosts(List.of(BLOCKED_HOST)) .build(); - IssuerConfig issuer = IssuerConfig.builder() - .name("benchmark-keycloak") - .issuer(ISSUER) - .jwks(jwks) - .build(); - TokenValidatorProducer producer = producerFor(issuer); - - // Act — the public entry point assembles the whole issuer graph - producer.gatewayTokenValidator(); - EgressPolicy withAllowlist = producer.toHttpJwksLoaderConfig(issuer, jwks).getEgressPolicy(); - - // Arrange the matched control — an otherwise identical issuer declaring no allowlist, - // driven through the same public entry point - IssuerConfig.Jwks plainJwks = IssuerConfig.Jwks.builder() + IssuerConfig.Jwks declaringNothing = IssuerConfig.Jwks.builder() .source("http") .url(JWKS_URL) .build(); - IssuerConfig plainIssuer = IssuerConfig.builder() - .name("benchmark-keycloak") - .issuer(ISSUER) - .jwks(plainJwks) - .build(); - TokenValidatorProducer plainProducer = producerFor(plainIssuer); - plainProducer.gatewayTokenValidator(); - EgressPolicy withoutAllowlist = - plainProducer.toHttpJwksLoaderConfig(plainIssuer, plainJwks).getEgressPolicy(); + + // Act — both policies come from the public producer entry point, not from the seam alone + EgressPolicy withAllowlist = producerPathEgressPolicy(declaringAllowlist); + EgressPolicy withoutAllowlist = producerPathEgressPolicy(declaringNothing); // Assert — the declared allowlist survived the assembly ... assertDoesNotThrow(() -> withAllowlist.check(BLOCKED_JWKS_URI), @@ -366,6 +349,26 @@ private static EgressPolicy egressPolicyFor(IssuerConfig.Jwks jwks) { .jwks(jwks).build(); return producerFor(issuer).toHttpJwksLoaderConfig(issuer, jwks).getEgressPolicy(); } + + /** + * The egress policy reached through the full producer path: unlike + * {@link #egressPolicyFor(IssuerConfig.Jwks)} this drives + * {@link TokenValidatorProducer#gatewayTokenValidator()} first, so a declaration that aborted + * the whole-graph assembly could never reach the seam the policy is read from. + * + * @param jwks the jwks block whose egress declaration is under test + * @return the egress policy the public producer entry point ends up with + */ + private static EgressPolicy producerPathEgressPolicy(IssuerConfig.Jwks jwks) { + IssuerConfig issuer = IssuerConfig.builder() + .name("benchmark-keycloak") + .issuer(ISSUER) + .jwks(jwks) + .build(); + TokenValidatorProducer producer = producerFor(issuer); + producer.gatewayTokenValidator(); + return producer.toHttpJwksLoaderConfig(issuer, jwks).getEgressPolicy(); + } } @Nested diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java index b802ed7d..a30869c2 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java @@ -203,8 +203,8 @@ private static boolean isListening(int port) { } } - // NOSONAR java:S2925 - Thread.sleep is load-bearing: SniFrontListener.stop() completes on the - // Vert.x event loop, so the unbind is a real asynchronous release with no virtual clock to advance. + // Thread.sleep is load-bearing: SniFrontListener.stop() completes on the Vert.x event loop, so the + // unbind is a real asynchronous release with no virtual clock to advance. @SuppressWarnings("java:S2925") private static void awaitNotListening(int port, String message) { for (int attempt = 0; attempt < 100 && isListening(port); attempt++) { From e942d6e3a29e040f8a3403ee0283a2a5f8ba0d95 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 4 Aug 2026 02:45:55 +0200 Subject: [PATCH 08/10] fix(tls): close the three new-code gate findings on PR 151 The cuioss SonarCloud gate failed at f802043 with three new-code findings. All three are closed here; the bug-class one was analysed before it was touched. javabugs:S6466 (ClientHelloSniParser.java:253, BUG) - re-derived every call path into uint16 and uint24 rather than trusting the earlier "the header guard already returned null" claim. Three call sites exist and every one is guarded: * parse() reaches uint16(bytes, pos + 3) only after recordHeaderVerdict returned null, which happens solely when bytes.length - pos >= RECORD_HEADER_LENGTH, so pos + 4 <= bytes.length - 1. pos starts at 0 and only advances to a recordEnd that recordBodyVerdict already bounded by MAX_CLIENT_HELLO_BYTES, so it is never negative and cannot overflow. * Cursor.readUint16() reaches uint16(data, position) only after require(2) asserted position + 2 <= data.length, raising MalformedHelloException otherwise. position starts at HANDSHAKE_HEADER_LENGTH and never decreases - seek() refuses a backwards target - so it is never negative. * completeHandshake() reaches uint24(handshake, 1) only after returning INCOMPLETE for handshake.length < HANDSHAKE_HEADER_LENGTH, so offsets 1..3 are always inside the buffer. The out-of-bounds access is therefore unreachable: no defect shipped and no behavioural fix is warranted. What the S3776 split did change is where the guards live - they moved from inline in the reassembly loop into named helpers, which is exactly what put them out of reach of the symbolic-execution engine. Suppressed in-code with // NOSONAR javabugs:S6466 on the two flagged reads, with the full per-call-site bounds contract written up as Javadoc on uint16 (and the matching one-liner on uint24) so a reviewer can challenge the claim rather than take it. Two regression tests turn that prose into executable assertions, both pinning the guard at the exact one-byte-short-of-a-record-header boundary - index 4 present in neither case, which is the precise read Sonar flagged: one on the first record (pos == 0) and one on the loop-back to a later record (pos > 0), the tail check the S3776 split delegated to the loop head. Remove the length guard and both throw ArrayIndexOutOfBoundsException instead of asking for more data. java:S7467 (TlsEdgeProducerTest.java:201, :213) - both catch bindings are unused, so both become the unnamed pattern. The refused-connection branch keeps its meaning in a comment now that the binding name no longer carries it. No assertion was weakened; these are the D1-strengthened tests and their strength is untouched. Both gates green with zero errors: verify -Ppre-commit and verify. Neither left any rewrite churn outside the three-file footprint. Co-Authored-By: Claude Opus 5 (1M context) --- .../gateway/tls/ClientHelloSniParser.java | 42 ++++++++++++++++++- .../gateway/tls/ClientHelloSniParserTest.java | 36 ++++++++++++++++ .../gateway/tls/TlsEdgeProducerTest.java | 5 ++- 3 files changed, 80 insertions(+), 3 deletions(-) diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java index 44b04304..9880b934 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java @@ -249,10 +249,50 @@ private static HandshakeSpan completeHandshake(byte[] handshake) { return null; } + /** + * Reads the big-endian {@code uint16} at {@code offset}. + *

+ * Bounds contract. Both call sites establish {@code 0 <= offset} and + * {@code offset + 1 < data.length} before calling, so neither read can run past the array: + *

+ * Both guards live in extracted helpers rather than inline in the reading method, which is why + * the symbolic-execution engine cannot see them; {@code ClientHelloSniParserTest} pins the + * {@code parse} guard at the exact one-byte-short-of-a-record-header boundary, on the first + * record and on the loop-back to a later record. + * + * @param data the buffer to read from + * @param offset the offset of the high-order byte; the caller guarantees {@code offset + 1} is + * within {@code data} + * @return the unsigned 16-bit big-endian value at {@code offset} + */ private static int uint16(byte[] data, int offset) { - return ((data[offset] & UINT8_MASK) << 8) | (data[offset + 1] & UINT8_MASK); + int high = data[offset] & UINT8_MASK; // NOSONAR javabugs:S6466 - caller-guarded, see Javadoc + int low = data[offset + 1] & UINT8_MASK; // NOSONAR javabugs:S6466 - caller-guarded, see Javadoc + return (high << 8) | low; } + /** + * Reads the big-endian {@code uint24} at {@code offset}. The sole caller + * {@link #completeHandshake(byte[])} has already returned {@link HandshakeSpan#INCOMPLETE} unless + * {@code handshake.length >= HANDSHAKE_HEADER_LENGTH}, so offsets {@code 1..3} are always within + * the buffer. + * + * @param data the buffer to read from + * @param offset the offset of the most significant byte + * @return the unsigned 24-bit big-endian value at {@code offset} + */ private static int uint24(byte[] data, int offset) { return ((data[offset] & UINT8_MASK) << 16) | ((data[offset + 1] & UINT8_MASK) << 8) diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java index 634aef95..72178c31 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java @@ -125,6 +125,42 @@ void needsMoreDataForRecordHeaderOnly() { assertFalse(result.complete()); } + @Test + @DisplayName("asks for more data when the first record header is one byte short") + void needsMoreDataForOneByteShortRecordHeader() { + // Arrange — exactly RECORD_HEADER_LENGTH - 1 bytes. The low byte of the record-length + // field (index 4) has not arrived, so reading it is precisely the out-of-bounds access + // the loop-head length guard exists to prevent. + byte[] hello = ClientHelloFixture.withSni("relay.internal"); + byte[] oneByteShortHeader = Arrays.copyOf(hello, 4); + + // Act + ClientHelloSniParser.Result result = parser.parse(oneByteShortHeader); + + // Assert + assertFalse(result.complete(), "a header one byte short is buffered, never read past"); + assertNull(result.serverName()); + } + + @Test + @DisplayName("asks for more data when a later record header is one byte short") + void needsMoreDataForOneByteShortLaterRecordHeader() { + // Arrange — a first handshake record that leaves the ClientHello incomplete, followed by + // only 4 of the 5 bytes of the second record header. The loop-back re-enters the header + // guard at pos > 0, which is the tail check the cognitive-complexity split delegated to + // the loop head. + byte[] twoRecords = ClientHelloFixture.withSniSplitAcrossRecords("split.example.org", 40); + byte[] truncated = Arrays.copyOf(twoRecords, 5 + 40 + 4); + + // Act + ClientHelloSniParser.Result result = parser.parse(truncated); + + // Assert + assertFalse(result.complete(), + "a later header one byte short is buffered, never read past"); + assertNull(result.serverName()); + } + @Test @DisplayName("reassembles a ClientHello split across two TLS handshake records") void reassemblesAcrossTwoRecords() { diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java index a30869c2..06bb5058 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java @@ -198,7 +198,8 @@ private static boolean isListening(int port) { try (Socket probe = new Socket()) { probe.connect(new InetSocketAddress("localhost", port), CONNECT_TIMEOUT_MILLIS); return true; - } catch (IOException refused) { + } catch (IOException _) { + // A refused connection IS the answer: nothing is listening on the probed port. return false; } } @@ -210,7 +211,7 @@ private static void awaitNotListening(int port, String message) { for (int attempt = 0; attempt < 100 && isListening(port); attempt++) { try { Thread.sleep(20); - } catch (InterruptedException interrupted) { + } catch (InterruptedException _) { Thread.currentThread().interrupt(); break; } From 8723ed47cb07998436467f8803fdf69b343c4391 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 4 Aug 2026 03:42:13 +0200 Subject: [PATCH 09/10] fix(tls): anchor the ClientHello give-up guard on the buffered byte count PR 151 CodeRabbit inline comment 28625f on ClientHelloSniParser.java:119. recordHeaderVerdict decided "keep buffering vs fail closed" with overBound(pos), where pos counts only the record bytes already CONSUMED. The invariant MAX_CLIENT_HELLO_BYTES declares is on the ACCUMULATED buffer: "the hard upper bound on the bytes buffered while reassembling a single ClientHello ... anything larger is treated as complete-with-no-SNI (fail closed) rather than buffered further". Those two are not the same number. recordBodyVerdict admits a record whose recordEnd equals MAX_CLIENT_HELLO_BYTES, so pos can advance to MAX - 1. The header branch fires while bytes.length - pos < RECORD_HEADER_LENGTH, so bytes.length can reach pos + 4 = MAX + 3 with overBound(pos) still false and parse() still returning needMoreData(). The caller was therefore told to keep buffering a buffer that had already passed the declared hard bound. The overshoot is self-terminating - one more byte completes the record header and recordBodyVerdict fails closed - so the class-level "buffering can never grow unbounded" claim always held. What did not hold is the constant's own contract, and a guard whose predicate does not match the scenario it exists for is a defect whether or not the blast radius is currently bounded. Fixed by anchoring the test on overBound(bytes.length). Since pos <= bytes.length always holds (pos only ever advances to a recordEnd that recordBodyVerdict has already confirmed is within the buffer), the buffer-anchored test subsumes the position-anchored one: every input that used to fail closed still does, and the 1..3 byte overshoot window now fails closed too. The rationale is written up on recordHeaderVerdict so a reviewer can challenge it rather than take it. This is NOT the javabugs:S6466 finding closed earlier in this PR. That one was about a uint16 read running past the array and was closed UNREACHABLE with a per-call-site bounds contract; index safety is untouched here. This is the SIZE bound, a distinct concern on the same method. Two regression tests pin the guard as a matched pair, both built from one fixture that puts an incomplete handshake record ahead of a partial next-record header - the shape that separates the two candidate anchors, because the bytes consumed stop at the first record's end while the bytes buffered run on: * a buffer of exactly MAX_CLIENT_HELLO_BYTES fails closed, though only MAX - 1 bytes have been consumed. This is the case the old anchor got wrong; it fails against the previous guard. * the identical shape one byte smaller still asks for more data, pinning the give-up point to MAX_CLIENT_HELLO_BYTES exactly so the assertion above cannot be satisfied by a guard that gives up early on every fragmented ClientHello. The pair is what makes either test meaningful: they differ by one buffered byte and disagree on the verdict, while the consumed position is under the bound in both - precisely the distinction the old predicate could not draw. Both gates green with zero errors: verify -Ppre-commit and verify. Neither left any rewrite churn outside the footprint. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8 --- .../gateway/tls/ClientHelloSniParser.java | 21 +++++- .../gateway/tls/ClientHelloSniParserTest.java | 64 +++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java index 9880b934..9df042cd 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java @@ -108,6 +108,16 @@ public Result parse(byte[] bytes) { * The terminal verdict, if any, implied by the record header at {@code pos}: too few bytes for a * header (keep buffering, or fail closed once the bound is passed), or a record that is not a TLS * handshake at all. + *

+ * The give-up test is anchored on {@code bytes.length} — the bytes already + * buffered — rather than on {@code pos}, the bytes already consumed. {@link #MAX_CLIENT_HELLO_BYTES} + * declares its bound on the accumulated buffer, and in this branch {@code pos} trails + * {@code bytes.length} by up to {@code RECORD_HEADER_LENGTH - 1}: a position-anchored test would + * therefore still answer {@link Result#needMoreData()} for a buffer holding up to + * {@code MAX_CLIENT_HELLO_BYTES + 3} bytes, telling the caller to keep buffering something that + * has already passed the declared hard bound. Since {@code pos <= bytes.length} always holds + * ({@link #recordBodyVerdict(byte[], int)} advances {@code pos} only to a {@code recordEnd} it has + * confirmed is within the buffer), the buffer-anchored test subsumes the position-anchored one. * * @param bytes the accumulated connection bytes * @param pos the offset of the record header being examined @@ -115,7 +125,7 @@ public Result parse(byte[] bytes) { */ private static @Nullable Result recordHeaderVerdict(byte[] bytes, int pos) { if (bytes.length - pos < RECORD_HEADER_LENGTH) { - return overBound(pos) ? Result.parsed(null) : Result.needMoreData(); + return overBound(bytes.length) ? Result.parsed(null) : Result.needMoreData(); } if ((bytes[pos] & UINT8_MASK) != RECORD_TYPE_HANDSHAKE) { // Not a TLS handshake record — fail closed to the terminated-strict path. @@ -161,8 +171,13 @@ public Result parse(byte[] bytes) { return null; } - private static boolean overBound(int pos) { - return pos >= MAX_CLIENT_HELLO_BYTES; + /** + * Whether {@code byteCount} has reached the reassembly bound. Callers pass the number of bytes + * buffered, never the number consumed — see + * {@link #recordHeaderVerdict(byte[], int)} for why the distinction is load-bearing. + */ + private static boolean overBound(int byteCount) { + return byteCount >= MAX_CLIENT_HELLO_BYTES; } /** diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java index 72178c31..78f83f4d 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java @@ -223,6 +223,42 @@ void oversizeRecordFailsClosed() { assertTrue(result.complete(), "an oversize declared record is decided, never buffered"); assertNull(result.serverName()); } + + @Test + @DisplayName("fails closed once the buffered bytes reach the bound, not once the consumed bytes do") + void bufferedBytesAtBoundFailClosed() { + // Arrange — MAX_CLIENT_HELLO_BYTES bytes buffered, of which only MAX - 1 have been + // consumed: the trailing byte opens a record header that cannot be read yet. A give-up + // test anchored on the consumed position answers needMoreData here, so the caller keeps + // buffering a buffer that has already reached the hard bound the constant declares. + byte[] atBound = ClientHelloFixture.incompleteHelloWithPartialNextHeader( + ClientHelloSniParser.MAX_CLIENT_HELLO_BYTES, 1); + + // Act + ClientHelloSniParser.Result result = parser.parse(atBound); + + // Assert + assertTrue(result.complete(), + "a buffer at the hard bound is decided, never handed back for more buffering"); + assertNull(result.serverName(), "reaching the buffering bound fails closed"); + } + + @Test + @DisplayName("still asks for more data one byte below the bound") + void bufferedBytesBelowBoundKeepBuffering() { + // Arrange — the matched control for the test above: the identical shape one byte smaller. + // It pins the give-up test to MAX_CLIENT_HELLO_BYTES exactly, so the fail-closed verdict + // above cannot be satisfied by a guard that gives up early on every fragmented ClientHello. + byte[] belowBound = ClientHelloFixture.incompleteHelloWithPartialNextHeader( + ClientHelloSniParser.MAX_CLIENT_HELLO_BYTES - 1, 1); + + // Act + ClientHelloSniParser.Result result = parser.parse(belowBound); + + // Assert + assertFalse(result.complete(), "a buffer below the bound is still reassembling"); + assertNull(result.serverName(), "an incomplete ClientHello carries no server name"); + } } @Nested @@ -325,6 +361,8 @@ static final class ClientHelloFixture { private static final byte RECORD_HANDSHAKE = 0x16; private static final byte HANDSHAKE_CLIENT_HELLO = 0x01; private static final int EXTENSION_TYPE_SERVER_NAME = 0x0000; + private static final int RECORD_HEADER_LENGTH = 5; + private static final int HANDSHAKE_HEADER_LENGTH = 4; private ClientHelloFixture() { } @@ -418,6 +456,32 @@ static byte[] concat(byte[]... arrays) { return out.toByteArray(); } + /** + * A buffer of exactly {@code totalLength} bytes made of one handshake record that leaves the + * ClientHello incomplete, followed by {@code partialHeaderBytes} bytes of the next record + * header — fewer than the {@value #RECORD_HEADER_LENGTH} a full header needs, so the parser + * stops at the loop-head length guard. + *

+ * This is the shape that separates the two candidate anchors of the give-up test: the bytes + * consumed stop at the first record's end, while the bytes buffered run on to + * {@code totalLength}. The first record's declared handshake body reaches the bound exactly, so + * the reassembled handshake is incomplete without being malformed. + */ + static byte[] incompleteHelloWithPartialNextHeader(int totalLength, int partialHeaderBytes) { + int recordEnd = totalLength - partialHeaderBytes; + byte[] payload = new byte[recordEnd - RECORD_HEADER_LENGTH]; + int declaredBody = ClientHelloSniParser.MAX_CLIENT_HELLO_BYTES - HANDSHAKE_HEADER_LENGTH; + payload[0] = HANDSHAKE_CLIENT_HELLO; + payload[1] = (byte) ((declaredBody >> 16) & 0xFF); + payload[2] = (byte) ((declaredBody >> 8) & 0xFF); + payload[3] = (byte) (declaredBody & 0xFF); + + ByteArrayOutputStream out = new ByteArrayOutputStream(); + writeRecord(out, payload); + out.write(new byte[]{RECORD_HANDSHAKE, 0x03, 0x01, 0x00}, 0, partialHeaderBytes); + return out.toByteArray(); + } + /** A record header declaring a length beyond the parser's reassembly bound. */ static byte[] oversizeRecordHeader() { int declared = ClientHelloSniParser.MAX_CLIENT_HELLO_BYTES + 1; From 5742019c0e9f73d00b44859057777e1f21ddecd4 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 4 Aug 2026 03:42:42 +0200 Subject: [PATCH 10/10] test(edge): assert the forced-h2 factory input in the gRPC assembly leg PR 151 CodeRabbit inline comment 094c4b on RouteRuntimeAssemblerTest.java:194. The gRPC leg of shouldAssembleSessionRoutes ended on assertNotNull(grpc.getHttpClient(), "a gRPC route carries the forced-h2 upstream client") The message claims forced-h2; the assertion only proved non-null. The test's client factory is clientFactory = _ -> vertx.createHttpClient(); which DISCARDS its UpstreamTarget, so the leg passed identically whether or not the assembler asked for a forced-h2 client. The only thing that could have failed it is the factory returning null, which it never does. That is exactly the weak-assertion class this plan exists to remove, reintroduced by the very hunk that replaced the leg's original assertDoesNotThrow - so it is in scope and authored by this PR, not pre-existing. RouteRuntimeAssembler builds UpstreamTarget.of(resolvedUpstream, route.protocol() == Protocol.GRPC) at line 159, so forcedHttp2 is the observable that proves the gRPC branch ran. A capturing UpstreamClientFactory records the target the assembler hands it, which is the direct, non-reflective way to assert on the factory's INPUT rather than on its never-null output. The gRPC leg now asserts exactly one client was resolved and that its target carries forcedHttp2(). The WebSocket leg becomes the matched negative control: the identical capture over a non-gRPC route asserts forcedHttp2() is false. Without it the new assertion would be only half-pinned - assertTrue on a flag would still pass if the flag were hardcoded true, or if forced-h2 were requested for every protocol. The two legs assert opposite values on the same field through the same capture path, so neither can be vacuous. Nothing was weakened or deleted: the original assertNotNull and both existing assertEquals assertions on the WebSocket leg are preserved verbatim, with the new assertions added alongside them. Both gates green with zero errors: verify -Ppre-commit and verify. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8 --- .../edge/RouteRuntimeAssemblerTest.java | 43 ++++++++++++++++--- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java index 0797b094..9269f01d 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java @@ -17,6 +17,7 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertNull; @@ -184,23 +185,51 @@ void shouldAssembleSessionRoutes() { assertEquals("session", webSocketSession.getEffectiveAuth().require(), "and keeps its require:session posture for the stage-4 runtime to dispatch on"); - // A gRPC route with non-session auth assembles cleanly — the forced-h2 upstream client is - // built by the injected client factory. + // A gRPC route with non-session auth assembles cleanly — and asks for a forced-h2 upstream + // client. The observable that proves the gRPC branch ran is the UpstreamTarget handed to the + // client factory, whose forcedHttp2 flag the assembler sets exactly for Protocol.GRPC. + // Capturing it is what makes this leg discriminating: the shared clientFactory discards its + // target and never returns null, so asserting only that a client came back would hold whether + // or not forced-h2 was ever requested. + List grpcTargets = new ArrayList<>(); RouteTable grpcTable = new RouteTable(List.of( route("g", Protocol.GRPC, "none", null, upstream("a.example")))); - RouteRuntime grpc = assembler.assemble(grpcTable, securityConfigFactory, clientFactory, - guardFactory, assetSourceFactory).getFirst(); + RouteRuntime grpc = assembler.assemble(grpcTable, securityConfigFactory, + capturingClientFactory(grpcTargets), guardFactory, assetSourceFactory).getFirst(); assertEquals("g", grpc.getId(), "the gRPC route reaches the assembled table"); assertNotNull(grpc.getHttpClient(), "a gRPC route carries the forced-h2 upstream client"); - - // A WebSocket route with non-session auth likewise assembles cleanly. + assertEquals(1, grpcTargets.size(), "the gRPC route resolves exactly one upstream client"); + assertTrue(grpcTargets.getFirst().forcedHttp2(), + "the gRPC route asks the client factory for a forced-h2 client"); + + // A WebSocket route with non-session auth likewise assembles cleanly, and doubles as the + // matched negative control for the forced-h2 assertion above: the identical capture over a + // non-gRPC route must report forcedHttp2() == false, so that assertion is pinned to the + // protocol rather than passing for every route the assembler builds. + List webSocketTargets = new ArrayList<>(); RouteTable webSocketNoneTable = new RouteTable(List.of( route("w", Protocol.WEBSOCKET, "none", null, upstream("a.example")))); RouteRuntime webSocketNone = assembler.assemble(webSocketNoneTable, securityConfigFactory, - clientFactory, guardFactory, assetSourceFactory).getFirst(); + capturingClientFactory(webSocketTargets), guardFactory, assetSourceFactory).getFirst(); assertEquals("w", webSocketNone.getId(), "the WebSocket route reaches the assembled table"); assertEquals("none", webSocketNone.getEffectiveAuth().require(), "and carries its declared require:none posture"); + assertEquals(1, webSocketTargets.size(), "the WebSocket route resolves exactly one upstream client"); + assertFalse(webSocketTargets.getFirst().forcedHttp2(), + "a non-gRPC route asks for a plain client, never a forced-h2 one"); + } + + /** + * A client factory that records every {@link RouteRuntimeAssembler.UpstreamTarget} the assembler + * asks it for, so a test can assert on the factory's input rather than only on its + * never-null output. + */ + private RouteRuntimeAssembler.UpstreamClientFactory capturingClientFactory( + List captured) { + return target -> { + captured.add(target); + return vertx.createHttpClient(); + }; } @Test