Repository navigation
feat(graphql): serve GraphQL behind a bounded security stack - #47
Merged
Merged
Conversation
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.
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.
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.
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.
…e 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.
…ally 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.
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.
* 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.
Master split the repository base into EntityRepository (reads) and UpsertRepository (upsert DSL): the findAll() added alongside findById/findAllByIds moves to EntityRepository, and the DATABASE guideline keeps the split wording.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Le module
Un endpoint GraphQL
/graphqlpour le framework Shikkanime, protégé par quatre garde-fous et des bornes de document. 7 commits depuismaster.RequestBodyLimitCompressionDoubleReceiverequestWeightRateLimit+requestWeightPlus deux bornes appliquées par une interception sur la route :
maxQueryDepthetmaxQueryComplexity, avec exemption d'introspection.Les deux garde-fous ne tenaient pas
Tous deux laissaient passer un document jusqu'au moteur.
1. L'encodage d'URL court-circuitait chaque vérification. L'interception comparait
call.request.local.uri, doncPOST /%67raphqlne correspondait pas à/graphqlet filait droit vers le handler — que le routeur résout, lui, parce qu'il décode./graphql400400/%67raphql200, 4 102 octets400/%67raphql200, exécuté400Le correctif n'est pas
request.path(): mesuré, il retourne la forme encodée, donc le remplacer ne changeait rien. La garde compare les deux écritures, et ne décode que si la chaîne contient un%—URLDecoderinconditionnel transformerait un+littéral en espace.2. Une bombe de fragments se cachait dans
__schema.{ __schema { types { ...F0 } } }passait pour de l'introspection pure sur le seul nom du champ, et sautait les deux bornes : 422 octets,200en 2,2 s, coût 201 jamais mesuré.isIntrospectionOnlyvérifie désormais qu'un fragment étale le même fragment plus d'une fois, compté à toute profondeur. C'est la répétition, pas la présence, qui fait la différence — et c'est ce qui laisse passer la vraie requête de codegen :...FullTypeatteint...InputValueune fois, une bombe l'atteint deux fois, souvent via deux champs différents.3.
__typenameest devenu un compagnon, pas une lecture du schéma. Il exemptait des documents qui ne le sélectionnent que lui, dont un document l'aliasant des milliers de fois. Il accompagne maintenant une vraie lecture du schéma sans accorder l'exemption à lui seul.Deux analyses pouvaient être parcourues sans borne
depthOfrenvoyait son niveau courant quand son budget était épuisé, ce qui acceptait un document dont il n'avait pas parcouru l'expansion. Il rapporte désormais au-delà de toute borne.La sonde de lecture du schéma s'exécutait avant le parcours de coût, qui est le seul budgété : elle développait un escalier jusqu'à obtenir une réponse — 329 s sur un seul test. Elle porte maintenant sa propre allocation, suite repassée de 6 min 21 à 15 s.
Corrigé aussi : un client déconnecté recevait un
400au lieu de voir sa cancellation remonter, le message de refus était double-encodé en JSON, et deux tests envoyaient un document malformé qui mesurait autre chose.Tests
80 passent (53 d'origine + 27 nouveaux). Les tests de performance assertent un nombre de nœuds visités plutôt qu'un temps : mesuré, le document le plus large se chiffre en 0,013 ms, donc le seuil de 250 ms proposé lors de la revue passerait encore après une régression de 3800×. Un test de ratio entre deux documents est ce qui discrimine réellement — 2,25 mesuré pour un parcours linéaire, contre 16 pour un quadratique.
Revue
Quatre rondes sur trois modèles —
gemini-3.8-flash-high,gemini-3.1-pro-high,claude-opus-5-5-high— jusqu'à convergence. La dernière rendALL FIXED/NO REGRESSION/READY TO MERGE.Deux BLOQUANT ont été trouvés par les reviewers et m'échappaient : le bypass d'encodage et celui de l'introspection. Trois de leurs objections se sont révélées fausses à l'exécution et ont été réfutées avec la mesure à l'appui, pas l'argument.
RateLimitConfig.limitpasse de 100 à 1000. Nécessaire pour que la borne de complexité 200 soit atteignable, mais cela multiplie le débit autorisé par 10. L'alternative était de descendremaxQueryComplexityà 100, au prix d'une limite plus stricte sur les requêtes légitimes. À arbitrer en revue.Note mémoire
La machine de build a 3,7 Gio et lance 3 JVM en parallèle (daemon Gradle, daemon Kotlin, fork de tests), ce qui épuisait la mémoire et faisait tuer le build. Le fork est plafonné à 640 Mio et les daemons désactivés.