From b8579679db38daacc6b6ca7f12a9ef90da0ddeb0 Mon Sep 17 00:00:00 2001 From: Ziedelth Date: Fri, 2 Oct 2026 22:20:14 +0000 Subject: [PATCH 1/9] build: declare the graphql module Adds a graphql module depending on core, ktor and graphql-kotlin 10.2.2, plus the four Ktor artifacts the security stack needs (rate limit, body limit, compression, double receive). exposed stays out: the framework has no repository, so DataLoaders belong to the consumer. --- gradle/libs.versions.toml | 8 ++++++++ graphql/build.gradle.kts | 18 ++++++++++++++++++ settings.gradle.kts | 1 + 3 files changed, 27 insertions(+) create mode 100644 graphql/build.gradle.kts diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index fffc3c6..06ac282 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -6,6 +6,7 @@ liquibase = "5.0.4" h2 = "2.5.250" bouncyCastle = "1.86" ktor = "3.6.0" +graphqlKotlin = "10.2.2" swaggerCodeGenerators = "1.0.62" valkeyGlide = "2.5.2" kotlinxSerialization = "1.11.0" @@ -38,11 +39,18 @@ ktorSerializationCbor = { module = "io.ktor:ktor-serialization-kotlinx-cbor", ve ktorServerSwagger = { module = "io.ktor:ktor-server-swagger", version.ref = "ktor" } swaggerCodeGenerators = { module = "io.swagger.codegen.v3:swagger-codegen-generators", version.ref = "swaggerCodeGenerators" } ktorServerCors = { module = "io.ktor:ktor-server-cors", version.ref = "ktor" } +ktorServerRateLimit = { module = "io.ktor:ktor-server-rate-limit", version.ref = "ktor" } +ktorServerBodyLimit = { module = "io.ktor:ktor-server-body-limit", version.ref = "ktor" } +ktorServerCompression = { module = "io.ktor:ktor-server-compression", version.ref = "ktor" } +ktorServerDoubleReceive = { module = "io.ktor:ktor-server-double-receive", version.ref = "ktor" } ktorServerTestHost = { module = "io.ktor:ktor-server-test-host", version.ref = "ktor" } ktorClientCore = { module = "io.ktor:ktor-client-core", version.ref = "ktor" } ktorClientOkHttp = { module = "io.ktor:ktor-client-okhttp", version.ref = "ktor" } ktorClientContentNegociation = { module = "io.ktor:ktor-client-content-negotiation", version.ref = "ktor" } ktorClientMock = { module = "io.ktor:ktor-client-mock", version.ref = "ktor" } +graphqlKotlinServer = { module = "com.expediagroup:graphql-kotlin-server", version.ref = "graphqlKotlin" } +graphqlKotlinKtorServer = { module = "com.expediagroup:graphql-kotlin-ktor-server", version.ref = "graphqlKotlin" } +graphqlKotlinDataLoader = { module = "com.expediagroup:graphql-kotlin-dataloader", version.ref = "graphqlKotlin" } kotlinxSerializationCore = { module = "org.jetbrains.kotlinx:kotlinx-serialization-core", version.ref = "kotlinxSerialization" } kotlinxSerializationJson = { module = "org.jetbrains.kotlinx:kotlinx-serialization-json", version.ref = "kotlinxSerialization" } kotlinxSerializationProtobuf = { module = "org.jetbrains.kotlinx:kotlinx-serialization-protobuf", version.ref = "kotlinxSerialization" } diff --git a/graphql/build.gradle.kts b/graphql/build.gradle.kts new file mode 100644 index 0000000..30bcf68 --- /dev/null +++ b/graphql/build.gradle.kts @@ -0,0 +1,18 @@ +plugins { + id("buildsrc.convention.kotlin-jvm") +} + +dependencies { + implementation(project(":core")) + api(project(":ktor")) + api(libs.graphqlKotlinServer) + api(libs.graphqlKotlinKtorServer) + api(libs.graphqlKotlinDataLoader) + implementation(libs.ktorServerRateLimit) + implementation(libs.ktorServerBodyLimit) + implementation(libs.ktorServerCompression) + implementation(libs.ktorServerDoubleReceive) + testImplementation(kotlin("test")) + testImplementation(libs.bundles.testEcosystem) + testImplementation(project(":ktor-test")) +} diff --git a/settings.gradle.kts b/settings.gradle.kts index fb0466d..1252417 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -18,6 +18,7 @@ include( ":validator", ":ktor", ":ktor-test", + ":graphql", ":cache", ":koin", ":koin-exposed", From 933ec821cc61ce886a12936d68e008fb3d87eefd Mon Sep 17 00:00:00 2001 From: Ziedelth Date: Fri, 2 Oct 2026 22:35:04 +0000 Subject: [PATCH 2/9] feat: bound the graphql cost model and expose its configuration Adds the typed configuration, the static cost estimator and the security bounds. Three of the tests found real holes in the first draft: - repeated fragment spreads were priced once, so a fragment bomb cost the same as a single expansion; a spread now pays every time it appears - the depth walk did not follow spreads, so a document three levels deep could expand to fifty through fragments - the introspection exemption used an OR, which a query mixing schema and business fields could exploit The estimate stays static by design: argument-dependent cost is bounded by the complexity instrumentation and the request timeout instead. --- graphql/src/main/kotlin/CostEstimator.kt | 98 +++++++ graphql/src/main/kotlin/DataLoaders.kt | 36 +++ graphql/src/main/kotlin/GraphQLConfig.kt | 59 ++++ .../main/kotlin/SecurityInstrumentations.kt | 148 ++++++++++ graphql/src/test/kotlin/CostEstimatorTest.kt | 217 +++++++++++++++ graphql/src/test/kotlin/DataLoadersTest.kt | 100 +++++++ graphql/src/test/kotlin/GraphQLConfigTest.kt | 95 +++++++ .../kotlin/SecurityInstrumentationsTest.kt | 252 ++++++++++++++++++ 8 files changed, 1005 insertions(+) create mode 100644 graphql/src/main/kotlin/CostEstimator.kt create mode 100644 graphql/src/main/kotlin/DataLoaders.kt create mode 100644 graphql/src/main/kotlin/GraphQLConfig.kt create mode 100644 graphql/src/main/kotlin/SecurityInstrumentations.kt create mode 100644 graphql/src/test/kotlin/CostEstimatorTest.kt create mode 100644 graphql/src/test/kotlin/DataLoadersTest.kt create mode 100644 graphql/src/test/kotlin/GraphQLConfigTest.kt create mode 100644 graphql/src/test/kotlin/SecurityInstrumentationsTest.kt diff --git a/graphql/src/main/kotlin/CostEstimator.kt b/graphql/src/main/kotlin/CostEstimator.kt new file mode 100644 index 0000000..a42c7f5 --- /dev/null +++ b/graphql/src/main/kotlin/CostEstimator.kt @@ -0,0 +1,98 @@ +package fr.shikkanime.graphql + +import graphql.language.Document +import graphql.language.Field +import graphql.language.FragmentDefinition +import graphql.language.FragmentSpread +import graphql.language.InlineFragment +import graphql.language.OperationDefinition +import graphql.language.Selection +import graphql.language.SelectionSet + +/** + * Estimates the static execution cost of a parsed GraphQL document. + * + * The estimate is a pure function of the document: no schema lookup, no database access. That is + * deliberate, because the cost has to be known *before* execution to feed the rate limiter. + * + * Every selected field costs one unit and contributes the cost of its own selections, so nesting + * and breadth both add up. Aliased fields are visited once per alias, which is what makes an + * aliasing bomb expensive: a few hundred bytes of payload resolve hundreds of times over. + * + * A fragment spread costs its whole selection set **every time it appears**. Counting a repeated + * spread once would be the opposite of what is needed: the fragment bomb is precisely a small + * fragment spread dozens of times, so a shared "already seen" set would price the attack at the + * cost of a single expansion. + * + * Known limitation: the estimate is static. It cannot tell that `episodes(limit: 1000)` costs more + * than `episodes(limit: 10)`, because that depends on the arguments and on the data. Anything + * data-dependent is bounded by the complexity instrumentation and the request timeout instead. + * + * @param document Parsed document to price. + * @return The estimated cost, never below one so a request is never free. + */ +fun estimateCost(document: Document): Int { + val fragments = document.definitions + .filterIsInstance() + .associateBy { fragment -> fragment.name } + + return document.definitions + .filterIsInstance() + .sumOf { operation -> selectionsCost(operation.selectionSet, fragments, HashSet()) } + .coerceAtLeast(1) +} + +private fun selectionsCost( + selectionSet: SelectionSet, + fragments: Map, + expandedFragments: Set +): Int = + selectionSet.selections.sumOf { selection -> selectionCost(selection, fragments, expandedFragments) } + +private fun selectionCost( + selection: Selection<*>, + fragments: Map, + expandedFragments: Set +): Int = + when (selection) { + is Field -> 1 + selection.selectionSet + ?.let { childSet -> selectionsCost(childSet, fragments, expandedFragments) } + .orZero() + + is InlineFragment -> selectionsCost(selection.selectionSet, fragments, expandedFragments) + + is FragmentSpread -> fragmentCost(selection, fragments, expandedFragments) + + else -> 0 + } + +/** + * Cost of one spread: the selections it brings in, or one unit when the fragment is unknown. + * + * The expanded set is copied down the branch rather than mutated, so two sibling spreads of the + * same fragment each pay full price while a fragment that spreads itself still terminates. + */ +private fun fragmentCost( + spread: FragmentSpread, + fragments: Map, + expandedFragments: Set +): Int { + if (spread.name in expandedFragments) { + return 0 + } + + val definition = fragments[spread.name] ?: return 1 + + return selectionsCost(definition.selectionSet, fragments, expandedFragments + spread.name) +} + +private fun Int?.orZero(): Int = this ?: 0 + +/** + * Documents this module is not expected to receive. + * + * Exposed so the cost model stays verifiable: a batch request is a JSON array of operations, and + * graphql-java would otherwise process each element as a separate, unrated document. + */ +internal fun isBatchRequest(body: String): Boolean = + body.trimStart().startsWith("[") diff --git a/graphql/src/main/kotlin/DataLoaders.kt b/graphql/src/main/kotlin/DataLoaders.kt new file mode 100644 index 0000000..fa37d5b --- /dev/null +++ b/graphql/src/main/kotlin/DataLoaders.kt @@ -0,0 +1,36 @@ +package fr.shikkanime.graphql + +import com.expediagroup.graphql.dataloader.KotlinDataLoaderRegistryFactory +import graphql.GraphQLContext +import org.dataloader.DataLoaderRegistry + +/** + * Builds the factory used when a consumer supplies no DataLoader. + * + * `KotlinDataLoaderRegistryFactory` is a final class, so the empty case is expressed by + * instantiating it with no loader rather than by subclassing it. The no-arg constructor is the + * library's own "no loaders" case, which keeps the module working out of the box for a project + * with no N+1 problem. + * + * @return A factory generating an empty registry. + */ +fun noOpDataLoaderRegistryFactory(): KotlinDataLoaderRegistryFactory = + KotlinDataLoaderRegistryFactory() + +/** + * Whether this factory carries at least one DataLoader. + * + * Lets the caller tell "the consumer configured batching" from "the consumer configured nothing", + * which decides whether the N+1 mitigation is actually in place. + */ +fun KotlinDataLoaderRegistryFactory.hasLoaders(): Boolean = + generate(GraphQLContext.newContext().build(), null).keys.isNotEmpty() + +/** + * Generates the DataLoader registry for one request. + * + * @param graphQLContext Context of the running operation, passed to each loader. + * @return The registry holding every loader the consumer declared. + */ +internal fun KotlinDataLoaderRegistryFactory.generateRegistry(): DataLoaderRegistry = + generate(GraphQLContext.newContext().build(), null) diff --git a/graphql/src/main/kotlin/GraphQLConfig.kt b/graphql/src/main/kotlin/GraphQLConfig.kt new file mode 100644 index 0000000..cb2d35b --- /dev/null +++ b/graphql/src/main/kotlin/GraphQLConfig.kt @@ -0,0 +1,59 @@ +package fr.shikkanime.graphql + +import kotlin.time.Duration +import kotlin.time.Duration.Companion.minutes + +/** + * Rate limiting applied to the GraphQL endpoint. + * + * The bucket is token-based: each request consumes as many tokens as the estimated cost of its + * query, so a heavy query drains the bucket faster than a light one. A flat per-request count + * would let a single expensive document through as cheaply as a trivial one. + * + * @property enabled Whether the endpoint is rate limited at all. + * @property limit Bucket capacity, expressed in cost units rather than requests. + * @property refillPeriod How often the bucket is refilled to full. + * @property minimumRequestWeight Weight charged when the query cost cannot be determined, so a + * request is never free. Zero would let a caller bypass the limit entirely. + */ +data class RateLimitConfig( + val enabled: Boolean = true, + val limit: Int = 100, + val refillPeriod: Duration = 1.minutes, + val minimumRequestWeight: Int = 1 +) + +/** + * Configuration of the GraphQL module. + * + * Every value here is a security decision rather than a cosmetic one: raising + * [maxQueryComplexity] or [requestBodyLimit] widens the DoS surface, so a consumer that overrides + * them should know what it is trading away. The defaults are conservative but generous enough for + * a normal admin query; they are starting points to measure, not constants to trust. + * + * @property path Route the GraphQL endpoint is bound to. A missing leading slash is added. + * @property maxQueryDepth Maximum selection nesting accepted before execution is refused. + * @property maxQueryComplexity Maximum estimated cost accepted before execution is refused. + * @property introspectionEnabled Whether the schema can be queried through introspection. Public + * by default, matching the already-public Swagger UI. + * @property rateLimit Rate limiting applied to the endpoint. + * @property requestBodyLimit Maximum accepted request body size, measured on the decoded stream so + * a compressed payload cannot exceed it after inflation. + */ +data class GraphQLConfig( + val path: String = DEFAULT_PATH, + val maxQueryDepth: Int = 8, + val maxQueryComplexity: Int = 200, + val introspectionEnabled: Boolean = true, + val rateLimit: RateLimitConfig = RateLimitConfig(), + val requestBodyLimit: Long = DEFAULT_REQUEST_BODY_LIMIT +) { + /** [path] guaranteed to start with a slash. */ + val normalizedPath: String = + if (path.startsWith("/")) path else "/$path" + + private companion object { + private const val DEFAULT_PATH = "/graphql" + private const val DEFAULT_REQUEST_BODY_LIMIT = 64L * 1024 + } +} diff --git a/graphql/src/main/kotlin/SecurityInstrumentations.kt b/graphql/src/main/kotlin/SecurityInstrumentations.kt new file mode 100644 index 0000000..df710d4 --- /dev/null +++ b/graphql/src/main/kotlin/SecurityInstrumentations.kt @@ -0,0 +1,148 @@ +package fr.shikkanime.graphql + +import graphql.language.Document +import graphql.language.Field +import graphql.language.FragmentDefinition +import graphql.language.FragmentSpread +import graphql.language.InlineFragment +import graphql.language.OperationDefinition +import graphql.language.Selection +import graphql.language.SelectionSet + +/** + * Bounds applied to a GraphQL document before it is executed. + * + * @property maxQueryDepth Maximum selection nesting accepted. + * @property maxQueryComplexity Maximum estimated cost accepted. + * @property introspectionEnabled Whether introspection is served. When false, an introspection + * query is refused even if it stays within the other bounds, because the schema is the thing + * being protected. + */ +data class SecurityLimits( + val maxQueryDepth: Int, + val maxQueryComplexity: Int, + val introspectionEnabled: Boolean = true +) + +/** + * Whether the document only asks for schema metadata. + * + * Introspection is deep and expensive by nature, so it is exempt from the depth and complexity + * bounds when public. Refusing it would break GraphiQL and every codegen client, which is why the + * exemption has to be explicit rather than a lowered threshold. + * + * A document that mixes introspection with business fields is **not** exempt: it would otherwise + * be a way to smuggle a heavy query past the limits. + */ +fun isIntrospectionOnly(document: Document): Boolean { + val operations = document.definitions.filterIsInstance() + + if (operations.isEmpty()) { + return false + } + + return operations.all { operation -> + operation.selectionSet.selections.all { isIntrospectionField(it) } + } +} + +private fun isIntrospectionField(selection: Selection<*>): Boolean = + when (selection) { + is Field -> { + val childSelections = selection.selectionSet + + if (childSelections == null) { + selection.name == "__schema" || selection.name == "__type" + } else { + selection.name == "__schema" || selection.name == "__type" || isIntrospectionField(childSelections) + } + } + + is InlineFragment -> isIntrospectionField(selection.selectionSet) + is FragmentSpread -> false + else -> false + } + +private fun isIntrospectionField(selectionSet: SelectionSet): Boolean = + selectionSet.selections.all { isIntrospectionField(it) } + +/** + * Whether the document breaks one of the [limits]. + * + * @param document Parsed document to check. + * @param limits Bounds to enforce. + * @return `true` when the document must not be executed. + */ +fun exceedsLimits(document: Document, limits: SecurityLimits): Boolean { + if (isIntrospectionOnly(document)) { + return !limits.introspectionEnabled + } + + val cost = estimateCost(document) + val depth = operationsMaxDepth(document) + + return depth > limits.maxQueryDepth || cost > limits.maxQueryComplexity +} + +/** + * Deepest selection nesting of the document, counting from 1 for a root field. + * + * The walk stops at [MAX_TRACKED_DEPTH] rather than recursing further: a document deep enough to + * hit the bound is already refused, so counting the exact depth past that point would cost time on + * exactly the inputs an attacker sends. + */ +private fun operationsMaxDepth(document: Document): Int = + document.definitions + .filterIsInstance() + .maxOfOrNull { depthOf(it.selectionSet, 0, documentFragments(document)) } + ?: 0 + +/** + * Deepest nesting, counting fragment spreads as the depth of the fragment they expand to. + * + * Resolving spreads matters for the limits to mean anything: the fragment bomb nests through + * fragments, so a document whose visible fields look three deep can expand to fifty. Cycle + * tracking is per branch, so a fragment that spreads itself stops instead of looping. + */ +private fun depthOf( + selectionSet: SelectionSet, + current: Int, + fragments: Map = emptyMap(), + expanding: Set = emptySet() +): Int { + if (current >= MAX_TRACKED_DEPTH) { + return current + } + + return selectionSet.selections.maxOfOrNull { selection -> + val childSelections = when (selection) { + is Field -> selection.selectionSet + is InlineFragment -> selection.selectionSet + is FragmentSpread -> { + val definition = fragments[selection.name] + + if (definition == null || selection.name in expanding) { + null + } else { + return@maxOfOrNull depthOf( + definition.selectionSet, + current + 1, + fragments, + expanding + selection.name + ) + } + } + + else -> null + } + + if (childSelections == null) current + 1 else depthOf(childSelections, current + 1, fragments, expanding) + } ?: current + 1 +} + +private fun documentFragments(document: Document): Map = + document.definitions + .filterIsInstance() + .associateBy { fragment -> fragment.name } + +private const val MAX_TRACKED_DEPTH = 64 diff --git a/graphql/src/test/kotlin/CostEstimatorTest.kt b/graphql/src/test/kotlin/CostEstimatorTest.kt new file mode 100644 index 0000000..2c4835e --- /dev/null +++ b/graphql/src/test/kotlin/CostEstimatorTest.kt @@ -0,0 +1,217 @@ +package fr.shikkanime.graphql + +import graphql.parser.Parser +import graphql.language.Document +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.ValueSource +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +class CostEstimatorTest { + private fun parse(query: String): Document = + Parser().parseDocument(query) + + @Nested + @DisplayName("Given a trivial query") + inner class GivenTrivialQuery { + @Test + fun `should cost exactly one`() { + // Given + val document = parse("{ ping }") + + // When + val cost = estimateCost(document) + + // Then + assertEquals(1, cost) + } + } + + @Nested + @DisplayName("Given aliased fields") + inner class GivenAliasedFields { + @Test + fun `should count one unit per alias`() { + // Given — the aliasing bomb: tiny payload, huge execution cost. + val document = parse("{ a: ping b: ping c: ping d: ping e: ping }") + + // When + val cost = estimateCost(document) + + // Then + assertEquals(5, cost) + } + + @Test + fun `should count the same field once when it is not aliased`() { + // Given + val document = parse("{ ping }") + + // When + val cost = estimateCost(document) + + // Then + assertEquals(1, cost) + } + + @Test + fun `should grow linearly with the alias count`() { + // Given + val five = parse("{ a1: ping a2: ping a3: ping a4: ping a5: ping }") + val ten = parse("{ a1: ping a2: ping a3: ping a4: ping a5: ping a6: ping a7: ping a8: ping a9: ping a10: ping }") + + // When + val costFive = estimateCost(five) + val costTen = estimateCost(ten) + + // Then + assertEquals(5, costFive) + assertEquals(10, costTen) + } + } + + @Nested + @DisplayName("Given nested selections") + inner class GivenNestedSelections { + @Test + fun `should add the children of each field`() { + // Given — `episodes` plus its `title`, versus `episodes`, `anime` and `title`. + val shallow = parse("{ episodes { title } }") + val deeper = parse("{ episodes { anime { title } } }") + + // When + val costShallow = estimateCost(shallow) + val costDeeper = estimateCost(deeper) + + // Then + assertEquals(2, costShallow) + assertEquals(3, costDeeper) + } + + @Test + fun `should count two for two sibling fields`() { + // Given + val flat = parse("{ a: ping b: ping }") + + // When + val cost = estimateCost(flat) + + // Then + assertEquals(2, cost) + } + + @Test + fun `should ignore an inline fragment`() { + // An inline fragment is a filter, not extra work: it selects the same fields. + // Given + val document = parse("{ episodes { ... on Episode { title } } }") + + // When + val cost = estimateCost(document) + + // Then + assertEquals(2, cost) + } + } + + @Nested + @DisplayName("Given a fragment") + inner class GivenFragment { + @Test + fun `should count the spread selections`() { + // Given + val document = parse( + """ + query Q { episodes { ...F } } + fragment F on Episode { title number } + """.trimIndent() + ) + + // When + val cost = estimateCost(document) + + // Then + assertEquals(3, cost) + } + + @Test + fun `should not hang on a cyclic spread`() { + // A self-referencing fragment is the recursion bomb; the walk must stop instead of + // overflowing the stack. + // Given + val document = parse( + """ + query Q { episodes { ...F } } + fragment F on Episode { title ...F } + """.trimIndent() + ) + + // When + val cost = estimateCost(document) + + // Then + assertEquals(2, cost) + } + } + + @Nested + @DisplayName("Given a large but shallow document") + inner class GivenLargeButShallowDocument { + @Test + fun `should still report a high cost`() { + // Depth limits do not catch this shape: a thousand flat fields is one level deep but + // a thousand times the work. + // Given + val document = parse("{ " + (1..1000).joinToString(" ") { "a$it: ping" } + " }") + + // When + val cost = estimateCost(document) + + // Then + assertEquals(1000, cost) + } + } + + @Nested + @DisplayName("Given an introspection query") + inner class GivenIntrospectionQuery { + @ParameterizedTest + @ValueSource( + strings = [ + "{ __schema { types { name } } }", + "{ __type(name: \"Episode\") { name fields { name } } }" + ] + ) + fun `should report a positive cost`(query: String) { + // Given + val document = parse(query) + + // When + val cost = estimateCost(document) + + // Then + assertTrue(cost > 0) + } + } + + @Nested + @DisplayName("Given an empty document") + inner class GivenEmptyDocument { + @Test + fun `should never report a zero cost`() { + // A zero weight would make the request free in the rate limiter, which is worse than + // having no rate limit at all. + // Given + val document = parse("query Empty { __typename }") + + // When + val cost = estimateCost(document) + + // Then + assertTrue(cost >= 1, "cost must be at least one, got $cost") + } + } +} diff --git a/graphql/src/test/kotlin/DataLoadersTest.kt b/graphql/src/test/kotlin/DataLoadersTest.kt new file mode 100644 index 0000000..071b5d9 --- /dev/null +++ b/graphql/src/test/kotlin/DataLoadersTest.kt @@ -0,0 +1,100 @@ +package fr.shikkanime.graphql + +import com.expediagroup.graphql.dataloader.KotlinDataLoader +import com.expediagroup.graphql.dataloader.KotlinDataLoaderRegistryFactory +import graphql.GraphQLContext +import org.dataloader.DataLoader +import org.dataloader.DataLoaderFactory +import org.dataloader.DataLoaderOptions +import org.dataloader.DataLoaderRegistry +import org.junit.jupiter.api.DisplayName +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import java.util.concurrent.CompletableFuture +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class DataLoadersTest { + // Fixtures first: everything the tests below rely on. + private val pingLoader = object : KotlinDataLoader { + override val dataLoaderName = "PingDataLoader" + + override fun getDataLoader(graphQLContext: GraphQLContext): DataLoader = + DataLoaderFactory.newDataLoader( + { keys: List -> CompletableFuture.completedFuture(keys) }, + DataLoaderOptions.newOptions().build() + ) + } + + private fun registryOf(factory: KotlinDataLoaderRegistryFactory): DataLoaderRegistry = + factory.generate(GraphQLContext.newContext().build(), null) + + @Nested + @DisplayName("Given no DataLoader registered") + inner class GivenNoDataLoader { + @Test + fun `should generate an empty registry without throwing`() { + // Given + val factory = noOpDataLoaderRegistryFactory() + + // When + val registry = registryOf(factory) + + // Then + assertTrue(registry.keys.isEmpty()) + } + + @Test + fun `should report that it is not usable`() { + // Given + val factory = noOpDataLoaderRegistryFactory() + + // Then + assertFalse(factory.hasLoaders()) + } + } + + @Nested + @DisplayName("Given a consumer registry factory") + inner class GivenConsumerRegistryFactory { + @Test + fun `should expose the consumer loader`() { + // Given + val factory = KotlinDataLoaderRegistryFactory(pingLoader) + + // When + val registry = registryOf(factory) + + // Then + assertTrue(registry.keys.contains("PingDataLoader")) + } + + @Test + fun `should report that it carries loaders`() { + // Given + val factory = KotlinDataLoaderRegistryFactory(pingLoader) + + // Then + assertTrue(factory.hasLoaders()) + } + } + + @Nested + @DisplayName("Given the default factory") + inner class GivenDefaultFactory { + @Test + fun `should register no key at all`() { + // The no-op factory must never register a key, or a resolver looking up a loader by + // name would find nothing while the registry claims to be populated. + // Given + val factory = noOpDataLoaderRegistryFactory() + + // When + val registry = registryOf(factory) + + // Then + assertEquals(0, registry.keys.size) + } + } +} diff --git a/graphql/src/test/kotlin/GraphQLConfigTest.kt b/graphql/src/test/kotlin/GraphQLConfigTest.kt new file mode 100644 index 0000000..4c834f8 --- /dev/null +++ b/graphql/src/test/kotlin/GraphQLConfigTest.kt @@ -0,0 +1,95 @@ +package fr.shikkanime.graphql + +import org.junit.jupiter.api.DisplayName +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +class GraphQLConfigTest { + @Nested + @DisplayName("Given the default configuration") + inner class GivenDefaultConfiguration { + private val config = GraphQLConfig() + + @Test + fun `should expose a usable endpoint path`() { + // Then + assertTrue(config.path.isNotBlank()) + } + + @Test + fun `should keep the security limits positive`() { + // Then + assertTrue(config.maxQueryDepth > 0) + assertTrue(config.maxQueryComplexity > 0) + assertTrue(config.requestBodyLimit > 0) + } + + @Test + fun `should leave introspection public`() { + // Then + assertTrue(config.introspectionEnabled) + } + + @Test + fun `should apply a rate limit by default`() { + // Then + assertTrue(config.rateLimit.enabled) + assertTrue(config.rateLimit.limit > 0) + } + } + + @Nested + @DisplayName("Given a custom configuration") + inner class GivenCustomConfiguration { + @Test + fun `should override every value`() { + // Given + val config = GraphQLConfig( + path = "/internal/graphql", + maxQueryDepth = 3, + maxQueryComplexity = 42, + introspectionEnabled = false, + requestBodyLimit = 1024, + rateLimit = RateLimitConfig(limit = 5, enabled = false) + ) + + // Then + assertEquals("/internal/graphql", config.path) + assertEquals(3, config.maxQueryDepth) + assertEquals(42, config.maxQueryComplexity) + assertEquals(false, config.introspectionEnabled) + assertEquals(1024L, config.requestBodyLimit) + assertEquals(false, config.rateLimit.enabled) + assertEquals(5, config.rateLimit.limit) + } + } + + @Nested + @DisplayName("Given a path without a leading slash") + inner class GivenPathWithoutLeadingSlash { + @Test + fun `should normalize it`() { + // Given + val config = GraphQLConfig(path = "graphql") + + // Then + assertEquals("/graphql", config.normalizedPath) + assertEquals("graphql", config.path, "the raw value must stay untouched") + } + } + + @Nested + @DisplayName("Given a path with a leading slash") + inner class GivenPathWithLeadingSlash { + @Test + fun `should keep it as-is`() { + // Given + val config = GraphQLConfig(path = "/internal/graphql") + + // Then + assertEquals("/internal/graphql", config.normalizedPath) + } + } +} diff --git a/graphql/src/test/kotlin/SecurityInstrumentationsTest.kt b/graphql/src/test/kotlin/SecurityInstrumentationsTest.kt new file mode 100644 index 0000000..3328baf --- /dev/null +++ b/graphql/src/test/kotlin/SecurityInstrumentationsTest.kt @@ -0,0 +1,252 @@ +package fr.shikkanime.graphql + +import graphql.parser.Parser +import graphql.parser.exceptions.ParseCancelledTooDeepException +import org.junit.jupiter.api.DisplayName +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertThrows +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class SecurityInstrumentationsTest { + // Fixtures first. + private val limits = SecurityLimits( + maxQueryDepth = 6, + maxQueryComplexity = 100, + introspectionEnabled = true + ) + + private fun isRefused(query: String, securityLimits: SecurityLimits = limits): Boolean = + exceedsLimits( + document = Parser().parseDocument(query), + limits = securityLimits + ) + + private fun aliasFields(count: Int): String = + (1..count).joinToString(" ") { index -> "a$index: ping" } + + private fun aliasBomb(count: Int): String = + "{ ${aliasFields(count)} }" + + private fun deepQuery(depth: Int): String { + var query = "" + repeat(depth) { query += "{ nested" } + query += " { ping }" + repeat(depth) { query += " }" } + + return query + } + + @Nested + @DisplayName("Given a hostile document") + inner class GivenHostileDocument { + @Test + fun `should refuse a deeply nested query`() { + // Given + val query = deepQuery(depth = 20) + + // Then + assertTrue(isRefused(query), "a depth-20 query must be refused") + } + + @Test + fun `should refuse an alias bomb`() { + // Given — 200 aliases of the same field in a few kilobytes. + val query = aliasBomb(count = 200) + + // Then + assertTrue(isRefused(query), "an alias bomb must be refused") + } + + @Test + fun `should refuse a fragment bomb`() { + // Given — nested fragment spreads, the classic graphql-java recursion attack. + val query = buildString { + append("query Q { ") + repeat(60) { append("f$it: episodes { ...F$it } ") } + append("}") + repeat(60) { append("fragment F$it on Episode { ...F${it + 1} } ") } + append("fragment F60 on Episode { title }") + } + + // Then + assertTrue(isRefused(query), "a fragment bomb must be refused") + } + } + + @Nested + @DisplayName("Given a legitimate document") + inner class GivenLegitimateDocument { + @Test + fun `should accept a normal query`() { + // Given + val query = "{ episodes { number title anime { title } } }" + + // Then + assertFalse(isRefused(query)) + } + + @Test + fun `should accept it even with tight limits`() { + // Given — the limits must not make GraphQL unusable. + val tight = SecurityLimits(maxQueryDepth = 3, maxQueryComplexity = 50) + + // Then + assertFalse(isRefused("{ episodes { title } }", tight)) + } + } + + @Nested + @DisplayName("Given introspection") + inner class GivenIntrospection { + @Test + fun `should survive the depth and complexity limits`() { + // The introspection query is deep and complex by nature; refusing it breaks GraphiQL + // and every codegen client. + // Given + val query = "{ __schema { types { name fields { name } } } }" + + // Then + assertFalse(isRefused(query)) + } + + @Test + fun `should be detected as introspection`() { + // Given + val schemaQuery = Parser().parseDocument("{ __schema { types { name } } }") + val typeQuery = Parser().parseDocument("{ __type(name: \"X\") { name } }") + val business = Parser().parseDocument("{ episodes { title } }") + + // Then + assertTrue(isIntrospectionOnly(schemaQuery)) + assertTrue(isIntrospectionOnly(typeQuery)) + assertFalse(isIntrospectionOnly(business)) + } + + @Test + fun `should be refused when disabled`() { + // Given + val closed = SecurityLimits(maxQueryDepth = 6, maxQueryComplexity = 100, introspectionEnabled = false) + + // Then + assertTrue(isRefused("{ __schema { types { name } } }", closed)) + } + + @Test + fun `should not exempt a query that mixes introspection and business fields`() { + // The smuggling case: claiming introspection to skip the limits while actually asking + // for data. An OR would be a hole; the check must be strict. + // Given + val mixed = "{ __schema { types { name } } episodes { title } }" + + // Then + assertFalse(isIntrospectionOnly(Parser().parseDocument(mixed))) + } + + @Test + fun `should refuse the mixed query on its own merits`() { + // Given — deep enough to break the limit, disguised as introspection. + val mixed = "{ __schema { types { name } } " + "episodes { ".repeat(12) + "title" + " }".repeat(12) + " }" + + // Then + assertTrue(isRefused(mixed)) + } + } + + @Nested + @DisplayName("Given multiple operations in one document") + inner class GivenMultipleOperations { + @Test + fun `should refuse when any operation breaks the limits`() { + // A client sends one document holding several operations and picks one by name. The + // bounds must apply to the whole document, not just the selected operation, or an + // attacker attaches a bomb as a second operation and calls it by `operationName`. + // Given — the cheap operation first, the bomb second: passing `operationName` must + // not let the bomb through. + val bomb = aliasFields(count = 200) + val document = "query Cheap { ping } query Expensive { $bomb }" + + // Then + assertTrue(isRefused(document)) + } + + @Test + fun `should accept when every operation is within the limits`() { + // Given + val document = "query A { ping } query B { episodes { title } }" + + // Then + assertFalse(isRefused(document)) + } + } + + @Nested + @DisplayName("Given a mutation") + inner class GivenMutation { + @Test + fun `should be bounded like a query`() { + // A mutation writes, so it must never be cheaper to abuse than a read. Same document + // shape as the alias-bomb query, declared as a mutation. + // Given + val document = "mutation M { ${aliasFields(count = 200)} }" + + // Then + assertTrue(isRefused(document)) + } + + @Test + fun `should accept a normal mutation`() { + // Given + val document = "mutation M { saveEpisode(id: 1) { id } }" + + // Then + assertFalse(isRefused(document)) + } + } + + @Nested + @DisplayName("Given a conditional field") + inner class GivenConditionalField { + @Test + fun `should be priced even though a directive may skip it`() { + // `@include(if: false)` does not run the resolver, but the caller controls the + // variable, so the field may well run. Pricing the static document is the safe choice: + // it over-charges a query that may be cheap rather than under-charges one that is not. + // Given — 200 conditionally included aliases, skippable at will. + val fields = (1..200).joinToString(" ") { index -> "a$index: ping @include(if: \$show)" } + val document = "query Q(\$show: Boolean!) { $fields }" + + // Then + assertTrue(isRefused(document)) + } + } + + @Nested + @DisplayName("Given an unbounded nesting") + inner class GivenUnboundedNesting { + @Test + fun `should be refused by the parser before the limits are reached`() { + // graphql-java caps nesting at 500 grammar levels and cancels parsing, so a document + // that deep never reaches our checks. The defence holds, just one layer earlier. + // Given + val query = deepQuery(depth = 5_000) + + // Then + assertThrows { + Parser().parseDocument(query) + } + } + + @Test + fun `should refuse a nesting that parses but breaks the limit`() { + // Just under the parser cap, so the document is valid and the depth check is what + // rejects it. + // Given + val query = deepQuery(depth = 20) + + // Then + assertTrue(isRefused(query)) + } + } +} From cbbaae6355cac3c9958b3d6b7974a0b63683da31 Mon Sep 17 00:00:00 2001 From: Ziedelth Date: Fri, 2 Oct 2026 22:54:35 +0000 Subject: [PATCH 3/9] feat: serve graphql behind rate limit, body limit and compression Binds the endpoint with the full security stack and wires the engine. Two ordering bugs surfaced while wiring it, both caught by the route tests: - the refusal check was registered as a sibling POST handler, so the plugin's own handler won by registration order and a 200-alias bomb was executed instead of refused; the guard is now an interception on the route, which runs before any handler - DoubleReceive is route-scoped, so it had to move to the GraphQL route; on the application it would not have cached the body requestWeight reads A refused document answers 400 with a GraphQL errors array rather than the 200 GraphQL uses for execution errors: it never reached the engine, so a caller branching on the status code must be able to tell the two apart. 50 tests. --- graphql/src/main/kotlin/GraphQLModules.kt | 267 ++++++++++++++++++ graphql/src/test/kotlin/GraphQLModulesTest.kt | 237 ++++++++++++++++ 2 files changed, 504 insertions(+) create mode 100644 graphql/src/main/kotlin/GraphQLModules.kt create mode 100644 graphql/src/test/kotlin/GraphQLModulesTest.kt diff --git a/graphql/src/main/kotlin/GraphQLModules.kt b/graphql/src/main/kotlin/GraphQLModules.kt new file mode 100644 index 0000000..c33d9af --- /dev/null +++ b/graphql/src/main/kotlin/GraphQLModules.kt @@ -0,0 +1,267 @@ +package fr.shikkanime.graphql + +import com.expediagroup.graphql.dataloader.KotlinDataLoaderRegistryFactory +import com.expediagroup.graphql.server.ktor.DefaultKtorGraphQLContextFactory +import com.expediagroup.graphql.server.ktor.GraphQL +import com.expediagroup.graphql.server.ktor.graphQLPostRoute +import com.expediagroup.graphql.server.operations.Mutation +import com.expediagroup.graphql.server.operations.Query +import com.expediagroup.graphql.server.operations.Subscription +import fr.shikkanime.core.LoggerFactory +import java.util.logging.Logger +import graphql.language.Document +import graphql.parser.InvalidSyntaxException +import graphql.parser.Parser +import graphql.parser.exceptions.ParseCancelledTooDeepException +import io.ktor.http.ContentType +import io.ktor.http.HttpStatusCode +import io.ktor.server.application.Application +import io.ktor.server.application.ApplicationCallPipeline +import io.ktor.server.application.PipelineCall +import io.ktor.server.application.ApplicationCall +import io.ktor.server.application.install +import io.ktor.server.plugins.bodylimit.RequestBodyLimit +import io.ktor.server.plugins.compression.Compression +import io.ktor.server.plugins.compression.gzip +import io.ktor.server.plugins.doublereceive.DoubleReceive +import io.ktor.server.plugins.ratelimit.RateLimit +import io.ktor.server.plugins.ratelimit.RateLimitName +import io.ktor.server.plugins.ratelimit.rateLimit +import io.ktor.server.request.receiveText +import io.ktor.server.response.respondText +import io.ktor.server.routing.intercept +import io.ktor.util.pipeline.PipelineContext +import io.ktor.server.routing.route +import io.ktor.server.routing.routing +import kotlinx.serialization.json.Json +import kotlinx.serialization.json.JsonPrimitive +import kotlinx.serialization.json.jsonObject +import kotlinx.serialization.json.jsonPrimitive + +/** Name of the rate limiter guarding the GraphQL endpoint. */ +internal const val GRAPHQL_RATE_LIMIT_NAME = "graphql" + +private val REQUEST_JSON = Json { ignoreUnknownKeys = true } + +/** + * Installs the GraphQL module: engine, schema, route, and the security stack guarding it. + * + * The order of the guards is deliberate. The body limit runs first, so an oversized payload never + * reaches the parser. The document is then priced and charged to the rate limit bucket, so a heavy + * query drains it faster than a light one. Only after that is the query executed. + * + * What the rate limit is **not**: a guard standing in front of the analysis. The weight comes from + * a document that has already been parsed, so pricing happens after parsing, not before. It limits + * sustained abuse, while the complexity and depth bounds refuse the single hostile query. Both are + * needed — bounds alone let an attacker trickle expensive queries, and rate limiting alone never + * notices that one query is a bomb. + * + * @property config Bounds and endpoint configuration. + * @property dataLoaderRegistryFactory DataLoaders the consumer provides. The framework only wires + * them: it has no repository and cannot know what to batch. + * @property schemaQueries Queries exposed by the schema. + * @property schemaMutations Mutations exposed by the schema. + * @property schemaSubscriptions Subscriptions exposed by the schema. Websocket transport is not + * enabled by this module, so declaring one would advertise an endpoint that cannot serve it. + * @property schemaPackages Packages scanned for annotated types. The generator refuses to build + * a schema without one, so it is required even though listing the queries explicitly is enough. + */ +fun Application.configureGraphQL( + config: GraphQLConfig = GraphQLConfig(), + dataLoaderRegistryFactory: KotlinDataLoaderRegistryFactory = noOpDataLoaderRegistryFactory(), + schemaPackages: List, + schemaQueries: List = emptyList(), + schemaMutations: List = emptyList(), + schemaSubscriptions: List = emptyList() +) { + val logger = LoggerFactory.getLogger() + val limits = config.toSecurityLimits() + + // Measured on the decoded stream, so a compressed payload cannot inflate past the limit. + install(RequestBodyLimit) { + bodyLimit { config.requestBodyLimit } + } + + install(Compression) { + gzip() + } + + install(GraphQL) { + schema { + packages = schemaPackages + queries = schemaQueries + mutations = schemaMutations + subscriptions = schemaSubscriptions + } + + engine { + this.dataLoaderRegistryFactory = dataLoaderRegistryFactory + } + + server { + this.contextFactory = DefaultKtorGraphQLContextFactory() + } + } + + if (config.rateLimit.enabled) { + install(RateLimit) { + register(RateLimitName(GRAPHQL_RATE_LIMIT_NAME)) { + rateLimiter(limit = config.rateLimit.limit, refillPeriod = config.rateLimit.refillPeriod) + requestKey { call -> call.request.local.remoteHost } + requestWeight { call, _ -> call.graphqlWeight(config, limits, logger) } + } + } + } + + routing { + rateLimit(RateLimitName(GRAPHQL_RATE_LIMIT_NAME)) { + // The plugin registers its own POST handler, so a sibling `post { }` would be tried + // first only by luck of registration order. The guard is therefore an interception on + // the route, which runs before any handler: a document that breaks the bounds is + // answered here and the engine never sees it. + intercept(ApplicationCallPipeline.Call) { + val call = context + val requestPath = call.request.local.uri.substringBefore('?') + + if (!isGraphQLPath(requestPath, config.normalizedPath)) { + proceed() + return@intercept + } + + val document = parseOrNull(call.receiveText(), logger) + + if (document != null && exceedsLimits(document, limits)) { + respondRefusal(refusalMessage(document, limits)) + finish() + } else { + proceed() + } + } + + route(config.normalizedPath) { + // requestWeight reads the body to price the query, which would otherwise consume + // the one-shot request stream and leave the GraphQL handler with an empty body. + // The in-memory cache suits a GraphQL body: small and short-lived, so a file cache + // would be pure overhead. + install(DoubleReceive) { + maxSize(config.requestBodyLimit) + useFileForCache { false } + } + } + + graphQLPostRoute(config.normalizedPath) + } + } +} + +/** + * Whether the request targets the GraphQL endpoint. + * + * Compared on the exact path rather than a prefix, so a sibling route such as `/graphqli` is not + * dragged through the GraphQL guards. + */ +private fun isGraphQLPath(requestPath: String, graphqlPath: String): Boolean = + requestPath == graphqlPath || requestPath == "$graphqlPath/" + +/** Bounds derived from the endpoint configuration. */ +internal fun GraphQLConfig.toSecurityLimits(): SecurityLimits = + SecurityLimits( + maxQueryDepth = maxQueryDepth, + maxQueryComplexity = maxQueryComplexity, + introspectionEnabled = introspectionEnabled + ) + +/** + * Answers a refused document. + * + * The body is a GraphQL `errors` array so a GraphQL client parses it like any other GraphQL + * response. The status is 400 rather than the 200 GraphQL uses for execution errors: this document + * never reached the engine, so it is a rejected request, not a failed execution — and a caller + * branching on the status code must be able to tell the two apart. + */ +private suspend fun PipelineContext.respondRefusal(message: String) { + context.respondText( + text = buildString { + append("""{"errors":[{"message":""") + append(REQUEST_JSON.encodeToString(JsonPrimitive(message).toString())) + append("}]}") + }, + contentType = ContentType.Application.Json, + status = HttpStatusCode.BadRequest + ) +} + +/** + * Cost charged to the rate limit bucket for one request. + * + * A document that cannot be priced — a batch array, invalid syntax, or a query too deep for the + * parser — is charged the whole bucket, so no caller gets cheap work by sending something + * unpriceable. A document that breaks the bounds is charged the whole bucket too: refusing it is + * far cheaper than running it, and the caller should feel the difference. The weight is never zero, + * because a zero-weight request is unlimited — worse than having no rate limit at all. + */ +private suspend fun ApplicationCall.graphqlWeight( + config: GraphQLConfig, + limits: SecurityLimits, + logger: Logger +): Int { + val body = readCachedText() ?: return config.rateLimit.minimumRequestWeight + val document = parseOrNull(body, logger) + + if (document == null || exceedsLimits(document, limits)) { + return maxOf(config.rateLimit.minimumRequestWeight, config.rateLimit.limit) + } + + return estimateCost(document).coerceAtLeast(config.rateLimit.minimumRequestWeight) +} + +/** Reads the cached body, which DoubleReceive made re-readable. */ +private suspend fun ApplicationCall.readCachedText(): String? = + runCatching { receiveText() }.getOrNull() + +/** + * Parses the query carried by a GraphQL request body. + * + * Returns null for a batch request, an unparseable body, or a body carrying no query. Each of + * those is charged the maximum weight upstream, so this function only has to be honest about + * failure, not about the consequence. + */ +private fun parseOrNull(body: String, logger: Logger): Document? { + if (isBatchRequest(body)) { + return null + } + + val query = body.extractQuery() ?: return null + + return try { + Parser().parseDocument(query) + } catch (exception: InvalidSyntaxException) { + logger.warning("Refusing a malformed graphql document: ${exception.message}") + null + } catch (exception: ParseCancelledTooDeepException) { + logger.warning("Refusing a graphql document too deep to parse") + null + } +} + +/** Message describing why a document was refused, safe to return to a client. */ +private fun refusalMessage(document: Document, limits: SecurityLimits): String { + return if (estimateCost(document) > limits.maxQueryComplexity) { + "Query refused: its estimated cost exceeds the maximum of ${limits.maxQueryComplexity}" + } else { + "Query refused: its nesting depth exceeds the maximum of ${limits.maxQueryDepth}" + } +} + +/** + * Reads the `query` member of a GraphQL request body. + * + * @return The query text, or null when the body is not a JSON object carrying one. + */ +internal fun String.extractQuery(): String? { + val queryField = runCatching { + REQUEST_JSON.parseToJsonElement(this).jsonObject["query"] + }.getOrNull() ?: return null + + return runCatching { queryField.jsonPrimitive.content }.getOrNull() +} diff --git a/graphql/src/test/kotlin/GraphQLModulesTest.kt b/graphql/src/test/kotlin/GraphQLModulesTest.kt new file mode 100644 index 0000000..af705ad --- /dev/null +++ b/graphql/src/test/kotlin/GraphQLModulesTest.kt @@ -0,0 +1,237 @@ +package fr.shikkanime.graphql + +import com.expediagroup.graphql.server.operations.Query +import com.expediagroup.graphql.server.ktor.DefaultKtorGraphQLContextFactory +import io.ktor.client.request.header +import io.ktor.client.request.post +import io.ktor.client.request.setBody +import io.ktor.client.statement.HttpResponse +import io.ktor.client.statement.bodyAsChannel +import io.ktor.client.statement.bodyAsText +import io.ktor.utils.io.jvm.javaio.toInputStream +import io.ktor.http.ContentType +import io.ktor.http.HttpHeaders +import io.ktor.http.HttpStatusCode +import io.ktor.http.contentType +import io.ktor.server.testing.ApplicationTestBuilder +import io.ktor.server.testing.testApplication +import org.junit.jupiter.api.DisplayName +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue +import java.util.zip.GZIPInputStream +import kotlin.time.Duration.Companion.minutes + +class GraphQLModulesTest { + // Fixtures first: everything the tests below rely on. + // graphql-java rejects a non-public query type, so the fixtures must stay public. + private val config = GraphQLConfig( + maxQueryDepth = 6, + maxQueryComplexity = 100, + requestBodyLimit = 4 * 1024, + rateLimit = RateLimitConfig(limit = 5, refillPeriod = 5.minutes) + ) + + class PingQuery : Query { + fun ping(): String = "pong" + } + + class NestedQuery : Query { + fun episodes(): EpisodeQuery = EpisodeQuery() + } + + class EpisodeQuery { + fun ping(): String = "pong" + } + + private fun ApplicationTestBuilder.graphqlApplication(config: GraphQLConfig = this@GraphQLModulesTest.config) { + application { + configureGraphQL( + config = config, + schemaPackages = listOf("fr.shikkanime.graphql"), + schemaQueries = listOf(PingQuery(), NestedQuery()) + ) + } + } + + private suspend fun ApplicationTestBuilder.post(body: String): HttpResponse = + client.post(config.normalizedPath) { + contentType(ContentType.Application.Json) + setBody(body) + } + + private suspend fun ApplicationTestBuilder.post(body: String, acceptEncoding: String): HttpResponse = + client.post(config.normalizedPath) { + contentType(ContentType.Application.Json) + header(HttpHeaders.AcceptEncoding, acceptEncoding) + setBody(body) + } + + private fun query(vararg fields: String): String { + val selection = if (fields.isEmpty()) "ping" else fields.joinToString(" ") + return """{"query":"{ $selection }"}""" + } + + private fun aliasFields(count: Int): String = + (1..count).joinToString(" ") { index -> "a$index: ping" } + + private suspend fun ApplicationTestBuilder.bodyOf(response: HttpResponse): String = + response.bodyAsText() + + @Nested + @DisplayName("Given a normal query") + inner class GivenNormalQuery { + @Test + fun `should be answered`() = testApplication { + // Given + graphqlApplication() + + // When + val response = post(query()) + + // Then + assertEquals(HttpStatusCode.OK, response.status) + assertTrue(bodyOf(response).contains("pong")) + } + + @Test + fun `should serve a gzip request`() = testApplication { + // Compression stays on: the body limit measures the decoded stream, so a compressed + // payload cannot slip past it. Ktor compresses the response, so the client has to + // decompress it — which also proves the plugin decompressed the *request* correctly + // rather than handing the engine raw bytes. + // Given + graphqlApplication() + + // When + val response = client.post(config.normalizedPath) { + contentType(ContentType.Application.Json) + header(HttpHeaders.AcceptEncoding, "gzip") + setBody(query()) + } + + // Then + assertEquals(HttpStatusCode.OK, response.status) + assertEquals("gzip", response.headers[HttpHeaders.ContentEncoding], "response must be compressed") + + val decompressed = GZIPInputStream(response.bodyAsChannel().toInputStream()) + .readBytes() + .decodeToString() + + assertTrue(decompressed.contains("pong"), "got $decompressed") + } + } + + @Nested + @DisplayName("Given introspection") + inner class GivenIntrospection { + @Test + fun `should be public`() = testApplication { + // The Swagger UI is already public, so the GraphQL schema is too. + // Given + graphqlApplication() + + // When + val response = post("""{"query":"{ __schema { types { name } } }"}""") + + // Then + assertEquals(HttpStatusCode.OK, response.status) + assertTrue(bodyOf(response).contains("__schema")) + } + } + + @Nested + @DisplayName("Given an oversized body") + inner class GivenOversizedBody { + @Test + fun `should be refused`() = testApplication { + // Given — larger than the configured 4 KiB limit. + graphqlApplication() + + // When + val padding = "x".repeat(8 * 1024) + val response = post("""{"query":"{ ping }","variables":{"pad":"$padding"}}""") + + // Then + assertEquals(HttpStatusCode.PayloadTooLarge, response.status) + } + } + + @Nested + @DisplayName("Given a hostile document") + inner class GivenHostileDocument { + @Test + fun `should refuse an alias bomb`() = testApplication { + // Given + graphqlApplication() + + // When + val response = post(query(*aliasFields(200).split(" ").toTypedArray())) + + // Then + assertTrue( + bodyOf(response).contains("errors"), + "expected a refusal, got ${response.status} ${bodyOf(response)}" + ) + } + + @Test + fun `should refuse a deep document`() = testApplication { + // Given + graphqlApplication() + + // When + val deep = "episodes { ".repeat(12) + "ping" + " }".repeat(12) + val response = post("""{"query":"{ $deep }"}""") + + // Then + assertTrue( + bodyOf(response).contains("errors"), + "expected a refusal, got ${response.status} ${bodyOf(response)}" + ) + } + } + + @Nested + @DisplayName("Given the rate limiter") + inner class GivenRateLimiter { + @Test + fun `should answer 429 once the bucket is empty`() = testApplication { + // The bucket holds `limit` cost units and refills on a 5 minute period, so a ping + // costs one and the sixth request inside the period has nothing left. + // Given + graphqlApplication() + + // When + val statuses = (1..config.rateLimit.limit + 2).map { post(query()).status } + + // Then + assertEquals(HttpStatusCode.TooManyRequests, statuses.last()) + } + + @Test + fun `should charge a heavy document more than a light one`() = testApplication { + // This is the point of requestWeight: a bucket of 5 units fits five pings but only + // two nested documents, since each costs two. + // Given + graphqlApplication() + + // When + val lightStatuses = (1..config.rateLimit.limit).map { post(query()).status } + val heavyStatuses = (1..config.rateLimit.limit).map { + post(query("episodes { ping }")).status + } + + // Then + assertTrue( + lightStatuses.none { it == HttpStatusCode.TooManyRequests }, + "light queries must fit the bucket, got $lightStatuses" + ) + assertTrue( + heavyStatuses.any { it == HttpStatusCode.TooManyRequests }, + "heavy queries must exhaust it faster, got $heavyStatuses" + ) + } + } +} From 6d79b28a61ce89c575ab8eb66d90cb8515fadb92 Mon Sep 17 00:00:00 2001 From: Ziedelth Date: Fri, 2 Oct 2026 23:13:39 +0000 Subject: [PATCH 4/9] fix: close the bypasses the security review found Five holes, each covered by a new test: - isIntrospectionField accepted a business field merely containing a meta field, so a heavy query could ride under the introspection exemption - a batch request was priced at the maximum but still executed, since the guard skips when no single document can be parsed; now refused outright - an unparseable document was let through, which is the exact shape used to slip past a check that only understands valid syntax; now refused - rateLimit.enabled = false crashed at startup, because the route still referenced a limiter that was never registered - the refusal message blamed depth when introspection was the actual reason Also reads the query from the query string on a GET, so that path is no longer unchecked, and reports the bound that actually broke. The route body moved to bindGraphQLRoute so the rate-limited and opt-out branches share it instead of duplicating it. 58 tests. --- AGENTS.md | 4 +- graphql/AGENTS.md | 85 +++++++++ graphql/src/main/kotlin/GraphQLModules.kt | 178 ++++++++++++++---- .../main/kotlin/SecurityInstrumentations.kt | 24 ++- graphql/src/test/kotlin/CostEstimatorTest.kt | 5 +- graphql/src/test/kotlin/GraphQLModulesTest.kt | 97 +++++++++- .../kotlin/SecurityInstrumentationsTest.kt | 9 +- guidelines/ARCHITECTURE.md | 18 ++ 8 files changed, 364 insertions(+), 56 deletions(-) create mode 100644 graphql/AGENTS.md diff --git a/AGENTS.md b/AGENTS.md index 50a7333..6d6d437 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,7 +8,7 @@ This file contains your core, non-negotiable root rules. For detailed framework The project is structured into distinct, decoupled framework modules. **Do not violate module boundaries or introduce circular dependencies.** -- **Dependencies Flow:** `core` (standalone) <- `cache`, `exposed` & `validator` <- `ktor` ; `koin` (standalone dependency carrier) <- `koin-exposed` ; `ktor-test` (standalone dependency carrier) ; `plugin` (Gradle convention plugins) +- **Dependencies Flow:** `core` (standalone) <- `cache`, `exposed` & `validator` <- `ktor` <- `graphql` ; `koin` (standalone dependency carrier) <- `koin-exposed` ; `ktor-test` (standalone dependency carrier) ; `plugin` (Gradle convention plugins) - **`core`**: Base logging (`LoggerFactory`), utilities. MUST remain zero-dependency relative to other submodules. - **`cache`**: Two-level caching engine (`Cache`), L1 LRU memory cache (`L1Cache`), L2 Valkey client (`ValkeyWrapper`), CBOR binary serialization (`BinaryCodec`), bucket versioning, and single-flight loader deduplication (`SingleFlight`). - **`exposed`**: Database connection management (`DatabaseWrapper`), Exposed ORM integration, Liquibase migrations, `@Transactional` annotations & proxying (`TransactionalProxy`), and repository base classes (`AbstractRepository`). @@ -17,6 +17,7 @@ The project is structured into distinct, decoupled framework modules. **Do not v - **`koin`**: Dependency carrier publishing the Koin runtime APIs (`koin-bom`, `koin-core`, `koin-annotations`) via `api(...)`; contains no source code. - **`koin-exposed`**: Koin bridge for transactions — `applyTransactionalProxies()` post-processor replacing eligible singleton definitions with `TransactionalProxy`-wrapping factories; depends on `exposed` and the `koin` carrier. - **`ktor-test`**: Dependency carrier publishing the Ktor server test host (`ktor-server-test-host`) and the Ktor client mock engine (`ktor-client-mock`) via `api(...)`; contains no source code. +- **`graphql`**: GraphQL engine wiring (`graphql-kotlin` on `graphql-java`), the `/graphql` route, the security stack guarding it (body limit, compression, `DoubleReceive`, rate limit charged by query cost), the static cost estimator and the document bounds, plus the orchestration of consumer-provided DataLoaders. It MUST NOT depend on `exposed`: the framework has no repository, so DataLoaders are consumer-owned. - **`plugin`**: Gradle convention plugins for downstream projects (e.g. `fr.shikkanime.framework.ktor`, `fr.shikkanime.framework.koin`). For detailed architectural principles, read the [Architecture Guide](guidelines/ARCHITECTURE.md). @@ -55,6 +56,7 @@ Each framework module has its own dedicated `AGENTS.md` specifying module-specif - [`ktor-test/AGENTS.md`](ktor-test/AGENTS.md): Ktor server test host dependency carrier (no source code). - [`koin/AGENTS.md`](koin/AGENTS.md): Koin dependency carrier (no source code). - [`koin-exposed/AGENTS.md`](koin-exposed/AGENTS.md): Koin bridge for `@Transactional` auto-proxying. +- [`graphql/AGENTS.md`](graphql/AGENTS.md): GraphQL engine, endpoint security stack, cost model, and the DataLoader boundary. - [`plugin/AGENTS.md`](plugin/AGENTS.md): Gradle convention plugins for downstream projects. ## 5. Before Submitting Changes diff --git a/graphql/AGENTS.md b/graphql/AGENTS.md new file mode 100644 index 0000000..91879b6 --- /dev/null +++ b/graphql/AGENTS.md @@ -0,0 +1,85 @@ +# Module Rules: GraphQL (`graphql`) + +This file contains specific rules for the `graphql` submodule. All agents working within this +submodule must strictly adhere to these guidelines in addition to the root [`AGENTS.md`](../AGENTS.md). + +## Module Purpose & Scope + +The `graphql` module serves the same use cases as REST over a second transport, using +`graphql-kotlin` (ExpediaGroup) on top of `graphql-java`. It owns the GraphQL engine wiring, the +schema configuration, the `/graphql` route, the security stack that guards it, and the +orchestration of consumer-provided DataLoaders. + +It does **not** own business logic, repositories, or resolvers. Those belong to the consumer. + +## 1. Module Hierarchy & Dependencies + +- `graphql` depends on `core`, `ktor`, and the graphql-kotlin libraries. +- `graphql` **MUST NOT** depend on `exposed`. The framework has no repository and cannot know what + to batch; a DataLoader that did would be a framework coupled to a domain that changes. +- Consumers that need DataLoaders declare a `KotlinDataLoaderRegistryFactory` instance in Koin and + pass it to `configureGraphQL`. Nothing else. + +## 2. Security Stack — non-negotiable + +The endpoint must carry all four guards. Each one covers a vector the others do not: + +| Guard | Covers | +|---|---| +| `RequestBodyLimit` | Oversized payload, measured on the **decoded** stream so compression cannot inflate past it | +| `Compression` | Response size; safe alongside the body limit precisely because that limit is measured after decoding | +| `DoubleReceive` | Makes the body re-readable, which `requestWeight` needs; without it every request breaks | +| `RateLimit` + `requestWeight` | Sustained abuse, charged by query cost rather than by request count | + +Plus the document bounds, enforced by an interception on the route: + +- `SecurityLimits.maxQueryDepth` +- `SecurityLimits.maxQueryComplexity` +- introspection exemption, when `introspectionEnabled` + +**The refusal guard must stay an interception.** Registering it as a sibling `post { }` handler +lets the plugin's own handler win by registration order, which silently executes the hostile +document instead of refusing it. This was a real bug, caught by the route tests. + +**`requestWeight` charges the whole bucket for an unpriceable document** — a batch array, invalid +syntax, a query too deep for the parser, or one that breaks the bounds. Never zero: a zero-weight +request is unlimited, which is worse than no rate limit at all. + +## 3. Cost Model Rules + +`estimateCost` is a pure function of the document: no schema lookup, no database access. The cost +must be knowable *before* execution. + +- Every field costs one unit, plus the cost of its own selections. +- A **fragment spread pays every time it appears**. A shared "already seen" set would price the + fragment bomb at the cost of a single expansion — the opposite of what is needed. +- The depth walk resolves spreads, so a document three levels deep cannot hide a fifty-level + expansion. +- Cycles are tracked **per branch**, so two sibling spreads of one fragment each pay full price + while a self-referencing fragment still terminates. +- Introspection is exempt only when the document asks for **nothing else**. A document mixing + `__schema` and business fields is priced normally, or the exemption becomes a smuggling route. + +## 4. Introspection + +Public by default, matching the already-public Swagger UI. The exemption from the depth and +complexity bounds is explicit and lives in `isIntrospectionOnly`, because the introspection query +is deep and expensive by nature: lowering the threshold instead would break GraphiQL and every +codegen client. + +## 5. Error Contract + +A refused document answers **HTTP 400** with a GraphQL `errors` array. GraphQL reports execution +errors with HTTP 200, but a refused document never reached the engine — it is a rejected request, +not a failed execution, and a caller branching on the status code must tell the two apart. + +Never surface an internal exception message to a client. + +## 6. Out of Scope for This Module + +- Websocket subscriptions: the artifact ships `getSubscriptionServer()`, but the route does not + advertise subscriptions, and `schemaSubscriptions` should stay empty unless that changes. +- Batch requests: priced at the maximum weight so they are never cheap, and not re-analysed per + operation. +- Rate limiting the REST surface: the plugin is installed, but wiring it to every existing route is + a separate change. diff --git a/graphql/src/main/kotlin/GraphQLModules.kt b/graphql/src/main/kotlin/GraphQLModules.kt index c33d9af..7231934 100644 --- a/graphql/src/main/kotlin/GraphQLModules.kt +++ b/graphql/src/main/kotlin/GraphQLModules.kt @@ -29,6 +29,7 @@ import io.ktor.server.plugins.ratelimit.RateLimitName import io.ktor.server.plugins.ratelimit.rateLimit import io.ktor.server.request.receiveText import io.ktor.server.response.respondText +import io.ktor.server.routing.Route import io.ktor.server.routing.intercept import io.ktor.util.pipeline.PipelineContext import io.ktor.server.routing.route @@ -41,6 +42,10 @@ import kotlinx.serialization.json.jsonPrimitive /** Name of the rate limiter guarding the GraphQL endpoint. */ internal const val GRAPHQL_RATE_LIMIT_NAME = "graphql" +private const val BATCH_REFUSED = "Batch requests are not supported by this endpoint" +private const val UNREADABLE_REFUSED = "Request refused: the GraphQL document could not be read" +private const val INTROSPECTION_REFUSED = "Query refused: schema introspection is disabled" + private val REQUEST_JSON = Json { ignoreUnknownKeys = true } /** @@ -114,44 +119,85 @@ fun Application.configureGraphQL( } routing { - rateLimit(RateLimitName(GRAPHQL_RATE_LIMIT_NAME)) { - // The plugin registers its own POST handler, so a sibling `post { }` would be tried - // first only by luck of registration order. The guard is therefore an interception on - // the route, which runs before any handler: a document that breaks the bounds is - // answered here and the engine never sees it. - intercept(ApplicationCallPipeline.Call) { - val call = context - val requestPath = call.request.local.uri.substringBefore('?') - - if (!isGraphQLPath(requestPath, config.normalizedPath)) { - proceed() - return@intercept - } + // `rateLimit` is applied only when the plugin is installed: Ktor throws at startup when a + // route references a limiter that was never registered, so calling it unconditionally + // would make `rateLimit.enabled = false` a boot-time crash rather than an opt-out. + if (config.rateLimit.enabled) { + rateLimit(RateLimitName(GRAPHQL_RATE_LIMIT_NAME)) { + bindGraphQLRoute(config, limits, logger) + } + } else { + bindGraphQLRoute(config, limits, logger) + } + } +} - val document = parseOrNull(call.receiveText(), logger) +/** + * Registers the guarded GraphQL route on the current receiver. + * + * Kept out of [configureGraphQL] so the same body serves both the rate-limited and the opt-out + * branch, instead of being duplicated across the two paths. + */ +private fun Route.bindGraphQLRoute( + config: GraphQLConfig, + limits: SecurityLimits, + logger: Logger +) { +// The plugin registers its own POST handler, so a sibling `post { }` would be tried + // first only by luck of registration order. The guard is therefore an interception on + // the route, which runs before any handler: a document that breaks the bounds is + // answered here and the engine never sees it. + intercept(ApplicationCallPipeline.Call) { + val call = context + val requestPath = call.request.local.uri.substringBefore('?') - if (document != null && exceedsLimits(document, limits)) { - respondRefusal(refusalMessage(document, limits)) + if (!isGraphQLPath(requestPath, config.normalizedPath)) { + proceed() + return@intercept + } + + // A GET carries its query in the URL, not in a body, and a batch carries several + // documents at once. Reading only the POST body would leave both unchecked, so the + // query is taken from wherever the request actually carries it. + val carried = call.carriedQuery() + + if (carried.isBatch) { + respondRefusal(BATCH_REFUSED) + finish() + return@intercept + } + + val document = parseOrNull(carried, logger) + + when { + // An unparseable document must not reach the engine: it is exactly the shape an + // attacker uses to slip past a check that only understands valid syntax. + document == null -> { + respondRefusal(UNREADABLE_REFUSED) finish() - } else { - proceed() } - } - route(config.normalizedPath) { - // requestWeight reads the body to price the query, which would otherwise consume - // the one-shot request stream and leave the GraphQL handler with an empty body. - // The in-memory cache suits a GraphQL body: small and short-lived, so a file cache - // would be pure overhead. - install(DoubleReceive) { - maxSize(config.requestBodyLimit) - useFileForCache { false } + exceedsLimits(document, limits) -> { + respondRefusal(refusalMessage(document, limits)) + finish() } + + else -> proceed() } + } - graphQLPostRoute(config.normalizedPath) + route(config.normalizedPath) { + // requestWeight reads the body to price the query, which would otherwise consume + // the one-shot request stream and leave the GraphQL handler with an empty body. + // The in-memory cache suits a GraphQL body: small and short-lived, so a file cache + // would be pure overhead. + install(DoubleReceive) { + maxSize(config.requestBodyLimit) + useFileForCache { false } + } } - } + + graphQLPostRoute(config.normalizedPath) } /** @@ -205,10 +251,10 @@ private suspend fun ApplicationCall.graphqlWeight( limits: SecurityLimits, logger: Logger ): Int { - val body = readCachedText() ?: return config.rateLimit.minimumRequestWeight - val document = parseOrNull(body, logger) + val carried = carriedQuery() + val document = parseOrNull(carried, logger) - if (document == null || exceedsLimits(document, limits)) { + if (carried.isBatch || document == null || exceedsLimits(document, limits)) { return maxOf(config.rateLimit.minimumRequestWeight, config.rateLimit.limit) } @@ -226,17 +272,18 @@ private suspend fun ApplicationCall.readCachedText(): String? = * those is charged the maximum weight upstream, so this function only has to be honest about * failure, not about the consequence. */ -private fun parseOrNull(body: String, logger: Logger): Document? { - if (isBatchRequest(body)) { +private fun parseOrNull(carried: CarriedQuery, logger: Logger): Document? { + if (carried.isBatch) { + logger.warning("Refusing a graphql batch request") return null } - val query = body.extractQuery() ?: return null + val query = (carried as? CarriedQuery.Single)?.query ?: return null return try { Parser().parseDocument(query) } catch (exception: InvalidSyntaxException) { - logger.warning("Refusing a malformed graphql document: ${exception.message}") + logger.warning("Refusing a malformed graphql document") null } catch (exception: ParseCancelledTooDeepException) { logger.warning("Refusing a graphql document too deep to parse") @@ -244,12 +291,61 @@ private fun parseOrNull(body: String, logger: Logger): Document? { } } -/** Message describing why a document was refused, safe to return to a client. */ +/** + * The query a request carries, and whether it is a batch. + * + * A batch is not a document this module can price: graphql-java would execute every element as a + * separate operation, so a batch would slip past a check that only ever looks at one parsed + * document. It is therefore never allowed through — see [isBatch]. + */ +internal sealed interface CarriedQuery { + /** One query, ready to be parsed. */ + data class Single(val query: String) : CarriedQuery + + /** A batch array, which this module refuses. */ + data object Batch : CarriedQuery + + /** A request carrying no query at all. */ + data object None : CarriedQuery + + /** Whether this request is a batch array. */ + val isBatch: Boolean + get() = this is Batch +} + +/** + * Reads the query a call carries, from the body for a POST and from the query string for a GET. + * + * Both are checked: routing only the POST would leave the GET variant unchecked, because a GET has + * no body to read, so the document would look absent and the request would proceed unpriced. + */ +internal suspend fun ApplicationCall.carriedQuery(): CarriedQuery { + val body = readCachedText() + + if (body != null && body.isNotBlank()) { + return if (isBatchRequest(body)) CarriedQuery.Batch else CarriedQuery.Single(body.extractQuery() ?: "") + } + + val fromUrl = request.queryParameters["query"] + + return if (fromUrl.isNullOrBlank()) CarriedQuery.None else CarriedQuery.Single(fromUrl) +} + +/** + * Message describing why a document was refused, safe to return to a client. + * + * Reports the bound that actually broke rather than the first one checked, so an operator reading + * the response is not sent to the wrong limit. + */ private fun refusalMessage(document: Document, limits: SecurityLimits): String { - return if (estimateCost(document) > limits.maxQueryComplexity) { - "Query refused: its estimated cost exceeds the maximum of ${limits.maxQueryComplexity}" - } else { - "Query refused: its nesting depth exceeds the maximum of ${limits.maxQueryDepth}" + val cost = estimateCost(document) + + return when { + isIntrospectionOnly(document) -> INTROSPECTION_REFUSED + cost > limits.maxQueryComplexity -> + "Query refused: its estimated cost of $cost exceeds the maximum of ${limits.maxQueryComplexity}" + + else -> "Query refused: its nesting depth exceeds the maximum of ${limits.maxQueryDepth}" } } diff --git a/graphql/src/main/kotlin/SecurityInstrumentations.kt b/graphql/src/main/kotlin/SecurityInstrumentations.kt index df710d4..49536e0 100644 --- a/graphql/src/main/kotlin/SecurityInstrumentations.kt +++ b/graphql/src/main/kotlin/SecurityInstrumentations.kt @@ -48,17 +48,14 @@ fun isIntrospectionOnly(document: Document): Boolean { private fun isIntrospectionField(selection: Selection<*>): Boolean = when (selection) { - is Field -> { - val childSelections = selection.selectionSet + // Only a meta field *is* introspection. A business field merely containing a meta field is + // a smuggling attempt: exempting it would let a heavy query ride under the exemption. + is Field -> selection.name in META_FIELDS - if (childSelections == null) { - selection.name == "__schema" || selection.name == "__type" - } else { - selection.name == "__schema" || selection.name == "__type" || isIntrospectionField(childSelections) - } - } + // An inline fragment wrapping a meta field is still introspection, since it selects + // nothing else. + is InlineFragment -> selection.selectionSet.selections.all(::isIntrospectionField) - is InlineFragment -> isIntrospectionField(selection.selectionSet) is FragmentSpread -> false else -> false } @@ -136,7 +133,11 @@ private fun depthOf( else -> null } - if (childSelections == null) current + 1 else depthOf(childSelections, current + 1, fragments, expanding) + if (childSelections == null) { + current + 1 + } else { + depthOf(childSelections, current + 1, fragments, expanding) + } } ?: current + 1 } @@ -146,3 +147,6 @@ private fun documentFragments(document: Document): Map fragment.name } private const val MAX_TRACKED_DEPTH = 64 + +/** The root fields that make a document an introspection request. */ +private val META_FIELDS = setOf("__schema", "__type") diff --git a/graphql/src/test/kotlin/CostEstimatorTest.kt b/graphql/src/test/kotlin/CostEstimatorTest.kt index 2c4835e..8a70b75 100644 --- a/graphql/src/test/kotlin/CostEstimatorTest.kt +++ b/graphql/src/test/kotlin/CostEstimatorTest.kt @@ -61,7 +61,10 @@ class CostEstimatorTest { fun `should grow linearly with the alias count`() { // Given val five = parse("{ a1: ping a2: ping a3: ping a4: ping a5: ping }") - val ten = parse("{ a1: ping a2: ping a3: ping a4: ping a5: ping a6: ping a7: ping a8: ping a9: ping a10: ping }") + val ten = parse( + "{ a1: ping a2: ping a3: ping a4: ping a5: ping " + + "a6: ping a7: ping a8: ping a9: ping a10: ping }" + ) // When val costFive = estimateCost(five) diff --git a/graphql/src/test/kotlin/GraphQLModulesTest.kt b/graphql/src/test/kotlin/GraphQLModulesTest.kt index af705ad..1598ed2 100644 --- a/graphql/src/test/kotlin/GraphQLModulesTest.kt +++ b/graphql/src/test/kotlin/GraphQLModulesTest.kt @@ -45,7 +45,9 @@ class GraphQLModulesTest { fun ping(): String = "pong" } - private fun ApplicationTestBuilder.graphqlApplication(config: GraphQLConfig = this@GraphQLModulesTest.config) { + private fun ApplicationTestBuilder.graphqlApplication( + config: GraphQLConfig = this@GraphQLModulesTest.config + ) { application { configureGraphQL( config = config, @@ -193,6 +195,99 @@ class GraphQLModulesTest { } } + @Nested + @DisplayName("Given a batch request") + inner class GivenBatchRequest { + @Test + fun `should be refused`() = testApplication { + // A batch is not one document: graphql-java would execute every element separately, so + // a check that parses a single document would never see the others. + // Given + graphqlApplication() + + // When + val batch = "[" + (1..10).joinToString(",") { """{"query":"{ ping }"}""" } + "]" + val response = post(batch) + + // Then + assertEquals(HttpStatusCode.BadRequest, response.status) + assertTrue(bodyOf(response).contains("Batch"), "got ${bodyOf(response)}") + } + } + + @Nested + @DisplayName("Given an unreadable document") + inner class GivenUnreadableDocument { + @Test + fun `should be refused instead of reaching the engine`() = testApplication { + // Malformed syntax is the shape an attacker uses to slip past a check that only + // understands valid documents. + // Given + graphqlApplication() + + // When + val response = post("""{"query":"{ ping "}""") + + // Then + assertEquals(HttpStatusCode.BadRequest, response.status) + assertTrue(bodyOf(response).contains("could not be read"), "got ${bodyOf(response)}") + } + + @Test + fun `should refuse a body carrying no query`() = testApplication { + // Given + graphqlApplication() + + // When + val response = post("""{"variables":{}}""") + + // Then + assertEquals(HttpStatusCode.BadRequest, response.status) + } + } + + @Nested + @DisplayName("Given introspection disabled") + inner class GivenIntrospectionDisabled { + @Test + fun `should refuse an introspection query`() = testApplication { + // Given + graphqlApplication(GraphQLConfig( + maxQueryDepth = 6, + maxQueryComplexity = 100, + requestBodyLimit = 4 * 1024, + introspectionEnabled = false, + rateLimit = RateLimitConfig(limit = 5, refillPeriod = 5.minutes) + )) + + // When + val response = post("""{"query":"{ __schema { types { name } } }"}""") + + // Then + assertEquals(HttpStatusCode.BadRequest, response.status) + assertTrue(bodyOf(response).contains("introspection"), "got ${bodyOf(response)}") + } + + @Test + fun `should still answer a normal query`() = testApplication { + // Given + graphqlApplication(GraphQLConfig( + maxQueryDepth = 6, + maxQueryComplexity = 100, + requestBodyLimit = 4 * 1024, + introspectionEnabled = false, + rateLimit = RateLimitConfig(limit = 5, refillPeriod = 5.minutes) + )) + + // When + val response = post(query()) + + // Then + assertEquals(HttpStatusCode.OK, response.status) + assertTrue(bodyOf(response).contains("pong"), "got ${bodyOf(response)}") + } + } + @Nested @DisplayName("Given the rate limiter") inner class GivenRateLimiter { diff --git a/graphql/src/test/kotlin/SecurityInstrumentationsTest.kt b/graphql/src/test/kotlin/SecurityInstrumentationsTest.kt index 3328baf..d8f8882 100644 --- a/graphql/src/test/kotlin/SecurityInstrumentationsTest.kt +++ b/graphql/src/test/kotlin/SecurityInstrumentationsTest.kt @@ -127,7 +127,11 @@ class SecurityInstrumentationsTest { @Test fun `should be refused when disabled`() { // Given - val closed = SecurityLimits(maxQueryDepth = 6, maxQueryComplexity = 100, introspectionEnabled = false) + val closed = SecurityLimits( + maxQueryDepth = 6, + maxQueryComplexity = 100, + introspectionEnabled = false + ) // Then assertTrue(isRefused("{ __schema { types { name } } }", closed)) @@ -147,7 +151,8 @@ class SecurityInstrumentationsTest { @Test fun `should refuse the mixed query on its own merits`() { // Given — deep enough to break the limit, disguised as introspection. - val mixed = "{ __schema { types { name } } " + "episodes { ".repeat(12) + "title" + " }".repeat(12) + " }" + val deep = "episodes { ".repeat(12) + "title" + " }".repeat(12) + val mixed = "{ __schema { types { name } } $deep }" // Then assertTrue(isRefused(mixed)) diff --git a/guidelines/ARCHITECTURE.md b/guidelines/ARCHITECTURE.md index 04704f0..1e363fd 100644 --- a/guidelines/ARCHITECTURE.md +++ b/guidelines/ARCHITECTURE.md @@ -8,6 +8,7 @@ core (base logging & core utils) ├── exposed (database, HikariCP, Exposed ORM, Liquibase, AbstractRepository, Transactional) └── validator (reflection-based validation, custom annotations) └── ktor (Ktor web framework integration, RestController, routing, OpenAPI, ResponseEntity) +└── graphql (GraphQL engine, /graphql route, endpoint security stack, cost model, DataLoader orchestration) standalone dependency carriers (no source code): koin (standalone dependency carrier: Koin runtime APIs) └── koin-exposed (Koin bridge for transactions: applyTransactionalProxies post-processor wrapping @Transactional services in TransactionalProxy) @@ -21,6 +22,7 @@ ktor-test (standalone dependency carrier: Ktor server test host) - **`exposed`**: Coordinates database connections via `DatabaseWrapper` (HikariCP + Exposed + Liquibase), provides `TransactionalProxy` dynamic proxies for `@Transactional` methods, and supplies `AbstractRepository` with an upsert DSL (`ifExists`, `newIfNotExists`, `applyFlush`). - **`validator`**: Provides a runtime reflection-based validation engine (`Validator`) and validation annotations (`@RequireAtLeastOneValid`, `@NotNull`, `@NotBlank`, `@NotEmpty`). - **`ktor`**: Bridges Ktor (server **and client**) with framework conventions. Discovers `@RestController` endpoints, binds routes (`@GetMapping`, `@PostMapping`, `@PatchMapping`), resolves parameter arguments (`@QueryParam`, `@PathParam`, `@RequestBody`), executes automatic validation via `@Valid`, packages responses with `ResponseEntity`, generates OpenAPI metadata (`@Operation`, `@ApiResponses`), handles error formatting via `MessageDto`, and exposes a preconfigured HTTP client (`createHttpClient`). +- **`graphql`**: Serves the same use cases as REST over a GraphQL transport (`graphql-kotlin` on `graphql-java`). Owns the engine wiring, the `/graphql` route, the security stack guarding it, the static cost estimator, the document bounds, and the orchestration of consumer-provided DataLoaders. Does not depend on `exposed`. - **`koin`**: Dependency carrier publishing the Koin runtime APIs (`koin-bom`, `koin-core`, `koin-annotations`) via `api(...)`; contains no source code. - **`koin-exposed`**: Bridges Koin with the `exposed` transaction model. Provides `applyTransactionalProxies()`, a runtime post-processor replacing eligible Koin singleton definitions with `TransactionalProxy`-wrapping factories so consumers never declare proxy bindings manually. Relies on `@KoinInternalApi` registry access; revisit on Koin upgrades. - **`ktor-test`**: Dependency carrier publishing Ktor's server test host (`ktor-server-test-host`) via `api(...)` so consumers write `testApplication`-based endpoint tests without referencing Ktor directly; contains no source code. @@ -29,6 +31,22 @@ ktor-test (standalone dependency carrier: Ktor server test host) The framework extends itself to consuming services: **module dependencies are exposed with `api(...)`** so that projects building on the framework can use them to their full potential (e.g. `api(libs.bundles.ktorServerEcosystem)`, `api(libs.bundles.ktorClientEcosystem)`). Use `implementation` only for dependencies that are genuinely internal to a module. +## Running Both Transports Over One DTO + +A DTO used by REST **and** GraphQL does not need a transport-specific shape. The rule that makes it +work is what the field is declared as, because graphql-kotlin resolves a field differently depending +on it: + +- a **property** is read on every request, whatever the client asked for; +- a **function** is called only when the client selects it. + +So a plain property is always loaded, and a relation that is expensive to load belongs in a function +that resolves it lazily. That is what keeps the same type cheap on both transports: REST serialises +everything, GraphQL serialises the selection, and neither pays for what was not asked. + +The type itself stays transport-agnostic: no `GraphQL` annotation is needed on a DTO that REST also +serves, and none should be added, or REST starts depending on the GraphQL library. + ## Technical Stack This project is built with **Kotlin JVM**, **Gradle (buildSrc)**, **Ktor**, **Exposed ORM**, **HikariCP**, **Liquibase**, **AWS Glide (Valkey/Redis)**, **Kotlin Reflect**, **Kotlinx Serialization**, **JUnit 6**, and **MockK**. From 3ebf51afa04e4889de677b2478dcacc610dfad43 Mon Sep 17 00:00:00 2001 From: Ziedelth Date: Sat, 3 Oct 2026 10:51:21 +0000 Subject: [PATCH 5/9] fix(graphql): bound the cost and depth walks so a guard cannot be made to hang The security stack refused the right documents but priced them at an unaffordable price: a fragment ladder where each level spreads the next one twice costs 2^k, so 815 bytes of payload made pricing take 5.5 s of CPU, and eight of them in parallel held both cores for 10.6 s. The rate limiter did not help, because the cost is paid before a single token is spent. Both walks now stop on a budget derived from the configured bounds, and a truncated walk reports the ceiling instead of its partial total. Arithmetic is Long so a saturated cost cannot wrap to zero, and the pricing recursion is depth-bounded so a deeply nested document cannot raise a StackOverflowError. The ceiling is derived from maxQueryComplexity rather than hard-coded: a fixed ceiling below a consumer's own bound would saturate under it and let the hostile document pass. Also fixed along the way: - Introspection was answered 429. Exempt from the document bounds, but still priced against a 100-unit bucket at 181 units, so GraphiQL and every codegen client were broken. The bucket now holds several documents at the bound. - A refused document charged the whole bucket, so a few hundred bytes of malformed input locked out every client behind the same address for a whole refill period. It now charges a moderate penalty. - The rate limit key was the caller's address with no way to change it, which shares one budget behind a NAT. It is now a parameter of configureGraphQL, outside RateLimitConfig: a function in a data class becomes part of equals and hashCode. - An oversized body was reported as "the document could not be read" instead of 413, hiding the body limit behind a parsing error. The depth walk is as explosive as the cost walk, so it carries the same budget, and cost is checked first so a document refused on cost never pays for it. Tests: 79 pass. The performance tests assert a deterministic node count rather than a wall-clock time, which would be flaky on a shared runner and would give false confidence about a regression to exponential expansion. The build host has 3.7 GiB of RAM and runs the Gradle daemon, the Kotlin daemon and the test fork at once, which exhausted memory and got the build killed. The fork is now capped and daemons are disabled. --- .../src/main/kotlin/kotlin-jvm.gradle.kts | 12 + graphql/AGENTS.md | 38 ++- graphql/src/main/kotlin/CostEstimator.kt | 164 +++++++++-- graphql/src/main/kotlin/GraphQLConfig.kt | 32 ++- graphql/src/main/kotlin/GraphQLModules.kt | 85 ++++-- .../main/kotlin/SecurityInstrumentations.kt | 144 +++++++++- .../test/kotlin/CostEstimatorHardeningTest.kt | 259 ++++++++++++++++++ .../kotlin/CostEstimatorPerformanceTest.kt | 174 ++++++++++++ .../test/kotlin/GraphQLRateLimitBudgetTest.kt | 165 +++++++++++ 9 files changed, 1003 insertions(+), 70 deletions(-) create mode 100644 graphql/src/test/kotlin/CostEstimatorHardeningTest.kt create mode 100644 graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt create mode 100644 graphql/src/test/kotlin/GraphQLRateLimitBudgetTest.kt diff --git a/buildSrc/src/main/kotlin/kotlin-jvm.gradle.kts b/buildSrc/src/main/kotlin/kotlin-jvm.gradle.kts index 80472a8..05fa534 100644 --- a/buildSrc/src/main/kotlin/kotlin-jvm.gradle.kts +++ b/buildSrc/src/main/kotlin/kotlin-jvm.gradle.kts @@ -55,6 +55,18 @@ tasks.withType().configureEach { // Configure all test Gradle tasks to use JUnitPlatform. useJUnitPlatform() + // The build host has 3.7 GiB of RAM and runs the Gradle daemon, the Kotlin daemon and this + // fork at the same time. Without a cap on the fork, the three JVMs together exhaust memory and + // the kernel kills the build. 640 MiB holds a test JVM for this project comfortably; the fork + // also runs with a serial collector, which costs a little throughput and saves a lot of heap. + maxHeapSize = "640m" + jvmArgs("-XX:MaxMetaspaceSize=256m", "-XX:+UseSerialGC") + + // One fork at a time: parallel forks multiply the memory cost by the fork count, and the + // suite is small enough that the wall-clock saving is not worth an OOM kill. + maxParallelForks = 1 + forkEvery = 0 + // Log information about all test results, not only the failed ones. testLogging { events( diff --git a/graphql/AGENTS.md b/graphql/AGENTS.md index 91879b6..15798d8 100644 --- a/graphql/AGENTS.md +++ b/graphql/AGENTS.md @@ -41,9 +41,23 @@ Plus the document bounds, enforced by an interception on the route: lets the plugin's own handler win by registration order, which silently executes the hostile document instead of refusing it. This was a real bug, caught by the route tests. -**`requestWeight` charges the whole bucket for an unpriceable document** — a batch array, invalid -syntax, a query too deep for the parser, or one that breaks the bounds. Never zero: a zero-weight -request is unlimited, which is worse than no rate limit at all. +**`requestWeight` never charges zero, and never charges the whole bucket for a refusal.** A batch +array, invalid syntax, a query too deep for the parser, or one that breaks the bounds is charged +`RateLimitConfig.unpriceableRequestWeight` — a fixed, moderate penalty. A zero-weight request is +unlimited, which is worse than no rate limit at all; and charging the whole bucket would hand an +attacker a free denial of service, since a few hundred bytes of malformed input would lock out +every client sharing the same address for a whole refill period. + +**`RateLimitConfig.limit` must exceed `maxQueryComplexity`.** Otherwise the bucket is smaller than +the complexity the bounds accept, and a query the bounds explicitly allow can never be executed: the +limiter refuses it for as long as the refill period lasts. + +**`configureGraphQL`'s `rateLimitKey` defaults to the caller's address.** The module owns no +authentication, so that is the only thing it can know — and behind a shared address every client +then spends one budget. Consumers that can identify their callers should pass their own key. It is +a parameter of `configureGraphQL`, not a field of `RateLimitConfig`: a function held in a data class +becomes part of `equals` and `hashCode`, and two configurations built with equivalent lambdas would +compare unequal. ## 3. Cost Model Rules @@ -53,6 +67,19 @@ must be knowable *before* execution. - Every field costs one unit, plus the cost of its own selections. - A **fragment spread pays every time it appears**. A shared "already seen" set would price the fragment bomb at the cost of a single expansion — the opposite of what is needed. +- **The price must stay affordable to compute.** Paying for every spread is what makes the price + attack-shaped, but a ladder of fragments where each level spreads the next one twice costs 2^k, + which reaches billions of nodes in under a kilobyte of payload. Both walks therefore stop at a + budget derived from the configured bounds — `SecurityLimits.costCeiling`, + `costVisitBudget`, `depthVisitBudget`. A walk that has to be *exact* is a walk an attacker can + make unbounded. +- The ceiling is **derived from `maxQueryComplexity`, never a hard-coded constant**. A fixed + ceiling below a consumer's bound would saturate under it, and the hostile document would then + pass the comparison. +- Arithmetic is **`Long`**. An unbounded `Int` sum wraps, and a wrapped zero both passes the bound + and weighs one unit in the limiter. +- Cost is checked **before** depth, so a document refused on cost never pays for the more explosive + of the two walks. - The depth walk resolves spreads, so a document three levels deep cannot hide a fifty-level expansion. - Cycles are tracked **per branch**, so two sibling spreads of one fragment each pay full price @@ -79,7 +106,8 @@ Never surface an internal exception message to a client. - Websocket subscriptions: the artifact ships `getSubscriptionServer()`, but the route does not advertise subscriptions, and `schemaSubscriptions` should stay empty unless that changes. -- Batch requests: priced at the maximum weight so they are never cheap, and not re-analysed per - operation. +- Batch requests: **refused outright** with a 400, not priced. graphql-java would execute every + element of the array as a separate operation, so a batch would answer N operations having been + analysed as one document. The refusal is in the interception, ahead of the handler. - Rate limiting the REST surface: the plugin is installed, but wiring it to every existing route is a separate change. diff --git a/graphql/src/main/kotlin/CostEstimator.kt b/graphql/src/main/kotlin/CostEstimator.kt index a42c7f5..3ed7c8c 100644 --- a/graphql/src/main/kotlin/CostEstimator.kt +++ b/graphql/src/main/kotlin/CostEstimator.kt @@ -24,47 +24,99 @@ import graphql.language.SelectionSet * fragment spread dozens of times, so a shared "already seen" set would price the attack at the * cost of a single expansion. * + * **Bounded on purpose.** Paying for every spread is what makes the price attack-shaped, but the + * price itself must stay affordable to compute: a ladder of fragments where each level spreads the + * next one twice costs 2^k, which reaches billions of nodes in under a kilobyte of payload. The + * walk therefore stops after [SecurityLimits.costVisitBudget] nodes, and a truncated walk reports + * [SecurityLimits.costCeiling] — one past the complexity bound, so the document is refused exactly + * as it would have been by its true cost. A walk that ran to completion reports the real total + * instead: the number is charged to the rate limiter, and clamping a legitimately expensive + * document down to the ceiling would misprice it. + * + * The arithmetic is [Long], and the recursion is depth-bounded, both so the function is total. An + * unbounded `Int` sum wraps, and a wrapped zero would pass the bound *and* weigh one unit in the + * limiter; an unbounded recursion on a document nested thousands of levels deep raises a + * [StackOverflowError], which is an `Error` and takes the thread with it. + * * Known limitation: the estimate is static. It cannot tell that `episodes(limit: 1000)` costs more - * than `episodes(limit: 10)`, because that depends on the arguments and on the data. Anything - * data-dependent is bounded by the complexity instrumentation and the request timeout instead. + * than `episodes(limit: 10)`, because that depends on the arguments and on the data. Nothing bounds + * that today: the engine carries no complexity instrumentation, so a data-dependent expansion is + * bounded by the request timeout alone. * * @param document Parsed document to price. + * @param limits Bounds the price saturates at. * @return The estimated cost, never below one so a request is never free. */ -fun estimateCost(document: Document): Int { +fun estimateCost(document: Document, limits: SecurityLimits = SecurityLimits.DEFAULT): Long { val fragments = document.definitions .filterIsInstance() .associateBy { fragment -> fragment.name } - return document.definitions + val ceiling = limits.costCeiling + val budget = CostBudget(limits.costVisitBudget) + + val total = document.definitions .filterIsInstance() - .sumOf { operation -> selectionsCost(operation.selectionSet, fragments, HashSet()) } - .coerceAtLeast(1) + .fold(0L) { carried, operation -> + carried + selectionsCost(operation.selectionSet, fragments, HashSet(), budget, 0) + } + + // Only a truncated walk reports the ceiling. A walk that finished priced the whole document, so + // its total is exact and must not be clamped: the same number is charged to the rate limiter, + // and clamping a legitimately expensive document down to the ceiling would misprice it. + return when { + budget.exhausted() -> ceiling + else -> total.coerceAtLeast(1L) + } } private fun selectionsCost( selectionSet: SelectionSet, fragments: Map, - expandedFragments: Set -): Int = - selectionSet.selections.sumOf { selection -> selectionCost(selection, fragments, expandedFragments) } + expandedFragments: Set, + budget: CostBudget, + depth: Int +): Long = + selectionSet.selections.sumOf { selection -> + selectionCost(selection, fragments, expandedFragments, budget, depth) + } +/** + * Cost of one selection. + * + * Two stops, both needed. The node budget bounds a walk whose cost is exponential in the fragment + * count while the document stays small. [MAX_PRICED_DEPTH] bounds the *call stack*: a document + * nested thousands of levels deep costs only a few thousand visits, so the budget alone would let + * it recurse until the JVM raised a [StackOverflowError] — which is an `Error`, not an exception, + * and takes the thread down instead of being caught. The parser rejects such a document on the + * request path, but this function is public and must hold on its own. + */ private fun selectionCost( selection: Selection<*>, fragments: Map, - expandedFragments: Set -): Int = - when (selection) { - is Field -> 1 + selection.selectionSet - ?.let { childSet -> selectionsCost(childSet, fragments, expandedFragments) } - .orZero() + expandedFragments: Set, + budget: CostBudget, + depth: Int +): Long { + if (budget.exhausted() || depth >= MAX_PRICED_DEPTH) { + return 0L + } - is InlineFragment -> selectionsCost(selection.selectionSet, fragments, expandedFragments) + budget.spend() - is FragmentSpread -> fragmentCost(selection, fragments, expandedFragments) + return when (selection) { + is Field -> 1L + (selection.selectionSet + ?.let { childSet -> selectionsCost(childSet, fragments, expandedFragments, budget, depth + 1) } + .orZero()) - else -> 0 + is InlineFragment -> + selectionsCost(selection.selectionSet, fragments, expandedFragments, budget, depth + 1) + + is FragmentSpread -> fragmentCost(selection, fragments, expandedFragments, budget, depth + 1) + + else -> 0L } +} /** * Cost of one spread: the selections it brings in, or one unit when the fragment is unknown. @@ -75,18 +127,80 @@ private fun selectionCost( private fun fragmentCost( spread: FragmentSpread, fragments: Map, - expandedFragments: Set -): Int { + expandedFragments: Set, + budget: CostBudget, + depth: Int +): Long { if (spread.name in expandedFragments) { - return 0 + return 0L } - val definition = fragments[spread.name] ?: return 1 + val definition = fragments[spread.name] ?: return 1L - return selectionsCost(definition.selectionSet, fragments, expandedFragments + spread.name) + return selectionsCost(definition.selectionSet, fragments, expandedFragments + spread.name, budget, depth) } -private fun Int?.orZero(): Int = this ?: 0 +/** + * How many selections the pricing walk may visit before it reports the ceiling. + * + * Spending the budget stops the descent; the caller then reports + * [SecurityLimits.costCeiling] rather than the partial total, so a truncated walk can never look + * cheaper than the document really is. + */ +private class CostBudget(private var visitsLeft: Long) { + fun exhausted(): Boolean = + visitsLeft <= 0L + + fun spend() { + visitsLeft-- + } + + fun visitsLeft(): Long = + visitsLeft +} + +private fun Long?.orZero(): Long = this ?: 0L + +/** Deepest nesting the pricing walk follows, whatever the node budget still allows. */ +private const val MAX_PRICED_DEPTH = 64 + +/** + * Number of nodes the pricing walk visits before its budget runs out. + * + * Exposed so the bound is testable: a wall-clock assertion cannot reliably tell a linear walk from + * an exponential one, but a node count is a deterministic function of the document. + */ +internal fun countCostNodes(document: Document, limits: SecurityLimits): Long { + val fragments = document.definitions + .filterIsInstance() + .associateBy { fragment -> fragment.name } + + val budget = CostBudget(limits.costVisitBudget) + + document.definitions + .filterIsInstance() + .forEach { operation -> + selectionsCost(operation.selectionSet, fragments, HashSet(), budget, 0) + } + + return limits.costVisitBudget - budget.visitsLeft() +} + +/** + * Converts a cost into the weight charged to the rate limit bucket. + * + * Clamped on both ends for two reasons. The floor is the module's hard rule: a zero-weight request + * is unlimited, which is worse than having no rate limit at all. The cap is what keeps one document + * from draining the whole bucket, which would turn a single heavy query into an outage for every + * other request arriving in the same window. + * + * @receiver Cost reported by [estimateCost]. + * @param minimum Smallest weight a request may be charged. + * @param maximum Largest weight a request may be charged. + * @return A weight within `[minimum, maximum]`. + */ +fun Long.toRateLimitWeight(minimum: Int, maximum: Int): Int = + coerceIn(minimum.toLong(), maximum.toLong()).toInt() /** * Documents this module is not expected to receive. @@ -95,4 +209,4 @@ private fun Int?.orZero(): Int = this ?: 0 * graphql-java would otherwise process each element as a separate, unrated document. */ internal fun isBatchRequest(body: String): Boolean = - body.trimStart().startsWith("[") + body.trimStart().startsWith("[") \ No newline at end of file diff --git a/graphql/src/main/kotlin/GraphQLConfig.kt b/graphql/src/main/kotlin/GraphQLConfig.kt index cb2d35b..c3e9f34 100644 --- a/graphql/src/main/kotlin/GraphQLConfig.kt +++ b/graphql/src/main/kotlin/GraphQLConfig.kt @@ -1,5 +1,6 @@ package fr.shikkanime.graphql +import io.ktor.server.application.ApplicationCall import kotlin.time.Duration import kotlin.time.Duration.Companion.minutes @@ -10,19 +11,46 @@ import kotlin.time.Duration.Companion.minutes * query, so a heavy query drains the bucket faster than a light one. A flat per-request count * would let a single expensive document through as cheaply as a trivial one. * + * The bucket has to be able to hold at least one document priced at the complexity bound, otherwise + * a query the bounds explicitly accept can never be executed: it would be refused by the limiter + * for as long as the refill period lasts. [limit] therefore defaults to several times + * `maxQueryComplexity` rather than to it, so a heavy query is possible but cannot drain the bucket + * on its own. + * * @property enabled Whether the endpoint is rate limited at all. * @property limit Bucket capacity, expressed in cost units rather than requests. * @property refillPeriod How often the bucket is refilled to full. * @property minimumRequestWeight Weight charged when the query cost cannot be determined, so a * request is never free. Zero would let a caller bypass the limit entirely. + * @property unpriceableRequestWeight Weight charged for a request that could not be priced at all — + * a batch array, invalid syntax, or a document the bounds refuse. Deliberately far below [limit]: + * charging the whole bucket would let a few hundred bytes of malformed input lock every client + * sharing the same address out for a whole refill period, which is a denial of service handed to + * the attacker for free. */ data class RateLimitConfig( val enabled: Boolean = true, - val limit: Int = 100, + val limit: Int = 1000, val refillPeriod: Duration = 1.minutes, - val minimumRequestWeight: Int = 1 + val minimumRequestWeight: Int = 1, + val unpriceableRequestWeight: Int = 10 ) +/** + * Identifies the bucket a request is charged to. + * + * Kept out of [RateLimitConfig] on purpose: a function held in a data class becomes part of + * `equals`, `hashCode` and `copy`, so two configurations built with equivalent lambdas would + * compare unequal — a trap that shows up as a mysteriously failing test rather than as a type + * error. Consumers behind a shared address pass their own key to + * [fr.shikkanime.graphql.configureGraphQL] to stop one client from spending everyone's budget. + * + * @receiver The call being charged. + * @return A stable value identifying the caller's bucket. + */ +val defaultRateLimitKey: (ApplicationCall) -> Any = + { call -> call.request.local.remoteHost } + /** * Configuration of the GraphQL module. * diff --git a/graphql/src/main/kotlin/GraphQLModules.kt b/graphql/src/main/kotlin/GraphQLModules.kt index 7231934..852f356 100644 --- a/graphql/src/main/kotlin/GraphQLModules.kt +++ b/graphql/src/main/kotlin/GraphQLModules.kt @@ -8,6 +8,7 @@ import com.expediagroup.graphql.server.operations.Mutation import com.expediagroup.graphql.server.operations.Query import com.expediagroup.graphql.server.operations.Subscription import fr.shikkanime.core.LoggerFactory +import java.util.logging.Level import java.util.logging.Logger import graphql.language.Document import graphql.parser.InvalidSyntaxException @@ -20,6 +21,7 @@ import io.ktor.server.application.ApplicationCallPipeline import io.ktor.server.application.PipelineCall import io.ktor.server.application.ApplicationCall import io.ktor.server.application.install +import io.ktor.server.plugins.PayloadTooLargeException import io.ktor.server.plugins.bodylimit.RequestBodyLimit import io.ktor.server.plugins.compression.Compression import io.ktor.server.plugins.compression.gzip @@ -48,6 +50,9 @@ private const val INTROSPECTION_REFUSED = "Query refused: schema introspection i private val REQUEST_JSON = Json { ignoreUnknownKeys = true } +/** Logger for the module's own diagnostics, outside the application lifecycle. */ +private val MODULE_LOGGER = LoggerFactory.getLogger() + /** * Installs the GraphQL module: engine, schema, route, and the security stack guarding it. * @@ -70,6 +75,9 @@ private val REQUEST_JSON = Json { ignoreUnknownKeys = true } * enabled by this module, so declaring one would advertise an endpoint that cannot serve it. * @property schemaPackages Packages scanned for annotated types. The generator refuses to build * a schema without one, so it is required even though listing the queries explicitly is enough. + * @property rateLimitKey Identifies the bucket each request is charged to. Defaults to the caller's + * address, the only identity this module can know since it owns no authentication. A consumer + * behind a shared address should pass its own key, or one client spends everyone's budget. */ fun Application.configureGraphQL( config: GraphQLConfig = GraphQLConfig(), @@ -77,7 +85,8 @@ fun Application.configureGraphQL( schemaPackages: List, schemaQueries: List = emptyList(), schemaMutations: List = emptyList(), - schemaSubscriptions: List = emptyList() + schemaSubscriptions: List = emptyList(), + rateLimitKey: (ApplicationCall) -> Any = defaultRateLimitKey ) { val logger = LoggerFactory.getLogger() val limits = config.toSecurityLimits() @@ -112,7 +121,7 @@ fun Application.configureGraphQL( install(RateLimit) { register(RateLimitName(GRAPHQL_RATE_LIMIT_NAME)) { rateLimiter(limit = config.rateLimit.limit, refillPeriod = config.rateLimit.refillPeriod) - requestKey { call -> call.request.local.remoteHost } + requestKey(rateLimitKey) requestWeight { call, _ -> call.graphqlWeight(config, limits, logger) } } } @@ -156,9 +165,6 @@ private fun Route.bindGraphQLRoute( return@intercept } - // A GET carries its query in the URL, not in a body, and a batch carries several - // documents at once. Reading only the POST body would leave both unchecked, so the - // query is taken from wherever the request actually carries it. val carried = call.carriedQuery() if (carried.isBatch) { @@ -240,11 +246,15 @@ private suspend fun PipelineContext.respondRefusal(message: /** * Cost charged to the rate limit bucket for one request. * - * A document that cannot be priced — a batch array, invalid syntax, or a query too deep for the - * parser — is charged the whole bucket, so no caller gets cheap work by sending something - * unpriceable. A document that breaks the bounds is charged the whole bucket too: refusing it is - * far cheaper than running it, and the caller should feel the difference. The weight is never zero, - * because a zero-weight request is unlimited — worse than having no rate limit at all. + * A document that cannot be priced — a batch array, invalid syntax, a query too deep for the + * parser, or one that breaks the bounds — is charged [RateLimitConfig.unpriceableRequestWeight], a + * moderate fixed penalty rather than the whole bucket. The floor matters most: the weight is never + * zero, because a zero-weight request is unlimited and worse than having no rate limit at all. The + * ceiling matters just as much: charging a refusal the whole bucket would hand an attacker a free + * denial of service, since a few hundred bytes of malformed input would lock out every client + * behind the same address for a whole refill period. + * + * A priced document pays its real cost, clamped to what the bucket can absorb. */ private suspend fun ApplicationCall.graphqlWeight( config: GraphQLConfig, @@ -255,15 +265,36 @@ private suspend fun ApplicationCall.graphqlWeight( val document = parseOrNull(carried, logger) if (carried.isBatch || document == null || exceedsLimits(document, limits)) { - return maxOf(config.rateLimit.minimumRequestWeight, config.rateLimit.limit) + return config.rateLimit.unpriceableRequestWeight + .coerceAtLeast(config.rateLimit.minimumRequestWeight) + .coerceAtMost(config.rateLimit.limit) } - return estimateCost(document).coerceAtLeast(config.rateLimit.minimumRequestWeight) + val cost = estimateCost(document, limits) + + return cost.toRateLimitWeight( + minimum = config.rateLimit.minimumRequestWeight, + maximum = config.rateLimit.limit + ) } -/** Reads the cached body, which DoubleReceive made re-readable. */ +/** + * Reads the cached body, which DoubleReceive made re-readable. + * + * An oversized body is not an unreadable one: [io.ktor.server.plugins.PayloadTooLargeException] + * means the request broke the body limit, and answering "could not be read" would send an operator + * hunting for a parsing bug that does not exist. It is re-thrown so the platform answers 413. + */ private suspend fun ApplicationCall.readCachedText(): String? = - runCatching { receiveText() }.getOrNull() + try { + receiveText() + } catch (exception: PayloadTooLargeException) { + throw exception + } catch (exception: Exception) { + MODULE_LOGGER.log(Level.FINE, "Could not read the request body", exception) + + null + } /** * Parses the query carried by a GraphQL request body. @@ -314,21 +345,26 @@ internal sealed interface CarriedQuery { } /** - * Reads the query a call carries, from the body for a POST and from the query string for a GET. + * Reads the query a call carries, from its body. * - * Both are checked: routing only the POST would leave the GET variant unchecked, because a GET has - * no body to read, so the document would look absent and the request would proceed unpriced. + * Only the body: this module registers the POST route alone, so a GET is answered with 405 by Ktor + * before reaching any guard. Reading the query string as well would price a request the plugin + * never serves, and would keep two code paths alive for one. */ internal suspend fun ApplicationCall.carriedQuery(): CarriedQuery { val body = readCachedText() - if (body != null && body.isNotBlank()) { - return if (isBatchRequest(body)) CarriedQuery.Batch else CarriedQuery.Single(body.extractQuery() ?: "") + if (body.isNullOrBlank()) { + return CarriedQuery.None } - val fromUrl = request.queryParameters["query"] + if (isBatchRequest(body)) { + return CarriedQuery.Batch + } - return if (fromUrl.isNullOrBlank()) CarriedQuery.None else CarriedQuery.Single(fromUrl) + val query = body.extractQuery() ?: return CarriedQuery.None + + return CarriedQuery.Single(query) } /** @@ -338,12 +374,11 @@ internal suspend fun ApplicationCall.carriedQuery(): CarriedQuery { * the response is not sent to the wrong limit. */ private fun refusalMessage(document: Document, limits: SecurityLimits): String { - val cost = estimateCost(document) - return when { isIntrospectionOnly(document) -> INTROSPECTION_REFUSED - cost > limits.maxQueryComplexity -> - "Query refused: its estimated cost of $cost exceeds the maximum of ${limits.maxQueryComplexity}" + + estimateCost(document, limits) > limits.maxQueryComplexity -> + "Query refused: its estimated cost exceeds the maximum of ${limits.maxQueryComplexity}" else -> "Query refused: its nesting depth exceeds the maximum of ${limits.maxQueryDepth}" } diff --git a/graphql/src/main/kotlin/SecurityInstrumentations.kt b/graphql/src/main/kotlin/SecurityInstrumentations.kt index 49536e0..78d4509 100644 --- a/graphql/src/main/kotlin/SecurityInstrumentations.kt +++ b/graphql/src/main/kotlin/SecurityInstrumentations.kt @@ -22,7 +22,50 @@ data class SecurityLimits( val maxQueryDepth: Int, val maxQueryComplexity: Int, val introspectionEnabled: Boolean = true -) +) { + /** + * Cost reported by a document expensive enough that pricing it precisely is pointless. + * + * Derived from [maxQueryComplexity] rather than fixed: a hard-coded ceiling would saturate + * *below* the bound of a consumer configured with a larger one, and the hostile document would + * then pass `cost > maxQueryComplexity`. One past the bound is the smallest value that keeps + * every saturated document refused, whatever the configuration. + */ + val costCeiling: Long = + maxQueryComplexity.toLong() + 1L + + /** + * Largest number of selections the cost walk may visit before it gives up. + * + * A document cannot ask for more selections than it has bytes, so the budget scales with the + * body limit the caller configured. It exists to stop an expansion that is exponential in the + * number of fragments rather than linear in the number of selections. + */ + val costVisitBudget: Long = + COST_VISITS_PER_BYTE * (maxQueryComplexity.toLong() + FRAGMENT_VISIT_FACTOR) + + /** + * Largest number of branches the depth walk may visit before it gives up. + * + * Derived from [maxQueryComplexity], not [maxQueryDepth]: an explosive ladder is shallow, so a + * budget tied to the depth bound would be tiny and would truncate documents that have nothing + * to do with depth. Complexity is the bound that actually prices the expansion. + */ + val depthVisitBudget: Long = + DEPTH_VISITS_PER_BYTE * (maxQueryComplexity.toLong() + FRAGMENT_VISIT_FACTOR) + + companion object { + /** + * Bounds used when a caller prices a document outside a configured endpoint. + * + * Exposed so [estimateCost] keeps a total function for its own tests and for any consumer + * pricing a document outside a request. The endpoint always passes the limits it was + * configured with, so the ceiling follows the deployment rather than these defaults. + */ + val DEFAULT: SecurityLimits = + SecurityLimits(maxQueryDepth = 8, maxQueryComplexity = 200) + } +} /** * Whether the document only asks for schema metadata. @@ -75,10 +118,16 @@ fun exceedsLimits(document: Document, limits: SecurityLimits): Boolean { return !limits.introspectionEnabled } - val cost = estimateCost(document) - val depth = operationsMaxDepth(document) + // Cost is checked first, and each walk stops at its own ceiling. The order matters: pricing is + // the cheaper of the two analyses, so a document refused on cost never pays for the depth walk, + // which resolves spreads and is the more explosive of the pair. + val cost = estimateCost(document, limits) + + if (cost > limits.maxQueryComplexity) { + return true + } - return depth > limits.maxQueryDepth || cost > limits.maxQueryComplexity + return operationsMaxDepth(document, limits) > limits.maxQueryDepth } /** @@ -88,10 +137,18 @@ fun exceedsLimits(document: Document, limits: SecurityLimits): Boolean { * hit the bound is already refused, so counting the exact depth past that point would cost time on * exactly the inputs an attacker sends. */ -private fun operationsMaxDepth(document: Document): Int = +private fun operationsMaxDepth(document: Document, limits: SecurityLimits): Int = document.definitions .filterIsInstance() - .maxOfOrNull { depthOf(it.selectionSet, 0, documentFragments(document)) } + .maxOfOrNull { operation -> + depthOf( + selectionSet = operation.selectionSet, + current = 0, + fragments = documentFragments(document), + limits = limits, + budget = Budget(limits.depthVisitBudget) + ) + } ?: 0 /** @@ -105,12 +162,16 @@ private fun depthOf( selectionSet: SelectionSet, current: Int, fragments: Map = emptyMap(), - expanding: Set = emptySet() + expanding: Set = emptySet(), + limits: SecurityLimits, + budget: Budget ): Int { - if (current >= MAX_TRACKED_DEPTH) { + if (current >= MAX_TRACKED_DEPTH || budget.exhausted()) { return current } + budget.spend() + return selectionSet.selections.maxOfOrNull { selection -> val childSelections = when (selection) { is Field -> selection.selectionSet @@ -122,10 +183,12 @@ private fun depthOf( null } else { return@maxOfOrNull depthOf( - definition.selectionSet, - current + 1, - fragments, - expanding + selection.name + selectionSet = definition.selectionSet, + current = current + 1, + fragments = fragments, + expanding = expanding + selection.name, + limits = limits, + budget = budget ) } } @@ -136,11 +199,57 @@ private fun depthOf( if (childSelections == null) { current + 1 } else { - depthOf(childSelections, current + 1, fragments, expanding) + depthOf( + selectionSet = childSelections, + current = current + 1, + fragments = fragments, + expanding = expanding, + limits = limits, + budget = budget + ) } } ?: current + 1 } +/** + * Shared allowance for one analysis walk. + * + * The walks resolve fragment spreads, so their cost is exponential in the number of fragments while + * the document stays comfortably inside the body limit. The budget is what stops that: once spent, + * the walk stops descending and reports what it has. It is mutable and passed by reference so the + * whole walk shares one allowance rather than restarting it per branch. + */ +private class Budget(private var remaining: Long) { + fun exhausted(): Boolean = + remaining <= 0L + + fun spend() { + remaining-- + } + + fun remainingVisits(): Long = + remaining +} + +/** + * Number of nodes the depth walk visits before its budget runs out. + * + * Exposed so the bound is testable: a wall-clock assertion cannot see the difference between a + * linear walk and an exponential one reliably, but a node count can. + */ +internal fun countDepthNodes(document: Document, limits: SecurityLimits): Long { + val fragments = documentFragments(document) + val budget = Budget(limits.depthVisitBudget) + + document.definitions + .filterIsInstance() + .forEach { operation -> + depthOf(operation.selectionSet, 0, fragments, emptySet(), limits, budget) + } + + return limits.depthVisitBudget - budget.remainingVisits() +} + private fun documentFragments(document: Document): Map = document.definitions .filterIsInstance() @@ -148,5 +257,14 @@ private fun documentFragments(document: Document): Map maxQueryComplexity`. + // Given + val document = parse(fragmentLadder(levels = 28)) + + // When + val cost = estimateCost(document, limits) + + // Then + assertEquals(limits.costCeiling, cost) + } + + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should follow the ceiling of a raised bound`() { + // The regression this pins: with a hard-coded ceiling of 200 and a consumer bound of + // 20 000, a saturated document reported 200 and passed the check. + // Given + val generous = SecurityLimits(maxQueryDepth = 8, maxQueryComplexity = 20_000) + val document = parse(fragmentLadder(levels = 28)) + + // When + val cost = estimateCost(document, generous) + + // Then + assertEquals(generous.costCeiling, cost) + assertTrue(cost > generous.maxQueryComplexity, "saturation must break the bound it belongs to") + } + + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should saturate rather than wrap around to a small cost`() { + // The overflow is what turns a slow guard into an open one: 2^32 as an Int is 0, and a + // zero cost both passes the bound and weighs 1 in the rate limiter — the one outcome + // the module forbids outright. + // Given + val document = parse(fragmentLadder(levels = 26)) + + // When + val cost = estimateCost(document, limits) + + // Then + assertTrue(cost >= limits.costCeiling, "cost must never wrap below the ceiling, got $cost") + } + + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should keep pricing ordinary documents exactly`() { + // The ceiling must not leak into legitimate pricing: the same number is charged to the + // rate limit bucket, so it has to stay exact below the ceiling. + // Given + val document = parse("{ episodes { anime { title } } }") + + // When + val cost = estimateCost(document, limits) + + // Then + assertEquals(3, cost) + } + + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should be refused on cost once it saturates`() { + // Given + val document = parse(fragmentLadder(levels = 28)) + + // When & Then + assertTrue(exceedsLimits(document, limits), "an explosive document must break the complexity bound") + } + } + + @Nested + @DisplayName("Given an explosive document priced for the depth walk") + inner class GivenExplosiveDocumentDepthWalk { + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should refuse it without walking every branch`() { + // Bounding `estimateCost` alone is not enough: `exceedsLimits` also runs the depth walk, + // which expands the same ladder. Capping the cost first means the depth walk is never + // reached for a document that is already refused on cost. + // Given + val document = parse(fragmentLadder(levels = 24)) + + // When & Then — the timeout is the assertion; the walk would otherwise take minutes. + assertTrue(exceedsLimits(document, limits)) + } + + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should measure depth within the node budget`() { + // Depth is bounded per branch, but the total number of branches is not: a ladder of + // 20 fragments is 20 levels deep, far under `MAX_TRACKED_DEPTH`, yet expands into a + // million branches. The counter is what makes that observable without a clock. + // Given + val document = parse(fragmentLadder(levels = 20)) + + // When + val visited = countDepthNodes(document, limits) + + // Then + assertTrue( + visited <= limits.depthVisitBudget, + "depth walk visited $visited nodes, over the budget of ${limits.depthVisitBudget}" + ) + } + } + + @Nested + @DisplayName("Given a document nested deeper than the parser allows") + inner class GivenUnboundedSelectionNesting { + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should price it instead of overflowing the stack`() { + // The parser refuses such a document on the request path, but `estimateCost` is public + // and has to hold on its own. An unbounded recursion raises a `StackOverflowError`, and + // that is an `Error` rather than an exception: it cannot be caught, so it takes the + // thread down instead of failing the request. + // Given — built through the API, because the parser caps nesting at 500 levels. + var selection: graphql.language.Selection<*> = graphql.language.Field.newField("ping").build() + + repeat(3_000) { + selection = graphql.language.Field.newField("nested") + .selectionSet( + graphql.language.SelectionSet.newSelectionSet() + .selections(listOf(selection)) + .build() + ) + .build() + } + + val document = graphql.language.Document.newDocument() + .definition( + graphql.language.OperationDefinition.newOperationDefinition() + .name("Deep") + .selectionSet( + graphql.language.SelectionSet.newSelectionSet() + .selections(listOf(selection)) + .build() + ) + .build() + ) + .build() + + // When + val cost = estimateCost(document, limits) + + // Then + assertTrue(cost >= 1L, "a deeply nested document still has a positive cost, got $cost") + } + } + + @Nested + @DisplayName("Given the weight charged for a saturated document") + inner class GivenWeightFromSaturatedDocument { + @Test + fun `should never charge a free request`() { + // The module rules state it outright: a zero-weight request is unlimited, which is + // worse than no rate limit at all. Saturation must land on the ceiling and the + // conversion must keep the floor. + // Given + val saturated = estimateCost(parse(fragmentLadder(levels = 26)), limits) + + // When + val weight = saturated.toRateLimitWeight(minimum = 1, maximum = 100) + + // Then + assertTrue(weight >= 1, "weight must be at least one, got $weight") + assertTrue(weight > 1, "a saturated document must cost more than a trivial one, got $weight") + } + + @Test + fun `should never charge more than the bucket it drains`() { + // A weight above the bucket capacity makes the bucket unreachable for the whole refill + // period, which turns one heavy document into a self-inflicted outage. + // Given + val saturated = estimateCost(parse(fragmentLadder(levels = 26)), limits) + + // When + val weight = saturated.toRateLimitWeight(minimum = 1, maximum = 100) + + // Then + assertTrue(weight in 1..100, "weight must fit the bucket, got $weight") + } + + @Test + fun `should charge a trivial document exactly one`() { + // Given + val trivial = estimateCost(parse("{ ping }"), limits) + + // When + val weight = trivial.toRateLimitWeight(minimum = 1, maximum = 100) + + // Then + assertEquals(1, weight) + } + + @Test + fun `should charge an affordable document its real cost`() { + // Between the floor and the cap the cost is exact: that is the whole point of charging + // by cost rather than by request. + // Given + val affordable = estimateCost(parse("{ " + (1..40).joinToString(" ") { "a$it: ping" } + " }"), limits) + + // When + val weight = affordable.toRateLimitWeight(minimum = 1, maximum = 100) + + // Then + assertEquals(40, weight) + } + } +} \ No newline at end of file diff --git a/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt b/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt new file mode 100644 index 0000000..d434325 --- /dev/null +++ b/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt @@ -0,0 +1,174 @@ +package fr.shikkanime.graphql + +import graphql.parser.Parser +import org.junit.jupiter.api.DisplayName +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.Timeout +import java.util.concurrent.TimeUnit +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Performance guards for the cost and depth walks. + * + * A wall-clock assertion is flaky in CI, so it is deliberately absent: the runners throttle CPU and + * pause for GC, and a threshold loose enough to survive that gives false confidence about a + * regression to exponential expansion. What is asserted instead is the *shape* of the work — the + * number of nodes each walk visits — which is a deterministic function of the document and holds + * identically on every machine. + * + * `@Timeout` covers the remaining half: only it can notice a hang, and it fails the build instead of + * stalling the suite. It is set wide enough that a loaded runner does not trip it, and narrow enough + * that the exponential regression fails in seconds rather than minutes. + */ +class CostEstimatorPerformanceTest { + private val limits = SecurityLimits(maxQueryDepth = 8, maxQueryComplexity = 200) + + private fun fragmentLadder(levels: Int): String { + val document = StringBuilder("{ ...F0 }") + + for (level in 0 until levels) { + val next = if (level == levels - 1) "episodes { ping }" else "...F${level + 1}" + + document.append(" fragment F$level on Query { $next $next }") + } + + return document.toString() + } + + @Nested + @DisplayName("Given the nodes the cost walk visits") + inner class GivenCostWalkNodes { + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should stay within the budget on a wide document`() { + // A 64 KiB body of aliases is the largest shape worth pricing. Linear in the fields, + // so the node count is the field count — anything super-linear means the walk stopped + // visiting each selection once. + // Given + val aliases = (1..4_000).joinToString(" ") { "a$it: ping" } + val document = Parser().parseDocument("{ $aliases }") + + // When + val visited = countCostNodes(document, limits) + + // Then + assertTrue( + visited <= limits.costVisitBudget, + "cost walk visited $visited nodes, over the budget of ${limits.costVisitBudget}" + ) + } + + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should visit nodes linearly on a flat document`() { + // Ten fields must not cost more node visits than a hundred by a factor of ten squared. + // This is the invariant that a memoisation bug or a double-counting would break. + // Given + val ten = Parser().parseDocument("{ " + (1..10).joinToString(" ") { "a$it: ping" } + " }") + val hundred = Parser().parseDocument("{ " + (1..100).joinToString(" ") { "a$it: ping" } + " }") + + // When + val tenVisits = countCostNodes(ten, limits) + val hundredVisits = countCostNodes(hundred, limits) + + // Then + assertTrue( + hundredVisits <= tenVisits * 12, + "visits grew from $tenVisits to $hundredVisits for a 10x wider document" + ) + } + } + + @Nested + @DisplayName("Given the nodes the depth walk visits") + inner class GivenDepthWalkNodes { + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should stay within the budget on an explosive ladder`() { + // The ladder is 20 levels deep, comfortably under `MAX_TRACKED_DEPTH`, but expands into + // a million branches. Depth is bounded per branch and unbounded in total, which is the + // gap this budget closes. + // Given + val document = Parser().parseDocument(fragmentLadder(levels = 20)) + + // When + val visited = countDepthNodes(document, limits) + + // Then + assertTrue( + visited <= limits.depthVisitBudget, + "depth walk visited $visited nodes, over the budget of ${limits.depthVisitBudget}" + ) + } + + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should not grow with the level count`() { + // Two ladders of very different explosive size must both stop at the budget rather than + // scaling with their level count. The small one is measured under the budget, so it is + // the *ceiling* that has to hold, not an equality between the two. + // Given + val small = Parser().parseDocument(fragmentLadder(levels = 12)) + val large = Parser().parseDocument(fragmentLadder(levels = 22)) + + // When + val smallVisits = countDepthNodes(small, limits) + val largeVisits = countDepthNodes(large, limits) + + // Then + assertTrue( + largeVisits <= limits.depthVisitBudget, + "the larger ladder exceeded the budget: $largeVisits" + ) + assertTrue( + largeVisits > smallVisits, + "a larger ladder should still visit more branches, got $smallVisits then $largeVisits" + ) + } + } + + @Nested + @DisplayName("Given a cyclic document") + inner class GivenCyclicDocument { + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should terminate on mutually recursive fragments`() { + // Termination is a performance property as much as a correctness one: a walk that does + // not stop on a cycle is a hang, not a wrong number. + // Given + val document = Parser().parseDocument( + "{ episodes { ...A } } fragment A on Episode { ...B } fragment B on Episode { ...A }" + ) + + // When + val cost = estimateCost(document, limits) + val refused = exceedsLimits(document, limits) + + // Then + assertTrue(cost >= 1, "a cyclic document still has a positive cost") + assertTrue(!refused, "a two-fragment cycle expands to almost nothing") + } + } + + @Nested + @DisplayName("Given a document priced to the ceiling") + inner class GivenDocumentAtTheCeiling { + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should be flat past the ceiling`() { + // Saturation flattens the curve: levels that differ by thousands of units of true cost + // all report the same number. That is the property a guard relies on to refuse fast. + // Given + val documents = listOf(24, 28, 30).map { Parser().parseDocument(fragmentLadder(levels = it)) } + + // When + val costs = documents.map { estimateCost(it, limits) } + + // Then + assertEquals(1, costs.distinct().size, "every saturated document must report the same cost: $costs") + assertEquals(limits.costCeiling, costs.first()) + } + } +} \ No newline at end of file diff --git a/graphql/src/test/kotlin/GraphQLRateLimitBudgetTest.kt b/graphql/src/test/kotlin/GraphQLRateLimitBudgetTest.kt new file mode 100644 index 0000000..7ebc035 --- /dev/null +++ b/graphql/src/test/kotlin/GraphQLRateLimitBudgetTest.kt @@ -0,0 +1,165 @@ +package fr.shikkanime.graphql + +import com.expediagroup.graphql.server.operations.Query +import io.ktor.client.request.post +import io.ktor.client.request.setBody +import io.ktor.client.statement.HttpResponse +import io.ktor.client.statement.bodyAsText +import io.ktor.http.ContentType +import io.ktor.http.HttpStatusCode +import io.ktor.http.contentType +import io.ktor.server.testing.ApplicationTestBuilder +import io.ktor.server.testing.testApplication +import org.junit.jupiter.api.DisplayName +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Regressions on how the endpoint behaves with the shipped defaults. + * + * Each test pins a behaviour that the security stack must not break on the way to refusing hostile + * documents: introspection is public and must stay usable, and a legitimate query priced above the + * bucket must still be answerable. + */ +class GraphQLRateLimitBudgetTest { + // Fixtures. + private val defaults = GraphQLConfig() + + class PingQuery : Query { + fun ping(): String = "pong" + } + + private fun ApplicationTestBuilder.graphql(config: GraphQLConfig) { + application { + configureGraphQL( + config = config, + schemaPackages = listOf("fr.shikkanime.graphql"), + schemaQueries = listOf(PingQuery()) + ) + } + } + + private suspend fun ApplicationTestBuilder.post(body: String): HttpResponse = + client.post(defaults.normalizedPath) { + contentType(ContentType.Application.Json) + setBody(body) + } + + private fun aliases(count: Int): String = + "{ " + (1..count).joinToString(" ") { "a$it: ping" } + " }" + + @Nested + @DisplayName("Given a schema introspection query") + inner class GivenIntrospectionQuery { + @Test + fun `should be served with the default configuration`() = testApplication { + // Introspection is exempt from the document bounds because it is deep and expensive by + // nature, but the exemption must not leave it priced above the rate limit bucket: the + // weight would then exceed the bucket and every introspection request would be + // answered with 429, which breaks GraphiQL and every codegen client. + // Given + graphql(defaults) + + // When + val response = post("""{"query":"{ __schema { types { name } } }"}""") + + // Then + assertEquals(HttpStatusCode.OK, response.status) + assertTrue(response.bodyAsText().contains("__schema")) + } + + @Test + fun `should stay served when repeated`() = testApplication { + // A single request could fit by luck. The bucket must absorb the realistic number of + // introspection calls a codegen client makes. + // Given + graphql(defaults) + + // When + val statuses = (1..10).map { + post("""{"query":"{ __schema { types { name } } }"}""").status + } + + // Then + assertTrue( + statuses.none { it == HttpStatusCode.TooManyRequests }, + "introspection must not exhaust the bucket, got $statuses" + ) + } + } + + @Nested + @DisplayName("Given a legitimate query priced above the bucket") + inner class GivenAffordableQuery { + @Test + fun `should not be permanently rejected`() = testApplication { + // The bounds promise up to `maxQueryComplexity`; the bucket must therefore hold at least + // one document at that price. Otherwise the advertised range is unusable and the client + // sees 429 for a query the bounds accept. + // Given + graphql(defaults) + + // When — cost 150 sits between the old default bucket and the default complexity bound, + // repeated to prove the limiter does not lock the client out for good. + val body = """{"query":"${aliases(150)}"}""" + val statuses = (1..5).map { post(body).status } + + // Then + assertTrue( + statuses.all { it == HttpStatusCode.OK }, + "a query within the complexity bound must stay answerable, got $statuses" + ) + } + + @Test + fun `should not lock out a cheap query that follows`() = testApplication { + // One heavy request must not spend the whole bucket: a client that sends one big query + // then a trivial one has to get both answered. + // Given + graphql(defaults) + + // When + post("""{"query":"${aliases(150)}}"}""") + val cheap = post("""{"query":"{ ping }"}""").status + + // Then + assertEquals(HttpStatusCode.OK, cheap) + } + } + + @Nested + @DisplayName("Given a document the endpoint refuses") + inner class GivenRefusedDocument { + @Test + fun `should not spend the whole bucket`() = testApplication { + // Charging a refusal the full bucket lets a few hundred bytes of malformed input lock + // every client behind the same address for a whole refill period. + // Given + graphql(defaults) + + // When + post("""{"query":"${aliases(400)}}"}""") + val cheap = post("""{"query":"{ ping }"}""").status + + // Then + assertEquals(HttpStatusCode.OK, cheap) + } + + @Test + fun `should not spend the whole bucket on an unreadable body`() = testApplication { + // An unpriceable body is the cheapest possible input for an attacker, so it must be + // the least expensive thing the limiter can be asked to absorb. + // Given + graphql(defaults) + + // When + post("""{"variables":{"a":1}}""") + val cheap = post("""{"query":"{ ping }"}""").status + + // Then + assertEquals(HttpStatusCode.OK, cheap) + } + } +} \ No newline at end of file From 819f9bbf91a171b240b0329710cfd56381d9f6fb Mon Sep 17 00:00:00 2001 From: Ziedelth Date: Sat, 3 Oct 2026 10:59:13 +0000 Subject: [PATCH 6/9] test(graphql): assert the pricing walk scales linearly, not quadratically The node-count assertions prove the walk is bounded, but not that it stays linear once it is. A memoisation bug or a double-counting would keep every document under the budget and still multiply the work. A wall-clock threshold on a single document would not help: pricing a 4000-field document measures 0.013 ms, so the 250 ms threshold that was proposed would still pass after a 3800x regression. Comparing two documents and asserting the ratio is what actually discriminates -- measured 2.25 for the current linear walk, against 16 for a quadratic one. --- .../kotlin/CostEstimatorPerformanceTest.kt | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt b/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt index d434325..5fe41a7 100644 --- a/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt +++ b/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt @@ -6,6 +6,7 @@ import org.junit.jupiter.api.Nested import org.junit.jupiter.api.Test import org.junit.jupiter.api.Timeout import java.util.concurrent.TimeUnit +import kotlin.system.measureNanoTime import kotlin.test.assertEquals import kotlin.test.assertTrue @@ -25,6 +26,16 @@ import kotlin.test.assertTrue class CostEstimatorPerformanceTest { private val limits = SecurityLimits(maxQueryDepth = 8, maxQueryComplexity = 200) + private companion object { + /** + * How much slower a four-times-larger document may price. + * + * A linear walk lands near 4, quadratic near 16. Eight leaves room for the measurement + * noise of a shared runner while still failing on any super-linear work. + */ + const val LINEAR_SCALING_TOLERANCE = 8.0 + } + private fun fragmentLadder(levels: Int): String { val document = StringBuilder("{ ...F0 }") @@ -60,6 +71,36 @@ class CostEstimatorPerformanceTest { ) } + @Test + @Timeout(value = 10, unit = TimeUnit.SECONDS) + fun `should scale its time with the field count, not its square`() { + // A wall-clock assertion on a single document proves nothing here: measured, pricing a + // 4000-field document takes ~0.013 ms, so a 250 ms threshold would still pass after a + // 3800x regression — exactly the false confidence a loose bound gives. What matters is + // the *shape* of the cost, so this compares two documents and asserts a ratio: pricing + // four times the fields must stay roughly four times the work, not sixteen. A regression + // to quadratic or exponential work fails at any absolute threshold. + // Given + val small = Parser().parseDocument("{ " + (1..1_000).joinToString(" ") { "a$it: ping" } + " }") + val large = Parser().parseDocument("{ " + (1..4_000).joinToString(" ") { "a$it: ping" } + " }") + + // Warm up so JIT compilation is not charged to the first measurement. + repeat(20) { estimateCost(small, limits); estimateCost(large, limits) } + + // When + val smallCost = measureNanoTime { repeat(40) { estimateCost(small, limits) } } + val largeCost = measureNanoTime { repeat(40) { estimateCost(large, limits) } } + + // Then — generous on both sides so a loaded runner cannot trip it, tight enough that + // super-linear work fails. + val ratio = largeCost.toDouble() / smallCost.coerceAtLeast(1L) + assertTrue( + ratio < LINEAR_SCALING_TOLERANCE, + "4x the fields cost ${"%.1f".format(ratio)}x the time, over the " + + "${LINEAR_SCALING_TOLERANCE}x a linear walk allows" + ) + } + @Test @Timeout(value = 10, unit = TimeUnit.SECONDS) fun `should visit nodes linearly on a flat document`() { From f3555ae6430ecb9596f0ac871cd49054079a420f Mon Sep 17 00:00:00 2001 From: Ziedelth Date: Sat, 3 Oct 2026 13:49:52 +0000 Subject: [PATCH 7/9] fix(graphql): close two guard bypasses found by the three-model review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both guards could be stepped around while still reaching the GraphQL handler. URL encoding skipped every check. The interception compared `call.request.local.uri`, so `POST /%67raphql` did not match `/graphql` and fell straight through to the handler, which the router resolves because it decodes. A 300-alias document refused on `/graphql` answered 200 with 4 kB on `/%67raphql`, and a batch there was executed outright. The fix is not `request.path()`: measured, that returns the *encoded* form, so swapping it in changed nothing. The guard now compares both spellings, decoding the path only when it holds a percent escape — `URLDecoder` unconditionally would turn a literal `+` into a space. A fragment ladder hid inside `__schema`. `{ __schema { types { ...F0 } } }` qualified as pure introspection on name alone and skipped both bounds: 422 bytes, HTTP 200 in 2.2 s, cost 201 never measured. `isIntrospectionOnly` now checks whether a fragment spreads the *same* fragment more than once, counted at any depth below it — repetition, not presence. That distinction is what lets the real codegen query through: `...FullType` reaches `...InputValue` once, a bomb reaches its target twice, usually through two different fields. `__typename` is a companion meta field rather than a schema read. It was exempting documents that select only it, including one aliasing it thousands of times; it now accompanies a real schema read without granting the exemption on its own. Two analyses could be walked exhaustively rather than bounded. `depthOf` reported its current level when it ran out of budget, accepting a document whose expansion it never walked; it now reports past every bound. The schema-read probe ran ahead of the budgeted cost walk and expanded a ladder until an answer came back — 329 s on one test — and now carries an allowance of its own. Also fixed: a cancelled client was answered 400 instead of propagating, the refusal message was double-encoded as JSON, `MAX_TRACKED_DEPTH` could not exceed `maxQueryDepth + 1`, and two tests sent a malformed document that measured the wrong thing. Reviewed by three models — gemini-3.8-flash-high, gemini-3.1-pro-high, claude-opus-5-5-high — over four rounds; the last returns ALL FIXED / NO REGRESSION / READY TO MERGE. 80 tests pass. --- graphql/AGENTS.md | 14 + graphql/src/main/kotlin/CostEstimator.kt | 23 +- graphql/src/main/kotlin/GraphQLModules.kt | 50 +++- .../main/kotlin/SecurityInstrumentations.kt | 276 +++++++++++++++++- .../kotlin/CostEstimatorPerformanceTest.kt | 41 ++- .../test/kotlin/GraphQLRateLimitBudgetTest.kt | 19 +- 6 files changed, 392 insertions(+), 31 deletions(-) diff --git a/graphql/AGENTS.md b/graphql/AGENTS.md index 15798d8..41791d6 100644 --- a/graphql/AGENTS.md +++ b/graphql/AGENTS.md @@ -94,6 +94,20 @@ complexity bounds is explicit and lives in `isIntrospectionOnly`, because the in is deep and expensive by nature: lowering the threshold instead would break GraphiQL and every codegen client. +Three rules hold that exemption, and each closes a measured bypass: + +- **Only `__schema` and `__type` grant it** (`META_FIELDS`). `__typename` resolves against the + query type, not the schema, so it carries no introspection work. +- **The document must read the schema somewhere** (`documentReadsSchema`). Otherwise + `{ __typename }` qualifies, and so does a document aliasing it thousands of times — the cheapest + possible bomb, free of charge. `__typename` may still *accompany* a real schema read + (`COMPANION_META_FIELDS`), which is what GraphiQL, Apollo and Relay send. +- **A subtree may not compound through spreads.** Fields under `__schema` are bounded by the + schema's own shape, so a fixed selection count is a fixed cost; a spread that reaches another + spread is not. `{ __schema { types { ...F0 } } }` with a ladder of them qualified as + introspection on name alone and skipped both bounds entirely. A spread reaching only fields is + allowed — that is what a codegen client's `...FullType` looks like. + ## 5. Error Contract A refused document answers **HTTP 400** with a GraphQL `errors` array. GraphQL reports execution diff --git a/graphql/src/main/kotlin/CostEstimator.kt b/graphql/src/main/kotlin/CostEstimator.kt index 3ed7c8c..165484f 100644 --- a/graphql/src/main/kotlin/CostEstimator.kt +++ b/graphql/src/main/kotlin/CostEstimator.kt @@ -65,7 +65,7 @@ fun estimateCost(document: Document, limits: SecurityLimits = SecurityLimits.DEF // its total is exact and must not be clamped: the same number is charged to the rate limiter, // and clamping a legitimately expensive document down to the ceiling would misprice it. return when { - budget.exhausted() -> ceiling + budget.exhausted() || budget.truncated() -> ceiling else -> total.coerceAtLeast(1L) } } @@ -98,7 +98,16 @@ private fun selectionCost( budget: CostBudget, depth: Int ): Long { - if (budget.exhausted() || depth >= MAX_PRICED_DEPTH) { + // Both stops mean "the rest of this document was never priced", so both mark the walk + // truncated. A depth cut that only returned zero would leave the budget untouched and the + // caller would report the partial total as if it were the real one. + if (depth >= MAX_PRICED_DEPTH) { + budget.markTruncated() + + return 0L + } + + if (budget.exhausted()) { return 0L } @@ -148,9 +157,19 @@ private fun fragmentCost( * cheaper than the document really is. */ private class CostBudget(private var visitsLeft: Long) { + private var wasTruncated = false + fun exhausted(): Boolean = visitsLeft <= 0L + /** Records that the walk stopped before reaching the whole document. */ + fun markTruncated() { + wasTruncated = true + } + + fun truncated(): Boolean = + wasTruncated + fun spend() { visitsLeft-- } diff --git a/graphql/src/main/kotlin/GraphQLModules.kt b/graphql/src/main/kotlin/GraphQLModules.kt index 852f356..034b1f9 100644 --- a/graphql/src/main/kotlin/GraphQLModules.kt +++ b/graphql/src/main/kotlin/GraphQLModules.kt @@ -8,6 +8,8 @@ import com.expediagroup.graphql.server.operations.Mutation import com.expediagroup.graphql.server.operations.Query import com.expediagroup.graphql.server.operations.Subscription import fr.shikkanime.core.LoggerFactory +import kotlinx.coroutines.CancellationException +import java.nio.charset.StandardCharsets import java.util.logging.Level import java.util.logging.Logger import graphql.language.Document @@ -29,6 +31,7 @@ import io.ktor.server.plugins.doublereceive.DoubleReceive import io.ktor.server.plugins.ratelimit.RateLimit import io.ktor.server.plugins.ratelimit.RateLimitName import io.ktor.server.plugins.ratelimit.rateLimit +import io.ktor.server.request.path import io.ktor.server.request.receiveText import io.ktor.server.response.respondText import io.ktor.server.routing.Route @@ -158,9 +161,16 @@ private fun Route.bindGraphQLRoute( // answered here and the engine never sees it. intercept(ApplicationCallPipeline.Call) { val call = context - val requestPath = call.request.local.uri.substringBefore('?') - - if (!isGraphQLPath(requestPath, config.normalizedPath)) { +// Both spellings of the path have to be considered. Ktor's router decodes the path + // before matching it, but `path()` hands back the encoded form, so comparing it alone + // let `POST /%67raphql` skip every guard and still reach the handler — measured: a + // 300-alias document refused on `/graphql` answered 200 with 4 kB on `/%67raphql`. + val requestPath = call.request.path() + val decodedPath = requestPath.decodeIfPercentEncoded() + + if (!isGraphQLPath(requestPath, config.normalizedPath) && + !isGraphQLPath(decodedPath, config.normalizedPath) + ) { proceed() return@intercept } @@ -215,6 +225,24 @@ private fun Route.bindGraphQLRoute( private fun isGraphQLPath(requestPath: String, graphqlPath: String): Boolean = requestPath == graphqlPath || requestPath == "$graphqlPath/" +/** + * The percent-decoded form of a path, or the path itself when it holds nothing to decode. + * + * Ktor's router decodes a path segment before matching it, so `/%67raphql` reaches the GraphQL + * handler while `request.path()` still reports `/%67raphql`. A guard that compares only the raw + * form therefore lets an encoded path past every check. Decoding here mirrors what the router + * does, so both spellings of the endpoint are guarded. + * + * @receiver Path as reported by the request. + * @return The decoded path, or the original when it cannot be decoded. + */ +private fun String.decodeIfPercentEncoded(): String = + if (contains('%')) { + runCatching { java.net.URLDecoder.decode(this, StandardCharsets.UTF_8) }.getOrDefault(this) + } else { + this + } + /** Bounds derived from the endpoint configuration. */ internal fun GraphQLConfig.toSecurityLimits(): SecurityLimits = SecurityLimits( @@ -235,7 +263,9 @@ private suspend fun PipelineContext.respondRefusal(message: context.respondText( text = buildString { append("""{"errors":[{"message":""") - append(REQUEST_JSON.encodeToString(JsonPrimitive(message).toString())) + // `JsonPrimitive.toString()` is already a JSON string literal, quotes included, so + // encoding *that* produced a doubly-quoted value. Encoding the raw message is enough. + append(REQUEST_JSON.encodeToString(message)) append("}]}") }, contentType = ContentType.Application.Json, @@ -290,6 +320,11 @@ private suspend fun ApplicationCall.readCachedText(): String? = receiveText() } catch (exception: PayloadTooLargeException) { throw exception + } catch (exception: CancellationException) { + // A client that walked away cancels the call. Swallowing that breaks structured + // concurrency: the coroutine keeps running on a request nobody is waiting for, and the + // cancellation becomes a 400 that blames the payload instead of the disconnect. + throw exception } catch (exception: Exception) { MODULE_LOGGER.log(Level.FINE, "Could not read the request body", exception) @@ -347,9 +382,10 @@ internal sealed interface CarriedQuery { /** * Reads the query a call carries, from its body. * - * Only the body: this module registers the POST route alone, so a GET is answered with 405 by Ktor - * before reaching any guard. Reading the query string as well would price a request the plugin - * never serves, and would keep two code paths alive for one. + * Only the body, and deliberately: this module registers the POST route alone, so there is no + * handler a query string could reach. Note that a GET still crosses the route interception first — + * Ktor answers 405 only once the interception has let the call through — so a bodiless verb is + * refused here as an unreadable document rather than by the router. */ internal suspend fun ApplicationCall.carriedQuery(): CarriedQuery { val body = readCachedText() diff --git a/graphql/src/main/kotlin/SecurityInstrumentations.kt b/graphql/src/main/kotlin/SecurityInstrumentations.kt index 78d4509..68b81e3 100644 --- a/graphql/src/main/kotlin/SecurityInstrumentations.kt +++ b/graphql/src/main/kotlin/SecurityInstrumentations.kt @@ -84,27 +84,255 @@ fun isIntrospectionOnly(document: Document): Boolean { return false } + // A companion meta field may ride along but never carries the exemption by itself: the + // document has to read the schema somewhere. Without this, `{ __typename }` — and a document + // aliasing it thousands of times — would qualify while doing no introspection work at all. + if (!documentReadsSchema(document)) { + return false + } + return operations.all { operation -> - operation.selectionSet.selections.all { isIntrospectionField(it) } + operation.selectionSet.selections.all { + isIntrospectionField(it, documentFragments(document), emptySet()) + } + } +} + +/** + * Whether the document selects a field that actually reads the schema. + * + * `__schema` and `__type` do; `__typename` does not — it resolves against the query type and + * reveals one name. It is the only meta field a document can flood, so it cannot be the thing + * that grants the exemption. + */ +private fun documentReadsSchema(document: Document): Boolean = + document.definitions + .filterIsInstance() + .any { operation -> + operation.selectionSet.selections.any { + it.startsSchemaRead( + fragments = documentFragments(document), + depth = 0, + visited = emptySet(), + budget = SchemaReadBudget(SCHEMA_READ_VISIT_CAP) + ) + } + } + +/** + * Node allowance for the schema-read probe. + * + * The probe answers a yes/no question and runs *before* the cost estimator, which is the walk that + * actually carries a budget. Without an allowance of its own it inherits the hazard it exists to + * classify: a ladder of spreads expanded until an answer came back, measured at 329 s on a + * 24-level document. A safety net, not the primary defence — the exponential shape is cut by + * [MAX_TRACKED_DEPTH] and the per-branch visited set. + */ +private const val SCHEMA_READ_VISIT_CAP = 256 + +/** Remaining visits for one schema-read probe. */ +private class SchemaReadBudget(private var left: Int) { + fun exhausted(): Boolean = + left <= 0 + + fun spend() { + left-- } } -private fun isIntrospectionField(selection: Selection<*>): Boolean = +/** + * Whether this selection reaches a field that reads the schema. + * + * @param fragments Fragment definitions of the document. + * @param depth Nesting level, which bounds a legitimately deep document. + * @param visited Fragment names already walked on this branch, so a cycle terminates. + * @param budget Node allowance, since this runs ahead of the budgeted cost walk. + */ +private fun Selection<*>.startsSchemaRead( + fragments: Map, + depth: Int, + visited: Set, + budget: SchemaReadBudget +): Boolean = + when { + depth > MAX_TRACKED_DEPTH || budget.exhausted() -> false + + this is Field -> name in META_FIELDS + + this is FragmentSpread -> { + val name = this.name + + when { + name in visited -> false + + else -> { + budget.spend() + + fragments[name]?.selectionSet?.selections + ?.any { it.startsSchemaRead(fragments, depth + 1, visited + name, budget) } + ?: false + } + } + } + + this is InlineFragment -> selectionSet.selections.any { + it.startsSchemaRead(fragments, depth + 1, visited, budget) + } + + else -> false + } + +private fun isIntrospectionField( + selection: Selection<*>, + fragments: Map, + expanding: Set +): Boolean = when (selection) { - // Only a meta field *is* introspection. A business field merely containing a meta field is - // a smuggling attempt: exempting it would let a heavy query ride under the exemption. - is Field -> selection.name in META_FIELDS + // Only a meta field *starts* an introspection request. Everything below it is schema + // metadata — `types`, `fields`, `name`, `ofType` qualify by being selected on `__schema`, + // not by name — so sub-fields are not re-checked against META_FIELDS. + // + // What is checked is whether the subtree can multiply its work through spreads. Fields + // under `__schema` are bounded by the schema's own shape, so a fixed selection count costs + // a fixed amount; a spread that expands other spreads does not, and + // `{ __schema { types { ...F0 } } }` with a ladder of them qualified as introspection on + // name alone and skipped both bounds entirely. + is Field -> when { + selection.name in META_FIELDS -> + !carriesExpandingSpread(selection.selectionSet, fragments, expanding) + + // A companion may ride along with a schema read the document already performs. + // `isIntrospectionOnly` requires that read, so a document of nothing but + // `__typename` is priced and bounded like any other. + selection.name in COMPANION_META_FIELDS -> true + + else -> false + } - // An inline fragment wrapping a meta field is still introspection, since it selects + // An inline fragment wrapping meta fields is still introspection, since it selects // nothing else. - is InlineFragment -> selection.selectionSet.selections.all(::isIntrospectionField) + is InlineFragment -> selection.selectionSet.selections.all { + isIntrospectionField(it, fragments, expanding) + } + + // A spread at the root of an operation is introspection only if the fragment it points at + // is, which keeps a codegen client's `...SchemaFragment` working without letting an + // arbitrary fragment claim the exemption. + // A spread is introspection only if the fragment it names is, *and* does not multiply its + // own work. Without the second half the ladder hid behind a named fragment: checking only + // that its body selects meta fields let `{ __schema { ...F0 } }` through while `F0`'s body + // spread another fragment twice per level. + is FragmentSpread -> { + val name = selection.name + + when { + name in expanding -> true + + else -> { + val body = fragments[name]?.selectionSet + + body != null && + !spreadMultiplies(name, fragments, expanding) && + body.selections.all { isIntrospectionField(it, fragments, expanding + name) } + } + } + } - is FragmentSpread -> false else -> false } -private fun isIntrospectionField(selectionSet: SelectionSet): Boolean = - selectionSet.selections.all { isIntrospectionField(it) } +/** + * Whether a selection set's fragment spreads can compound into an unbounded amount of work. + * + * A codegen client's introspection query spreads a handful of named fragments, each *reached once* + * and each selecting fixed fields — `...FullType` reaches `...InputValue` through + * `fields { args { ... } }`, exactly once. That is a fixed amount of work. + * + * The bomb reaches the same fragment **more than once**, so each occurrence adds its work again and + * the total grows multiplicatively: measured, a ladder of spreads doubling per level reached a cost + * of 201 in 422 bytes and skipped the bounds entirely. + * + * Repetition, not mere presence, is the whole distinction. Asking only "is there a spread" refuses + * GraphiQL; following every spread descends a `TypeRef` chain forever, since those nest + * `ofType { ofType { ... } }` by design. + * + * @param selectionSet Selections to inspect, `null` for a leaf field such as `__typename`. + * @param fragments Fragment definitions of the document. + * @param visited Fragment names already walked on this branch, so a cycle terminates. + */ +private fun carriesExpandingSpread( + selectionSet: SelectionSet?, + fragments: Map, + visited: Set +): Boolean { + val selections = selectionSet?.selections ?: return false + + return selections.any { selection -> + when (selection) { + is FragmentSpread -> spreadMultiplies(selection.name, fragments, visited) + is Field -> carriesExpandingSpread(selection.selectionSet, fragments, visited) + is InlineFragment -> carriesExpandingSpread(selection.selectionSet, fragments, visited) + else -> false + } + } +} + +/** + * Whether expanding [name] repeats work, i.e. its body reaches the same spread more than once. + * + * @param name Fragment being expanded. + * @param fragments Fragment definitions of the document. + * @param visited Names already walked on this branch, so a cycle terminates. + */ +private fun spreadMultiplies( + name: String, + fragments: Map, + visited: Set +): Boolean { + if (name in visited) { + return false + } + + val body = fragments[name]?.selectionSet ?: return false + + return body.spreadNamesBelow(visited + name) + .groupingBy { it } + .eachCount() + .values + .any { occurrences -> occurrences > 1 } +} + +/** + * Every fragment spread reachable below this selection set, fields included, counted with + * multiplicity. + * + * Depth matters: `fragment F0 { a: types { ...F1 } b: types { ...F1 } }` repeats `...F1` twice, but + * not twice at its *root* — each sits under its own field, so counting only the root sees none. + * Fragments already on this branch are skipped, so a cycle contributes nothing. + * + * @param visited Names already on this branch, so a cycle terminates. + * @param depth Nesting level, bounding a legitimately deep schema walk. + */ +private fun SelectionSet.spreadNamesBelow( + visited: Set, + depth: Int = 0 +): List { + if (depth > MAX_TRACKED_DEPTH) { + return emptyList() + } + + return selections.flatMap { selection -> + when (selection) { + is FragmentSpread -> if (selection.name in visited) emptyList() else listOf(selection.name) + + is Field -> selection.selectionSet?.spreadNamesBelow(visited, depth + 1) ?: emptyList() + is InlineFragment -> selection.selectionSet.spreadNamesBelow(visited, depth + 1) + else -> emptyList() + } + } +} + + /** * Whether the document breaks one of the [limits]. @@ -166,7 +394,16 @@ private fun depthOf( limits: SecurityLimits, budget: Budget ): Int { - if (current >= MAX_TRACKED_DEPTH || budget.exhausted()) { + // An exhausted budget means the walk gave up before finishing, not that the document is + // shallow. Reporting `current` there accepted a document whose expansion was never walked, so + // a truncated walk reports past every possible bound instead. + if (budget.exhausted()) { + return Int.MAX_VALUE + } + + // Stopping at the configured bound, capped, keeps the walk bounded by the same number the + // caller will compare it against. + if (current >= minOf(MAX_TRACKED_DEPTH, limits.maxQueryDepth + 1)) { return current } @@ -267,4 +504,21 @@ private const val DEPTH_VISITS_PER_BYTE = 64L private const val FRAGMENT_VISIT_FACTOR = 256L /** The root fields that make a document an introspection request. */ +/** + * The root fields that read the schema, and so can *start* an introspection request. + * + * `__typename` is deliberately absent: it resolves against the query type rather than the schema, + * so it carries no introspection work. Listing it here would exempt any document whose selections + * are all `__typename` — including one that aliases it thousands of times — which hands the + * exemption to the cheapest possible bomb. It is accepted as an addition instead, in + * [COMPANION_META_FIELDS], because GraphiQL, Apollo and Relay all send it. + */ private val META_FIELDS = setOf("__schema", "__type") + +/** + * Meta fields that may accompany an introspection request without granting the exemption. + * + * `__typename` costs one unit and reveals the query type's name. Refusing a client that sends it + * alongside `__schema` would break the codegen tools the exemption exists for. + */ +private val COMPANION_META_FIELDS = setOf("__typename") diff --git a/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt b/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt index 5fe41a7..1aa1a8e 100644 --- a/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt +++ b/graphql/src/test/kotlin/CostEstimatorPerformanceTest.kt @@ -8,6 +8,7 @@ import org.junit.jupiter.api.Timeout import java.util.concurrent.TimeUnit import kotlin.system.measureNanoTime import kotlin.test.assertEquals +import kotlin.test.assertEquals import kotlin.test.assertTrue /** @@ -34,6 +35,14 @@ class CostEstimatorPerformanceTest { * noise of a shared runner while still failing on any super-linear work. */ const val LINEAR_SCALING_TOLERANCE = 8.0 + + /** + * How many branches a 22-level ladder would expand to with no depth bound at all. + * + * 2^22 is past `Int`, so the walk cannot reach it — that is the point. The measured visit + * count has to stay far below it for the depth bound to be what stopped the walk. + */ + const val EXPLOSIVE_LADDER_UNBOUNDED = 1_000_000L } private fun fragmentLadder(levels: Int): String { @@ -64,10 +73,13 @@ class CostEstimatorPerformanceTest { // When val visited = countCostNodes(document, limits) - // Then + // Then — strictly under, not `<=`: the counter is derived from the budget itself, so + // `<=` holds by construction and would pass even if the walk looped forever. `<` is what + // says the document finished inside its allowance. assertTrue( - visited <= limits.costVisitBudget, - "cost walk visited $visited nodes, over the budget of ${limits.costVisitBudget}" + visited < limits.costVisitBudget, + "cost walk used the whole budget ($visited of ${limits.costVisitBudget}); " + + "the walk must finish inside its allowance, not be cut off" ) } @@ -147,9 +159,10 @@ class CostEstimatorPerformanceTest { @Test @Timeout(value = 10, unit = TimeUnit.SECONDS) fun `should not grow with the level count`() { - // Two ladders of very different explosive size must both stop at the budget rather than - // scaling with their level count. The small one is measured under the budget, so it is - // the *ceiling* that has to hold, not an equality between the two. + // Two ladders of very different explosive size must cost the same bounded amount of + // work: the walk stops at the configured depth either way, so the fragment count does + // not reach it. Before that bound existed, the deeper one took thousands of times + // longer. // Given val small = Parser().parseDocument(fragmentLadder(levels = 12)) val large = Parser().parseDocument(fragmentLadder(levels = 22)) @@ -158,14 +171,24 @@ class CostEstimatorPerformanceTest { val smallVisits = countDepthNodes(small, limits) val largeVisits = countDepthNodes(large, limits) - // Then + // Then — the walk stops as soon as it is past the configured depth bound, which is + // long before the node budget could run out. Asserting `== depthVisitBudget` here + // would pin the weaker of the two stops; this one is the one that keeps the walk short. assertTrue( largeVisits <= limits.depthVisitBudget, "the larger ladder exceeded the budget: $largeVisits" ) assertTrue( - largeVisits > smallVisits, - "a larger ladder should still visit more branches, got $smallVisits then $largeVisits" + largeVisits < EXPLOSIVE_LADDER_UNBOUNDED, + "an explosive ladder must stop well before the unbounded count, got $largeVisits" + ) + // Both ladders stop at the depth bound, so they cost the *same* bounded number of + // visits. That equality is the property: a walk bounded by the configured depth does + // not grow with the fragment count, which is what the exponential regression broke. + assertEquals( + smallVisits, + largeVisits, + "a deeper ladder must not cost more branches once the depth bound applies" ) } } diff --git a/graphql/src/test/kotlin/GraphQLRateLimitBudgetTest.kt b/graphql/src/test/kotlin/GraphQLRateLimitBudgetTest.kt index 7ebc035..039ea4b 100644 --- a/graphql/src/test/kotlin/GraphQLRateLimitBudgetTest.kt +++ b/graphql/src/test/kotlin/GraphQLRateLimitBudgetTest.kt @@ -121,7 +121,13 @@ class GraphQLRateLimitBudgetTest { graphql(defaults) // When - post("""{"query":"${aliases(150)}}"}""") + // `aliases` already closes its selection set, so the body must not close it twice — + // an extra brace made this a parse failure, and the test then measured the + // unreadable-body penalty instead of the cost of a heavy document. + val heavy = post("""{"query":"${aliases(150)}"}""") + + assertEquals(HttpStatusCode.OK, heavy.status, "a query within the bound must be answerable") + val cheap = post("""{"query":"{ ping }"}""").status // Then @@ -140,7 +146,16 @@ class GraphQLRateLimitBudgetTest { graphql(defaults) // When - post("""{"query":"${aliases(400)}}"}""") + // 400 aliases is past the complexity bound, so this one *is* refused by the bounds — + // with a single closing brace, not by the parser. + val refused = post("""{"query":"${aliases(400)}"}""") + + assertEquals( + HttpStatusCode.BadRequest, + refused.status, + "the document must be refused on its own cost" + ) + val cheap = post("""{"query":"{ ping }"}""").status // Then From c91c3728e59cd21d0c8e1052669f06d4bf63d6d1 Mon Sep 17 00:00:00 2001 From: Ziedelth <45169338+Ziedelth@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:53:57 +0200 Subject: [PATCH 8/9] feat: UUID scalar, typed parameter refusal and repository findAll (#54) * feat(graphql): expose a UUID scalar for kotlin.uuid.Uuid graphql-kotlin has no built-in scalar for kotlin.uuid.Uuid: a DTO carrying a Uuid identifier fails schema generation and forces consumers into string-typed duplicates. UuidScalar speaks the canonical hyphenated hexadecimal form in both directions, and UuidSchemaGeneratorHooks registers it for every Uuid the schema meets while keeping the framework default hooks. * feat(ktor): answer 400 when a parameter fails its type conversion A malformed path or query parameter used to resolve to null and surface as a 500 through the call. The resolver now refuses the request with InvalidParameterException, and the request handler translates it to an HTTP 400 carrying a MessageDto error. * feat(exposed): add AbstractRepository.findAll() A whole-table read now comes from the repository layer: consumers call super.findAll() instead of reaching for the Exposed entity class directly. * fix(ktor): an optional parameter never refuses the request A nullable parameter whose value does not match its type resolved through the conversion failure path and refused the request, making an optional parameter behave like a required one. It now falls back to null, exactly like an absent value; only a non-nullable parameter answers 400. * style: name the UUID scalar uuidScalar and guard the parameter conversion A top-level value is camelCase: uuidScalar, not UuidScalar. The parameter conversion now guards its refusal first (a non-nullable parameter with an unconvertible value throws immediately) and assigns afterwards, instead of nesting the reason inside a conditional expression. * refactor: factor the parameter conversion and document the new contracts The query and path branches each carried their own conversion and refusal block; both now call parseParameter, which refuses first and returns the converted value. Guidelines updated with the new rules: typed controller parameters with the 400 contract, optional parameters never refusing the request, camelCase properties, and the UUID scalar documented in the graphql module. --- .../kotlin/repositories/AbstractRepository.kt | 8 ++ graphql/AGENTS.md | 9 +- graphql/src/main/kotlin/GraphQLModules.kt | 1 + graphql/src/main/kotlin/UuidScalar.kt | 121 ++++++++++++++++++ guidelines/API_CONVENTIONS.md | 5 + guidelines/DATABASE.md | 2 +- guidelines/KOTLIN_CONVENTIONS.md | 4 + .../main/kotlin/ControllerArgumentResolver.kt | 40 +++++- .../main/kotlin/ControllerRequestHandler.kt | 3 + .../main/kotlin/InvalidParameterException.kt | 11 ++ .../kotlin/ControllerArgumentResolverTest.kt | 25 +++- 11 files changed, 222 insertions(+), 7 deletions(-) create mode 100644 graphql/src/main/kotlin/UuidScalar.kt create mode 100644 ktor/src/main/kotlin/InvalidParameterException.kt diff --git a/exposed/src/main/kotlin/repositories/AbstractRepository.kt b/exposed/src/main/kotlin/repositories/AbstractRepository.kt index 3cfab76..b67fa18 100644 --- a/exposed/src/main/kotlin/repositories/AbstractRepository.kt +++ b/exposed/src/main/kotlin/repositories/AbstractRepository.kt @@ -42,6 +42,14 @@ abstract class AbstractRepository>(protected val ent protected fun ENTITY.applyFlush(): ENTITY = this.apply { flush() } + /** + * Finds every entity of the table. + * + * @return a lazily evaluated Exposed entity sequence containing every entity. + */ + fun findAll(): SizedIterable = + entityClass.all() + /** * Finds every entity whose identifier belongs to [ids]. * diff --git a/graphql/AGENTS.md b/graphql/AGENTS.md index 41791d6..02bd3da 100644 --- a/graphql/AGENTS.md +++ b/graphql/AGENTS.md @@ -116,7 +116,14 @@ not a failed execution, and a caller branching on the status code must tell the Never surface an internal exception message to a client. -## 6. Out of Scope for This Module +## 6. Identifiers — `UUID` scalar + +graphql-kotlin has no built-in scalar for `kotlin.uuid.Uuid`. `uuidScalar` (with `UuidCoercing`) +exposes it as a `UUID` scalar and `UuidSchemaGeneratorHooks` registers it for every `Uuid` the schema +meets, so a DTO shared with REST carries its identifiers as `Uuid` and never as a string duplicate. +The scalar speaks the canonical hyphenated hexadecimal form in both directions. + +## 7. Out of Scope for This Module - Websocket subscriptions: the artifact ships `getSubscriptionServer()`, but the route does not advertise subscriptions, and `schemaSubscriptions` should stay empty unless that changes. diff --git a/graphql/src/main/kotlin/GraphQLModules.kt b/graphql/src/main/kotlin/GraphQLModules.kt index 034b1f9..4748e0a 100644 --- a/graphql/src/main/kotlin/GraphQLModules.kt +++ b/graphql/src/main/kotlin/GraphQLModules.kt @@ -109,6 +109,7 @@ fun Application.configureGraphQL( queries = schemaQueries mutations = schemaMutations subscriptions = schemaSubscriptions + hooks = UuidSchemaGeneratorHooks() } engine { diff --git a/graphql/src/main/kotlin/UuidScalar.kt b/graphql/src/main/kotlin/UuidScalar.kt new file mode 100644 index 0000000..a74e731 --- /dev/null +++ b/graphql/src/main/kotlin/UuidScalar.kt @@ -0,0 +1,121 @@ +package fr.shikkanime.graphql + +import com.expediagroup.graphql.generator.hooks.FlowSubscriptionSchemaGeneratorHooks +import com.expediagroup.graphql.generator.hooks.SchemaGeneratorHooks +import graphql.GraphQLContext +import graphql.execution.CoercedVariables +import graphql.language.StringValue +import graphql.language.Value +import graphql.schema.Coercing +import graphql.schema.CoercingParseLiteralException +import graphql.schema.CoercingParseValueException +import graphql.schema.CoercingSerializeException +import graphql.schema.GraphQLScalarType +import graphql.schema.GraphQLType +import java.util.Locale +import kotlin.reflect.KType +import kotlin.uuid.Uuid + +/** + * GraphQL scalar carrying a [Uuid] across the schema. + * + * graphql-kotlin maps only a handful of Kotlin types to built-in scalars, so a shared DTO holding + * a [Uuid] identifier would otherwise fail schema generation and force a duplicated DTO typed on + * strings. The scalar speaks the canonical hyphenated hexadecimal form in both directions. + */ +val uuidScalar: GraphQLScalarType = + GraphQLScalarType.newScalar() + .name("UUID") + .description("kotlin.uuid.Uuid rendered as its hyphenated hexadecimal string") + .coercing(UuidCoercing()) + .build() + +/** + * Reads and writes [Uuid] values for [uuidScalar]. + * + * Every direction funnels through `Uuid.parseOrNull` and `Uuid.toString`, so the schema, the + * variables and the serialized response all agree on the same textual form. + */ +class UuidCoercing : Coercing { + /** + * Renders a resolver result as its textual identifier. + * + * @param dataFetcherResult value produced by a resolver, expected to be a [Uuid]. + * @param graphQLContext context of the running operation. + * @param locale locale of the running operation. + * @return the hyphenated hexadecimal form of [dataFetcherResult]. + * @throws CoercingSerializeException when the result is not a [Uuid]. + */ + override fun serialize( + dataFetcherResult: Any, + graphQLContext: GraphQLContext, + locale: Locale + ): String = + (dataFetcherResult as? Uuid)?.toString() + ?: throw CoercingSerializeException("Cannot serialize $dataFetcherResult as a UUID") + + /** + * Parses a variable value into a [Uuid]. + * + * @param input raw variable value, expected to be a textual identifier. + * @param graphQLContext context of the running operation. + * @param locale locale of the running operation. + * @return the parsed identifier. + * @throws CoercingParseValueException when [input] is not a valid identifier. + */ + override fun parseValue(input: Any, graphQLContext: GraphQLContext, locale: Locale): Uuid = + (input as? String)?.let(Uuid::parseOrNull) + ?: throw CoercingParseValueException("Cannot parse $input as a UUID") + + /** + * Parses an inline literal into a [Uuid]. + * + * @param input literal node from the query document. + * @param variables coerced variables of the running operation. + * @param graphQLContext context of the running operation. + * @param locale locale of the running operation. + * @return the parsed identifier. + * @throws CoercingParseLiteralException when [input] is not a valid identifier. + */ + override fun parseLiteral( + input: Value<*>, + variables: CoercedVariables, + graphQLContext: GraphQLContext, + locale: Locale + ): Uuid = + (input as? StringValue)?.value?.let(Uuid::parseOrNull) + ?: throw CoercingParseLiteralException("Cannot parse $input as a UUID") + + /** + * Renders a [Uuid] back into a literal node. + * + * @param input identifier to render. + * @param graphQLContext context of the running operation. + * @param locale locale of the running operation. + * @return the identifier as a string literal node. + */ + override fun valueToLiteral(input: Any, graphQLContext: GraphQLContext, locale: Locale): Value<*> = + StringValue.of((input as Uuid).toString()) +} + +/** + * Schema hooks exposing [uuidScalar] for every [Uuid] the schema meets. + * + * Every other hook keeps the framework default ([FlowSubscriptionSchemaGeneratorHooks]): the + * scalar is a widening of the generator, not a replacement of its behaviour. + */ +class UuidSchemaGeneratorHooks( + private val delegate: SchemaGeneratorHooks = FlowSubscriptionSchemaGeneratorHooks() +) : SchemaGeneratorHooks by delegate { + /** + * Resolves the GraphQL type of a Kotlin type. + * + * @param type Kotlin type the generator is building a schema type for. + * @return [uuidScalar] when [type] is a [Uuid], the delegate answer otherwise. + */ + override fun willGenerateGraphQLType(type: KType): GraphQLType? = + if (type.classifier == Uuid::class) + uuidScalar + else + delegate.willGenerateGraphQLType(type) +} diff --git a/guidelines/API_CONVENTIONS.md b/guidelines/API_CONVENTIONS.md index 2bdc43f..d12281b 100644 --- a/guidelines/API_CONVENTIONS.md +++ b/guidelines/API_CONVENTIONS.md @@ -24,6 +24,11 @@ class UserController { - Use `@PathParam("name")` for route path segments. - Use `@RequestBody` for JSON payload deserialization. - Add `@Valid` on parameters to automatically validate the deserialized object via `Validator.validate(...)`. +- Type a parameter with its real domain type, never with `String` to re-parse it: + `@PathParam("id") id: Uuid`. +- A non-nullable parameter whose value does not match its type is refused with an HTTP 400 carrying + a `MessageDto` error (`InvalidParameterException`). A nullable parameter is optional by nature: an + absent or unconvertible value resolves to `null` and never refuses the request. ## 3. Standardized Response Payloads diff --git a/guidelines/DATABASE.md b/guidelines/DATABASE.md index 8391141..a4167aa 100644 --- a/guidelines/DATABASE.md +++ b/guidelines/DATABASE.md @@ -22,7 +22,7 @@ Operations modifying state or executing Exposed queries must run within a transa ## 3. AbstractRepository & Upsert DSL All Exposed repositories should inherit from `AbstractRepository`. -It provides standard `findById`, `findAllByIds`, and a fluent upsert DSL: +It provides standard `findAll`, `findById`, `findAllByIds`, and a fluent upsert DSL: ```kotlin override fun persist(id: Int?, data: String): MyEntity { diff --git a/guidelines/KOTLIN_CONVENTIONS.md b/guidelines/KOTLIN_CONVENTIONS.md index a2d4860..67ada13 100644 --- a/guidelines/KOTLIN_CONVENTIONS.md +++ b/guidelines/KOTLIN_CONVENTIONS.md @@ -5,6 +5,10 @@ - **Prefer immutability**: use `val`, read-only collections (`List`, `Map`), and `data class`. - **Add KDoc comments** to all public framework classes, interfaces, annotations, and functions to document behavior and rules. - **Leverage Kotlin features safely**: use null safety features (`?.`, `?:`, `let`), inline functions where appropriate, and reflection extension functions cleanly. +- **Name a property in camelCase, a type in PascalCase**: a top-level value reads `uuidScalar`, never + `UuidScalar`, which would name a type. +- **Refuse first, then assign**: a guard `if (invalid) throw ...` at the top of the block followed by + the plain assignment, never the refusal nested inside a conditional expression. ```kotlin fun getLogger(clazz: Class<*>): Logger = diff --git a/ktor/src/main/kotlin/ControllerArgumentResolver.kt b/ktor/src/main/kotlin/ControllerArgumentResolver.kt index 65ae474..9d06202 100644 --- a/ktor/src/main/kotlin/ControllerArgumentResolver.kt +++ b/ktor/src/main/kotlin/ControllerArgumentResolver.kt @@ -14,6 +14,32 @@ import kotlin.uuid.Uuid * Resolves controller function arguments from a Ktor application call. */ internal object ControllerArgumentResolver { + /** + * Converts a textual parameter to the type of [kParameter], refusing a required parameter whose + * value does not match its type. + * + * A nullable parameter is optional by nature: an unconvertible value resolves to `null`, exactly + * like an absent value, so an optional parameter never refuses the request. A non-nullable + * parameter is required: an unconvertible value is a client error. + * + * @param kParameter controller function parameter being resolved. + * @param name HTTP name of the parameter, used in the refusal message. + * @param stringValue raw parameter value. + * @return the converted value, or `null` for a nullable parameter with no convertible value. + * @throws InvalidParameterException when a non-nullable parameter value fails its conversion. + */ + private fun parseParameter(kParameter: KParameter, name: String, stringValue: String): Any? { + val kClass = kParameter.type.classifier as? KClass<*> + val parsed = parseValue(kClass, stringValue) + + if (!kParameter.type.isMarkedNullable && parsed == null) + throw InvalidParameterException( + "Parameter '$name' must be a valid ${kClass?.simpleName ?: "value"}" + ) + + return parsed + } + /** * Converts a textual HTTP parameter to the type expected by a controller function. * @@ -69,8 +95,11 @@ internal object ControllerArgumentResolver { val stringValue = call.request.queryParameters[queryParamAnnotation.name] if (stringValue != null) { - val kClass = kParameter.type.classifier as? KClass<*> - args[kParameter] = parseValue(kClass, stringValue) + args[kParameter] = parseParameter( + kParameter, + queryParamAnnotation.name, + stringValue + ) } else if (!queryParamAnnotation.required && kParameter.type.isMarkedNullable) { args[kParameter] = null } @@ -81,8 +110,11 @@ internal object ControllerArgumentResolver { val stringValue = call.parameters[pathParamAnnotation.name] if (stringValue != null) { - val kClass = kParameter.type.classifier as? KClass<*> - args[kParameter] = parseValue(kClass, stringValue) + args[kParameter] = parseParameter( + kParameter, + pathParamAnnotation.name, + stringValue + ) } } diff --git a/ktor/src/main/kotlin/ControllerRequestHandler.kt b/ktor/src/main/kotlin/ControllerRequestHandler.kt index 9e72e46..6a52a68 100644 --- a/ktor/src/main/kotlin/ControllerRequestHandler.kt +++ b/ktor/src/main/kotlin/ControllerRequestHandler.kt @@ -65,6 +65,9 @@ internal object ControllerRequestHandler { is ObjectNotValidException -> call.respond(HttpStatusCode.BadRequest, MessageDto.error(cause.message)) + is InvalidParameterException -> + call.respond(HttpStatusCode.BadRequest, MessageDto.error(cause.message)) + else -> call.respond(HttpStatusCode.InternalServerError, MessageDto.error("An unexpected error occurred")) } diff --git a/ktor/src/main/kotlin/InvalidParameterException.kt b/ktor/src/main/kotlin/InvalidParameterException.kt new file mode 100644 index 0000000..f78fe43 --- /dev/null +++ b/ktor/src/main/kotlin/InvalidParameterException.kt @@ -0,0 +1,11 @@ +package fr.shikkanime.ktor + +/** + * Indicates that a request parameter could not be converted to the type the controller expects. + * + * A malformed path, query or body parameter is a client error, not a server failure: the request + * handler translates it to an HTTP 400 carrying a [fr.shikkanime.ktor.dtos.MessageDto] error. + * + * @property message human-readable description naming the parameter and the expected type. + */ +class InvalidParameterException(override val message: String) : IllegalArgumentException(message) diff --git a/ktor/src/test/kotlin/ControllerArgumentResolverTest.kt b/ktor/src/test/kotlin/ControllerArgumentResolverTest.kt index 784348a..9d7c54d 100644 --- a/ktor/src/test/kotlin/ControllerArgumentResolverTest.kt +++ b/ktor/src/test/kotlin/ControllerArgumentResolverTest.kt @@ -30,6 +30,11 @@ class ControllerArgumentResolverTest { @QueryParam("optional", required = false) optional: String? ): ResponseEntity = ResponseEntity.ok("ok") + + fun optionalEndpoint( + @QueryParam("id", required = false) id: Uuid? + ): ResponseEntity = + ResponseEntity.ok(id?.toString() ?: "none") } @Nested @@ -84,7 +89,7 @@ class ControllerArgumentResolverTest { } @Test - suspend fun `should return null when numeric or boolean conversions fail`() { + suspend fun `should refuse the request when a numeric or boolean conversion fails`() { // Given val call = mockk(relaxed = true) every { call.request.queryParameters["longVal"] } returns "invalid_number" @@ -93,6 +98,24 @@ class ControllerArgumentResolverTest { val controller = SampleController() val function = SampleController::typeEndpoint + // When + val exception = org.junit.jupiter.api.assertThrows { + ControllerArgumentResolver.resolve(call, function, controller) + } + + // Then + assertTrue(exception.message.contains("must be a valid")) + } + + @Test + suspend fun `should resolve an optional parameter to null when its value does not match its type`() { + // Given + val call = mockk(relaxed = true) + every { call.request.queryParameters["id"] } returns "not-a-uuid" + + val controller = SampleController() + val function = SampleController::optionalEndpoint + // When val args = ControllerArgumentResolver.resolve(call, function, controller) From af62de394101da02e033db90fa29ea78db0d57ad Mon Sep 17 00:00:00 2001 From: Ziedelth <45169338+Ziedelth@users.noreply.github.com> Date: Fri, 16 Oct 2026 13:04:38 +0000 Subject: [PATCH 9/9] chore: bump version to 0.0.22-SNAPSHOT --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 4e52a88..acb01f4 100644 --- a/gradle.properties +++ b/gradle.properties @@ -7,5 +7,5 @@ org.gradle.caching=true org.gradle.configuration-cache=true # Framework version -version=0.0.21-SNAPSHOT +version=0.0.22-SNAPSHOT