diff --git a/.plan/marshal.json b/.plan/marshal.json index 474e4ccb..60ed916b 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": "off" + }, + "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" } } 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/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java index 1336bb53..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 @@ -81,41 +81,103 @@ 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. + *

+ * 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 + * @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(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. + 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) { - 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; } /** @@ -168,7 +230,7 @@ private static HandshakeSpan completeHandshake(byte[] handshake) { cursor.seek(next); } return null; - } catch (MalformedHelloException e) { + } catch (MalformedHelloException _) { return null; } } @@ -202,10 +264,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/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..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 @@ -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 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"); + 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 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); - // 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,44 @@ 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() - .name("benchmark-keycloak") - .issuer(ISSUER) - .jwks(IssuerConfig.Jwks.builder() - .source("http") - .url(JWKS_URL) - .allowedEgressHosts(List.of(BLOCKED_HOST)) - .build()) - .build()); - - // Act - TokenValidator validator = producer.gatewayTokenValidator(); + // 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.Jwks declaringNothing = IssuerConfig.Jwks.builder() + .source("http") + .url(JWKS_URL) + .build(); - // Assert - assertNotNull(validator, "an issuer carrying an egress allowlist still builds a validator"); + // 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), + "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) { @@ -268,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/edge/RouteRuntimeAssemblerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java index b29c5d4e..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; @@ -172,27 +173,63 @@ 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"); - - // A gRPC route with non-session auth assembles cleanly — the forced-h2 upstream client is - // built by the injected client factory. + 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 — 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")))); - assertDoesNotThrow( - () -> assembler.assemble(grpcTable, securityConfigFactory, clientFactory, guardFactory, assetSourceFactory), - "a gRPC route with non-session auth assembles cleanly"); - - // A WebSocket route with non-session auth likewise assembles cleanly. + 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"); + 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")))); - assertDoesNotThrow( - () -> assembler.assemble(webSocketNoneTable, securityConfigFactory, clientFactory, guardFactory, assetSourceFactory), - "a WebSocket route with non-session auth assembles cleanly"); + RouteRuntime webSocketNone = assembler.assemble(webSocketNoneTable, securityConfigFactory, + 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 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..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; @@ -115,8 +114,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/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); 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/ClientHelloSniParserTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java index 634aef95..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 @@ -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() { @@ -187,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 @@ -289,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() { } @@ -382,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; 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..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 @@ -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 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. + // 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,103 @@ class PassthroughUnconfigured { @Test @DisplayName("never starts the front listener when passthrough_sni is empty") - void noFrontListenerWhenPassthroughEmpty() { + 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(); 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 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. + // 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 _) { + // A refused connection IS the answer: nothing is listening on the probed port. + return false; + } + } + + // 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 _) { + 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..51e977a5 --- /dev/null +++ b/doc/development/test-corpus-integrity.adoc @@ -0,0 +1,865 @@ += 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. + +=== 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, +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.