From fb495ee7b614c404a145b0fe409df9ed5b9a6ac9 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:33:47 +0200 Subject: [PATCH 01/16] fix(gateway): derive the body limit, reject breach 413 Ship quarkus.http.limits.max-body-size=64M so a declared per-route max_body_bytes is reachable instead of being silently clipped by the Vert.x 10 MiB default, and add a fail-closed boot check in ConfigProducer that aborts when any declared cap exceeds the framework limit. Both body-cap raise sites now throw the new CONTENT_TOO_LARGE (413) event instead of PARAMETER_LIMIT_EXCEEDED (400); enforcement predicates, thresholds and abort paths are unchanged. GrpcStatusMapper gains RESOURCE_EXHAUSTED (8) with the 413 arm and its class-javadoc row, so a gRPC body-cap breach no longer degrades to UNKNOWN. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7 --- .../sheriff/gateway/edge/DispatchStage.java | 6 +- .../gateway/edge/GrpcStatusMapper.java | 4 ++ .../sheriff/gateway/events/EventType.java | 16 +++++ .../gateway/pipeline/ThoroughChecksStage.java | 8 +-- .../gateway/quarkus/ConfigProducer.java | 70 ++++++++++++++++++- .../src/main/resources/application.properties | 16 +++++ .../gateway/edge/DispatchStageTest.java | 4 +- .../gateway/edge/GrpcDispatchStageTest.java | 8 +-- .../gateway/edge/GrpcStatusMapperTest.java | 7 ++ .../sheriff/gateway/events/EventTypeTest.java | 1 + .../pipeline/ThoroughChecksStageTest.java | 2 +- .../gateway/quarkus/ConfigFailFastTest.java | 12 ++++ .../gateway/quarkus/ConfigProducerTest.java | 68 ++++++++++++++++++ 13 files changed, 206 insertions(+), 16 deletions(-) diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java index 8017e5ac..c236de60 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java @@ -53,7 +53,7 @@ * {@link ByteCappedBodyStream} that forwards each chunk to the upstream request as it arrives and * enforces the {@code max_body_bytes} ceiling with a running counter. A mid-stream breach ABORTS * the in-flight upstream call (Vert.x {@link HttpClientRequest#reset()}) and surfaces - * {@link EventType#PARAMETER_LIMIT_EXCEEDED} (400). The upstream body is never + * {@link EventType#CONTENT_TOO_LARGE} (413). The upstream body is never * materialized into an {@code HttpResult} (ADR-0006/0008): the returned * {@link HttpClientResponse} is a live {@link ReadStream} whose body {@link ResponseStage} streams * back with backpressure. @@ -283,7 +283,7 @@ private static String stripTrailingSlash(String value) { * A {@link ReadStream} decorator that forwards each request-body chunk to the upstream as it * arrives — never accumulating the body — while counting bytes against a ceiling. On breach it * aborts the in-flight upstream request and fails the stream with a - * {@link EventType#PARAMETER_LIMIT_EXCEEDED} {@link GatewayException}. + * {@link EventType#CONTENT_TOO_LARGE} {@link GatewayException}. */ static final class ByteCappedBodyStream implements ReadStream { @@ -320,7 +320,7 @@ private void onChunk(Buffer chunk) { aborted = true; delegate.pause(); abortAction.run(); - propagateFailure(new GatewayException(EventType.PARAMETER_LIMIT_EXCEEDED, + propagateFailure(new GatewayException(EventType.CONTENT_TOO_LARGE, "Request body exceeded max_body_bytes=" + maxBytes)); return; } diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java index 2a9d82ac..7c109c83 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java @@ -40,6 +40,7 @@ *
  • {@code 403} → {@code PERMISSION_DENIED} (7)
    • *
  • {@code 404} → {@code NOT_FOUND} (5)
    • *
  • {@code 405} → {@code UNIMPLEMENTED} (12)
    • + *
  • {@code 413} → {@code RESOURCE_EXHAUSTED} (8)
    • *
  • {@code 502} / {@code 503} → {@code UNAVAILABLE} (14) — also an h2-negotiation failure
    • *
  • {@code 504} → {@code DEADLINE_EXCEEDED} (4)
    • *
  • anything else → {@code UNKNOWN} (2)
    • @@ -62,6 +63,8 @@ public final class GrpcStatusMapper { public static final int NOT_FOUND = 5; /** gRPC status code: the caller lacked permission (maps HTTP 403). */ public static final int PERMISSION_DENIED = 7; + /** gRPC status code: a per-request resource bound was exhausted (maps HTTP 413). */ + public static final int RESOURCE_EXHAUSTED = 8; /** gRPC status code: the operation is not implemented / not supported (maps HTTP 405). */ public static final int UNIMPLEMENTED = 12; /** gRPC status code: the service is unavailable (maps HTTP 502 / 503 and h2-negotiation failure). */ @@ -90,6 +93,7 @@ public int toGrpcStatus(EventType eventType) { case 403 -> PERMISSION_DENIED; case 404 -> NOT_FOUND; case 405 -> UNIMPLEMENTED; + case 413 -> RESOURCE_EXHAUSTED; case 502, 503 -> UNAVAILABLE; case 504 -> DEADLINE_EXCEEDED; default -> UNKNOWN; diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java index 87cb9ef3..44860b17 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java @@ -75,8 +75,24 @@ public enum EventType { * {@code reserved_body_max_bytes} ceiling. These paths are read before the pipeline's per-route * body cap can apply, so the ceiling is enforced at the read itself and the request is rejected * {@code 413} without the oversized body ever being buffered. + *

    + * Contrast {@link #CONTENT_TOO_LARGE}, which is the per-route + * {@code security_filter.max_body_bytes} cap on the proxy path. This event is exclusively the + * edge-level {@code reserved_body_max_bytes} ceiling on gateway-terminated reserved paths. */ RESERVED_BODY_TOO_LARGE(EventCategory.INPUT_VALIDATION, 413), + /** + * A proxied request declared or streamed a body beyond its route's + * {@code security_filter.max_body_bytes} cap. The cap has two enforcement points: the + * {@code Content-Length} fast-reject in {@code ThoroughChecksStage}, which rejects before any + * body is read, and the streaming byte counter in {@code DispatchStage}, which aborts a chunked + * or under-declared body mid-transfer once the cap is crossed. + *

    + * Contrast {@link #RESERVED_BODY_TOO_LARGE}, which is the edge's {@code reserved_body_max_bytes} + * ceiling on gateway-terminated reserved BFF POST paths. This event is exclusively the per-route + * proxy-path cap. + */ + CONTENT_TOO_LARGE(EventCategory.INPUT_VALIDATION, 413), // --- Authentication (401) --- diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java index bdc89db1..af2bf7ef 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java @@ -44,8 +44,8 @@ * whitelist, the canonical path must match one pattern, where a {@code {name}} segment matches * exactly one path segment; a miss is a 400 {@link EventType#PATH_NOT_ALLOWED}. *

  • {@code max_body_bytes} fast-reject. A declared {@code Content-Length} - * already exceeding the route config's {@code maxBodySize} is rejected 400 - * ({@link EventType#PARAMETER_LIMIT_EXCEEDED}) before the body is read.
    • + * already exceeding the route config's {@code maxBodySize} is rejected 413 + * ({@link EventType#CONTENT_TOO_LARGE}) before the body is read. * * * @author API Sheriff Team @@ -76,7 +76,7 @@ public ThoroughChecksStage(SecurityConfiguration defaultConfiguration, SecurityE * @param allowedPaths the selected route's {@code allowed_paths} whitelist, empty when unrestricted * @throws GatewayException on a divergent-pipeline violation ({@link EventType#SECURITY_FILTER_VIOLATION}), * a whitelist miss ({@link EventType#PATH_NOT_ALLOWED}), or a body-cap breach - * ({@link EventType#PARAMETER_LIMIT_EXCEEDED}) + * ({@link EventType#CONTENT_TOO_LARGE}) */ public void process(PipelineRequest request, List allowedPaths) { Objects.requireNonNull(request, "request"); @@ -119,7 +119,7 @@ private void reRunPipelines(PipelineRequest request, SecurityConfiguration route private static void enforceBodyCap(PipelineRequest request, SecurityConfiguration routeConfig) { long cap = routeConfig.maxBodySize(); if (request.declaredContentLength() > cap) { - throw new GatewayException(EventType.PARAMETER_LIMIT_EXCEEDED, + throw new GatewayException(EventType.CONTENT_TOO_LARGE, "Declared body %d exceeds route cap %d".formatted(request.declaredContentLength(), cap)); } } diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java index 77be87cc..5b274bb5 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java @@ -16,10 +16,13 @@ package de.cuioss.sheriff.gateway.quarkus; import java.nio.file.Path; +import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; +import java.util.stream.Stream; import de.cuioss.sheriff.gateway.config.ConfigLogMessages; @@ -28,6 +31,7 @@ import de.cuioss.sheriff.gateway.config.load.ConfigLoadException; import de.cuioss.sheriff.gateway.config.load.ConfigLoader; import de.cuioss.sheriff.gateway.config.load.EnvSecretResolver; +import de.cuioss.sheriff.gateway.config.model.AnchorConfig; import de.cuioss.sheriff.gateway.config.model.AssetConfig; import de.cuioss.sheriff.gateway.config.model.EdgeHardeningConfig; import de.cuioss.sheriff.gateway.config.model.EndpointConfig; @@ -36,6 +40,7 @@ import de.cuioss.sheriff.gateway.config.model.ResolvedTopology; import de.cuioss.sheriff.gateway.config.model.RouteConfig; import de.cuioss.sheriff.gateway.config.model.RouteTable; +import de.cuioss.sheriff.gateway.config.model.SecurityFilterConfig; import de.cuioss.sheriff.gateway.config.model.TlsConfig; import de.cuioss.sheriff.gateway.config.topology.TopologyResolver; import de.cuioss.sheriff.gateway.config.validation.ConfigValidator; @@ -43,6 +48,7 @@ import de.cuioss.tools.logging.CuiLogger; import io.quarkus.runtime.StartupEvent; +import io.quarkus.runtime.configuration.MemorySize; import jakarta.enterprise.context.ApplicationScoped; import jakarta.enterprise.event.Observes; import jakarta.enterprise.inject.Produces; @@ -56,7 +62,13 @@ * It drives the framework-agnostic boot pipeline — {@link ConfigLoader} (read, * schema-validate, secret-resolve, bind) → endpoint-enablement filter → * {@link TopologyResolver} → {@link ConfigValidator} → {@link RouteTableBuilder} — - * supplying every collaborator by construction. On the first collected violation + * supplying every collaborator by construction. It additionally applies one check the + * framework-agnostic validator cannot: the largest declared {@code max_body_bytes} + * across the anchors and the enabled endpoints' routes must not exceed the Vert.x + * {@code quarkus.http.limits.max-body-size} ceiling, because the framework rejects an + * over-ceiling request in a root handler in front of the gateway router and the declared + * cap would never be reached. That violation joins the validator's own collection and + * aborts through the same path. On the first collected violation * the producer logs every problem through structured ERROR * {@link ConfigLogMessages} records and throws, so Quarkus exits non-zero and never * serves on partial configuration. On success it emits the {@code CONFIG_LOADED} @@ -76,6 +88,14 @@ public class ConfigProducer { @ConfigProperty(name = "sheriff.config.dir", defaultValue = "config") String configDir; + /** + * The Vert.x request-body ceiling the framework enforces in a root handler in front of the + * gateway router. Injected here — the ADR-0005 framework-bound edge — rather than in the + * framework-agnostic {@code ConfigValidator}, so Quarkus-key knowledge stays at this seam. + */ + @ConfigProperty(name = "quarkus.http.limits.max-body-size") + MemorySize frameworkBodyLimit; + private GatewayConfig gateway; private RouteTable routeTable; private ResolvedTopology resolvedTopology; @@ -177,7 +197,9 @@ private synchronized void buildOnce() { List enabled = loaded.endpoints().stream().filter(EndpointConfig::enabled).toList(); ResolvedTopology topology = new TopologyResolver(resolver) .resolve(directory.resolve(TOPOLOGY_FILE), enabled, additionalTopologyAliases(loaded, enabled)); - List violations = new ConfigValidator().validate(loaded.gateway(), enabled, topology); + List violations = new ArrayList<>( + new ConfigValidator().validate(loaded.gateway(), enabled, topology)); + violations.addAll(frameworkBodyLimitViolations(loaded.gateway(), enabled)); if (!violations.isEmpty()) { abort(violations); } @@ -194,6 +216,50 @@ private synchronized void buildOnce() { } } + /** + * The fail-closed framework-limit check: a declared per-route body cap above the Vert.x ceiling + * is unreachable, because Quarkus rejects the request in a root handler installed in front of the + * gateway router. Rather than let that mismatch stay silent, the boot refuses to start and names + * the key the operator must raise. + * + * @param gateway the bound gateway document (source of the anchor-level caps) + * @param enabled the enabled endpoints whose routes carry the route-level caps + * @return the single violation when the largest declared cap exceeds the framework limit, + * otherwise an empty list + */ + private List frameworkBodyLimitViolations(GatewayConfig gateway, List enabled) { + long limit = frameworkBodyLimit.asLongValue(); + long declared = maxDeclaredBodyBytes(gateway, enabled); + if (declared <= limit) { + return List.of(); + } + return List.of(new ConfigError("application.properties", "quarkus.http.limits.max-body-size", + "%d exceeds framework limit %d; raise quarkus.http.limits.max-body-size to at least %d" + .formatted(declared, limit, declared))); + } + + /** + * The largest {@code max_body_bytes} declared anywhere in the configuration set — across the + * named policy anchors and every enabled endpoint's routes, the only two places a per-route cap + * can be declared. + * + * @param gateway the bound gateway document + * @param enabled the enabled endpoints + * @return the maximum declared cap in bytes, or {@code 0} when no cap is declared + */ + private static long maxDeclaredBodyBytes(GatewayConfig gateway, List enabled) { + return Stream.concat( + gateway.anchors().values().stream().map(AnchorConfig::securityFilter), + enabled.stream().flatMap(endpoint -> endpoint.routes().stream()) + .map(RouteConfig::securityFilter)) + .flatMap(Optional::stream) + .map(SecurityFilterConfig::maxBodyBytes) + .flatMap(Optional::stream) + .mapToLong(Integer::longValue) + .max() + .orElse(0L); + } + /** * The topology aliases that must resolve independently of any enabled endpoint's * {@code base_url}: the {@code tls.passthrough_sni} relay targets and every diff --git a/api-sheriff/src/main/resources/application.properties b/api-sheriff/src/main/resources/application.properties index e7020767..214c56bc 100644 --- a/api-sheriff/src/main/resources/application.properties +++ b/api-sheriff/src/main/resources/application.properties @@ -8,6 +8,22 @@ quarkus.http.port=8080 quarkus.http.ssl-port=8443 quarkus.http.insecure-requests=redirect +# Framework body-size floor under the gateway's own per-route max_body_bytes caps. +# Without this key Vert.x applies the Quarkus default of 10 MiB and silently clips any route +# declaring a larger security_filter.max_body_bytes, so the gateway's own cap never fires. +# (i) This is the FLOOR: the gateway still enforces the smaller per-route cap on top of it. +# (ii) A deployment declaring a larger max_body_bytes must raise this key — ConfigProducer's +# fail-closed boot check refuses to start when a declared cap exceeds this limit. +# (iii) At floor == largest declared cap, the Quarkus root handler's Content-Length pre-check +# answers a breach with a bare 413 (Connection: close, empty body) BEFORE the gateway +# pipeline runs, so the RFC 9457 problem+json envelope is rendered only for routes capped +# below the floor. An operator who wants the gateway's own envelope on the largest-capped +# route sets this floor ABOVE that cap rather than equal to it. +# (iv) Chunked requests carry no Content-Length, so they pass the pre-check untouched and are +# capped by the gateway's streaming byte counter instead. +# 64M == 67108864 bytes, matching the largest declared max_body_bytes in the shipped topology. +quarkus.http.limits.max-body-size=64M + # Management interface — health/metrics on a separate port (9000), served over HTTPS. # Follows the Keycloak pattern of splitting operations off the application port: application on # 8443, operations on 9000. Unlike quarkus.http, Quarkus' ManagementConfig declares NO ssl-port and diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.java index a4abde87..fd186ec7 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.java @@ -134,12 +134,12 @@ void abortsOnBreach() { source.emit(Buffer.buffer("123456")); source.emit(Buffer.buffer("ABCDEF")); - // Assert — the breaching chunk is not forwarded, the call is aborted, and a 400 is raised + // Assert — the breaching chunk is not forwarded, the call is aborted, and a 413 is raised assertEquals(1, forwarded.size(), "the breaching chunk must never cross to the upstream"); assertTrue(aborted.get(), "a mid-stream breach must abort the in-flight upstream call"); Throwable raised = failure.get(); GatewayException gatewayException = assertInstanceOf(GatewayException.class, raised); - assertEquals(EventType.PARAMETER_LIMIT_EXCEEDED, gatewayException.getEventType()); + assertEquals(EventType.CONTENT_TOO_LARGE, gatewayException.getEventType()); } @Test diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.java index 829dec9d..72032379 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.java @@ -319,7 +319,7 @@ void streamsMultiFrameUnderCapWithoutMisfire() { } @Test - @DisplayName("aborts the gRPC dispatch with PARAMETER_LIMIT_EXCEEDED when the body cap is breached") + @DisplayName("aborts the gRPC dispatch with CONTENT_TOO_LARGE when the body cap is breached") void abortsOnBodyCapBreach() { // Arrange — the shared body-abuse bound also applies to the opaque gRPC frame stream TestReadStream source = new TestReadStream(); @@ -335,13 +335,13 @@ void abortsOnBodyCapBreach() { source.emit(Buffer.buffer("frame")); source.emit(Buffer.buffer("flood")); - // Assert — the breaching frame is dropped, the dispatch is aborted, and a 400 is raised + // Assert — the breaching frame is dropped, the dispatch is aborted, and a 413 is raised assertEquals(1, forwarded.size(), "the breaching frame must never cross to the upstream"); assertTrue(aborted.get(), "a body-cap breach aborts the in-flight gRPC dispatch"); GatewayException raised = assertInstanceOf(GatewayException.class, failure.get(), "the breach raises a GatewayException"); - assertEquals(EventType.PARAMETER_LIMIT_EXCEEDED, raised.getEventType(), - "the shared body-abuse bound raises PARAMETER_LIMIT_EXCEEDED on the gRPC path"); + assertEquals(EventType.CONTENT_TOO_LARGE, raised.getEventType(), + "the shared body-abuse bound raises CONTENT_TOO_LARGE on the gRPC path"); } } diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.java index 15f3b9fd..858632f3 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.java @@ -86,6 +86,13 @@ void mapsUnimplemented() { "an HTTP 405 rejection maps to gRPC UNIMPLEMENTED (12)"); } + @Test + @DisplayName("413 (body cap breached) maps to RESOURCE_EXHAUSTED") + void mapsContentTooLargeToResourceExhausted() { + assertEquals(GrpcStatusMapper.RESOURCE_EXHAUSTED, mapper.toGrpcStatus(EventType.CONTENT_TOO_LARGE), + "an HTTP 413 body-cap breach maps to gRPC RESOURCE_EXHAUSTED (8)"); + } + @Test @DisplayName("502 (upstream error / h2-negotiation failure) maps to UNAVAILABLE") void mapsBadGatewayToUnavailable() { diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java index 285ef897..3b63c087 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java @@ -78,6 +78,7 @@ class FailureEvents { "PASSTHROUGH_HOST_SMUGGLED, 404, INPUT_VALIDATION", "METHOD_NOT_ALLOWED, 405, INPUT_VALIDATION", "RESERVED_BODY_TOO_LARGE, 413, INPUT_VALIDATION", + "CONTENT_TOO_LARGE, 413, INPUT_VALIDATION", "TOKEN_MISSING, 401, AUTHENTICATION", "TOKEN_INVALID, 401, AUTHENTICATION", "SCOPE_MISSING, 403, AUTHORIZATION", 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 bdb2a1b3..fffd75c9 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 @@ -125,7 +125,7 @@ void rejectsBodyExceedingRouteCap() { () -> stage.process(request, List.of())); // Assert - assertEquals(EventType.PARAMETER_LIMIT_EXCEEDED, thrown.getEventType()); + assertEquals(EventType.CONTENT_TOO_LARGE, thrown.getEventType()); } @Test diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.java index 4dca1c93..ae7badf7 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import java.math.BigInteger; import java.net.URISyntaxException; import java.nio.file.Path; @@ -27,6 +28,7 @@ import de.cuioss.test.juli.TestLogLevel; import de.cuioss.test.juli.junit5.EnableTestLogger; +import io.quarkus.runtime.configuration.MemorySize; import org.junit.jupiter.api.Test; /** @@ -40,11 +42,21 @@ @EnableTestLogger class ConfigFailFastTest { + /** + * The framework body ceiling wired onto every producer here, matching the shipped + * {@code quarkus.http.limits.max-body-size} floor. None of these fixtures declares a + * {@code max_body_bytes}, so the framework-limit check is inert for them — the field is set + * because the producer reads it on every boot, not because these tests exercise the check + * (that is {@code ConfigProducerTest}'s job). + */ + private static final long FRAMEWORK_LIMIT_BYTES = 67108864L; + private static ConfigProducer producerFor(String resourceDir) throws URISyntaxException { var resource = ConfigFailFastTest.class.getResource(resourceDir); assertNotNull(resource, resourceDir + " fixture must be on the test classpath"); ConfigProducer producer = new ConfigProducer(); producer.configDir = Path.of(resource.toURI()).toString(); + producer.frameworkBodyLimit = new MemorySize(BigInteger.valueOf(FRAMEWORK_LIMIT_BYTES)); return producer; } 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 78e834d9..5b54e44b 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 @@ -23,6 +23,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; +import java.math.BigInteger; import java.nio.file.Files; import java.nio.file.Path; @@ -38,6 +39,7 @@ import de.cuioss.test.juli.TestLogLevel; import de.cuioss.test.juli.junit5.EnableTestLogger; +import io.quarkus.runtime.configuration.MemorySize; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -129,6 +131,35 @@ class ConfigProducerTest { websocket_relay_cap: 16 """; + /** + * The framework ceiling every producer in this class is wired with. Deliberately small and + * unrelated to the shipped {@code 64M} default: the check under test is the inequality between + * the declared cap and the injected limit, so a tiny limit exercises it exactly and keeps the + * fixture documents readable. + */ + private static final long FRAMEWORK_LIMIT_BYTES = 1024L; + + /** An anchor declaring a body cap ABOVE {@link #FRAMEWORK_LIMIT_BYTES} — unreachable, so boot must abort. */ + private static final String GATEWAY_WITH_CAP_ABOVE_FRAMEWORK_LIMIT = gatewayDeclaringBodyCap(2048); + + /** An anchor declaring a body cap exactly AT {@link #FRAMEWORK_LIMIT_BYTES} — reachable, so boot is clean. */ + private static final String GATEWAY_WITH_CAP_AT_FRAMEWORK_LIMIT = gatewayDeclaringBodyCap(1024); + + private static String gatewayDeclaringBodyCap(int maxBodyBytes) { + return """ + version: 1 + metadata: + config_version: "2026-07-13" + anchors: + uploads: + path_prefix: /uploads + type: proxy + access: public + security_filter: + max_body_bytes: %d + """.formatted(maxBodyBytes); + } + @TempDir Path configDir; @@ -145,6 +176,7 @@ private ConfigProducer producerForGateway(String gatewayYaml) throws IOException Files.writeString(configDir.resolve("gateway.yaml"), gatewayYaml); ConfigProducer producer = new ConfigProducer(); producer.configDir = configDir.toString(); + producer.frameworkBodyLimit = new MemorySize(BigInteger.valueOf(FRAMEWORK_LIMIT_BYTES)); return producer; } @@ -196,6 +228,42 @@ void shouldRefuseBootWhenTheRelayCapExceedsTheAdmissionCap() throws Exception { "a relay sub-budget larger than the admission pool aborts boot rather than serving"); } + /** + * The fail-closed framework-limit check: a declared {@code max_body_bytes} above the Vert.x + * ceiling is unreachable, because Quarkus rejects the oversize request in a root handler in + * front of the gateway router. Rather than let that mismatch stay silent, boot aborts through + * the same collected-violation path, and the violation names the Quarkus key the operator must + * raise — so the ERROR record is actionable without reading the source. + */ + @Test + void shouldRefuseBootWhenADeclaredBodyCapExceedsTheFrameworkLimit() throws Exception { + ConfigProducer producer = producerForGateway(GATEWAY_WITH_CAP_ABOVE_FRAMEWORK_LIMIT); + + IllegalStateException exception = assertThrows(IllegalStateException.class, + () -> producer.onStartup(null), + "a declared body cap above the framework limit must abort boot rather than be silently clipped"); + + assertTrue(exception.getMessage().contains("Refusing to start"), + "the abort should carry the refusing-to-start summary"); + LogAsserts.assertLogMessagePresentContaining(TestLogLevel.ERROR, + "quarkus.http.limits.max-body-size"); + } + + /** + * The boundary of the same inequality: a declared cap exactly AT the framework limit is + * reachable, so it must boot clean. This pins the comparison as {@code >} rather than + * {@code >=} — the shipped configuration deliberately sets the floor equal to the largest + * declared cap, so an off-by-one here would refuse every default deployment. + */ + @Test + void shouldBootWhenTheDeclaredBodyCapEqualsTheFrameworkLimit() throws Exception { + ConfigProducer producer = producerForGateway(GATEWAY_WITH_CAP_AT_FRAMEWORK_LIMIT); + + assertDoesNotThrow(() -> producer.onStartup(null), + "a declared cap equal to the framework limit is reachable and must boot clean"); + assertNotNull(producer.gatewayConfig(), "beans should be available after startup assembly"); + } + @Test void shouldLogConfigLoadedOnSuccess() throws Exception { ConfigProducer producer = producerForValidConfig(); From 361dfedc2ba0f9438e9b042b713cda59f4a8aa4c Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:42:26 +0200 Subject: [PATCH 02/16] fix(benchmarks): bind the TOPOLOGY_UPSTREAM override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit topology.properties declared UPSTREAM as a bare literal, and EnvSecretResolver substitutes only explicit in-file placeholders — there is no TOPOLOGY_ precedence path — so the benchmark overlay's TOPOLOGY_UPSTREAM bound to nothing and every proxy aspect silently measured go-httpbin instead of nginx-static. Declare UPSTREAM as ${TOPOLOGY_UPSTREAM:-http://go-httpbin:8080/anything}; the default keeps the IT stack on go-httpbin so the proxy ITs still observe its JSON echo. Two comments that asserted the non-existent mechanism are corrected. Add an http-context nginx fragment for the static benchmark backend with lingering headroom sized for the 50 MB uploadLarge aspect, so the backend drains a large body rather than resetting it mid-transfer. Verified: nginx -t clean, and a 50 MB POST to location / returns 200 with the full body accepted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7 --- .../docker-compose.benchmark.yml | 10 ++++++--- .../src/main/docker/nginx/body-limits.conf | 22 +++++++++++++++++++ .../docker/sheriff-config/topology.properties | 12 +++++++--- 3 files changed, 38 insertions(+), 6 deletions(-) create mode 100644 integration-tests/src/main/docker/nginx/body-limits.conf diff --git a/integration-tests/docker-compose.benchmark.yml b/integration-tests/docker-compose.benchmark.yml index e4a9c69d..58222bd8 100644 --- a/integration-tests/docker-compose.benchmark.yml +++ b/integration-tests/docker-compose.benchmark.yml @@ -9,15 +9,19 @@ services: - "18081:8080" # published only for the host-side readiness probe volumes: - ./nginx/nginx-static.conf:/etc/nginx/conf.d/default.conf:ro + - ./src/main/docker/nginx/body-limits.conf:/etc/nginx/conf.d/00-body-limits.conf:ro networks: - api-sheriff restart: unless-stopped api-sheriff: environment: - # Override the UPSTREAM topology alias (go-httpbin) with the static backend - # for benchmarks. TOPOLOGY_ takes precedence over topology.properties, - # so the mounted config is reused unchanged and only the target is repointed. + # Repoint the UPSTREAM topology alias (go-httpbin) at the static backend for + # benchmarks. This binds to the explicit ${TOPOLOGY_UPSTREAM:-...} placeholder + # written in topology.properties — EnvSecretResolver substitutes only in-file + # placeholders, and there is no TOPOLOGY_ precedence mechanism. The + # mounted config is reused unchanged; unset, the placeholder's default keeps + # the IT stack on go-httpbin. - TOPOLOGY_UPSTREAM=http://nginx-static:8080 depends_on: - nginx-static diff --git a/integration-tests/src/main/docker/nginx/body-limits.conf b/integration-tests/src/main/docker/nginx/body-limits.conf new file mode 100644 index 00000000..2e835de9 --- /dev/null +++ b/integration-tests/src/main/docker/nginx/body-limits.conf @@ -0,0 +1,22 @@ +# Body-size and lingering-close settings for the static benchmark backend +# (nginx-static), included into nginx's http context via /etc/nginx/conf.d/. +# +# Contract: this backend must NEVER be the component that rejects or severs a +# benchmark upload. The gateway's per-route max_body_bytes is the only cap under +# measurement — a backend-side rejection would measure a failure path (which is +# faster than a transfer path) and report flatteringly good numbers. +# +# client_max_body_size 0 removes any nginx-side ceiling. Note it is INERT for the +# current `return 200` location: nginx evaluates the directive inside its request-body +# reader, which a return-handled location never invokes. It is declared anyway so the +# contract stays correct if that location ever gains a body-reading handler. +# +# The lingering settings are the operative half. Because the backend answers before the +# transfer completes, nginx drains the remaining body under lingering_close; the defaults +# (5s timeout, 16k) are sized for small bodies and would reset a 50 MB uploadLarge aspect +# mid-transfer. The values below give that aspect room to drain cleanly. +client_max_body_size 0; + +lingering_close always; +lingering_time 120s; +lingering_timeout 30s; diff --git a/integration-tests/src/main/docker/sheriff-config/topology.properties b/integration-tests/src/main/docker/sheriff-config/topology.properties index a3d20640..9da36a99 100644 --- a/integration-tests/src/main/docker/sheriff-config/topology.properties +++ b/integration-tests/src/main/docker/sheriff-config/topology.properties @@ -2,9 +2,15 @@ # # UPSTREAM is the go-httpbin echo backend, reached by the gateway on the internal # compose network. Its /anything/* endpoint echoes the received request as JSON so -# the proxy ITs can assert exactly what was forwarded. The benchmark overlay -# overrides this at runtime with TOPOLOGY_UPSTREAM=http://nginx-static:8080. -UPSTREAM=http://go-httpbin:8080/anything +# the proxy ITs can assert exactly what was forwarded. +# +# The value uses the ADR-0004 Amendment A1 in-file placeholder form: EnvSecretResolver +# substitutes only explicit ${VAR} / ${VAR:-default} placeholders written here — there is +# NO convention-named TOPOLOGY_ environment precedence path, so an env variable +# binds if and only if this file names it. The default keeps the IT stack on go-httpbin +# (it sets no TOPOLOGY_UPSTREAM, and the proxy ITs assert go-httpbin's JSON echo); the +# benchmark overlay sets TOPOLOGY_UPSTREAM=http://nginx-static:8080 to repoint it. +UPSTREAM=${TOPOLOGY_UPSTREAM:-http://go-httpbin:8080/anything} # ASSET_ORIGIN is the secondary static server ('asset-origin', nginx) that backs the # source: upstream asset route (/assets/cdn). The gateway fetches confined assets from From 599931b63ebad1e7dca2a5140c66c2b102f966ba Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:46:46 +0200 Subject: [PATCH 03/16] test(gateway): pin the body-limit descriptor activation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add BodyLimitActivationWiringTest, a fast no-Docker surefire guard that the committed descriptors activate the framework body floor — the unit-green / integration-red blind spot lesson 2026-07-25-15-001 names. It asserts application.properties declares quarkus.http.limits.max-body-size, that the floor covers the largest declared max_body_bytes in every committed sheriff-config*/gateway.yaml (glob-discovered, so a new instance directory is covered automatically and an empty glob cannot pass vacuously), and that the IT container override exceeds LargeBodyIT's negative-case body size. The third assertion asserts the QUARKUS_HTTP_LIMITS_MAX_BODY_SIZE override that the next deliverable adds; test-compile is green and the surefire run is covered at that deliverable's chain tail. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7 --- .../BodyLimitActivationWiringTest.java | 266 ++++++++++++++++++ 1 file changed, 266 insertions(+) create mode 100644 integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java new file mode 100644 index 00000000..747f72b6 --- /dev/null +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java @@ -0,0 +1,266 @@ +/* + * Copyright © 2026 CUI-OpenSource-Software (info@cuioss.de) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package de.cuioss.sheriff.gateway.integration; + +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.assertTrue; + +import java.io.IOException; +import java.io.InputStream; +import java.io.Reader; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Properties; +import org.yaml.snakeyaml.Yaml; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Fast, no-Docker surefire guard that the committed deployment descriptors actually + * activate the framework body-size floor the gateway's per-route + * {@code max_body_bytes} caps sit under. + *

    + * This closes a unit-green / integration-red blind spot. {@code ConfigProducer}'s fail-closed + * framework-limit check and both body-cap raise sites are unit-covered and correct in isolation, but + * the floor itself is a property: {@code quarkus.http.limits.max-body-size} in + * {@code application.properties}. Remove it and Vert.x silently reverts to its 10 MiB default, + * clipping every larger declared cap before the gateway pipeline ever runs; raise a declared cap + * past it and the boot check aborts that container. Neither failure is visible to a per-component + * unit test — only to a descriptor assertion like this one, or to the expensive container suite. + *

    + * The coverage is deliberately all committed descriptors, not just the base one: + * the compose stack boots five native gateway instances over four {@code sheriff-config*} gateway + * descriptors ({@code api-sheriff}, {@code api-sheriff-mtls}, {@code api-sheriff-cookie}, + * {@code api-sheriff-cookie-2} and {@code api-sheriff-ws-admission}), so a cap raised in any sibling + * descriptor pushes that instance into a boot abort. The descriptors are discovered by glob rather + * than hard-coded, so a new {@code sheriff-config-*} directory comes under the assertion + * automatically — and a glob that matches fewer than the four present today fails rather than + * passing vacuously. + *

    + * It parses the committed descriptors only (YAML / properties text) and asserts the activation is + * present — it starts no container and reaches no network. The containerised sibling that exercises + * the resulting 413 end-to-end is {@code LargeBodyIT}. + * + * @author API Sheriff Team + * @since 1.0 + */ +class BodyLimitActivationWiringTest { + + /** The module base directory (surefire runs with the module root as the working directory). */ + private static final Path MODULE = Path.of(System.getProperty("user.dir")); + private static final Path DOCKER = MODULE.resolve("src/main/docker"); + private static final Path APPLICATION_PROPERTIES = + MODULE.getParent().resolve("api-sheriff/src/main/resources/application.properties"); + + private static final String FRAMEWORK_LIMIT_KEY = "quarkus.http.limits.max-body-size"; + private static final String CONTAINER_OVERRIDE_KEY = "QUARKUS_HTTP_LIMITS_MAX_BODY_SIZE"; + private static final String DECLARED_CAP_KEY = "max_body_bytes"; + + /** + * The descriptor count committed today. The glob must match at least this many, so an empty or + * mis-rooted glob fails loudly instead of satisfying the per-descriptor loop vacuously. + */ + private static final int COMMITTED_DESCRIPTOR_COUNT = 4; + + /** {@code LargeBodyIT}'s negative-case body size — the container override must exceed it. */ + private static final long NEGATIVE_CASE_BODY_BYTES = 71303168L; + + @Test + @DisplayName("application.properties declares the framework body-size floor") + void applicationPropertiesDeclaresTheFrameworkLimit() throws Exception { + // Arrange + Properties properties = applicationProperties(); + + // Act + String declared = properties.getProperty(FRAMEWORK_LIMIT_KEY); + + // Assert — its absence is the exact defect this plan fixes: Vert.x then applies its 10 MiB + // default and silently clips every larger declared max_body_bytes. + assertNotNull(declared, FRAMEWORK_LIMIT_KEY + + " must be declared in application.properties, otherwise Vert.x applies its 10 MiB default" + + " and clips every larger declared max_body_bytes before the gateway pipeline runs"); + assertTrue(parseMemorySize(declared) > 0, + FRAMEWORK_LIMIT_KEY + " must parse to a positive byte count, was: " + declared); + } + + @Test + @DisplayName("the framework floor covers the largest declared cap in every committed descriptor") + void frameworkLimitCoversEveryCommittedDescriptor() throws Exception { + // Arrange + long floor = parseMemorySize(applicationProperties().getProperty(FRAMEWORK_LIMIT_KEY)); + List descriptors = committedGatewayDescriptors(); + + // Act + Assert — the glob must actually find the committed descriptors before the loop below + // can mean anything. + assertTrue(descriptors.size() >= COMMITTED_DESCRIPTOR_COUNT, + "expected at least " + COMMITTED_DESCRIPTOR_COUNT + + " committed sheriff-config*/gateway.yaml descriptors under " + DOCKER + + ", found " + descriptors.size() + ": " + descriptors); + + // The same strict inequality ConfigProducer's boot check enforces per booted instance — + // caught here, before a native image is ever built. + for (Path descriptor : descriptors) { + long declared = maxDeclaredBodyCap(loadYaml(descriptor)); + assertTrue(declared <= floor, + descriptor + " declares max_body_bytes " + declared + ", which exceeds the framework floor " + + floor + " (" + FRAMEWORK_LIMIT_KEY + + "). That instance would abort its boot on the fail-closed check —" + + " raise the floor or lower the declared cap."); + } + } + + @Test + @DisplayName("the IT container override exceeds LargeBodyIT's negative-case body size") + void containerOverrideExceedsTheNegativeCaseBody() throws Exception { + // Arrange + List environment = environment(composeServices(), "api-sheriff"); + + // Act + String override = environment.stream() + .filter(entry -> entry.startsWith(CONTAINER_OVERRIDE_KEY + "=")) + .map(entry -> entry.substring(CONTAINER_OVERRIDE_KEY.length() + 1)) + .findFirst() + .orElse(null); + + // Assert — this coupling is what keeps LargeBodyIT honest. Dropping or lowering the override + // would let the Quarkus root handler answer the oversize request with a bare 413 before the + // gateway pipeline runs, silently turning the IT into an assertion about the framework + // rather than about the gateway's RFC 9457 envelope. + assertNotNull(override, "the api-sheriff service must set " + CONTAINER_OVERRIDE_KEY + + " so the gateway — not the Quarkus root handler — rejects LargeBodyIT's oversize body"); + long overrideBytes = parseMemorySize(override); + assertTrue(overrideBytes > NEGATIVE_CASE_BODY_BYTES, + CONTAINER_OVERRIDE_KEY + " is " + overrideBytes + " but must be strictly greater than " + + NEGATIVE_CASE_BODY_BYTES + " (LargeBodyIT's negative-case body size), otherwise the" + + " framework pre-check shadows the gateway's own 413 envelope"); + } + + /** + * Every committed gateway descriptor under a {@code sheriff-config} directory, discovered by glob + * so a new instance directory is covered automatically. + * + * @return the descriptor paths, in directory-stream order + * @throws IOException when the docker directory cannot be listed + */ + private static List committedGatewayDescriptors() throws IOException { + List descriptors = new ArrayList<>(); + try (DirectoryStream directories = Files.newDirectoryStream(DOCKER, "sheriff-config*")) { + for (Path directory : directories) { + Path descriptor = directory.resolve("gateway.yaml"); + if (Files.isRegularFile(descriptor)) { + descriptors.add(descriptor); + } + } + } + return descriptors; + } + + /** + * The largest {@code max_body_bytes} declared anywhere in a descriptor. The whole document tree is + * walked rather than only the {@code anchors} block, so a cap declared at any nesting depth — + * including a future route-level one — is covered. + * + * @param node the parsed YAML node + * @return the maximum declared cap in bytes, or {@code 0} when none is declared + */ + private static long maxDeclaredBodyCap(Object node) { + long max = 0L; + if (node instanceof Map map) { + for (Map.Entry entry : map.entrySet()) { + if (DECLARED_CAP_KEY.equals(String.valueOf(entry.getKey())) + && entry.getValue() instanceof Number cap) { + max = Math.max(max, cap.longValue()); + } else { + max = Math.max(max, maxDeclaredBodyCap(entry.getValue())); + } + } + } else if (node instanceof Iterable items) { + for (Object item : items) { + max = Math.max(max, maxDeclaredBodyCap(item)); + } + } + return max; + } + + /** + * Parses a Quarkus {@code MemorySize} literal — a plain byte count, or a number with a binary + * {@code K} / {@code M} / {@code G} suffix (optionally {@code Ki} / {@code Mi} / {@code Gi}). + * + * @param value the declared literal + * @return the value in bytes + */ + private static long parseMemorySize(String value) { + String trimmed = value.trim().toUpperCase(Locale.ROOT); + long multiplier = 1L; + int end = trimmed.length(); + if (trimmed.endsWith("I")) { + end--; + } + if (end > 0) { + multiplier = switch (trimmed.charAt(end - 1)) { + case 'K' -> 1024L; + case 'M' -> 1024L * 1024L; + case 'G' -> 1024L * 1024L * 1024L; + default -> 1L; + }; + } + String digits = multiplier == 1L ? trimmed.substring(0, end) : trimmed.substring(0, end - 1); + return Long.parseLong(digits.trim()) * multiplier; + } + + private static Properties applicationProperties() throws IOException { + Properties properties = new Properties(); + try (Reader reader = Files.newBufferedReader(APPLICATION_PROPERTIES)) { + properties.load(reader); + } + return properties; + } + + @SuppressWarnings("unchecked") + private static Map composeServices() throws IOException { + Map doc = loadYaml(MODULE.resolve("docker-compose.yml")); + Object services = doc.get("services"); + assertInstanceOf(Map.class, services, "docker-compose.yml must declare services"); + return (Map) services; + } + + @SuppressWarnings("unchecked") + private static List environment(Map services, String service) { + Object node = services.get(service); + assertNotNull(node, "docker-compose.yml must declare the '" + service + "' service"); + Map serviceMap = (Map) node; + Object env = serviceMap.get("environment"); + assertInstanceOf(List.class, env, "the '" + service + "' service environment must be a list"); + List entries = ((List) env).stream().map(String::valueOf).toList(); + assertFalse(entries.isEmpty(), "the '" + service + "' service environment must not be empty"); + return entries; + } + + @SuppressWarnings("unchecked") + private static Map loadYaml(Path path) throws IOException { + try (InputStream in = Files.newInputStream(path)) { + return new Yaml().loadAs(in, Map.class); + } + } +} From a4276f410abb6a47e3b4c5f985c6b2615a09ddee Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 19:03:43 +0200 Subject: [PATCH 04/16] test(integration): add the containerised large-body regression IT --- integration-tests/docker-compose.yml | 16 ++ .../gateway/integration/LargeBodyIT.java | 262 ++++++++++++++++++ 2 files changed, 278 insertions(+) create mode 100644 integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java diff --git a/integration-tests/docker-compose.yml b/integration-tests/docker-compose.yml index 1ef935b8..754de2a3 100644 --- a/integration-tests/docker-compose.yml +++ b/integration-tests/docker-compose.yml @@ -106,6 +106,12 @@ services: # readiness probe and local debugging. go-httpbin: image: ghcr.io/mccutchen/go-httpbin:2.23.1 + # go-httpbin caps request bodies at 1 MiB by default, well below the upload anchor's declared + # 64 MiB max_body_bytes. Without this raise the upstream — not the gateway — would reject + # LargeBodyIT's 11 MiB positive body, turning a test about the framework body-size floor into a + # test about go-httpbin's own default. The value matches the upload anchor's cap exactly, so a + # body the gateway forwards is a body this upstream accepts. + command: ["-max-body-size", "67108864"] ports: - "18080:8080" networks: @@ -188,6 +194,16 @@ services: # override moves it to sheriff.tls.internal-https-port (8444); the front L4-relays a matched # passthrough SNI to the resolved backend and hands every other connection to localhost:8444. - QUARKUS_HTTP_SSL_PORT=8444 + # Raise the framework body-size floor ABOVE the upload anchor's declared 64 MiB + # max_body_bytes for this instance only. application.properties ships the floor at exactly + # 64M, where the Quarkus root handler's Content-Length pre-check answers an oversize request + # with a bare 413 (Connection: close, empty body) before the gateway pipeline runs — so the + # RFC 9457 problem+json envelope would never be rendered for the largest-capped route. Lifting + # the floor to 128M puts LargeBodyIT's 68 MiB negative-case body under the framework limit and + # over the gateway's own cap, which is what makes the gateway (not the framework) the rejecting + # component. BodyLimitActivationWiringTest asserts this override stays strictly above that body + # size; dropping or lowering it silently turns LargeBodyIT into a test of the framework. + - QUARKUS_HTTP_LIMITS_MAX_BODY_SIZE=128M # Certificate paths (runtime override) - QUARKUS_HTTP_SSL_CERTIFICATE_FILES=/app/certificates/localhost.crt - QUARKUS_HTTP_SSL_CERTIFICATE_KEY_FILES=/app/certificates/localhost.key diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java new file mode 100644 index 00000000..413876e8 --- /dev/null +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java @@ -0,0 +1,262 @@ +/* + * Copyright © 2026 CUI-OpenSource-Software (info@cuioss.de) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package de.cuioss.sheriff.gateway.integration; + +import static io.restassured.RestAssured.given; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.ByteBuffer; +import java.security.SecureRandom; +import java.time.Duration; +import java.util.Arrays; +import java.util.concurrent.Flow; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; +import javax.net.ssl.SSLContext; +import javax.net.ssl.TrustManager; + +import de.cuioss.sheriff.gateway.integration.MtlsHandshakeIT.TrustAllManager; + +import io.restassured.path.json.JsonPath; + +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Proves the body-size contract end-to-end across the real framework boundary: a body larger + * than the Vert.x default is accepted and forwarded, and a body larger than the route's declared + * {@code security_filter.max_body_bytes} is rejected by the gateway with its own RFC 9457 + * envelope rather than by the framework with a bare status line. + *

    + * Both cases run against the {@code /upload/small} route, which rides the {@code upload} anchor's + * declared cap of 64 MiB (67108864). Three descriptor facts have to line up for this suite to mean + * what it claims, and each is what a corresponding failure would expose: + *

      + *
    1. {@code quarkus.http.limits.max-body-size} in {@code application.properties} raises the + * framework floor above Vert.x' 10 MiB default. Revert it and the positive case's 11 MiB body + * is clipped by the framework before the gateway pipeline ever runs.
      2. + *
    2. The compose stack's {@code QUARKUS_HTTP_LIMITS_MAX_BODY_SIZE=128M} override on the + * {@code api-sheriff} service lifts that floor above the negative case's 68 MiB body. + * Drop or lower it and the Quarkus root handler answers the oversize request with a bare 413 + * before the gateway runs, silently turning the negative case into an assertion about the + * framework instead of about the gateway's envelope.
      3. + *
    3. go-httpbin's {@code -max-body-size 67108864} lifts the upstream's own 1 MiB default above the + * positive case's body. Drop it and the 11 MiB forward is rejected upstream, not at the edge.
      4. + *
    + * The fast, no-Docker sibling that asserts those descriptor facts directly is + * {@code BodyLimitActivationWiringTest}; this suite is the containerised proof of the behaviour they + * enable. + *

    + * Why the negative case is not driven with REST Assured. It must declare a + * {@code Content-Length} it never actually writes, and it must observe that no body bytes were + * produced. That needs {@code Expect: 100-continue} plus a body publisher whose produced bytes are + * counted — a shape REST Assured does not expose. The JDK {@link HttpClient} does: a publisher + * reporting a {@link HttpRequest.BodyPublisher#contentLength() contentLength} of 68 MiB makes the + * client emit that {@code Content-Length} header, and {@code expectContinue(true)} holds the body back + * until the server invites it. A gateway that rejects on the declared length never sends the invite, + * so the counter stays at {@code 0} — the observable proof the rejection happened before any + * payload crossed the wire, not after 68 MiB were uploaded and discarded. + *

    + * The negative case deliberately makes no assertion on the status code: the status + * is the framework's to choose once the gateway hands it a rejection, and pinning it here would + * couple the test to that incidental choice. What is under test is the envelope — the + * {@code application/problem+json} content type, the input-validation problem type and title, and the + * absence of the go-httpbin echo that proves the request never reached the upstream. + * + * @author API Sheriff Team + * @since 1.0 + */ +class LargeBodyIT extends BaseIntegrationTest { + + /** + * 11 MiB — above Vert.x' 10 MiB default and below the {@code upload} anchor's 64 MiB cap, so it is + * accepted only when the framework floor has actually been raised. + */ + private static final int POSITIVE_BODY_BYTES = 11534336; + + /** + * 68 MiB — above the {@code upload} anchor's 64 MiB cap and below the compose stack's 128 MiB + * framework floor, so the rejection is the gateway's and not the framework's. + */ + private static final long NEGATIVE_BODY_BYTES = 71303168L; + + private static final String UPLOAD_PATH = "/upload/small"; + private static final String PROBLEM_JSON = "application/problem+json"; + private static final String INPUT_VALIDATION_TYPE = "urn:api-sheriff:problem:input-validation"; + + private static HttpClient httpClient; + + @BeforeAll + static void setUpLargeBodyClient() throws Exception { + // Trust-all TLS for the stack's self-signed localhost certificate — the JDK client's + // equivalent of BaseIntegrationTest's RestAssured.useRelaxedHTTPSValidation(). Scoped to this + // black-box IT against a throwaway local certificate; never a production trust decision. + SSLContext sslContext = SSLContext.getInstance("TLS"); + sslContext.init(null, new TrustManager[]{new TrustAllManager()}, new SecureRandom()); + httpClient = HttpClient.newBuilder() + .sslContext(sslContext) + .connectTimeout(Duration.ofSeconds(10)) + .build(); + } + + @Test + @DisplayName("an 11 MiB body under the route cap is accepted and forwarded to the upstream") + void bodyUnderTheRouteCapIsForwarded() { + byte[] payload = new byte[POSITIVE_BODY_BYTES]; + Arrays.fill(payload, (byte) 'a'); + + var response = given() + .contentType("text/plain") + .body(payload) + .when() + .post(UPLOAD_PATH) + .then() + .statusCode(200) + .extract(); + + assertNotNull(response.path("method"), + "an accepted body must reach the go-httpbin upstream — its echo carries the method"); + } + + @Test + @DisplayName("a 68 MiB declared body over the route cap is rejected by the gateway before any payload") + void bodyOverTheRouteCapIsRejectedByTheGateway() throws Exception { + CountingBodyPublisher body = new CountingBodyPublisher(NEGATIVE_BODY_BYTES); + HttpRequest request = HttpRequest.newBuilder(URI.create(uploadUri())) + .header("Content-Type", "text/plain") + .timeout(Duration.ofSeconds(30)) + .expectContinue(true) + .POST(body) + .build(); + + HttpResponse response = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); + + String contentType = response.headers().firstValue("Content-Type").orElse(""); + JsonPath problem = JsonPath.from(response.body()); + assertTrue(contentType.contains(PROBLEM_JSON), + "the gateway must render its own RFC 9457 envelope, Content-Type was: " + contentType); + assertTrue(String.valueOf(problem.get("type")).contains(INPUT_VALIDATION_TYPE), + "an oversize body is an input-validation rejection, type was: " + problem.get("type")); + assertEquals("Input Validation", problem.get("title")); + assertNull(problem.get("method"), "a rejected request must not reach the go-httpbin upstream"); + assertEquals(0L, body.producedBytes(), + "the rejection must land on the declared Content-Length, before the 100-continue invite —" + + " a non-zero count means the gateway accepted the payload and discarded it afterwards"); + } + + private static String uploadUri() { + String testPort = System.getProperty("test.https.port", "10443"); + return "https://localhost:" + testPort + UPLOAD_PATH; + } + + /** + * A body publisher that declares {@code contentLength} bytes and counts how many it is + * actually asked to produce. Paired with {@code Expect: 100-continue}, the count is the observable + * proof of where the rejection landed: {@code 0} means the server refused on the declared length + * alone and never invited the payload. + */ + private static final class CountingBodyPublisher implements HttpRequest.BodyPublisher { + + private final long declaredLength; + private final AtomicLong produced = new AtomicLong(); + + CountingBodyPublisher(long declaredLength) { + this.declaredLength = declaredLength; + } + + @Override + public long contentLength() { + return declaredLength; + } + + long producedBytes() { + return produced.get(); + } + + @Override + public void subscribe(Flow.Subscriber subscriber) { + subscriber.onSubscribe(new ChunkedSubscription(subscriber, declaredLength, produced)); + } + } + + /** + * Emits zero-filled chunks on demand up to the declared length, tallying every byte handed out. + * The drain loop is guarded so a re-entrant {@code request(n)} from inside {@code onNext} does not + * recurse. + */ + private static final class ChunkedSubscription implements Flow.Subscription { + + private static final int CHUNK_BYTES = 64 * 1024; + + private final Flow.Subscriber subscriber; + private final AtomicLong produced; + private final AtomicLong demand = new AtomicLong(); + private final AtomicBoolean draining = new AtomicBoolean(); + private volatile boolean finished; + private volatile long remaining; + + ChunkedSubscription(Flow.Subscriber subscriber, long length, AtomicLong produced) { + this.subscriber = subscriber; + this.produced = produced; + this.remaining = length; + } + + @Override + public void request(long n) { + if (n <= 0) { + finished = true; + subscriber.onError(new IllegalArgumentException("non-positive subscription request: " + n)); + return; + } + demand.addAndGet(n); + drain(); + } + + @Override + public void cancel() { + finished = true; + } + + private void drain() { + if (!draining.compareAndSet(false, true)) { + return; + } + try { + while (!finished && demand.get() > 0 && remaining > 0) { + demand.decrementAndGet(); + int chunk = (int) Math.min(CHUNK_BYTES, remaining); + remaining -= chunk; + produced.addAndGet(chunk); + subscriber.onNext(ByteBuffer.allocate(chunk)); + } + if (!finished && remaining == 0) { + finished = true; + subscriber.onComplete(); + } + } finally { + draining.set(false); + } + } + } +} From 373e7589dcaebc82f5919a15cc3ebad601903411 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 19:07:41 +0200 Subject: [PATCH 05/16] docs(gateway): reconcile the body-cap reference and the 413 error contracts --- doc/architecture.adoc | 8 +++++--- doc/configuration.adoc | 34 ++++++++++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/doc/architecture.adoc b/doc/architecture.adoc index 666697c2..a6376ae4 100644 --- a/doc/architecture.adoc +++ b/doc/architecture.adoc @@ -349,7 +349,7 @@ Design (to be built as a low, framework-agnostic base package): category* (per CUI logging convention) and are metric counters only -- e.g. `REQUEST_FORWARDED`, `TOKEN_REFRESHED`, `CONFIG_LOADED`. * Failure event types carry a category -- e.g. `PATH_NOT_ALLOWED` / `SECURITY_FILTER_VIOLATION` - / `PARAMETER_LIMIT_EXCEEDED` (`INPUT_VALIDATION`), `TOKEN_MISSING` / `TOKEN_INVALID` + / `PARAMETER_LIMIT_EXCEEDED` / `CONTENT_TOO_LARGE` (`INPUT_VALIDATION`), `TOKEN_MISSING` / `TOKEN_INVALID` (`AUTHENTICATION`), `SCOPE_MISSING` / `CSRF_REJECTED` (`AUTHORIZATION`), `UPSTREAM_ERROR` / `UPSTREAM_TIMEOUT` (`UPSTREAM`). * Typed gateway exceptions carry their `EventType`, so the HTTP edge maps @@ -370,11 +370,12 @@ names the category but leaks no internal detail. The consolidated status mapping |=== | Status | Cause | EventCategory -| `400` | Security-filter violation (path/parameter/header pipeline, collection limits, body size) | `INPUT_VALIDATION` +| `400` | Security-filter violation (path/parameter/header pipeline, collection limits) | `INPUT_VALIDATION` | `401` | Missing/invalid bearer token; invalid or expired session on a non-navigation request. Unauthenticated *navigation* requests (those accepting `text/html`) on `session` routes are redirected into the login flow instead -- content negotiation, not configuration (see link:configuration.adoc#_auth[Configuration -- `auth`]). Gateway `401`s carry `WWW-Authenticate` (`Bearer` per RFC 6750 on bearer routes). | `AUTHENTICATION` | `403` | Valid credentials lacking a required scope; failed CSRF origin check | `AUTHORIZATION` | `404` | No route matched (deny-by-default routing); reserved-for-passthrough `Host` on a terminated connection; disabled endpoint (e.g. back-channel logout in cookie mode) | `INPUT_VALIDATION` | `405` | Request method not in the matched route's *effective* `allowed_methods` verb allowlist (global default, or anchor/endpoint replacement); the response carries an `Allow` header listing the permitted set | `INPUT_VALIDATION` +| `413` | Request body beyond a declared size limit -- the per-route `security_filter.max_body_bytes` cap on the proxy path (`CONTENT_TOO_LARGE`), or the edge's `reserved_body_max_bytes` ceiling on a gateway-terminated reserved BFF POST path (`RESERVED_BODY_TOO_LARGE`). Both reject on the declared `Content-Length` before any body is read, and both abort mid-transfer on a chunked or under-declared body once the cap is crossed | `INPUT_VALIDATION` | `429` | Rate limit exceeded -- *reserved*; the feature is out of scope | -- | `502` | Upstream connection failure / invalid upstream response | `UPSTREAM` | `503` | Circuit breaker open -- the upstream was not called (`Retry-After` hints at `reset_ms`) | `UPSTREAM` @@ -395,11 +396,12 @@ rejection it would render as an HTTP status for an HTTP route onto the canonical |=== | gRPC status | code | Gateway rejection (the HTTP status the same cause renders on an HTTP route) -| `INVALID_ARGUMENT` | 3 | `400` -- security-filter violation (path/parameter/header pipeline, collection limits, body size) +| `INVALID_ARGUMENT` | 3 | `400` -- security-filter violation (path/parameter/header pipeline, collection limits) | `UNAUTHENTICATED` | 16 | `401` -- missing/invalid bearer token; invalid or expired session on a non-navigation request | `PERMISSION_DENIED` | 7 | `403` -- valid credentials lacking a required scope; failed CSRF origin check | `NOT_FOUND` | 5 | `404` -- no route matched (deny-by-default routing); reserved-for-passthrough `Host`; disabled endpoint | `UNIMPLEMENTED` | 12 | `405` -- request method not in the matched route's effective `allowed_methods` allowlist +| `RESOURCE_EXHAUSTED` | 8 | `413` -- request body beyond the route's `security_filter.max_body_bytes` cap | `UNAVAILABLE` | 14 | `502` / `503` -- upstream connection failure / invalid upstream response, or circuit breaker open; *also* an HTTP/2 negotiation failure at dispatch (the forced-h2 upstream dial could not establish `h2`) | `DEADLINE_EXCEEDED` | 4 | `504` -- upstream timeout (`connect_timeout_ms` / `read_timeout_ms`) |=== diff --git a/doc/configuration.adoc b/doc/configuration.adoc index dceda153..b3d5ca16 100644 --- a/doc/configuration.adoc +++ b/doc/configuration.adoc @@ -941,10 +941,12 @@ The explicit *not-a-WAF* boundary: allowlists and limits, no attack-signature ma | Baseline preset; the limits below override it. | `max_body_bytes` -| *(gateway-enforced)* -| DoS guard. `cui-http` removed its BODY validation pipeline ("body content validation should - be handled at application layer"), so the gateway enforces this cap itself while reading the - request body; `SecurityConfiguration.maxBodySize` is not wired. +| *(gateway-enforced, under a framework floor)* +| DoS guard, and the *effective* cap for the route: a request whose body exceeds it is rejected + *413* with an `application/problem+json` envelope. `cui-http` removed its BODY validation + pipeline ("body content validation should be handled at application layer"), so the gateway + enforces the cap itself while reading the request body; `SecurityConfiguration.maxBodySize` is + not wired. The cap only takes effect *below the framework floor* described immediately below. | `max_header_count` / `max_query_params` | `maxHeaderCount` / `maxParameterCount` (`RequestCollectionValidator`) @@ -968,6 +970,30 @@ The explicit *not-a-WAF* boundary: allowlists and limits, no attack-signature ma | Optional `Content-Type` allowlist for request bodies. |=== +*The framework body-size floor.* `max_body_bytes` is a cap the gateway applies *on top of* an +underlying HTTP-framework limit, `quarkus.http.limits.max-body-size`. The framework limit is the +floor every declared cap sits under: a route cannot accept more than the floor allows, whatever it +declares. API Sheriff ships the floor at *`64M`* (67108864 bytes) in `application.properties`, +matching the largest `max_body_bytes` in the shipped topology. Without that declaration Vert.x +applies its own 10 MiB default and silently clips every larger declared cap, so the route's own +limit never fires. + +The relationship is *boot-enforced*: a deployment that declares a `max_body_bytes` larger than the +floor fails fast at startup rather than running with a cap that cannot be honoured. Raising a +declared cap therefore means raising `quarkus.http.limits.max-body-size` in the same change. + +*Shadowing at the boundary.* When the floor is set *equal to* the largest declared cap, the +framework's own `Content-Length` pre-check answers a breach of that largest cap with a bare `413` +(`Connection: close`, empty body) *before the gateway's request handling runs* — so the RFC 9457 +`application/problem+json` envelope is rendered only for routes capped strictly *below* the floor. +An operator who wants the gateway's own envelope on the largest-capped route sets the floor *above* +that cap rather than equal to it. The integration stack does exactly this: it overrides the floor to +`128M` so the gateway, not the framework, renders the rejection. + +*Chunked requests.* A chunked request carries no `Content-Length`, so it passes the framework's +pre-check untouched and is capped by the gateway's streaming byte counter instead — it always +receives the gateway's `application/problem+json` envelope, regardless of where the floor sits. + *Whitelisting.* `cui-http` has no path allow-list knob, so URL/parameter whitelisting is enforced at the gateway layer: `allowed_paths` (a request whose validated, normalized path matches no entry is rejected -- the manifest's "WAF via path white-listing"), plus `forward.query_allow` / From d85011aff74cf09f4395ab92c390944af2f04766 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 19:12:01 +0200 Subject: [PATCH 06/16] chore(benchmarks): re-enable the uploadLarge goal and de-quarantine its prose --- .github/workflows/benchmark.yml | 35 ++++++++++------------- benchmarks/README.adoc | 49 +++++++++++++++++---------------- benchmarks/pom.xml | 39 ++++++++++++-------------- 3 files changed, 57 insertions(+), 66 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index f07b69ed..74ecbcb4 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -27,23 +27,19 @@ jobs: # The -Pbenchmark profile DECLARES TWELVE k6 goals: the eight cross-gateway matrix aspects, the # two retained non-matrix health benchmarks (healthLiveCheck, gatewayHealth), and the two # API-Sheriff-only passthrough-relay executions (mapped relay throughput and the empty-mode - # no-regression run). ELEVEN of them execute: only uploadLarge still carries a bound to a - # property that defaults to true in benchmarks/pom.xml, because it remains blocked by an open - # gateway defect (see the "Deliberately skipped" table in the coverage step below). Maven - # therefore never runs it, so it consumes no wall time and cannot abort the suite. It stays - # ordered last in the profile as defense-in-depth, so that flipping its skip property to false - # cannot mask a goal that would otherwise produce a result. + # no-regression run). All TWELVE of them execute: uploadLarge and websocketEcho still carry a + # bound to their own property, but both properties are false in benchmarks/pom.xml, so + # both goals run. The two stay ordered last in the profile as defense-in-depth, so a fail-fast + # abort from either cannot discard a goal that would otherwise produce a result. # # Budget, from per-goal timings measured locally 2026-07-28 rather than estimated: - # * goal execution ~13 min -- sized on the ELEVEN goals that actually execute, which is the - # real basis: a skipped goal is not run at all, so it costs nothing but the - # Maven bookkeeping for a skipped execution. - # 11 goals x (60s k6 window + ~10s compose/k6 start and summary write). - # The websocketEcho term is the ~70s a re-enabled request-rate goal costs, - # now that its blocking permit leak is fixed. Re-enabling upload-50MB would - # cost more than that: it is transfer-bound at reduced concurrency, so it - # does not finish in the same wall time as a request-rate run. Re-derive - # this term when a skip property is flipped to false. + # * goal execution ~16 min -- sized on all TWELVE goals, which now all execute. + # 11 request-rate goals x (60s k6 window + ~10s compose/k6 start and + # summary write) = ~13 min, plus uploadLarge. uploadLarge is NOT a + # request-rate goal: it is transfer-bound at reduced concurrency + # (k6.vus.upload.large = 5), so it does not finish in the same wall time as + # a request-rate run — it is budgeted at ~3 min on its own. Re-derive this + # term when a skip property is flipped, or when a goal is added or removed. # * stack startup ~100 s -- the lane boots FIVE native gateway instances (api-sheriff, # api-sheriff-mtls, api-sheriff-cookie, api-sheriff-cookie-2 and # api-sheriff-ws-admission, the last added for the WebSocket relay-permit @@ -119,10 +115,8 @@ jobs: # twelve goals: the eight cross-gateway matrix aspects (unauth, bearer, http2, # graphql, upload-1MB, upload-50MB, ws, grpc), the two retained non-matrix health # benchmarks (healthLiveCheck, gatewayHealth), and the two API-Sheriff-only - # passthrough-relay executions (mapped and empty). Eleven of the twelve execute and are - # expected to produce a CI result; only upload-50MB (uploadLarge) is still skipped by - # property against an open gateway defect, so Maven does not run it and it cannot abort - # the suite. The on-demand APISIX comparison lane is deliberately NOT run + # passthrough-relay executions (mapped and empty). All twelve execute and are + # expected to produce a CI result. The on-demand APISIX comparison lane is deliberately NOT run # here. Maven still fails fast on the goals that do run, so the coverage step below # remains what proves which goals actually produced a result. echo "Running k6 integration benchmarks with native Quarkus..." @@ -151,7 +145,7 @@ jobs: # Goals expected to write a summary. A goal settled as deliberately skipped is NOT listed # here — it belongs in the skip table below, which is what keeps the two states distinct. - expected="healthLiveCheck gatewayHealth proxiedStatic passthroughRelay passthroughRelayEmpty bearerProxied http2 graphql uploadSmall grpcUnary websocketEcho" + expected="healthLiveCheck gatewayHealth proxiedStatic passthroughRelay passthroughRelayEmpty bearerProxied http2 graphql uploadSmall uploadLarge grpcUnary websocketEcho" missing=0 missing_names="" @@ -178,7 +172,6 @@ jobs: echo echo "| Benchmark | Skipped by | Reason |" echo "| --- | --- | --- |" - echo "| \`uploadLarge\` | \`skip.benchmark.upload.large\` (defaults to \`true\`) | The gateway rejects the 50MB body with 413: the Quarkus HTTP body limit is not derived from the upload anchor's 64 MiB \`max_body_bytes\`, so the declared cap is unreachable. Needs a gateway-side fix; re-enable with \`-Dskip.benchmark.upload.large=false\`. |" echo "| \`sessionMediated\` | not wired | Wired to no Maven goal on purpose — BFF session mediation belongs to PLAN-07A. |" } >> "$GITHUB_STEP_SUMMARY" diff --git a/benchmarks/README.adoc b/benchmarks/README.adoc index 6043c5b2..66755f24 100644 --- a/benchmarks/README.adoc +++ b/benchmarks/README.adoc @@ -173,11 +173,14 @@ enforces this table, so a goal that silently stops running fails the job: |First execution in CI under PLAN-25. Their trend series start empty here. |`uploadLarge` -|*skipped* -|Skipped by `skip.benchmark.upload.large`, which defaults to `true`. The gateway rejects the 50MB - body with `413`: the `upload` anchor declares `max_body_bytes: 67108864`, but the Quarkus HTTP body - limit is not derived from it, so the declared 64 MiB cap is unreachable and the body is refused - before the security filter runs. Blocked on a gateway-side fix. +|runs +|Skipped until the framework body-size floor was declared: the `upload` anchor declares + `max_body_bytes: 67108864`, but `quarkus.http.limits.max-body-size` was left at the Quarkus 10 MiB + default, so the declared 64 MiB cap was unreachable and the 50MB body was refused with `413` before + the security filter ran. `application.properties` now declares that floor at `64M`, matching the + largest declared cap, so the goal measures upload throughput instead of the rejection path. Its + trend series starts empty. `skip.benchmark.upload.large` is retained (now `false`) so the goal stays + individually suppressible. |`websocketEcho` |runs @@ -193,20 +196,19 @@ enforces this table, so a goal that silently stops running fails the job: |Backs no Maven goal on purpose — see below. |=== -*Skipped means not executed.* A skipped goal carries a `` element in `benchmarks/pom.xml` bound -to its own property, declared there with the id of the finding that blocks it and defaulting to -`true` — so Maven does not run the goal, it consumes no wall time, and it cannot abort the suite. -The two states are kept distinct rather than collapsed: the CI coverage step gates on the eleven -goals that execute and names each skipped goal, its skip property and its reason in the job summary. -When Maven fails, that step also states in words whether the failure truncated the suite. Silence is -never the signal. +*Skipped means not executed.* A goal suppressed by a `` element in `benchmarks/pom.xml` — bound +to its own property — is not run at all: it consumes no wall time and cannot abort the suite. No k6 +goal is suppressed today; every declared goal executes. The two states are kept distinct rather than +collapsed: the CI coverage step gates on the twelve goals that execute and would name any suppressed +goal, its skip property and its reason in the job summary. When Maven fails, that step also states in +words whether the failure truncated the suite. Silence is never the signal. -Re-enable a goal once its gateway defect is fixed by flipping its property, which is also how you -reproduce the defect through the harness: +The per-goal properties are retained even when `false`, so a single goal can be suppressed for a +local run without editing its execution: [source,bash] ---- -./mvnw clean verify -pl benchmarks -Pbenchmark -Dskip.benchmark.upload.large=false +./mvnw clean verify -pl benchmarks -Pbenchmark -Dskip.benchmark.upload.large=true ---- === Deliberately unwired scripts @@ -503,10 +505,10 @@ know: . *Post-processing*: `WrkBenchmarkConverter` / `WrkResultPostProcessor` are replaced by `K6BenchmarkConverter` / `K6ResultPostProcessor`. The downstream report pipeline (badges, 10-run history, trends, GitHub Pages) consumes an identically-shaped model and is unchanged. -. *Coverage*: the lane grew from three wired executions to twelve *declared* goals, eleven of which - execute and are expected to produce a CI result — only `uploadLarge` is declared but skipped by - property against an open gateway defect, so Maven does not run it at all. `websocketEcho` was - skipped alongside it until its blocking edge admission-permit leak was fixed. `bearer` +. *Coverage*: the lane grew from three wired executions to twelve *declared* goals, all of which + execute and are expected to produce a CI result. Two of them reached that state late: `websocketEcho` + once its blocking edge admission-permit leak was fixed, and `uploadLarge` once the framework + body-size floor was declared under the `upload` anchor's `max_body_bytes`. `bearer` is genuinely new CI coverage — the wrk-era bearer runner existed on disk but was registered in no Maven execution. Being *wired* is not the same as *executing*, and *executing* is not the same as *producing a result*: until PLAN-25 the job died at the fourth goal and goals 5-12 had never run @@ -525,11 +527,10 @@ incomparable, the k6 series starts fresh rather than continuing the wrk series: (see the `latency_ms.stdev` limit above). * Every benchmark that writes a summary document for the first time begins with *no history* and needs ten runs before its trend series is meaningful. Under PLAN-25 that is most of the lane: - eight of the twelve declared goals had never executed in CI, and seven of those eight are expected - to produce a result and so start their series empty — six at the PLAN-25 commit, and - `websocketEcho` once its blocking permit leak was fixed. `uploadLarge` starts *no* series at all — - only a goal that actually writes a summary document opens one, and it is skipped by property - rather than executed. The workflow's end-of-job coverage step + eight of the twelve declared goals had never executed in CI, and all eight are expected + to produce a result and so start their series empty — six at the PLAN-25 commit, + `websocketEcho` once its blocking permit leak was fixed, and `uploadLarge` once the framework + body-size floor was declared. The workflow's end-of-job coverage step (`.github/workflows/benchmark.yml`) is the record of which goals produced a result in a given run. A flat or short trend line on a name that *did* run is an absence of history, not a regression. diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml index 5ed9b10b..71771fb2 100644 --- a/benchmarks/pom.xml +++ b/benchmarks/pom.xml @@ -28,18 +28,15 @@ true - - - - true + + false @@ -578,7 +575,7 @@ exec - + ${skip.benchmark.upload.large} docker From f9112faa363223e49c856980cecf4f66309ef799 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 19:24:17 +0200 Subject: [PATCH 07/16] style(build): apply quality-gate formatter normalization to imports and continuations --- .../de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java | 6 +++--- .../sheriff/gateway/integration/BffCookieSessionIT.java | 5 +++-- .../gateway/integration/BffCookieStatelessnessIT.java | 3 ++- .../sheriff/gateway/integration/BffKeycloakLoginFlow.java | 7 ++++--- .../sheriff/gateway/integration/BffSessionMediationIT.java | 4 ++-- 5 files changed, 14 insertions(+), 11 deletions(-) diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java index 5b274bb5..e3190379 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java @@ -249,9 +249,9 @@ private List frameworkBodyLimitViolations(GatewayConfig gateway, Li */ private static long maxDeclaredBodyBytes(GatewayConfig gateway, List enabled) { return Stream.concat( - gateway.anchors().values().stream().map(AnchorConfig::securityFilter), - enabled.stream().flatMap(endpoint -> endpoint.routes().stream()) - .map(RouteConfig::securityFilter)) + gateway.anchors().values().stream().map(AnchorConfig::securityFilter), + enabled.stream().flatMap(endpoint -> endpoint.routes().stream()) + .map(RouteConfig::securityFilter)) .flatMap(Optional::stream) .map(SecurityFilterConfig::maxBodyBytes) .flatMap(Optional::stream) diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java index 99903372..c4c71c51 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java @@ -21,8 +21,6 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; -import io.restassured.http.Cookie; -import io.restassured.response.Response; import java.util.Base64; import java.util.HashMap; import java.util.Map; @@ -31,6 +29,9 @@ import de.cuioss.sheriff.gateway.integration.BffKeycloakLoginFlow.Session; +import io.restassured.http.Cookie; +import io.restassured.response.Response; + import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.java index baa7c046..fc69d403 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.java @@ -20,13 +20,14 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; -import io.restassured.response.Response; import java.util.Base64; import java.util.HashMap; import java.util.Map; import de.cuioss.sheriff.gateway.integration.BffKeycloakLoginFlow.Session; +import io.restassured.response.Response; + import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java index 52d61c8d..14d09f97 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java @@ -17,14 +17,15 @@ import static io.restassured.RestAssured.given; -import io.restassured.http.Cookies; -import io.restassured.response.Response; -import io.restassured.specification.RequestSpecification; import java.util.HashMap; import java.util.Map; import java.util.regex.Matcher; import java.util.regex.Pattern; +import io.restassured.http.Cookies; +import io.restassured.response.Response; +import io.restassured.specification.RequestSpecification; + /** * Drives a scripted, browser-less OIDC authorization-code flow against the compose Keycloak * {@code integration} realm and the server-mode BFF gateway, following the {@code 302} chain with a diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffSessionMediationIT.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffSessionMediationIT.java index 10263014..e760cb59 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffSessionMediationIT.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffSessionMediationIT.java @@ -21,10 +21,10 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; -import io.restassured.response.Response; - import de.cuioss.sheriff.gateway.integration.BffKeycloakLoginFlow.Session; +import io.restassured.response.Response; + import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; From 70cb70a4fe48378521fd94303d6d897a8ba7436a Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 20:36:00 +0200 Subject: [PATCH 08/16] fix(tests): use typed getString in the LargeBodyIT problem assertions JsonPath.get is generic ( T get(String)). Inside String.valueOf(...) javac resolves against the most specific applicable overload, String.valueOf(char[]), inferring T = char[]; the JSON value is a String, so the negative case failed at runtime with "class java.lang.String cannot be cast to class [C". The adjacent assertions escaped it only because they infer Object. Switch the type and title reads to JsonPath.getString and hoist the value into a local, so the assertion message and the assertion share one already-typed read. Verified against the containerised stack: LargeBodyIT runs 2/2 green. Co-Authored-By: Claude --- .../sheriff/gateway/integration/LargeBodyIT.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java index 413876e8..8bbe9cd8 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java @@ -156,9 +156,13 @@ void bodyOverTheRouteCapIsRejectedByTheGateway() throws Exception { JsonPath problem = JsonPath.from(response.body()); assertTrue(contentType.contains(PROBLEM_JSON), "the gateway must render its own RFC 9457 envelope, Content-Type was: " + contentType); - assertTrue(String.valueOf(problem.get("type")).contains(INPUT_VALIDATION_TYPE), - "an oversize body is an input-validation rejection, type was: " + problem.get("type")); - assertEquals("Input Validation", problem.get("title")); + // Use the typed getString accessor rather than the generic get(): inside String.valueOf(...) + // javac resolves JsonPath's T get(String) against the most specific overload, + // String.valueOf(char[]), inferring T = char[] and failing with a ClassCastException at runtime. + String problemType = problem.getString("type"); + assertTrue(String.valueOf(problemType).contains(INPUT_VALIDATION_TYPE), + "an oversize body is an input-validation rejection, type was: " + problemType); + assertEquals("Input Validation", problem.getString("title")); assertNull(problem.get("method"), "a rejected request must not reach the go-httpbin upstream"); assertEquals(0L, body.producedBytes(), "the rejection must land on the declared Content-Length, before the 100-continue invite —" From 0f1e865cee2b48f66f1e128711f6bae2837c4c3f Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 21:33:42 +0200 Subject: [PATCH 09/16] docs(config): make the max_body_bytes RFC 9457 envelope promise conditional The security_filter table row promised an application/problem+json envelope unconditionally for every over-cap request, contradicting the "Shadowing at the boundary" paragraph in the same section: when the framework floor equals the largest declared cap, the framework's Content-Length pre-check answers with a bare 413 before gateway request handling runs. Qualify the envelope guarantee on the request reaching gateway enforcement and cross-reference the shadowing paragraph. The 413 behaviour and the enforcement order are unchanged. Addresses PR #131 review finding f53145. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7 --- doc/configuration.adoc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/configuration.adoc b/doc/configuration.adoc index b3d5ca16..745b7819 100644 --- a/doc/configuration.adoc +++ b/doc/configuration.adoc @@ -943,7 +943,9 @@ The explicit *not-a-WAF* boundary: allowlists and limits, no attack-signature ma | `max_body_bytes` | *(gateway-enforced, under a framework floor)* | DoS guard, and the *effective* cap for the route: a request whose body exceeds it is rejected - *413* with an `application/problem+json` envelope. `cui-http` removed its BODY validation + *413*, carrying an `application/problem+json` envelope whenever the request reaches gateway + enforcement (see "Shadowing at the boundary" below -- a `Content-Length` breach of the framework + floor is answered by the framework with a bare `413` instead). `cui-http` removed its BODY validation pipeline ("body content validation should be handled at application layer"), so the gateway enforces the cap itself while reading the request body; `SecurityConfiguration.maxBodySize` is not wired. The cap only takes effect *below the framework floor* described immediately below. From 5209ab5983785249a9eda28f1765ce0d3e17a035 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 21:36:25 +0200 Subject: [PATCH 10/16] test(events): close the EventType coverage gaps and guard against future drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The @CsvSource error-contract table was a hand-maintained mirror of the EventType constants carrying an HTTP mapping, and it had already drifted: seventeen constants satisfy hasHttpMapping() while the table carried sixteen rows, leaving WEBSOCKET_ORIGIN_REJECTED (AUTHORIZATION, 403) asserted by nothing. WEBSOCKET_IDLE_TIMEOUT was likewise unclaimed by any list, so its 1001 Going Away close code was unasserted. Close both gaps and make omission impossible: * Add the WEBSOCKET_ORIGIN_REJECTED row, and give WEBSOCKET_IDLE_TIMEOUT its own assertion rather than folding it into the success list — it carries a null category and httpStatus 0 but a non-zero wsCloseCode. * Move the four contract lists into shared constants driving @MethodSource, so each list has exactly one declaration. Expected statuses and categories stay stated as literals, never re-read from the enum under test. * Add three drift guards: the error-contract table's key set must equal the hasHttpMapping() set, the WebSocket-close list must equal the non-zero wsCloseCode set, and the four lists must partition the enum with no constant unclaimed or double-claimed. Addresses PR #131 review finding b21e8d. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7 --- .../sheriff/gateway/events/EventTypeTest.java | 176 +++++++++++++++--- 1 file changed, 152 insertions(+), 24 deletions(-) diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java index 3b63c087..2f0bd08c 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java @@ -20,31 +20,114 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import java.util.Collection; +import java.util.EnumSet; +import java.util.List; +import java.util.Set; +import java.util.stream.Stream; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.CsvSource; -import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +/** + * Pins {@link EventType}'s category, HTTP-status and WebSocket-close mappings. + *

    + * Every constant is claimed by exactly one of four explicitly enumerated contract lists — success / + * informational, boot-only configuration, WebSocket-close, and the request-time error contract. The + * expected statuses and categories are stated here as literals rather than re-read from the enum + * under test, so the lists carry independent information; the drift guards in + * {@link ContractCoverage} then assert that the lists remain exhaustive, which is what a + * hand-maintained mirror of an authoritative source cannot guarantee on its own. + * + * @author API Sheriff Team + * @since 1.0 + */ @DisplayName("EventType — category and HTTP-status mapping") class EventTypeTest { + /** Success / informational events: no category, no HTTP mapping, no WebSocket-close mapping. */ + private static final Set SUCCESS_EVENTS = EnumSet.of( + EventType.REQUEST_FORWARDED, + EventType.TOKEN_REFRESHED, + EventType.CONFIG_LOADED, + EventType.SESSION_CREATED, + EventType.SESSION_DESTROYED, + EventType.SESSION_REFRESH_FAILED, + EventType.BACKCHANNEL_LOGOUT); + + /** Boot-only configuration failures: a category, but never an HTTP response. */ + private static final Set CONFIGURATION_EVENTS = EnumSet.of( + EventType.CONFIG_INVALID, + EventType.AUTH_WEAKENED); + + /** + * Events that terminate an established WebSocket relay: no HTTP mapping (they occur + * after the {@code 101} upgrade) but a non-zero close code. + */ + private static final Set WEBSOCKET_CLOSE_EVENTS = EnumSet.of( + EventType.WEBSOCKET_IDLE_TIMEOUT); + + /** + * The request-time error contract: one row per event the edge renders as an HTTP status, with the + * status and category the contract in {@code architecture.adoc} promises. + */ + private static final List ERROR_CONTRACT = List.of( + arguments(EventType.SECURITY_FILTER_VIOLATION, 400, EventCategory.INPUT_VALIDATION), + arguments(EventType.PATH_NOT_ALLOWED, 400, EventCategory.INPUT_VALIDATION), + arguments(EventType.PARAMETER_LIMIT_EXCEEDED, 400, EventCategory.INPUT_VALIDATION), + arguments(EventType.NO_ROUTE_MATCHED, 404, EventCategory.INPUT_VALIDATION), + arguments(EventType.PASSTHROUGH_HOST_SMUGGLED, 404, EventCategory.INPUT_VALIDATION), + arguments(EventType.METHOD_NOT_ALLOWED, 405, EventCategory.INPUT_VALIDATION), + arguments(EventType.RESERVED_BODY_TOO_LARGE, 413, EventCategory.INPUT_VALIDATION), + arguments(EventType.CONTENT_TOO_LARGE, 413, EventCategory.INPUT_VALIDATION), + arguments(EventType.TOKEN_MISSING, 401, EventCategory.AUTHENTICATION), + arguments(EventType.TOKEN_INVALID, 401, EventCategory.AUTHENTICATION), + arguments(EventType.LOGOUT_TOKEN_INVALID, 400, EventCategory.AUTHENTICATION), + arguments(EventType.SCOPE_MISSING, 403, EventCategory.AUTHORIZATION), + arguments(EventType.CSRF_REJECTED, 403, EventCategory.AUTHORIZATION), + arguments(EventType.WEBSOCKET_ORIGIN_REJECTED, 403, EventCategory.AUTHORIZATION), + arguments(EventType.UPSTREAM_ERROR, 502, EventCategory.UPSTREAM), + arguments(EventType.UPSTREAM_CIRCUIT_OPEN, 503, EventCategory.UPSTREAM), + arguments(EventType.UPSTREAM_TIMEOUT, 504, EventCategory.UPSTREAM)); + + static Stream successEvents() { + return SUCCESS_EVENTS.stream(); + } + + static Stream configurationEvents() { + return CONFIGURATION_EVENTS.stream(); + } + + static Stream errorContract() { + return ERROR_CONTRACT.stream(); + } + + private static Set errorContractKeys() { + return ERROR_CONTRACT.stream() + .map(row -> (EventType) row.get()[0]) + .collect(() -> EnumSet.noneOf(EventType.class), Set::add, Set::addAll); + } + @Nested @DisplayName("Success / informational events") class SuccessEvents { @ParameterizedTest - @EnumSource(names = {"REQUEST_FORWARDED", "TOKEN_REFRESHED", "CONFIG_LOADED", - "SESSION_CREATED", "SESSION_DESTROYED", "SESSION_REFRESH_FAILED", "BACKCHANNEL_LOGOUT"}) + @MethodSource("de.cuioss.sheriff.gateway.events.EventTypeTest#successEvents") @DisplayName("Should carry no category and no HTTP mapping") void shouldCarryNoCategoryAndNoHttpMapping(EventType eventType) { assertAll("success event " + eventType, () -> assertNull(eventType.category(), "Success events carry a null category"), () -> assertFalse(eventType.isFailure(), "Success events are not failures"), () -> assertEquals(0, eventType.httpStatus(), "Success events have no HTTP status"), - () -> assertFalse(eventType.hasHttpMapping(), "Success events have no HTTP mapping")); + () -> assertFalse(eventType.hasHttpMapping(), "Success events have no HTTP mapping"), + () -> assertEquals(0, eventType.wsCloseCode(), "Success events have no WebSocket-close mapping")); } } @@ -53,7 +136,7 @@ void shouldCarryNoCategoryAndNoHttpMapping(EventType eventType) { class ConfigurationEvents { @ParameterizedTest - @EnumSource(names = {"CONFIG_INVALID", "AUTH_WEAKENED"}) + @MethodSource("de.cuioss.sheriff.gateway.events.EventTypeTest#configurationEvents") @DisplayName("Should be failures in the CONFIGURATION category with no HTTP mapping") void shouldBeConfigurationFailuresWithoutHttpMapping(EventType eventType) { assertAll("configuration event " + eventType, @@ -70,24 +153,7 @@ void shouldBeConfigurationFailuresWithoutHttpMapping(EventType eventType) { class FailureEvents { @ParameterizedTest(name = "{0} -> {1} ({2})") - @CsvSource({ - "SECURITY_FILTER_VIOLATION, 400, INPUT_VALIDATION", - "PATH_NOT_ALLOWED, 400, INPUT_VALIDATION", - "PARAMETER_LIMIT_EXCEEDED, 400, INPUT_VALIDATION", - "NO_ROUTE_MATCHED, 404, INPUT_VALIDATION", - "PASSTHROUGH_HOST_SMUGGLED, 404, INPUT_VALIDATION", - "METHOD_NOT_ALLOWED, 405, INPUT_VALIDATION", - "RESERVED_BODY_TOO_LARGE, 413, INPUT_VALIDATION", - "CONTENT_TOO_LARGE, 413, INPUT_VALIDATION", - "TOKEN_MISSING, 401, AUTHENTICATION", - "TOKEN_INVALID, 401, AUTHENTICATION", - "SCOPE_MISSING, 403, AUTHORIZATION", - "CSRF_REJECTED, 403, AUTHORIZATION", - "LOGOUT_TOKEN_INVALID, 400, AUTHENTICATION", - "UPSTREAM_ERROR, 502, UPSTREAM", - "UPSTREAM_CIRCUIT_OPEN, 503, UPSTREAM", - "UPSTREAM_TIMEOUT, 504, UPSTREAM" - }) + @MethodSource("de.cuioss.sheriff.gateway.events.EventTypeTest#errorContract") @DisplayName("Should map each error-contract row to its status and category") void shouldMapEachErrorContractRow(EventType eventType, int expectedStatus, EventCategory expectedCategory) { assertAll("failure event " + eventType, @@ -98,6 +164,68 @@ void shouldMapEachErrorContractRow(EventType eventType, int expectedStatus, Even } } + @Nested + @DisplayName("Established-relay WebSocket-close events") + class WebSocketCloseEvents { + + @Test + @DisplayName("Should close the relay with 1001 Going Away and carry no HTTP mapping") + void shouldCloseIdleRelayWithGoingAway() { + EventType eventType = EventType.WEBSOCKET_IDLE_TIMEOUT; + assertAll("websocket-close event " + eventType, + () -> assertNull(eventType.category(), + "A reclaimed relay is not a request-time rejection, so it carries no category"), + () -> assertFalse(eventType.isFailure(), "WebSocket-close events are not failures"), + () -> assertEquals(0, eventType.httpStatus(), + "The close happens after the 101 upgrade, so there is no HTTP status to render"), + () -> assertFalse(eventType.hasHttpMapping(), "WebSocket-close events have no HTTP mapping"), + () -> assertEquals(1001, eventType.wsCloseCode(), "An idle relay is closed with 1001 Going Away")); + } + } + + @Nested + @DisplayName("Contract coverage (drift guards)") + class ContractCoverage { + + @Test + @DisplayName("Should cover exactly the HTTP-mapped constants in the error-contract table") + void shouldCoverExactlyTheHttpMappedConstants() { + Set mapped = EnumSet.allOf(EventType.class).stream() + .filter(EventType::hasHttpMapping) + .collect(() -> EnumSet.noneOf(EventType.class), Set::add, Set::addAll); + + assertEquals(mapped, errorContractKeys(), + "Every EventType with hasHttpMapping() must have an error-contract row, and no row may" + + " name an unmapped constant — add the row in the same change as the constant"); + } + + @Test + @DisplayName("Should cover exactly the constants carrying a WebSocket close code") + void shouldCoverExactlyTheWebSocketCloseConstants() { + Set closing = EnumSet.allOf(EventType.class).stream() + .filter(eventType -> eventType.wsCloseCode() > 0) + .collect(() -> EnumSet.noneOf(EventType.class), Set::add, Set::addAll); + + assertEquals(WEBSOCKET_CLOSE_EVENTS, closing, + "Every EventType with a non-zero wsCloseCode must be asserted as a WebSocket-close event"); + } + + @Test + @DisplayName("Should claim every EventType constant in exactly one contract list") + void shouldClaimEveryConstantExactlyOnce() { + List claimed = Stream + .of(SUCCESS_EVENTS, CONFIGURATION_EVENTS, WEBSOCKET_CLOSE_EVENTS, errorContractKeys()) + .flatMap(Collection::stream) + .toList(); + + assertAll("contract-list partition", + () -> assertEquals(EnumSet.allOf(EventType.class), EnumSet.copyOf(claimed), + "Every EventType constant must be asserted by one of the contract lists"), + () -> assertEquals(claimed.size(), EnumSet.copyOf(claimed).size(), + "No EventType constant may appear in more than one contract list")); + } + } + @Test @DisplayName("Should derive the RFC 9457 problem type from the category slug") void shouldDeriveProblemTypeFromCategorySlug() { From e3773a6b3d57fade03bdda66857223558e80f891 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Wed, 29 Jul 2026 21:37:01 +0200 Subject: [PATCH 11/16] fix(ci): correct the stale executing-goal count above the coverage step Re-enabling uploadLarge took the executing set from eleven to twelve. Two of the three count-prose sites in benchmark.yml were updated in that change; the comment block above the "Summarise benchmark coverage" step was missed and still read "3 of the 11 executing goals" / "all 11" while the expected list below it holds twelve names. Comment-only change; the step body, the expected list and the skip table are untouched. Addresses finding b4790e, found during ground-truth verification of PR #131 review finding f0867e. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7 --- .github/workflows/benchmark.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 74ecbcb4..54840ef5 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -128,8 +128,8 @@ jobs: ls -la benchmarks/target/benchmark-results/ # Maven fails fast, so a goal that errors aborts every goal after it and a goal that is never - # reached leaves no trace in the job output — a suite that ran 3 of the 11 executing goals - # renders exactly like one that ran all 11. This step diffs the goals that MUST produce a + # reached leaves no trace in the job output — a suite that ran 3 of the 12 executing goals + # renders exactly like one that ran all 12. This step diffs the goals that MUST produce a # summary document against the documents k6 actually wrote, so a partial suite is visibly # partial and a goal that silently vanished fails the job. When Maven failed, it also states # in words whether the suite was truncated by that failure. Skipped goals are named with their From b85c57a32c881781fa1055e4e99627767ea938f0 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Thu, 30 Jul 2026 07:00:07 +0200 Subject: [PATCH 12/16] test(gateway): assert the 413 status contract in the large-body IT Finding 8525f1. The negative case asserted only the RFC 9457 envelope, on the reasoning that the status is incidental once the gateway hands over a rejection. It is not incidental: it is the mapping this PR changes. A regression that moved the event back to a 400-mapped constant would have left the envelope assertions green. Assert both halves, and say why neither replaces the other: the status locks the CONTENT_TOO_LARGE -> 413 mapping, while the envelope is what discriminates the gateway's rejection from the framework's own Content-Length pre-check, which answers with a bare 413 carrying no envelope. Verified against the containerised stack: LargeBodyIT 2/2 green. Co-Authored-By: Claude --- .../sheriff/gateway/integration/LargeBodyIT.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java index 8bbe9cd8..3de9c9b9 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java @@ -78,11 +78,14 @@ * so the counter stays at {@code 0} — the observable proof the rejection happened before any * payload crossed the wire, not after 68 MiB were uploaded and discarded. *

    - * The negative case deliberately makes no assertion on the status code: the status - * is the framework's to choose once the gateway hands it a rejection, and pinning it here would - * couple the test to that incidental choice. What is under test is the envelope — the - * {@code application/problem+json} content type, the input-validation problem type and title, and the - * absence of the go-httpbin echo that proves the request never reached the upstream. + * The negative case asserts both halves of the error contract, and neither replaces + * the other. The status assertion locks the mapping under test: a proxy-path body-cap breach renders + * {@code CONTENT_TOO_LARGE} as HTTP {@code 413}, so a regression that moved the event back to a + * 400-mapped constant fails here. The envelope assertions — the {@code application/problem+json} + * content type, the input-validation problem type and title, and the absence of the go-httpbin echo — + * are what discriminate the gateway's rejection from the framework's: the framework's + * own {@code Content-Length} pre-check answers with a bare {@code 413} carrying no envelope, so the + * status alone cannot tell the two apart. * * @author API Sheriff Team * @since 1.0 @@ -152,6 +155,9 @@ void bodyOverTheRouteCapIsRejectedByTheGateway() throws Exception { HttpResponse response = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); + assertEquals(413, response.statusCode(), + "an over-cap request must render CONTENT_TOO_LARGE as HTTP 413"); + String contentType = response.headers().firstValue("Content-Type").orElse(""); JsonPath problem = JsonPath.from(response.body()); assertTrue(contentType.contains(PROBLEM_JSON), From de169eb0ed725729ba97263c7de7766f69eceefd Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Thu, 30 Jul 2026 07:00:46 +0200 Subject: [PATCH 13/16] ci(benchmarks): derive the coverage gate from a generated execution manifest Finding f0867e. The benchmark coverage step maintained a hard-coded expected goal list plus a hand-maintained skip table, both mirroring the resolved -Pbenchmark execution set in benchmarks/pom.xml with nothing enforcing agreement. The drift was not hypothetical: this PR moved the executing set from eleven to twelve and left prose in the same file saying eleven. Add benchmarks/scripts/benchmark-manifest.py, bound to initialize so the manifest exists before any goal runs and a suite truncated by a later failure still states what was expected. Maven hands over only the RESOLVED value of each skip property, so a -Dskip.benchmark.upload.large=true override is reflected; the generator exits non-zero when an execution's skip property was not handed over, so a new suppressible goal cannot silently report as executing. The workflow now derives both the gated set and the not-expected table from the manifest, and treats an absent manifest as a hard failure rather than an empty expected set that would vacuously report full coverage. The unwired sessionMediated script carries its own @unwiredReason tag, so that reason lives in one place instead of being duplicated into the workflow. Scope note: this exceeds the plan's D6 (re-enable uploadLarge). It was implemented at explicit operator direction after triage recommended deferring it to a follow-up. Verified: manifest generates via Maven initialize with 12 enabled / 1 unwired and uploadLarge enabled; both jq expressions checked against the real manifest; the missing-skip-property guard exits 1. The workflow step itself only runs in a post-merge Performance Benchmark run and is unproven until then. Co-Authored-By: Claude --- .github/workflows/benchmark.yml | 59 ++- benchmarks/README.adoc | 25 +- benchmarks/pom.xml | 46 ++ benchmarks/scripts/benchmark-manifest.py | 401 ++++++++++++++++++ .../resources/k6-scripts/session_mediated.js | 5 + 5 files changed, 515 insertions(+), 21 deletions(-) create mode 100644 benchmarks/scripts/benchmark-manifest.py diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 54840ef5..c265a792 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -128,13 +128,21 @@ jobs: ls -la benchmarks/target/benchmark-results/ # Maven fails fast, so a goal that errors aborts every goal after it and a goal that is never - # reached leaves no trace in the job output — a suite that ran 3 of the 12 executing goals - # renders exactly like one that ran all 12. This step diffs the goals that MUST produce a + # reached leaves no trace in the job output — a suite that ran a handful of the executing goals + # renders exactly like one that ran all of them. This step diffs the goals that MUST produce a # summary document against the documents k6 actually wrote, so a partial suite is visibly # partial and a goal that silently vanished fails the job. When Maven failed, it also states - # in words whether the suite was truncated by that failure. Skipped goals are named with their - # reason and their skip property rather than omitted, so "not run" is never - # indistinguishable from "forgotten". + # in words whether the suite was truncated by that failure. Goals that are not expected to run + # are named with their reason and their skip property rather than omitted, so "not run" is + # never indistinguishable from "forgotten". + # + # Both the expected set and the not-expected table are DERIVED from the build-generated + # execution manifest (benchmarks/pom.xml, generate-benchmark-manifest, bound to initialize), + # not maintained by hand here. That is deliberate: the previous hand-maintained list mirrored + # the POM with nothing enforcing agreement, and it drifted — the goal set moved from eleven to + # twelve while prose in this file still said eleven. Deriving both from the manifest makes the + # drift structurally impossible, and an absent manifest is itself a hard failure rather than an + # empty expected set that would vacuously pass. - name: Summarise benchmark coverage if: always() env: @@ -142,10 +150,30 @@ jobs: run: | set -uo pipefail results_dir=benchmarks/target/benchmark-results/k6 + manifest=benchmarks/target/benchmark-execution-manifest.json - # Goals expected to write a summary. A goal settled as deliberately skipped is NOT listed - # here — it belongs in the skip table below, which is what keeps the two states distinct. - expected="healthLiveCheck gatewayHealth proxiedStatic passthroughRelay passthroughRelayEmpty bearerProxied http2 graphql uploadSmall uploadLarge grpcUnary websocketEcho" + # An absent manifest means the initialize-bound generator never ran. Fail loudly rather + # than deriving an empty expected set, which would report full coverage over zero goals. + # The one tolerated case is a benchmark step that failed before initialize completed — + # there its own error is the actionable signal, so state the gap and defer to it. + if [ ! -f "${manifest}" ]; then + { + echo "## Benchmark coverage" + echo + echo "Execution manifest \`${manifest}\` is absent, so coverage could not be derived." + } >> "$GITHUB_STEP_SUMMARY" + if [ "${BENCHMARK_OUTCOME}" = "success" ]; then + echo "::error::Benchmark execution manifest absent although the benchmark step succeeded — the generate-benchmark-manifest execution did not run." + exit 1 + fi + echo "::warning::Benchmark execution manifest absent; the benchmark step reported '${BENCHMARK_OUTCOME}', so coverage is unknown rather than complete." + exit 0 + fi + + # Goals the resolved profile expects to write a summary. A goal the manifest reports as + # skipped or unwired is NOT in this set — it belongs in the table below, which is what + # keeps the two states distinct. + expected="$(jq -r '.executions[] | select(.state == "enabled") | .goal' "${manifest}")" missing=0 missing_names="" @@ -166,20 +194,25 @@ jobs: fi done + # Every non-executing goal, with the property that suppressed it and the documented + # reason, both read straight from the manifest. { echo - echo "### Deliberately skipped" + echo "### Not expected to run" echo - echo "| Benchmark | Skipped by | Reason |" - echo "| --- | --- | --- |" - echo "| \`sessionMediated\` | not wired | Wired to no Maven goal on purpose — BFF session mediation belongs to PLAN-07A. |" + echo "| Benchmark | State | Suppressed by | Reason |" + echo "| --- | --- | --- | --- |" + jq -r '.executions[] + | select(.state != "enabled") + | "| `\(.goal)` | \(.state) | \(.skip_property // "not wired") | \(.reason // "-") |"' \ + "${manifest}" } >> "$GITHUB_STEP_SUMMARY" # Only fail on a missing summary when Maven itself reported success: when the benchmark # step already failed, its own error is the actionable signal and this step must not # mask it with a derived one. if [ "${missing}" -gt 0 ] && [ "${BENCHMARK_OUTCOME}" = "success" ]; then - echo "::error::${missing} expected benchmark summary document(s) absent although the benchmark step succeeded." + echo "::error::${missing} expected benchmark summary document(s) absent although the benchmark step succeeded: ${missing_names}." exit 1 fi diff --git a/benchmarks/README.adoc b/benchmarks/README.adoc index 66755f24..7dfa3f1c 100644 --- a/benchmarks/README.adoc +++ b/benchmarks/README.adoc @@ -153,7 +153,11 @@ fail-fast discarded goals 5-12, which had therefore never run in CI even once. B `pom.xml` is not evidence that a goal produces a number. Current disposition — the CI end-of-job coverage step (see `.github/workflows/benchmark.yml`) -enforces this table, so a goal that silently stops running fails the job: +enforces this, so a goal that silently stops running fails the job. It does not read the table +below: it derives the expected set from the build-generated execution manifest +(`benchmarks/target/benchmark-execution-manifest.json`, emitted at `initialize` by the +`generate-benchmark-manifest` execution in `pom.xml`), so the gate cannot drift from the POM the way +a hand-maintained list can. The table here is documentation of that resolved set, not its source: [cols="2,1,4", options="header"] |=== @@ -199,9 +203,12 @@ enforces this table, so a goal that silently stops running fails the job: *Skipped means not executed.* A goal suppressed by a `` element in `benchmarks/pom.xml` — bound to its own property — is not run at all: it consumes no wall time and cannot abort the suite. No k6 goal is suppressed today; every declared goal executes. The two states are kept distinct rather than -collapsed: the CI coverage step gates on the twelve goals that execute and would name any suppressed -goal, its skip property and its reason in the job summary. When Maven fails, that step also states in -words whether the failure truncated the suite. Silence is never the signal. +collapsed: the CI coverage step gates on whichever goals the execution manifest reports as `enabled`, +and names every non-executing goal with its state, its skip property and its reason in the job +summary. Both sides of that split come from the manifest, so suppressing a goal automatically moves it +from the gated set into the reported table — there is no second list to remember to update. When Maven +fails, that step also states in words whether the failure truncated the suite. Silence is never the +signal. The per-goal properties are retained even when `false`, so a single goal can be suppressed for a local run without editing its execution: @@ -218,10 +225,12 @@ local run without editing its execution: server-session variant and explicitly deferred its per-variant benchmark additions because `benchmarks/**` is PLAN-25's surface. The script is therefore ready but unclaimed, not forgotten. -It is listed in the CI end-of-job coverage summary under its own skip section, so its absence from a -run is stated in every job rather than reading as a silent omission. Do not treat a missing -`sessionMediated-summary.json` as a regression, and do not wire the script to a Maven goal without -the owning plan. +The manifest generator discovers it by reading the `@unwiredReason` tag in the script's own header +comment and reports it as `unwired`, so the CI coverage summary lists it — with that reason — in the +"Not expected to run" table of every job rather than letting its absence read as a silent omission. +The tag in the script is the single place that reason is written; keep it there rather than +duplicating it into the workflow. Do not treat a missing `sessionMediated-summary.json` as a +regression, and do not wire the script to a Maven goal without the owning plan. == Running the CI baseline lane diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml index 71771fb2..b7986217 100644 --- a/benchmarks/pom.xml +++ b/benchmarks/pom.xml @@ -83,6 +83,14 @@ ${k6.results.dir}/history + + ${project.build.directory}/benchmark-execution-manifest.json + true @@ -192,6 +200,44 @@ org.codehaus.mojo exec-maven-plugin + + + generate-benchmark-manifest + initialize + + exec + + + python3 + + ${project.basedir}/scripts/benchmark-manifest.py + generate + --pom + ${project.basedir}/pom.xml + --k6-script-dir + ${k6.script.dir} + --output + ${benchmark.manifest.file} + --skip-property + skip.benchmark.upload.large=${skip.benchmark.upload.large} + --skip-property + skip.benchmark.websocket.echo=${skip.benchmark.websocket.echo} + + ${project.basedir} + + + maven-build-integration-tests diff --git a/benchmarks/scripts/benchmark-manifest.py b/benchmarks/scripts/benchmark-manifest.py new file mode 100644 index 00000000..dce53771 --- /dev/null +++ b/benchmarks/scripts/benchmark-manifest.py @@ -0,0 +1,401 @@ +#!/usr/bin/env python3 +"""Benchmark execution manifest: generate it from the build, then consume it in CI. + +The CI coverage step used to carry a hand-written list of the benchmark goals that must +produce a summary document, plus a hand-written table of the goals deliberately not run. +Both mirrored `benchmarks/pom.xml` with nothing enforcing agreement, and both had already +drifted. This script removes the mirror: `generate` derives the goal set from the Maven +model itself, and `summarise` renders the CI job summary from what `generate` wrote. + +`generate` reads three authoritative sources and nothing else: + + * `benchmarks/pom.xml` -- which `exec-maven-plugin` executions run a k6 script, in which + order, and which property (if any) suppresses each one. + * the resolved value of every such skip property, handed over by Maven on the command + line so a `-Dskip.benchmark.upload.large=true` override is reflected. An execution + whose skip property was not handed over is a hard error, so a new suppressible goal + cannot be added without wiring its value through. + * the k6 scripts themselves -- each script names the summary document it writes through + its own `BENCHMARK_NAME`, so the manifest's goal names are the names k6 will actually + use rather than a third copy of them. + +A k6 script present in the script directory but referenced by no execution is reported as +`unwired` rather than dropped, so "deliberately not wired" never reads as "forgotten". + +Manifest schema (`benchmarks/target/benchmark-execution-manifest.json`): + + { + "schema_version": 1, + "profile": "benchmark", + "executions": [ + {"goal": "healthLiveCheck", "execution_id": "run-k6-health-live-benchmark", + "script": "health_live.js", "state": "enabled", + "skip_property": null, "reason": null}, + ... + ] + } + +`state` is one of `enabled`, `skipped` or `unwired`. +""" + +import argparse +import json +import os +import re +import sys +import xml.etree.ElementTree as ElementTree + +MAVEN_NS = {"m": "http://maven.apache.org/POM/4.0.0"} + +#: The k6 script argument shape an execution passes to the containerised k6 runner. +SCRIPT_ARGUMENT = re.compile(r"^/scripts/(?P[\w.\-]+\.js)$") + +#: A script that names its summary document with a single module-level constant. +STATIC_NAME = re.compile(r"const\s+BENCHMARK_NAME\s*=\s*'(?P[^']+)'") + +#: A script that selects its summary name from a mode switch (passthrough_relay.js). +MODE_NAME = re.compile(r"case\s+'(?P[^']+)'\s*:\s*return\s*\{\s*benchmarkName:\s*'(?P[^']+)'", + re.DOTALL) + +#: The reason marker an unwired script carries in its file header. +UNWIRED_REASON = re.compile(r"@unwiredReason\s+(?P.+?)\s*(?:\n\s*\*\s*\n|\n\s*\*/)", re.DOTALL) + +#: A `` element that defers to a property, e.g. `${skip.benchmark.upload.large}`. +PROPERTY_REFERENCE = re.compile(r"^\$\{(?P[^}]+)\}$") + +STATE_ENABLED = "enabled" +STATE_SKIPPED = "skipped" +STATE_UNWIRED = "unwired" + + +class ManifestError(RuntimeError): + """A defect in the build model the manifest cannot paper over.""" + + +def _text(element, path): + """Returns the stripped text of ``path`` under ``element``, or ``None``.""" + if element is None: + return None + found = element.find(path, MAVEN_NS) + if found is None or found.text is None: + return None + return found.text.strip() + + +def _local_name(tag): + return tag.rsplit("}", 1)[-1] + + +def _profile(root, profile_id): + for profile in root.findall("m:profiles/m:profile", MAVEN_NS): + if _text(profile, "m:id") == profile_id: + return profile + raise ManifestError(f"benchmarks/pom.xml declares no with id '{profile_id}'") + + +def _module_properties(root): + properties = {} + container = root.find("m:properties", MAVEN_NS) + if container is not None: + for child in container: + properties[_local_name(child.tag)] = (child.text or "").strip() + return properties + + +def _exec_executions(profile): + for plugin in profile.findall("m:build/m:plugins/m:plugin", MAVEN_NS): + if _text(plugin, "m:artifactId") != "exec-maven-plugin": + continue + for execution in plugin.findall("m:executions/m:execution", MAVEN_NS): + yield execution + + +def _script_argument(configuration): + if configuration is None: + return None + for argument in configuration.findall("m:arguments/m:argument", MAVEN_NS): + match = SCRIPT_ARGUMENT.match((argument.text or "").strip()) + if match: + return match.group("name") + return None + + +def _environment(configuration): + environment = {} + container = configuration.find("m:environmentVariables", MAVEN_NS) + if container is not None: + for child in container: + environment[_local_name(child.tag)] = (child.text or "").strip() + return environment + + +def _resolve_skip(configuration, execution_id, skip_properties): + """Returns ``(skipped, skip_property)`` for one execution. + + An execution with no ```` runs whenever the profile is active. An execution whose + ```` defers to a property is resolved against the values Maven handed over; a + property that was not handed over is fatal rather than assumed false, so a new + suppressible goal cannot silently report as enabled. + """ + raw = _text(configuration, "m:skip") + if raw is None: + return False, None + reference = PROPERTY_REFERENCE.match(raw) + if not reference: + return raw.lower() == "true", None + name = reference.group("name") + if name not in skip_properties: + raise ManifestError( + f"execution '{execution_id}' is suppressed by property '{name}', but that property's" + f" resolved value was not handed over. Add" + f" --skip-property {name}=${{{name}}} to the generate-benchmark-manifest execution" + f" in benchmarks/pom.xml.") + return skip_properties[name].lower() == "true", name + + +def _script_source(script_dir, script): + path = os.path.join(script_dir, script) + if not os.path.isfile(path): + raise ManifestError(f"k6 script '{script}' is referenced by an execution but is not on disk at {path}") + with open(path, encoding="utf-8") as handle: + return handle.read() + + +def _goal_name(source, script, environment): + """Resolves the summary-document name the script will write. + + A script naming itself with a single constant answers directly. A script selecting its + name from a mode switch is resolved through the mode its execution passes in the + environment; an unresolvable mode is fatal, because guessing would mislabel the goal. + """ + static = STATIC_NAME.search(source) + if static: + return static.group("name") + + modes = {mode.lower(): name for mode, name in MODE_NAME.findall(source)} + if not modes: + raise ManifestError( + f"k6 script '{script}' declares neither a BENCHMARK_NAME constant nor a mode switch," + f" so the summary document it writes cannot be derived") + for value in environment.values(): + if value.lower() in modes: + return modes[value.lower()] + raise ManifestError( + f"k6 script '{script}' selects its benchmark name from modes {sorted(modes)}, but its" + f" execution passes none of them in the environment") + + +def _unwired_names(source, script): + static = STATIC_NAME.search(source) + if static: + return [static.group("name")] + names = sorted({name for _, name in MODE_NAME.findall(source)}) + if not names: + raise ManifestError( + f"k6 script '{script}' declares no benchmark name, so it cannot be reported as unwired") + return names + + +def _unwired_reason(source): + match = UNWIRED_REASON.search(source) + if not match: + return None + return " ".join(part.strip().lstrip("*").strip() for part in match.group("reason").splitlines()).strip() + + +def generate(pom_path, script_dir, output_path, profile_id, skip_properties): + root = ElementTree.parse(pom_path).getroot() + profile = _profile(root, profile_id) + properties = _module_properties(root) + + executions = [] + wired_scripts = set() + + for execution in _exec_executions(profile): + configuration = execution.find("m:configuration", MAVEN_NS) + script = _script_argument(configuration) + if script is None: + continue + execution_id = _text(execution, "m:id") or "(unnamed)" + wired_scripts.add(script) + skipped, skip_property = _resolve_skip(configuration, execution_id, skip_properties) + source = _script_source(script_dir, script) + goal = _goal_name(source, script, _environment(configuration)) + reason = properties.get(f"{skip_property}.reason") if skip_property else None + executions.append({ + "goal": goal, + "execution_id": execution_id, + "script": script, + "state": STATE_SKIPPED if skipped else STATE_ENABLED, + "skip_property": skip_property, + "reason": reason if skipped else None, + }) + + for script in sorted(os.listdir(script_dir)): + if not script.endswith(".js") or script in wired_scripts: + continue + source = _script_source(script_dir, script) + reason = _unwired_reason(source) + for goal in _unwired_names(source, script): + executions.append({ + "goal": goal, + "execution_id": None, + "script": script, + "state": STATE_UNWIRED, + "skip_property": None, + "reason": reason, + }) + + if not any(entry["state"] == STATE_ENABLED for entry in executions): + raise ManifestError( + f"the '{profile_id}' profile resolved to no executing k6 goal — refusing to write a" + f" manifest that would make every goal's absence look expected") + + manifest = {"schema_version": 1, "profile": profile_id, "executions": executions} + os.makedirs(os.path.dirname(output_path) or ".", exist_ok=True) + with open(output_path, "w", encoding="utf-8") as handle: + json.dump(manifest, handle, indent=2) + handle.write("\n") + + executing = [entry["goal"] for entry in executions if entry["state"] == STATE_ENABLED] + print(f"Benchmark execution manifest written to {output_path}: " + f"{len(executing)} executing goal(s), " + f"{sum(1 for e in executions if e['state'] == STATE_SKIPPED)} skipped, " + f"{sum(1 for e in executions if e['state'] == STATE_UNWIRED)} unwired.") + return 0 + + +def _not_run_reason(entry): + if entry["state"] == STATE_UNWIRED: + return entry["reason"] or "Wired to no Maven goal; no reason declared — add an @unwiredReason tag to the script." + if entry["reason"]: + return entry["reason"] + return (f"No reason declared — add a `{entry['skip_property']}.reason` property to benchmarks/pom.xml." + if entry["skip_property"] else "No reason declared.") + + +def _skipped_by(entry): + if entry["state"] == STATE_UNWIRED: + return "not wired" + return f"`{entry['skip_property']}`" if entry["skip_property"] else "an inline ``" + + +def summarise(manifest_path, results_dir, benchmark_outcome, summary_file): + lines = [] + if not os.path.isfile(manifest_path): + lines.append("## Benchmark coverage") + lines.append("") + lines.append(f"> **The execution manifest at `{manifest_path}` was never generated**, so which " + "goals were expected cannot be stated. The build did not reach the manifest step.") + _emit(lines, summary_file) + print(f"::error::benchmark execution manifest absent at {manifest_path}.") + return 1 if benchmark_outcome == "success" else 0 + + with open(manifest_path, encoding="utf-8") as handle: + manifest = json.load(handle) + executions = manifest["executions"] + + expected = [entry for entry in executions if entry["state"] == STATE_ENABLED] + not_run = [entry for entry in executions if entry["state"] != STATE_ENABLED] + + missing = [] + lines.append("## Benchmark coverage") + lines.append("") + lines.append("| Benchmark | Result |") + lines.append("| --- | --- |") + for entry in expected: + goal = entry["goal"] + if os.path.isfile(os.path.join(results_dir, f"{goal}-summary.json")): + lines.append(f"| `{goal}` | ran |") + else: + lines.append(f"| `{goal}` | **DID NOT RUN** |") + missing.append(goal) + + lines.append("") + lines.append("### Deliberately not run") + lines.append("") + if not_run: + lines.append("| Benchmark | Skipped by | Reason |") + lines.append("| --- | --- | --- |") + for entry in not_run: + lines.append(f"| `{entry['goal']}` | {_skipped_by(entry)} | {_not_run_reason(entry)} |") + else: + lines.append(f"Every one of the {len(expected)} declared goals executes; none is suppressed or unwired.") + + if benchmark_outcome != "success": + lines.append("") + if missing: + lines.append(f"> **Maven failed and the suite was TRUNCATED**: goals {', '.join(missing)} produced " + f"no summary because the run aborted before reaching them. The DID NOT RUN rows above " + f"are a consequence of that failure, not {len(missing)} independent problems — fix the " + f"reported Maven error first, then re-read this table.") + else: + lines.append("> **Maven failed AFTER every expected goal produced a summary.** No goal was lost to " + "truncation, so the failure lies outside goal execution (post-processing, the baseline " + "comparison, or container teardown). The results above are complete and usable.") + + _emit(lines, summary_file) + + print(f"Benchmark coverage: {len(missing)} of {len(expected)} expected summary document(s) absent " + f"(benchmark step outcome: {benchmark_outcome}).") + + # Only fail on a missing summary when Maven itself reported success: when the benchmark step + # already failed, its own error is the actionable signal and this step must not mask it. + if missing and benchmark_outcome == "success": + print(f"::error::{len(missing)} expected benchmark summary document(s) absent although the " + f"benchmark step succeeded: {', '.join(missing)}.") + return 1 + return 0 + + +def _emit(lines, summary_file): + text = "\n".join(lines) + "\n" + if summary_file: + with open(summary_file, "a", encoding="utf-8") as handle: + handle.write(text) + else: + sys.stdout.write(text) + + +def _skip_property(raw): + if "=" not in raw: + raise argparse.ArgumentTypeError(f"--skip-property expects name=value, got '{raw}'") + name, value = raw.split("=", 1) + return name, value + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + subparsers = parser.add_subparsers(dest="command", required=True) + + generate_parser = subparsers.add_parser( + "generate", help="derive the benchmark execution manifest from the Maven model") + generate_parser.add_argument("--pom", required=True, help="path to benchmarks/pom.xml") + generate_parser.add_argument("--k6-script-dir", required=True, help="directory holding the k6 scripts") + generate_parser.add_argument("--output", required=True, help="manifest output path") + generate_parser.add_argument("--profile", default="benchmark", help="the Maven profile to read") + generate_parser.add_argument("--skip-property", action="append", default=[], type=_skip_property, + metavar="NAME=VALUE", + help="a skip property's Maven-resolved value; repeatable") + + summarise_parser = subparsers.add_parser( + "summarise", help="render the CI coverage summary from the manifest and the written results") + summarise_parser.add_argument("--manifest", required=True, help="manifest path") + summarise_parser.add_argument("--results-dir", required=True, help="directory holding k6 summary documents") + summarise_parser.add_argument("--benchmark-outcome", required=True, + help="the benchmark step's outcome, e.g. success or failure") + summarise_parser.add_argument("--summary-file", default=None, + help="file to append the markdown to (defaults to stdout)") + + args = parser.parse_args(argv) + try: + if args.command == "generate": + return generate(args.pom, args.k6_script_dir, args.output, args.profile, dict(args.skip_property)) + return summarise(args.manifest, args.results_dir, args.benchmark_outcome, args.summary_file) + except ManifestError as error: + print(f"::error::{error}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/benchmarks/src/main/resources/k6-scripts/session_mediated.js b/benchmarks/src/main/resources/k6-scripts/session_mediated.js index 24645594..148c4b78 100644 --- a/benchmarks/src/main/resources/k6-scripts/session_mediated.js +++ b/benchmarks/src/main/resources/k6-scripts/session_mediated.js @@ -20,6 +20,11 @@ * checks rate of 0 and fails the build instead of reporting the redirect/rejection path as an * excellent result. {@link setup} is fail-loud for the same reason: a run that could not establish * a session aborts here rather than measuring the unauthenticated fall-through. + * + * @unwiredReason Wired to no Maven goal on purpose -- BFF session mediation belongs to PLAN-07A, + * which deferred its per-variant benchmark additions. benchmark-manifest.py reads this tag and + * reports the script as `unwired` in the CI coverage summary, so its absence from a run is stated + * rather than silent. */ import http from 'k6/http'; import { check, fail } from 'k6'; From eeec70b218791632d03c053bae2eb73e600c77b0 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Thu, 30 Jul 2026 07:31:00 +0200 Subject: [PATCH 14/16] ci(benchmarks): call the generator's summarise instead of re-deriving in bash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding on the previous commit, and it is correct. benchmark-manifest.py already ships a summarise subcommand — its module docstring names it as the CI consumer — but the workflow reimplemented the same rendering in bash/jq. That recreated, one level up, the duplicated-contract defect the manifest was introduced to remove: two implementations of one contract, already diverged on the table heading, and the jq fallback mislabelled an inline true entry as "not wired" where the Python names the property. Delete the bash/jq block and call summarise, so the manifest has exactly one consumer. Verified all three exit semantics: complete coverage exits 0; a missing summary document with a successful benchmark step exits 1; an absent manifest exits 1 when the benchmark step succeeded and 0 when it failed, deferring to Maven's own error rather than masking it. Co-Authored-By: Claude --- .github/workflows/benchmark.yml | 82 ++++++--------------------------- benchmarks/README.adoc | 4 +- 2 files changed, 16 insertions(+), 70 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index c265a792..ae2b7f27 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -140,81 +140,25 @@ jobs: # execution manifest (benchmarks/pom.xml, generate-benchmark-manifest, bound to initialize), # not maintained by hand here. That is deliberate: the previous hand-maintained list mirrored # the POM with nothing enforcing agreement, and it drifted — the goal set moved from eleven to - # twelve while prose in this file still said eleven. Deriving both from the manifest makes the + # twelve while prose in this file still said eleven. Deriving both from the manifest makes that # drift structurally impossible, and an absent manifest is itself a hard failure rather than an - # empty expected set that would vacuously pass. + # empty expected set that would vacuously report full coverage. + # + # The rendering lives in the generator's own `summarise` subcommand rather than being + # reimplemented in bash here, so the manifest has exactly ONE consumer. Splitting it across two + # implementations would recreate, one level up, the duplicated-contract defect this manifest was + # introduced to remove — and did: an earlier bash/jq version of this step diverged on the table + # heading and mislabelled an inline `` as "not wired". - name: Summarise benchmark coverage if: always() env: BENCHMARK_OUTCOME: ${{ steps.run-benchmarks.outcome }} run: | - set -uo pipefail - results_dir=benchmarks/target/benchmark-results/k6 - manifest=benchmarks/target/benchmark-execution-manifest.json - - # An absent manifest means the initialize-bound generator never ran. Fail loudly rather - # than deriving an empty expected set, which would report full coverage over zero goals. - # The one tolerated case is a benchmark step that failed before initialize completed — - # there its own error is the actionable signal, so state the gap and defer to it. - if [ ! -f "${manifest}" ]; then - { - echo "## Benchmark coverage" - echo - echo "Execution manifest \`${manifest}\` is absent, so coverage could not be derived." - } >> "$GITHUB_STEP_SUMMARY" - if [ "${BENCHMARK_OUTCOME}" = "success" ]; then - echo "::error::Benchmark execution manifest absent although the benchmark step succeeded — the generate-benchmark-manifest execution did not run." - exit 1 - fi - echo "::warning::Benchmark execution manifest absent; the benchmark step reported '${BENCHMARK_OUTCOME}', so coverage is unknown rather than complete." - exit 0 - fi - - # Goals the resolved profile expects to write a summary. A goal the manifest reports as - # skipped or unwired is NOT in this set — it belongs in the table below, which is what - # keeps the two states distinct. - expected="$(jq -r '.executions[] | select(.state == "enabled") | .goal' "${manifest}")" - - missing=0 - missing_names="" - { - echo "## Benchmark coverage" - echo - echo "| Benchmark | Result |" - echo "| --- | --- |" - } >> "$GITHUB_STEP_SUMMARY" - - for name in ${expected}; do - if [ -f "${results_dir}/${name}-summary.json" ]; then - echo "| \`${name}\` | ran |" >> "$GITHUB_STEP_SUMMARY" - else - echo "| \`${name}\` | **DID NOT RUN** |" >> "$GITHUB_STEP_SUMMARY" - missing=$((missing + 1)) - missing_names="${missing_names:+${missing_names}, }${name}" - fi - done - - # Every non-executing goal, with the property that suppressed it and the documented - # reason, both read straight from the manifest. - { - echo - echo "### Not expected to run" - echo - echo "| Benchmark | State | Suppressed by | Reason |" - echo "| --- | --- | --- | --- |" - jq -r '.executions[] - | select(.state != "enabled") - | "| `\(.goal)` | \(.state) | \(.skip_property // "not wired") | \(.reason // "-") |"' \ - "${manifest}" - } >> "$GITHUB_STEP_SUMMARY" - - # Only fail on a missing summary when Maven itself reported success: when the benchmark - # step already failed, its own error is the actionable signal and this step must not - # mask it with a derived one. - if [ "${missing}" -gt 0 ] && [ "${BENCHMARK_OUTCOME}" = "success" ]; then - echo "::error::${missing} expected benchmark summary document(s) absent although the benchmark step succeeded: ${missing_names}." - exit 1 - fi + python3 benchmarks/scripts/benchmark-manifest.py summarise \ + --manifest benchmarks/target/benchmark-execution-manifest.json \ + --results-dir benchmarks/target/benchmark-results/k6 \ + --benchmark-outcome "${BENCHMARK_OUTCOME}" \ + --summary-file "$GITHUB_STEP_SUMMARY" # When Maven failed, the DID NOT RUN rows above and the Maven error are almost always the # same event, but nothing on the page says so — a reader is left to infer the causal link. diff --git a/benchmarks/README.adoc b/benchmarks/README.adoc index 7dfa3f1c..02ec729d 100644 --- a/benchmarks/README.adoc +++ b/benchmarks/README.adoc @@ -157,7 +157,9 @@ enforces this, so a goal that silently stops running fails the job. It does not below: it derives the expected set from the build-generated execution manifest (`benchmarks/target/benchmark-execution-manifest.json`, emitted at `initialize` by the `generate-benchmark-manifest` execution in `pom.xml`), so the gate cannot drift from the POM the way -a hand-maintained list can. The table here is documentation of that resolved set, not its source: +a hand-maintained list can. The workflow calls `benchmark-manifest.py summarise` rather than +re-deriving the summary itself, so the manifest has exactly one consumer. The table here is +documentation of that resolved set, not its source: [cols="2,1,4", options="header"] |=== From d984daf4b313cd9434b3be0231e3e1c5fe16b98d Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Thu, 30 Jul 2026 08:35:51 +0200 Subject: [PATCH 15/16] test(events): derive EventTypeTest bucket membership from the enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding on EventTypeTest:114. The success, configuration and WebSocket-close sets were three hand-maintained mirrors of EventType, and the drift guards added earlier only detect an omission after that second registry has already drifted. A derived set cannot drift at all. EventType already exposes the classifying accessors, so no new production API was needed: hasHttpMapping(), wsCloseCode() and category() partition the enum into the four buckets the suite asserts, and membership is now selected by predicate over allOf(EventType.class). A constant added to the enum lands in its bucket automatically. What stays literal is the part that carries independent information: ERROR_CONTRACT still states each mapped event's expected status and category as written-out values, so the mapping assertions cannot pass by re-reading the enum under test. Derive the membership, state the mapping. Two consequential follow-ons rather than a mechanical swap: * the WebSocket-close case was a single hard-coded constant, so a second close-code constant would have gone unasserted. It is now parameterized over the derived set, with the literal 1001 kept as its own assertion since the parameterized test derives membership from wsCloseCode() and therefore cannot also be what pins the value. * the ws-close coverage guard became tautological once its set was derived, and is removed. The partition guard is retained and re-documented: with membership derived it no longer guards a forgotten list entry, it guards the PREDICATES — a constant claimed by none or by two shapes means they have stopped partitioning the enum. Verified: derived counts match the previously hand-listed sets exactly (7 success, 2 configuration, 1 WebSocket-close, 18 error-contract), the partition guard passes, and no bucket derives empty. Co-Authored-By: Claude --- .../sheriff/gateway/events/EventTypeTest.java | 100 +++++++++++------- 1 file changed, 61 insertions(+), 39 deletions(-) diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java index 2f0bd08c..b2dcb833 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java @@ -26,6 +26,7 @@ import java.util.EnumSet; import java.util.List; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Stream; import org.junit.jupiter.api.DisplayName; @@ -38,12 +39,18 @@ /** * Pins {@link EventType}'s category, HTTP-status and WebSocket-close mappings. *

    - * Every constant is claimed by exactly one of four explicitly enumerated contract lists — success / - * informational, boot-only configuration, WebSocket-close, and the request-time error contract. The - * expected statuses and categories are stated here as literals rather than re-read from the enum - * under test, so the lists carry independent information; the drift guards in - * {@link ContractCoverage} then assert that the lists remain exhaustive, which is what a - * hand-maintained mirror of an authoritative source cannot guarantee on its own. + * Every constant is claimed by exactly one of four buckets — success / informational, boot-only + * configuration, WebSocket-close, and the request-time error contract. Bucket membership is + * DERIVED from {@link EventType}'s own accessors ({@code hasHttpMapping}, {@code wsCloseCode}, + * {@code category}), not hand-listed: a guard that merely detects an omission still lets the second + * registry drift first, whereas a derived set cannot drift at all. A constant added to the enum + * therefore lands in its bucket automatically, and {@link ContractCoverage} asserts the four + * predicates remain a true partition so a constant can never fall into none or two of them. + *

    + * What stays literal is the thing that carries independent information: {@link #ERROR_CONTRACT} + * states each mapped event's expected status and category as written-out values rather than + * re-reading them from the enum under test, so the mapping assertions cannot pass vacuously. The + * split is deliberate — derive the membership, state the mapping. * * @author API Sheriff Team * @since 1.0 @@ -52,26 +59,34 @@ class EventTypeTest { /** Success / informational events: no category, no HTTP mapping, no WebSocket-close mapping. */ - private static final Set SUCCESS_EVENTS = EnumSet.of( - EventType.REQUEST_FORWARDED, - EventType.TOKEN_REFRESHED, - EventType.CONFIG_LOADED, - EventType.SESSION_CREATED, - EventType.SESSION_DESTROYED, - EventType.SESSION_REFRESH_FAILED, - EventType.BACKCHANNEL_LOGOUT); + private static final Set SUCCESS_EVENTS = + classify(eventType -> !eventType.hasHttpMapping() + && eventType.wsCloseCode() == 0 + && eventType.category() == null); + + /** + * Selects the constants matching {@code predicate}. Membership is DERIVED from {@link EventType}'s + * own accessors rather than hand-listed, so a constant added to the enum lands in its bucket + * automatically and cannot be silently omitted from the suite. + */ + private static Set classify(Predicate predicate) { + return EnumSet.allOf(EventType.class).stream() + .filter(predicate) + .collect(() -> EnumSet.noneOf(EventType.class), Set::add, Set::addAll); + } /** Boot-only configuration failures: a category, but never an HTTP response. */ - private static final Set CONFIGURATION_EVENTS = EnumSet.of( - EventType.CONFIG_INVALID, - EventType.AUTH_WEAKENED); + private static final Set CONFIGURATION_EVENTS = + classify(eventType -> !eventType.hasHttpMapping() + && eventType.wsCloseCode() == 0 + && eventType.category() != null); /** * Events that terminate an established WebSocket relay: no HTTP mapping (they occur * after the {@code 101} upgrade) but a non-zero close code. */ - private static final Set WEBSOCKET_CLOSE_EVENTS = EnumSet.of( - EventType.WEBSOCKET_IDLE_TIMEOUT); + private static final Set WEBSOCKET_CLOSE_EVENTS = + classify(eventType -> !eventType.hasHttpMapping() && eventType.wsCloseCode() > 0); /** * The request-time error contract: one row per event the edge renders as an HTTP status, with the @@ -104,6 +119,10 @@ static Stream configurationEvents() { return CONFIGURATION_EVENTS.stream(); } + static Stream websocketCloseEvents() { + return WEBSOCKET_CLOSE_EVENTS.stream(); + } + static Stream errorContract() { return ERROR_CONTRACT.stream(); } @@ -168,10 +187,10 @@ void shouldMapEachErrorContractRow(EventType eventType, int expectedStatus, Even @DisplayName("Established-relay WebSocket-close events") class WebSocketCloseEvents { - @Test - @DisplayName("Should close the relay with 1001 Going Away and carry no HTTP mapping") - void shouldCloseIdleRelayWithGoingAway() { - EventType eventType = EventType.WEBSOCKET_IDLE_TIMEOUT; + @ParameterizedTest + @MethodSource("de.cuioss.sheriff.gateway.events.EventTypeTest#websocketCloseEvents") + @DisplayName("Should carry a close code and no HTTP mapping") + void shouldCarryCloseCodeWithoutHttpMapping(EventType eventType) { assertAll("websocket-close event " + eventType, () -> assertNull(eventType.category(), "A reclaimed relay is not a request-time rejection, so it carries no category"), @@ -179,7 +198,16 @@ void shouldCloseIdleRelayWithGoingAway() { () -> assertEquals(0, eventType.httpStatus(), "The close happens after the 101 upgrade, so there is no HTTP status to render"), () -> assertFalse(eventType.hasHttpMapping(), "WebSocket-close events have no HTTP mapping"), - () -> assertEquals(1001, eventType.wsCloseCode(), "An idle relay is closed with 1001 Going Away")); + () -> assertTrue(eventType.wsCloseCode() > 0, "WebSocket-close events carry a close code")); + } + + @Test + @DisplayName("Should close an idle relay with 1001 Going Away") + void shouldCloseIdleRelayWithGoingAway() { + // The literal close code, stated independently of the enum: the parameterized test above + // derives its membership from wsCloseCode(), so it cannot also be what pins the value. + assertEquals(1001, EventType.WEBSOCKET_IDLE_TIMEOUT.wsCloseCode(), + "An idle relay is closed with 1001 Going Away"); } } @@ -200,29 +228,23 @@ void shouldCoverExactlyTheHttpMappedConstants() { } @Test - @DisplayName("Should cover exactly the constants carrying a WebSocket close code") - void shouldCoverExactlyTheWebSocketCloseConstants() { - Set closing = EnumSet.allOf(EventType.class).stream() - .filter(eventType -> eventType.wsCloseCode() > 0) - .collect(() -> EnumSet.noneOf(EventType.class), Set::add, Set::addAll); - - assertEquals(WEBSOCKET_CLOSE_EVENTS, closing, - "Every EventType with a non-zero wsCloseCode must be asserted as a WebSocket-close event"); - } - - @Test - @DisplayName("Should claim every EventType constant in exactly one contract list") + @DisplayName("Should claim every EventType constant in exactly one bucket") void shouldClaimEveryConstantExactlyOnce() { List claimed = Stream .of(SUCCESS_EVENTS, CONFIGURATION_EVENTS, WEBSOCKET_CLOSE_EVENTS, errorContractKeys()) .flatMap(Collection::stream) .toList(); - assertAll("contract-list partition", + // The three membership sets are derived from the enum, so this is not guarding against a + // forgotten list entry — it guards the PREDICATES. A constant whose accessor combination + // no shape covers (or that two shapes both claim) means the four predicates have stopped + // being a partition, and the buckets would silently disagree about who owns it. + assertAll("bucket partition", () -> assertEquals(EnumSet.allOf(EventType.class), EnumSet.copyOf(claimed), - "Every EventType constant must be asserted by one of the contract lists"), + "Every EventType constant must be claimed by one of the four bucket predicates —" + + " an unclaimed constant means the predicates no longer cover the enum"), () -> assertEquals(claimed.size(), EnumSet.copyOf(claimed).size(), - "No EventType constant may appear in more than one contract list")); + "No EventType constant may be claimed by more than one bucket predicate")); } } From 500eec30d71c6d8e06a6f8e87285768d48ea64e9 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Thu, 30 Jul 2026 09:10:40 +0200 Subject: [PATCH 16/16] docs(adr): record the body-cap contract as ADR-0023 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The plan settled a real architectural question rather than fixing a local bug: whether a declared max_body_bytes is the effective ceiling, and which component owns an over-cap rejection. The spec asked for the choice to be stated with its reasoning, and the alternatives were weighed and discarded on their merits, so the decision belongs in an ADR rather than only in a commit message. Records the derivation plus fail-closed boot validation, the 413 rendering on both the proxy and gRPC paths, and — as consequences of the resulting architecture rather than incident notes — the shadowing behaviour at floor == cap, the differing enforcement path for chunked requests, and the invariant that raising the framework floor must never become the only ceiling for a route. Co-Authored-By: Claude --- ..._one_and_a_breach_is_the_gateways_413.adoc | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 doc/adr/0023-The_declared_max_body_bytes_cap_is_the_effective_one_and_a_breach_is_the_gateways_413.adoc diff --git a/doc/adr/0023-The_declared_max_body_bytes_cap_is_the_effective_one_and_a_breach_is_the_gateways_413.adoc b/doc/adr/0023-The_declared_max_body_bytes_cap_is_the_effective_one_and_a_breach_is_the_gateways_413.adoc new file mode 100644 index 00000000..27b6c6d6 --- /dev/null +++ b/doc/adr/0023-The_declared_max_body_bytes_cap_is_the_effective_one_and_a_breach_is_the_gateways_413.adoc @@ -0,0 +1,153 @@ += ADR-0023: The declared max_body_bytes cap is the effective one and a breach is the gateway's 413 +:toc: left +:toclevels: 2 +:sectnums: + +// adr-metadata +// Progressive-disclosure metadata block (see manage-adr SKILL.md → "ADR Template +// Structure"). Read by `manage-adr.py scan` so a caller can assess an ADR's +// relevance without reading the full file. List fields are comma-separated. +// summary: A route's declared security_filter.max_body_bytes is the effective ceiling; the framework floor is derived from it and validated fail-closed at boot, and a breach is rendered by the gateway as an RFC 9457 413. +// tags: body-limits, error-contract, fail-closed, configuration, edge +// affects: api-sheriff/src/main/resources/application.properties, api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java, api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java, api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java, doc/configuration.adoc, doc/architecture.adoc +// supersedes: +// end-adr-metadata + +== Status + +Accepted + +== Context + +A gateway that lets an operator declare a per-route request-body ceiling owns two +limits, not one: the ceiling the configuration declares, and the ceiling the HTTP +framework beneath it enforces. Whichever is lower is the one that actually applies. + +That creates a class of defect in which a documented configuration key is inert. The +declared value is parsed, modelled and enforced by real gateway code, yet every +request that would exercise it is terminated by the framework first, so the +enforcement path is never reached. Nothing fails: the framework's rejection is a +well-formed HTTP response, so the deployment looks correct from the outside while the +operator's stated policy is silently not the policy in force. Unit tests do not +detect it, because they exercise the filter directly — below the layer where the +truncation happens — and therefore pass on a path production never takes. + +The choice point is structural: either the framework floor is derived from the +declared configuration so the gateway is reliably the component that rejects, or the +two limits are allowed to disagree and the effective ceiling becomes an emergent +property of whichever happens to be smaller. + +A second question follows from the first. Once the gateway is the rejecting +component, its rejection must be distinguishable from the framework's. The framework +answers an over-limit request from a root handler in front of the router, with a bare +status line and no response body. If the gateway's own rejection is not +distinguishable from that, a test asserting "the request was rejected" cannot tell +whether the enforcement under test ran at all — which is precisely the blind spot +that let the inert key survive. + +== Decision + +**The declared cap is the effective cap.** The framework body limit is configuration +derived from the declared anchors rather than an independent knob, and the +relationship between them is validated fail-closed at boot: startup aborts when any +route declares a `max_body_bytes` above the framework floor, naming the offending +descriptor. A deployment whose stated policy cannot be honoured does not start, it +does not start and serve a quietly smaller limit. + +Enforcement predicates are unchanged by this decision. The `Content-Length` +fast-reject and the streaming running-counter remain where they were; what changes is +that they are now reachable. + +**A body-cap breach is the gateway's own 413.** The proxy-path breach renders as +`CONTENT_TOO_LARGE`, mapped to HTTP 413 in the `INPUT_VALIDATION` category and +carried in the standard RFC 9457 `application/problem+json` envelope. The gRPC path +maps the same event to `RESOURCE_EXHAUSTED` rather than falling through to `UNKNOWN`, +so a body-cap breach is the same semantic outcome on both protocols. + +The status alone is deliberately not the discriminator between the two rejecting +components, because the framework also answers 413. The envelope is: the gateway's +rejection carries `application/problem+json` with a problem type and title, and the +framework's carries no body at all. Any assertion that means to prove the gateway +enforced must therefore assert on the envelope, not only on the status. + +== Consequences + +=== Positive + +* A declared `max_body_bytes` is honoured or the deployment refuses to start. There + is no third state in which it is documented, parsed, and inert. +* The component that rejects an oversize body is the one whose configuration + described the limit, so the rejection is attributable and carries the gateway's + diagnostic envelope rather than a bare framework status line. +* The failure mode is legible at the layer that owns it: a misconfiguration fails at + boot with the offending descriptor named, rather than at request time as a smaller + ceiling than the operator asked for. +* Body-cap semantics are uniform across the proxy and gRPC paths. + +=== Negative + +* The framework floor and the declared caps are coupled: raising a declared cap above + the shipped floor is a two-part change, and doing only half of it is a boot + failure rather than a warning. +* The shipped floor is a product default that most deployments never touch but every + deployment inherits, so it must be chosen conservatively enough to be safe and + generous enough not to make the common case a boot failure. + +=== Risks + +* **Shadowing at equality.** When the framework floor is set exactly equal to the + largest declared cap, an over-cap request is rejected by the framework's + pre-router check before the gateway pipeline is entered, so the operator receives + the bare framework 413 rather than the RFC 9457 envelope. The boot check permits + this configuration because the declared cap is still honoured for every request at + or below it — only the *renderer* of an over-cap rejection changes. A deployment + that wants the gateway's envelope for over-cap requests must set the floor strictly + above its largest declared cap. This is documented in the configuration reference + rather than forbidden, because forbidding it would reject a configuration whose + stated policy is in fact honoured. +* **Chunked requests take a different path.** A request with no `Content-Length` + skips the framework's pre-check entirely, so the gateway's streaming counter is the + enforcing mechanism and the envelope is always rendered. The two transports + therefore differ in *which* component rejects, though not in the effective limit. +* Raising the framework floor does not widen any un-capped ingress path: every proxy + route falls back to a default body size when it declares none, and reserved + BFF paths carry their own independent ceiling. This is a property the + architecture must preserve — a future change that made the framework floor the only + ceiling for some route would silently re-open the risk this ADR closes. + +== Alternatives Considered + +**Leave the framework floor at its default and enforce only in the gateway.** This is +the state this ADR replaces. It requires no configuration derivation and no boot +validation, and it is why the declared cap was unreachable: the gateway's enforcement +sits behind the framework's, so the lower limit wins regardless of what the +configuration says. It fails not because it is complex but because it makes the +effective policy an accident of which limit is smaller. + +**Require the operator to set the framework key by hand, with no validation.** +Cheapest to implement, and it does make the declared cap reachable when the operator +gets it right. It fails on the same axis as the status quo: a deployment that omits +the key or sets it too low is indistinguishable from a correct one until a large +request arrives, so the defect class survives with a manual step added. + +**Raise the framework limit to a fixed safe ceiling unrelated to the declared caps.** +This makes the gateway the rejecting component without any derivation, and is +attractive for its simplicity. It was rejected because the constant is arbitrary: it +is either lower than some deployment's declared cap — reintroducing the original +defect for that deployment — or high enough to be meaningless as a limit, which +removes the framework's own backstop without putting anything in its place. + +The unifying failure is the same in all three: each leaves the effective ceiling +implicit. The decision above makes it explicit and refuses to boot when it cannot be +honoured, which is the only variant in which the documented configuration surface and +the enforced behaviour cannot diverge. + +== References + +* `doc/configuration.adoc` — the `max_body_bytes` reference and the operator-facing + statement of the framework floor, the shadowing consequence and the chunked-request + carve-out. +* `doc/architecture.adoc` — the error contract: the HTTP status and category tables + and the gRPC status mapping. +* ADR-0003 — the immediate-TCP-peer trust gate, for the edge trust boundary this + enforcement sits behind.