From ae2a1bcdc1357a56d436acab5782da9a348d2cd5 Mon Sep 17 00:00:00 2001 From: Hiroki Osame Date: Fri, 28 Jun 2024 22:09:14 +0900 Subject: [PATCH] fix(cjs): leaking implicit extension resolver --- src/cjs/api/module-resolve-filename.ts | 100 +++++++------- src/cjs/api/resolve-implicit-extensions.ts | 2 + src/utils/path-utils.ts | 3 +- tests/specs/api.ts | 148 +++++++++++---------- 4 files changed, 128 insertions(+), 125 deletions(-) diff --git a/src/cjs/api/module-resolve-filename.ts b/src/cjs/api/module-resolve-filename.ts index 635221f8..3b336ace 100644 --- a/src/cjs/api/module-resolve-filename.ts +++ b/src/cjs/api/module-resolve-filename.ts @@ -156,71 +156,69 @@ const resolveRequest = ( export const createResolveFilename = ( nextResolve: ResolveFilename, namespace?: string, -): ResolveFilename => { +): ResolveFilename => ( + request, + parent, + isMain, + options, +) => { + const resolve: SimpleResolve = request_ => nextResolve( + request_, + parent, + isMain, + options, + ); + + request = interopCjsExports(request); + + if (parent?.filename) { + const filePath = getOriginalFilePath(parent.filename); + if (filePath) { + parent.filename = filePath.split('?')[0]; + } + } + + // Strip query string + const requestAndQuery = request.split('?'); + const searchParams = new URLSearchParams(requestAndQuery[1]); + + // Inherit parent namespace if it exists + if (parent?.filename) { + const parentQuery = new URLSearchParams(parent.filename.split('?')[1]); + const parentNamespace = parentQuery.get('namespace'); + if (parentNamespace) { + searchParams.append('namespace', parentNamespace); + } + } + + // If request namespace doesnt match the namespace, ignore + if ((searchParams.get('namespace') ?? undefined) !== namespace) { + return resolve(request); + } + if (namespace) { /** * When namespaced, the loaders are registered to the extensions in a hidden way * so Node's built-in resolver will not try those extensions * - * To support implicit extensions, we need to wrap the resolver with our own + * To support implicit extensions, we need to enhance the resolver with our own * re-implementation of the implicit extension resolution */ nextResolve = createImplicitResolver(nextResolve); } - return ( - request, - parent, - isMain, - options, - ) => { - const resolve: SimpleResolve = request_ => nextResolve( - request_, - parent, - isMain, - options, - ); - - request = interopCjsExports(request); - - if (parent?.filename) { - const filePath = getOriginalFilePath(parent.filename); - if (filePath) { - parent.filename = filePath.split('?')[0]; - } - } - - // Strip query string - const requestAndQuery = request.split('?'); - const searchParams = new URLSearchParams(requestAndQuery[1]); + let resolved = resolveRequest(requestAndQuery[0], parent, resolve); - // Inherit parent namespace if it exists - if (parent?.filename) { - const parentQuery = new URLSearchParams(parent.filename.split('?')[1]); - const parentNamespace = parentQuery.get('namespace'); - if (parentNamespace) { - searchParams.append('namespace', parentNamespace); - } - } - - // If request namespace doesnt match the namespace, ignore - if ((searchParams.get('namespace') ?? undefined) !== namespace) { - return resolve(request); - } - - let resolved = resolveRequest(requestAndQuery[0], parent, resolve); - - // Only add query back if it's a file path (not a core Node module) - if ( - path.isAbsolute(resolved) + // Only add query back if it's a file path (not a core Node module) + if ( + path.isAbsolute(resolved) // These two have native loaders which don't support queries && !resolved.endsWith('.json') && !resolved.endsWith('.node') - ) { - resolved += urlSearchParamsStringify(searchParams); - } + ) { + resolved += urlSearchParamsStringify(searchParams); + } - return resolved; - }; + return resolved; }; diff --git a/src/cjs/api/resolve-implicit-extensions.ts b/src/cjs/api/resolve-implicit-extensions.ts index 474026a2..7943ba5f 100644 --- a/src/cjs/api/resolve-implicit-extensions.ts +++ b/src/cjs/api/resolve-implicit-extensions.ts @@ -1,5 +1,6 @@ import path from 'node:path'; import type { NodeError } from '../../types.js'; +import { isBarePackageNamePattern } from '../../utils/path-utils.js'; import type { ResolveFilename } from './types.js'; export const implicitlyResolvableExtensions = [ @@ -34,6 +35,7 @@ export const createImplicitResolver = ( const nodeError = _error as NodeError; if ( nodeError.code === 'MODULE_NOT_FOUND' + && !isBarePackageNamePattern.test(request) ) { const resolved = ( tryExtensions(resolve, request, ...args) diff --git a/src/utils/path-utils.ts b/src/utils/path-utils.ts index 8d100612..055c5e13 100644 --- a/src/utils/path-utils.ts +++ b/src/utils/path-utils.ts @@ -53,4 +53,5 @@ export const isJsonPattern = /\.json($|\?)/; export const isDirectoryPattern = /\/(?:$|\?)/; // Only matches packages names without subpaths (e.g. `foo` but not `foo/bar`) -export const isBarePackageNamePattern = /^(?:@[^/]+\/)?[^/]+$/; +// Back slash included to exclude Windows paths +export const isBarePackageNamePattern = /^(?:@[^/]+\/)?[^/\\]+$/; diff --git a/tests/specs/api.ts b/tests/specs/api.ts index 5354bd2a..2760c1b8 100644 --- a/tests/specs/api.ts +++ b/tests/specs/api.ts @@ -135,42 +135,84 @@ export default testSuite(({ describe }, node: NodeApis) => { expect(stdout).toBe('js working\nts working'); }); - test('register / unregister', async () => { - await using fixture = await createFixture({ - 'register.cjs': ` - const { register } = require(${JSON.stringify(tsxCjsApiPath)}); - try { - require('./file'); - } catch { - console.log('Fails as expected'); - } - - const unregister = register(); - - const loaded = require('./file'); - console.log(loaded.message); - - // Remove from cache - const loadedPath = require.resolve('./file'); - delete require.cache[loadedPath]; - - unregister(); - - try { - require('./file'); - } catch { - console.log('Unregistered'); - } - `, - ...tsFiles, - }); + describe('register', ({ test }) => { + test('register / unregister', async () => { + await using fixture = await createFixture({ + 'register.cjs': ` + const { register } = require(${JSON.stringify(tsxCjsApiPath)}); + try { + require('./file'); + } catch { + console.log('Fails as expected'); + } + + const unregister = register(); + + const loaded = require('./file'); + console.log(loaded.message); + + // Remove from cache + const loadedPath = require.resolve('./file'); + delete require.cache[loadedPath]; + + unregister(); + + try { + require('./file'); + } catch { + console.log('Unregistered'); + } + `, + ...tsFiles, + }); - const { stdout } = await execaNode(fixture.getPath('register.cjs'), [], { - nodePath: node.path, - nodeOptions: [], + const { stdout } = await execaNode(fixture.getPath('register.cjs'), [], { + nodePath: node.path, + nodeOptions: [], + }); + + expect(stdout).toBe('Fails as expected\nfoo bar json file.ts\nUnregistered'); }); - expect(stdout).toBe('Fails as expected\nfoo bar json file.ts\nUnregistered'); + test('namespace', async () => { + await using fixture = await createFixture({ + 'require.cjs': ` + const { expectErrors } = require('expect-errors'); + const path = require('node:path'); + const tsx = require(${JSON.stringify(tsxCjsApiPath)}); + + const api = tsx.register({ namespace: 'abcd' }); + + expectErrors( + // Loading explicit/resolved file path should be ignored by loader (extensions) + [() => require('./file.ts'), 'SyntaxError'], + + // resolver should preserve full file path when ignoring + [() => require('./file.ts?asdf'), "Cannot find module './file.ts?asdf'"] + ); + + const { message, async } = api.require('./file', __filename); + console.log(message); + async.then(m => console.log(m.default)); + + api.require('./tsx?query=1', __filename); + api.require('./jsx', __filename); + api.require('./dir?query=3', __filename); + `, + ...tsFiles, + + 'tsx.tsx': 'console.log(\'tsx\');', + 'jsx.jsx': 'console.log(\'jsx\');', + 'dir/index.jsx': 'console.log(\'dir\');', + }); + + const { stdout } = await execaNode(fixture.getPath('require.cjs'), [], { + nodePath: node.path, + nodeOptions: [], + }); + + expect(stdout).toBe('foo bar json file.ts\ntsx\njsx\ndir\nasync'); + }); }); describe('tsx.require()', ({ test }) => { @@ -265,46 +307,6 @@ export default testSuite(({ describe }, node: NodeApis) => { expect(stdout).toBe('foo bar json file.ts\nfoo bar json file.ts\nfoo bar json file.ts\nUnregistered'); }); - - test('namespace', async () => { - await using fixture = await createFixture({ - 'require.cjs': ` - const { expectErrors } = require('expect-errors'); - const path = require('node:path'); - const tsx = require(${JSON.stringify(tsxCjsApiPath)}); - - const api = tsx.register({ namespace: 'abcd' }); - - expectErrors( - // Loading explicit/resolved file path should be ignored by loader (extensions) - [() => require('./file.ts'), 'SyntaxError'], - - // resolver should preserve full file path when ignoring - [() => require('./file.ts?asdf'), "Cannot find module './file.ts?asdf'"] - ); - - const { message, async } = api.require('./file', __filename); - console.log(message); - async.then(m => console.log(m.default)); - - api.require('./tsx?query=1', __filename); - api.require('./jsx', __filename); - api.require('./dir?query=3', __filename); - `, - ...tsFiles, - - 'tsx.tsx': 'console.log(\'tsx\');', - 'jsx.jsx': 'console.log(\'jsx\');', - 'dir/index.jsx': 'console.log(\'dir\');', - }); - - const { stdout } = await execaNode(fixture.getPath('require.cjs'), [], { - nodePath: node.path, - nodeOptions: [], - }); - - expect(stdout).toBe('foo bar json file.ts\ntsx\njsx\ndir\nasync'); - }); }); });