mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-17 04:33:45 +00:00
fix(routes): reject middleware and route metadata false positives (#3561)
* fix(routes): reject middleware and route metadata false positives * fix(routes): invalidate cached phantom routes on upgrade * test(routes): cover cache upgrades across merged schema versions Verify that schemas 137 through 140 invalidate both parse stores after merging main into PR #3561 and advancing the schema to 141. --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
This commit is contained in:
parent
447345ff71
commit
94f167a90f
8 changed files with 98 additions and 10 deletions
|
|
@ -198,6 +198,12 @@ That extractor is deliberately **precision-weighted**: `route_map` presents its
|
|||
output as fact, so a `startsWith` namespace test, a bare `pathname === '/'`
|
||||
without a verb, and any regex it cannot translate exactly are all dropped rather
|
||||
than guessed at. A missing route is a coverage limit; an invented one is a lie.
|
||||
Express `app.get(RegExp, handler)` registrations are likewise omitted: a regex
|
||||
matcher does not have one finite URL to use as a Route node key.
|
||||
Express `app.use(path, middleware)` is middleware attachment, not an endpoint;
|
||||
it does not create a Route node. Dispatch-guard inference also ignores stored
|
||||
`route.routePath` metadata: comparing a recorded route name with a literal
|
||||
does not prove that the request path is being dispatched.
|
||||
|
||||
Two rules there need more than one comparison to decide, and are worth knowing
|
||||
about before changing either:
|
||||
|
|
|
|||
|
|
@ -242,7 +242,7 @@ function isPathExpression(node: SyntaxNode): boolean {
|
|||
if (node.type === 'member_expression') {
|
||||
const property = node.childForFieldName('property');
|
||||
if (property === null) return false;
|
||||
if (PATH_IDENTIFIERS.has(property.text)) return true;
|
||||
if (property.text === 'pathname') return true;
|
||||
if (property.text === 'url') {
|
||||
const object = node.childForFieldName('object');
|
||||
return object !== null && RAW_URL_RECEIVERS.has(object.text);
|
||||
|
|
|
|||
|
|
@ -2054,7 +2054,7 @@ const processFileGroup = (
|
|||
) {
|
||||
const method = captureMap['express_route.method'].text;
|
||||
const routePath = captureMap['express_route.path'].text;
|
||||
if (EXPRESS_ROUTE_METHODS.has(method) && routePath.startsWith('/')) {
|
||||
if (EXPRESS_ROUTE_METHODS.has(method) && method !== 'use' && routePath.startsWith('/')) {
|
||||
// Extract the receiver (the object the method is called on) to filter out
|
||||
// HTTP client calls like axios.get('/api/users') that match the same pattern
|
||||
// as Express route registrations.
|
||||
|
|
@ -2130,11 +2130,7 @@ const processFileGroup = (
|
|||
}
|
||||
for (const registration of registrations) {
|
||||
const httpMethod =
|
||||
registration.method === 'all'
|
||||
? '*'
|
||||
: registration.method === 'use'
|
||||
? 'GET'
|
||||
: registration.method.toUpperCase();
|
||||
registration.method === 'all' ? '*' : registration.method.toUpperCase();
|
||||
result.decoratorRoutes.push({
|
||||
filePath: file.path,
|
||||
routePath,
|
||||
|
|
|
|||
|
|
@ -869,7 +869,10 @@ import { copyV8CacheIfPresent, tryLoadV8Cache, writeV8CacheFile } from './v8-sid
|
|||
// v142: fetch-wrapper consumers are AST candidates emitted by providers.
|
||||
// Re-extract v141 shards: main used 141 for detached Go ownership, while
|
||||
// this branch used it for wrapper candidates. Neither contains both changes.
|
||||
const SCHEMA_BUMP = 142;
|
||||
// v143 (#3561): combine v142 wrapper candidates with the route corrections.
|
||||
// Older caches may retain phantom middleware and stored route-metadata edges.
|
||||
// The PR also used v142 without wrapper candidates; invalidate both histories.
|
||||
const SCHEMA_BUMP = 143;
|
||||
const GITNEXUS_PKG_VERSION = (() => {
|
||||
try {
|
||||
// package.json sits at gitnexus/package.json — two levels up from
|
||||
|
|
|
|||
|
|
@ -5,6 +5,8 @@ function mcp() { return 'mcp'; }
|
|||
app.get('/api/info', realInfo);
|
||||
app.post('/api/query', realQuery);
|
||||
app.all('/api/mcp', mcp);
|
||||
app.use('/api/mcp', middleware);
|
||||
app.get(/^\/web(?:\/|$)/, realInfo);
|
||||
app.route('/api/chained').get(realInfo);
|
||||
app.route('/api/chained-post').post(realQuery);
|
||||
app.route('/api/chained-multi').get(realInfo).post(realQuery);
|
||||
|
|
@ -22,3 +24,7 @@ counts.get('/ghost-block-comment' /* cached key */);
|
|||
counts.get(
|
||||
'/ghost-line-comment', // cached key
|
||||
);
|
||||
|
||||
function inspectRoute(route) {
|
||||
return route.routePath === '/comparison-ghost';
|
||||
}
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@ import {
|
|||
} from '../../../src/storage/parse-cache.js';
|
||||
import {
|
||||
getDurableParsedFileDir,
|
||||
loadDurableParsedFileIndex,
|
||||
pruneAndSaveDurableParsedFileStore,
|
||||
} from '../../../src/storage/parsedfile-store.js';
|
||||
import {
|
||||
|
|
@ -102,6 +103,10 @@ describe('Express route identity and source priority', () => {
|
|||
expect(mcpRoutes[0].properties.method).toBe('*');
|
||||
});
|
||||
|
||||
it('does not turn an Express RegExp matcher into an invented URL', () => {
|
||||
expect(getNodesByLabel(result, 'Route')).not.toContain('/web');
|
||||
});
|
||||
|
||||
it.each(['/ghost', '/ghost-block-comment', '/ghost-line-comment'])(
|
||||
'does not classify the one-argument Map.get lookup %s as a route',
|
||||
(route) => {
|
||||
|
|
@ -109,6 +114,10 @@ describe('Express route identity and source priority', () => {
|
|||
},
|
||||
);
|
||||
|
||||
it('does not turn stored route metadata into a dispatch guard', () => {
|
||||
expect(getNodesByLabel(result, 'Route')).not.toContain('/comparison-ghost');
|
||||
});
|
||||
|
||||
it.each([
|
||||
['/api/chained', ['GET']],
|
||||
['/api/chained-post', ['POST']],
|
||||
|
|
@ -234,4 +243,58 @@ describe('Express route identity and source priority', () => {
|
|||
fs.rmSync(tempDir, { recursive: true, force: true });
|
||||
}
|
||||
}, 120_000);
|
||||
|
||||
it.each([137, 138, 139, 140])(
|
||||
'reparses route captures from a same-package v%i cache on upgrade',
|
||||
async (previousSchema) => {
|
||||
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-express-route-upgrade-'));
|
||||
const repoDir = path.join(tempDir, 'repo');
|
||||
const storageDir = path.join(tempDir, 'storage');
|
||||
const previousVersion = `${previousSchema}+${PARSE_CACHE_VERSION.split('+').slice(1).join('+')}`;
|
||||
try {
|
||||
fs.cpSync(path.join(FIXTURES, 'express-route-priority'), repoDir, { recursive: true });
|
||||
const cache: ParseCache = {
|
||||
version: PARSE_CACHE_VERSION,
|
||||
entries: new Map(),
|
||||
usedKeys: new Set<string>(),
|
||||
storagePath: storageDir,
|
||||
onDiskKeys: new Set<string>(),
|
||||
};
|
||||
const initial = await runPipelineFromRepo(repoDir, () => {}, {
|
||||
parseCache: cache,
|
||||
workerPoolSize: 1,
|
||||
});
|
||||
expect(initial.reparsedFileCount).toBe(2);
|
||||
pruneCache(cache, cache.usedKeys);
|
||||
cache.version = previousVersion;
|
||||
const savedKeys = await saveParseCache(storageDir, cache);
|
||||
expect(savedKeys).toHaveLength(2);
|
||||
const durableDir = getDurableParsedFileDir(storageDir);
|
||||
await pruneAndSaveDurableParsedFileStore(durableDir, previousVersion, new Set(savedKeys));
|
||||
expect((await loadDurableParsedFileIndex(durableDir, previousVersion)).size).toBe(2);
|
||||
|
||||
const upgraded = await loadParseCache(storageDir);
|
||||
expect(upgraded.onDiskKeys?.size ?? 0).toBe(0);
|
||||
expect(await loadDurableParsedFileIndex(durableDir, PARSE_CACHE_VERSION)).toEqual(
|
||||
new Map(),
|
||||
);
|
||||
const result = await runPipelineFromRepo(repoDir, () => {}, {
|
||||
parseCache: upgraded,
|
||||
workerPoolSize: 1,
|
||||
});
|
||||
expect(result.reparsedFileCount).toBe(2);
|
||||
expect(result.parseCacheHitFileCount).toBe(0);
|
||||
const routes = getNodesByLabel(result, 'Route');
|
||||
expect(routes).toContain('/api/info');
|
||||
expect(routes).not.toContain('/ghost');
|
||||
expect(routes).not.toContain('/comparison-ghost');
|
||||
expect(
|
||||
getNodesByLabelFull(result, 'Route').filter((node) => node.name === '/api/mcp'),
|
||||
).toHaveLength(1);
|
||||
} finally {
|
||||
fs.rmSync(tempDir, { recursive: true, force: true });
|
||||
}
|
||||
},
|
||||
120_000,
|
||||
);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -118,6 +118,12 @@ describe('dispatch-guard route extraction', () => {
|
|||
});
|
||||
|
||||
describe('what must NOT become a route', () => {
|
||||
it('ignores comparisons against stored route metadata', () => {
|
||||
expect(paths("function f(route) { return route.routePath === '/comparison-ghost' }")).toEqual(
|
||||
[],
|
||||
);
|
||||
});
|
||||
|
||||
it('ignores a comparison against something that is not a request path', () => {
|
||||
// `mode` is not a path expression, so `/full` is just a string.
|
||||
expect(paths(`function f() { if (mode === '/full') { return 1 } }`)).toEqual([]);
|
||||
|
|
@ -1092,6 +1098,12 @@ describe('dispatch-guard route extraction', () => {
|
|||
});
|
||||
|
||||
describe('path expressions the rule accepts', () => {
|
||||
it('accepts a bare routePath request-path guard', () => {
|
||||
expect(paths("function f(routePath) { if (routePath === '/api/x') return 1 }")).toEqual([
|
||||
'/api/x',
|
||||
]);
|
||||
});
|
||||
|
||||
it('accepts a member access ending in .pathname', () => {
|
||||
expect(
|
||||
paths(`function f(req) { if (new URL(req.url, base).pathname === '/api/x') { return 1 } }`),
|
||||
|
|
|
|||
|
|
@ -324,8 +324,9 @@ describe('PARSE_CACHE_VERSION', () => {
|
|||
// Moved 139 -> 140 for Ruby accessor ownership captures used by rename (#3557).
|
||||
// Moved 140 -> 141 for detached Go ownership captures used by rename (#3559).
|
||||
// Moved 141 -> 142 to combine Go ownership with fetch-wrapper candidates (#3567).
|
||||
it('pins SCHEMA_BUMP to 142 so concurrent bumps cannot silently collide (#2766, #3015, #3088, #2885, #3128, #2865, #3130, #1432, #3161, #3179, #3219, #3190, #3253, #3273, #3339, #3354, #3371, #2965, #3390, #3398, #3396, #3394, #3399, #3414, #3408, #3402, #3446, #3450, #3499, #3502, #3487, #3505, #3504, #3497, #3486, #3559, #3567)', () => {
|
||||
expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(142);
|
||||
// Moved 142 -> 143 to combine fetch-wrapper candidates with #3561 route corrections.
|
||||
it('pins SCHEMA_BUMP to 143 so concurrent bumps cannot silently collide (#2766, #3015, #3088, #2885, #3128, #2865, #3130, #1432, #3161, #3179, #3219, #3190, #3253, #3273, #3339, #3354, #3371, #2965, #3390, #3398, #3396, #3394, #3399, #3414, #3408, #3402, #3446, #3450, #3499, #3502, #3487, #3505, #3504, #3497, #3486, #3559, #3567, #3561)', () => {
|
||||
expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(143);
|
||||
expect(PARSE_CACHE_BUCKET_COUNT).toBe(128);
|
||||
// The PREVIOUS version must fail the reuse gate, not merely differ from the
|
||||
// current one — a hardcoded number outside the conflict hunk rebases cleanly
|
||||
|
|
@ -336,6 +337,7 @@ describe('PARSE_CACHE_VERSION', () => {
|
|||
82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103,
|
||||
104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122,
|
||||
123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141,
|
||||
142,
|
||||
]) {
|
||||
expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).not.toBe(taken);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue