scriptoriumadmin/GitNexus
1
0
Fork 0
mirror of https://github.com/abhigyanpatwari/GitNexus.git synced 2026-08-28 05:25:25 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 9

fix(ingestion): refuse a @Controller options object whose path is not provable

Browse source 
`literalPaths` took the first `pair` named `path` and ignored everything after
it, so a member that overrides `path` at runtime produced a URL the app never
serves:

    @Controller({ path: 'cats', ...options })   ->  /cats, but options.path wins
    @Controller({ path: 'cats', path: 'dogs' }) ->  /cats, but dogs wins

Both are the wrong-fact class this module's header forbids, and both cost every
route on the class rather than one route.

The object branch is now a fail-closed walk, mirroring `routeFromObject` in
data-route-table.ts: skip comments first, then refuse on any non-`pair` child
(spread, computed key, shorthand, method), any unreadable key, and any repeated
key name. Key names compare through `propertyName`, so `path` and `'path'`
collide as duplicates rather than reading as two different keys.

Refusing on a spread positioned BEFORE the literal is deliberate even though JS
evaluation order makes that shape provably safe. The rule is not cost — the walk
already runs in source order, so position tracking would be one boolean — and it
is not blanket parity with the sibling extractor. It is that the shape's real
frequency is unmeasured (this repo contains no NestJS application; all four
`@Controller({` occurrences are its own test fixtures) and the failure directions
are asymmetric: reading it wrong publishes a URL that does not exist, refusing it
omits a route still findable in source. If the new log line shows the shape in
real repos, the position-sensitive rule is the ready upgrade.

`containsExecutingExpression` is deliberately not ported from that sibling. It
guards whole-entry declarativeness for a static route table; here only `path`
needs to be readable, so a computed value on an unrelated key such as
`scope: Scope.REQUEST` stays benign and is pinned as such.

Because a refusal silently costs a whole controller, it now logs under `isDev`
and names the file plus the offending shape. Emitted at `info`, not `debug`:
the logger's base level is already `info`, so an isDev-gated `debug` would be
gated twice and stay silent in exactly the run it exists for. `filePath` is
threaded from `extractNestRoutes` down to the object branch to make that line
useful; without it the message names a shape but not where to find it.

7 of the new cases were red on base for the right reason — each published a
prefix the application never mounts.
This commit is contained in:
Gergo Magyar 2026-08-26 17:46:38 +00:00
parent 575944084c
commit 60021f9ab8
2 changed files with 181 additions and 17 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

115
gitnexus/src/core/ingestion/route-extractors/nest.ts
View file

