JS: recognize Fastify servers reached through chainable server methods - #22544
Open
dtorres-fgf wants to merge 2 commits into
Open
JS: recognize Fastify servers reached through chainable server methods#22544dtorres-fgf wants to merge 2 commits into
dtorres-fgf wants to merge 2 commits into
Conversation
`Fastify::server()` tracks the server from the `fastify()` invocation, but did not step through methods that configure the instance and return it, such as `withTypeProvider()` and the `set*` family. On a server built as `fastify().withTypeProvider<T>()` the instance was therefore not recognized as a server at all, so neither the plugins registered on it nor the routes declared on it were attributed to it. The effect ran in both directions. Routes on such an instance could be missed entirely, and where they were still reported through another model, a globally registered plugin such as `@fastify/rate-limit` was not seen as guarding them, which produced false positives in `js/missing-rate-limiting`. Route-registering methods (`register`, `addHook`, and the shorthand route methods) also return the server, but they are deliberately left out: they already have a meaning in the routing model, so including them would change the shape of the routing tree rather than only how a server reference is resolved. The step is added inside the type-tracked predicate rather than to the public one, so a chained instance is still resolved when it crosses a function boundary, for example when it is returned from a factory function. A test covers that case, and it fails if the step is placed in the public predicate instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The method list omits a valid chainable configuration API, and the change note is future-dated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extends Fastify modeling so chained configuration calls preserve server identity across routing and security analysis.
Changes:
- Adds chainable Fastify server-method tracking.
- Adds direct, multi-method, and factory-based test cases.
- Updates expected analysis results and change notes.
File summaries
| File | Description |
|---|---|
Fastify.qll |
Models chained server-returning methods. |
tst.js |
Tests rate-limiting with chained servers. |
MissingRateLimiting.expected |
Updates expected security alerts. |
src/fastify.js |
Adds a chained-server library fixture. |
tests.expected |
Updates Fastify library-test results. |
2026-09-11-fastify-chainable-config-methods.md |
Documents the analysis improvement. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ote in UTC `addContentTypeParser` returns the server, so it belongs in the list. Its type declaration says `void`, which is why it was missed: it is declared as a property of an interface type rather than as a method, so it did not look like the `set*` family. Checking the rest of the instance against the runtime rather than against the declarations then showed that `ready` never returns the server, in either of its call forms, despite the declaration saying it does, and that `after` returns it only when given a callback. Both are now excluded, which also makes the predicate exactly what its name says. `onClose` returns the server too, but it registers a hook, so it is excluded for the same reason as `addHook`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Fixes #22412.
Fastify::server()tracks the server from thefastify()invocation but doesn't step through methods that configure the instance and return it, likewithTypeProvider()and theset*family. So on a server built asfastify().withTypeProvider<T>(), the instance isn't recognized as a server at all, and neither the plugins registered on it nor the routes declared on it get attributed to it.That loses results in both directions, which is a bit broader than what I described in the issue. Routes on such an instance can be missed entirely. And where they do still get reported through another model, a globally registered
@fastify/rate-limitisn't seen as guarding them, which is the false positive I filed.The change
A private predicate lists the chainable configuration methods, and
server()treats a call to one of them as still referring to the server. Every name in the list was checked against the runtime rather than against the type declarations, which matters more than I expected (see below).register,addHook,onCloseand the shorthand route methods also return the server, but I've left them out. They already have a meaning in the routing model, so including them would reshape the routing tree rather than just resolve a server reference. Happy to add them if you'd rather.afterandreadyare left out for a different reason.afterreturns the server only when it's given a callback, andreadynever returns it in either call form, even though its declaration says it does.The step goes inside the type-tracked predicate rather than on the public one. That way a chained instance still resolves when it crosses a function boundary, such as being returned from a factory.
tst.jscovers that case, and it fails if the step is put on the public predicate instead.Tests
query-tests/Security/CWE-770/MissingRateLimit/tst.js: a chained instance with a global limiter (no alert), one with no limiter (alerts, so the test can't pass just by the routes being invisible), several methods chained together, and the factory case.library-tests/frameworks/fastify: a chained server whoseRouteSetupnow resolves throughgetServer()back to itsServerDefinition.Measured on a real app
A Fastify + TypeBox application with one global
@fastify/rate-limit, CLI 2.27.0: 18 alerts before, 6 after. Comparing the result sets rather than the counts, that's 12 removed and 0 added. The 6 that remain are unrelated to this change and present either way (an authpreHandlerhook treated as a route handler, plus two health-check handlers).These supersede the 13 to 4 figures in the issue thread, which I measured a couple of weeks ago on an older commit of that app, with CLI 2.26.3 and a narrower method list.
codeql test runpasses for the Fastify library tests and all of CWE-770, plus CWE-094, CWE-117 and CWE-352, which also have Fastify fixtures. Formatted withcodeql query format, and no compiler warnings.