fix(fastify): anchor middleware path regex for non-prefix routes#16862
fix(fastify): anchor middleware path regex for non-prefix routes#16862pierluigilenoci wants to merge 1 commit into
Conversation
Coverage Report for CI Build 420Coverage remained the same at 90.028%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Hi, FastifyAdapter.createMiddlewareFactory now uses prefix matching (end:false) when requestMethod is RequestMethod.ALL, which can cause middleware registered for a specific path/method=ALL to also run on sub-routes. This diverges from ExpressAdapter behavior (RequestMethod.ALL maps to app.all => exact-path match) and can cause middleware to execute on unintended endpoints. Severity: action required | Category: correctness How to fix: Prefix-match only for -1 Agent prompt to fix - you can give this to your LLM of choice:
Found by Qodo. Free code review for open-source maintainers. |
There was a problem hiding this comment.
Pull request overview
This PR fixes NestJS Fastify middleware route matching so that middleware registered with a prefix string (e.g., forRoutes('/my-prefix')) also applies to sub-routes provided by Fastify plugins registered under that prefix (e.g., instance.register(plugin, { prefix: '/my-prefix' })), aligning behavior more closely with Express-style prefix middleware.
Changes:
- Update Fastify adapter middleware route regex generation to optionally use prefix matching (
pathToRegexp(..., { end: false })) depending on the route “method” passed intocreateMiddlewareFactory. - Add an e2e regression test covering Fastify plugin routes registered with a prefix and middleware registered via
forRoutes('/my-prefix').
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/platform-fastify/adapters/fastify-adapter.ts | Adjusts path-to-regexp matching strategy (exact vs prefix) when registering middie middleware. |
| integration/hello-world/e2e/middleware-fastify.spec.ts | Adds an e2e scenario to validate middleware matching for prefixed Fastify plugin routes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -693,10 +705,10 @@ export class FastifyAdapter< | |||
| } | |||
|
|
|||
| try { | |||
| let { regexp: re } = pathToRegexp(normalizedPath); | |||
| re = hasEndOfStringCharacter | |||
| ? new RegExp(re.source + '$', re.flags) | |||
| : re; | |||
| const endMatch = hasEndOfStringCharacter || !isMethodAll; | |||
| const { regexp: re } = pathToRegexp(normalizedPath, { | |||
| end: endMatch, | |||
| }); | |||
There was a problem hiding this comment.
Fixed in b19ccea. Renamed isMethodAll → isStringRoute and tightened the predicate to only match (requestMethod as number) === -1. RequestMethod.ALL now uses exact matching (end: true); only string-route forRoutes('/prefix') (which uses the -1 sentinel) keeps prefix matching.
| async instance => { | ||
| instance.get('/', async () => MIDDLEWARE_VALUE); | ||
| instance.get('/sub-route', async req => { | ||
| return (req.raw as any).middlewareApplied | ||
| ? MIDDLEWARE_VALUE | ||
| : 'no_middleware'; | ||
| }); | ||
| }, | ||
| { prefix: '/my-prefix' }, | ||
| ); | ||
|
|
||
| await app.init(); | ||
| await app.getHttpAdapter().getInstance().ready(); | ||
| }); | ||
|
|
||
| it(`forRoutes('/my-prefix') should match the prefix root`, () => { | ||
| return app | ||
| .inject({ | ||
| method: 'GET', | ||
| url: '/my-prefix', | ||
| }) | ||
| .then(({ payload }) => expect(payload).to.be.eql(MIDDLEWARE_VALUE)); | ||
| }); |
There was a problem hiding this comment.
Fixed in b19ccea. The handler now sets req.raw.middlewareApplied = true and the assertion checks that flag, so the test actually verifies the middleware ran. Also added a new test covering that forRoutes({ path: '/a', method: RequestMethod.ALL }) does NOT match /a/b — confirming exact-match semantics for ALL.
|
Hi — friendly follow-up. CI is green and all checks pass. Would you be able to review when you get a chance? Thank you! |
When middleware is registered with forRoutes({ path, method }) (any
RequestMethod including ALL), pathToRegexp now uses end: true (exact
match) so that middleware on '/a' does not accidentally fire on '/a/b'.
Only string routes from forRoutes('/prefix') — internally represented
as method -1 — use prefix matching (end: false) to mirror Express's
app.use() semantics for prefix-based middleware.
Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
pierluigilenoci
force-pushed
the
fix/fastify-middleware-route-matching
branch
from
b19ccea to
58d0e60
Compare
pierluigilenoci
changed the title
|
im still investigating if this doesnt cause any unexpected regression that might not have been caught by our tests |
Summary
When middleware is registered with
forRoutes({ path, method })(anyRequestMethodincludingALL),pathToRegexpnow usesend: true(exact match) so that middleware on/adoes not accidentally fire on/a/b.Only string routes from
forRoutes('/prefix')— internally represented as method-1— use prefix matching (end: false) to mirror Express'sapp.use()semantics for prefix-based middleware.Changes
packages/platform-fastify/adapters/fastify-adapter.ts: IntroduceisStringRoutecheck (requestMethod === -1) to select between prefix matching (end: false) and exact matching (end: true) when building the middleware path regex.integration/hello-world/e2e/middleware-fastify.spec.ts: Add two new e2e test blocks — one verifying that prefix-based middleware (forRoutes('/my-prefix')) correctly matches sub-routes via Fastify plugins, and another verifying thatRequestMethod.ALLuses exact matching and does NOT match sub-paths.