@ -44,6 +44,8 @@
import type Parser from 'tree-sitter';
import type { ExtractedDecoratorRoute } from '../workers/parse-worker.js';
import { plainString, propertyName } from './data-route-table.js';
import { isDev } from '../utils/env.js';
import { logger } from '../../logger.js';
/**
* NestJS method decorators → HTTP verb. A Map rather than an object literal
@ -129,7 +131,10 @@ function decoratorName(decorator: Parser.SyntaxNode): string | null {
* `:id(d+)`, and `@Get('/v\u0069ews')` came out as `/vews`. Both are paths the
* app never serves, i.e. the wrong-URL outcome the paragraph above forbids.
*/
function decoratorLiteralPaths(decorator: Parser.SyntaxNode): readonly string[] | null {
function decoratorLiteralPaths(
decorator: Parser.SyntaxNode,
filePath: string,
): readonly string[] | null {
const call = decorator.namedChild(0);
// A bare `@Injectable` with no call, or `@Get()` with no argument — legal,
// and both mean "no path segment of my own".
@ -141,7 +146,42 @@ function decoratorLiteralPaths(decorator: Parser.SyntaxNode): readonly string[]
// Reading it as a route would mint a URL the app never serves, which is the
// invented fact this module refuses; an unreadable shape drops instead.
if (first.type === 'object' && decoratorName(decorator) !== 'Controller') return null;
return literalPaths(first);
return literalPaths(first, filePath);
}
/**
* How much of a refused options object to quote in the dev line. Enough to
* recognise the shape, bounded because an options object is arbitrary source.
*/
const REFUSED_OBJECT_LOG_LIMIT = 160;
/**
* Decline an options object whose `path` is not provable, and say so.
*
* Gated on `isDev` exactly as the routes phase gates its own registry line
* (`pipeline-phases/routes.ts`), and emitted at `info` rather than `debug`
* because the logger's base level IS `info`: an `isDev`-gated `logger.debug`
* would be gated twice and stay silent in the very dev run it exists for.
*
* Names the FILE, not just the shape — a controller dropped without a path to
* look at is only marginally louder than one dropped in silence, and this
* refusal costs every route on the class. The line number is deliberately
* absent: `lineOffset` (a Vue SFC `<script>` block shifts every row) is not
* threaded this deep, and a row that is wrong for embedded scripts would send
* the reader to a line the decorator is not on. The quoted shape locates it.
*/
function refuseUnprovableObject(node: Parser.SyntaxNode, filePath: string): null {
if (isDev) {
const shape = node.text.replace(/\s+/g, ' ');
const quoted =
shape.length > REFUSED_OBJECT_LOG_LIMIT
? `${shape.slice(0, REFUSED_OBJECT_LOG_LIMIT)}…`
: shape;
logger.info(
`🗺️ NestJS: dropped @Controller in ${filePath} — a member of its options object could override \`path\`: ${quoted}`,
);
}
return null;
}
/**
@ -151,23 +191,61 @@ function decoratorLiteralPaths(decorator: Parser.SyntaxNode): readonly string[]
* what keeps `@Controller({ path: ['a', 'b'] })` from being read by a second,
* laxer set of rules that has drifted from this one.
*/
function literalPaths(node: Parser.SyntaxNode): readonly string[] | null {
function literalPaths(node: Parser.SyntaxNode, filePath: string): readonly string[] | null {
// `@Controller({ path: 'cats', version: '1' })` is the documented form for
// URI/header versioning, and its path is a plain literal sitting right there.
// Worth reading rather than dropping, because the asymmetry is severe: an
// unreadable METHOD path costs one route, an unreadable PREFIX costs every
// route on the class.
if (node.type === 'object') {
// `propertyName` reads both spellings that carry a name — `{ path: … }` and
// `{ 'path': … }` — so the class is not dropped over a pair of quotes. A
// computed key (`{ [KEY]: … }`) has none, and keeps the drop, as does a
// computed value.
const path = node.namedChildren.find((child) => {
const key = child.type === 'pair' ? child.childForFieldName('key') : null;
return key !== null && propertyName(key) === 'path';
});
const value = path?.childForFieldName('value');
return value ? literalPaths(value) : null;
// But a `path` pair only PROVES the mount when nothing else in the object
// can replace it, and the first match proves nothing on its own:
// `{ path: 'cats', ...options }` mounts wherever `options.path` says, and
// `{ path: 'cats', path: 'dogs' }` mounts at `dogs` — last write wins in
// both. Either one publishes `/cats`, a URL the app never serves, and it
// looks exactly like a correct one, which is the wrong-answer-dressed-as-
// fact this module refuses. So the object is read only when EVERY member is
// a named, non-repeated pair. That whole-entry fail-closed walk is the
// shape `routeFromObject` uses in `data-route-table.ts`.
const values = new Map<string, Parser.SyntaxNode>();
for (const child of node.namedChildren) {
// Skipped FIRST. A comment between two pairs is ordinary formatting; run
// through the not-a-pair test below it would refuse the object and cost
// the class every route it has, over a comment.
if (child.type === 'comment') continue;
// `spread_element` (`{ ...options }`), `shorthand_property_identifier`
// (`{ path }`) and `method_definition` (`{ getFoo() {} }`) all land here
// — probed and identical across the three grammars this extractor runs
// under. None offers a key/value this file can read, and the first can
// introduce or overwrite `path` from a value declared elsewhere.
if (child.type !== 'pair') return refuseUnprovableObject(node, filePath);
const key = child.childForFieldName('key');
const value = child.childForFieldName('value');
if (key === null || value === null) return refuseUnprovableObject(node, filePath);
// Compared through `propertyName`, the same judge used to READ the key —
// so `{ path: … }` and `{ 'path': … }` are one key and collide as
// duplicates. Comparing raw key text instead makes them two distinct
// keys, and `{ path: 'cats', 'path': 'dogs' }` silently mounts the loser.
const name = propertyName(key);
// No readable name means a computed key (`{ [dynamicKey]: 'b' }`), which
// could evaluate to `path` and take the mount with it — refused, not
// ignored. A repeated key is refused wherever it appears, not only on
// `path`: a duplicate anywhere is evidence the object is not the fixed
// literal it reads as, and cost is one controller against a wrong URL.
if (name === null || values.has(name)) return refuseUnprovableObject(node, filePath);
values.set(name, value);
}
// Deliberately NOT `containsExecutingExpression` (data-route-table.ts): that
// guards whole-entry declarativeness for a static route table, a different
// invariant. Here only `path` has to be provable, so a non-literal value on
// an unrelated key — `{ path: 'a', scope: Scope.REQUEST }`, ordinary Nest —
// stays benign and keeps its controller.
const path = values.get('path');
// A missing `path` keeps the existing drop and must never read as `''`:
// `@Controller({ version: '1' })` mounts at a prefix this decorator does
// not state, and `''` would publish every one of its methods at the root.
return path === undefined ? null : literalPaths(path, filePath);
}
// `array` is the node type in all three grammars this extractor runs under —
@ -265,10 +343,13 @@ function classDecorators(classNode: Parser.SyntaxNode): Parser.SyntaxNode[] {
* extractors solve the same shape and should not disagree about which half of
* it is supported.
*/
function controllerPrefix(classNode: Parser.SyntaxNode): string | null | undefined {
function controllerPrefix(
classNode: Parser.SyntaxNode,
filePath: string,
): string | null | undefined {
for (const decorator of classDecorators(classNode)) {
if (decoratorName(decorator) !== 'Controller') continue;
const paths = decoratorLiteralPaths(decorator);
const paths = decoratorLiteralPaths(decorator, filePath);
// `@Controller([])` lands here too and needs no answer of its own: a
// controller mounted at no path serves no route, so "emit nothing for this
// class" is what both readings of it come to.
@ -297,7 +378,7 @@ export function extractNestRoutes(
const visit = (node: Parser.SyntaxNode): void => {
if (CLASS_DECLARATION_TYPES.has(node.type)) {
const prefix = controllerPrefix(node);
const prefix = controllerPrefix(node, filePath);
// `undefined` — not a controller at all. `null` — a controller whose
// prefix could not be read, so its routes' URLs are unknowable.
if (prefix !== undefined) {
@ -362,7 +443,7 @@ function collectClassRoutes(
const httpMethod = NEST_METHOD_DECORATORS.get(name);
if (httpMethod === undefined) continue;
const routePaths = decoratorLiteralPaths(decorator);
const routePaths = decoratorLiteralPaths(decorator, filePath);
if (routePaths === null) continue; // unreadable → skip
const handlerName = member.childForFieldName('name')?.text;

83
gitnexus/test/unit/nest-decorator-routes.test.ts
View file

@ -403,9 +403,92 @@ describe('NestJS decorator routes', () => {
).toEqual([]);
});
// A `path` pair proves the mount point only when nothing ELSE in the object
// can replace it. `{ path: 'cats', ...options }` reads as `cats` under a
// first-match scan and mounts wherever `options.path` says at runtime;
// `{ path: 'cats', path: 'dogs' }` mounts at `dogs`. Both are the wrong-URL
// outcome this module calls worse than a missing one, and both are silent —
// a published `/cats` looks exactly like a correct one. So the object is read
// only when every member is a named, non-repeated pair: the whole-entry
// fail-closed shape `routeFromObject` uses in data-route-table.ts.
it.each([
// `options.path` overrides the pair above it, so the extracted prefix and
// the served prefix disagree with nothing in the file to say so.
{ label: 'a trailing spread', argument: "{ path: 'cats', ...options }" },
// Deterministically SAFE under JS evaluation order — a later `path` pair
// always wins over an earlier spread — and refused anyway. Reading member
// order as proof makes the verdict turn on which side of the spread the
// author happened to type `path`, and how often each spelling occurs in
// real controllers is unmeasured. Meanwhile the two failure directions are
// not symmetric: reading it wrong publishes a URL the app never serves, and
// `route_map`/`api_impact` present that as fact, while refusing omits a
// route that is still findable in source.
{ label: 'a leading spread', argument: "{ ...options, path: 'cats' }" },
{ label: 'nothing but a spread', argument: '{ ...options }' },
// Last write wins at runtime, so a first-match scan names the loser.
{ label: 'a repeated path key', argument: "{ path: 'cats', path: 'dogs' }" },
// Visible only through `propertyName`: compared as raw key text, `path` and
// `'path'` are two different keys and the duplicate check never fires.
{
label: 'a repeated path key in its quoted spelling',
argument: "{ path: 'cats', 'path': 'dogs' }",
},
// Refusal is on ANY repeated key, not only `path` — a duplicate anywhere is
// evidence the object is not the fixed literal it reads as.
{
label: 'a repeated key other than path',
argument: "{ path: 'a', version: '1', version: '2' }",
},
// A computed key could evaluate to `path` and take the mount with it.
{ label: 'a computed key beside the path', argument: "{ path: 'a', [dynamicKey]: 'b' }" },
// `shorthand_property_identifier` and `method_definition`; neither is a
// `pair`, so neither offers a key/value this file can read.
{ label: 'a shorthand property', argument: '{ path }' },
{ label: 'a method', argument: "{ path: 'a', getFoo() {} }" },
])('refuses an object form whose path another member could override: $label', ({ argument }) => {
expect(
extract(`
@Controller(${argument})
export class C {
@Get('b') b() {}
}
`),
).toEqual([]);
});
it.each([
// A comment between two pairs is ordinary formatting. It has to be skipped
// BEFORE the not-a-pair test above, or the refusal fires on it and costs
// the controller every route it has.
{
label: 'a comment between its pairs',
argument: "{ path: 'a', /* URI versioning */ version: '1' }",
},
// Only `path` has to be provable. A non-literal value on an unrelated key
// is benign: `containsExecutingExpression` in data-route-table.ts refuses
// these, but it guards whole-entry declarativeness for a static route
// table — a different invariant from "can this member move the mount".
{
label: 'non-literal values on keys other than path',
argument: "{ path: 'a', host: 'x', scope: Scope.REQUEST, durable: true }",
},
])('still reads the prefix out of an object form with $label', ({ argument }) => {
expect(
urls(`
@Controller(${argument})
export class C {
@Get('b') b() {}
}
`),
).toEqual(['GET /a/b']);
});
it.each([
{ label: 'a single path', argument: "{ path: 'a' }" },
{ label: 'an array of paths', argument: "{ path: ['a', 'b'] }" },
// The verb gate short-circuits on the object form before the object is
// walked at all, so the class-form refusal never gets a say here.
{ label: 'an object the class form would also refuse', argument: "{ path: 'a', ...options }" },
])('mints nothing from the object form on a VERB decorator ($label)', ({ argument }) => {
// `@Controller` takes the object form; `@Get` and friends take
// `string | string[]`. Nest mounts nothing here, so emitting a route would