From 8903f302b11da493214654a041ad45e83a6b2f1b Mon Sep 17 00:00:00 2001 From: Kevin Beier Date: Tue, 11 Aug 2026 19:35:41 +0200 Subject: [PATCH 1/2] fix(modernjs): harden SSR bundle serving against path traversal Confine /bundles and SSR JSON middleware to their roots and set Content-Length from UTF-8 byte length so non-ASCII federated chunks are not truncated. Co-authored-by: Cursor --- .changeset/modernjs-ssr-bundle-serving.md | 6 + packages/modernjs-v3/src/cli/ssrPlugin.ts | 17 ++- packages/modernjs-v3/src/server/fileCache.ts | 23 +--- .../src/server/staticMiddleware.spec.ts | 103 ++++++++++++------ .../src/server/staticMiddleware.ts | 53 ++++++--- packages/modernjs/src/cli/ssrPlugin.ts | 17 ++- packages/modernjs/src/server/fileCache.ts | 23 +--- .../src/server/staticMiddleware.spec.ts | 103 ++++++++++++------ .../modernjs/src/server/staticMiddleware.ts | 53 ++++++--- 9 files changed, 252 insertions(+), 146 deletions(-) create mode 100644 .changeset/modernjs-ssr-bundle-serving.md diff --git a/.changeset/modernjs-ssr-bundle-serving.md b/.changeset/modernjs-ssr-bundle-serving.md new file mode 100644 index 00000000000..9d8a6cd7229 --- /dev/null +++ b/.changeset/modernjs-ssr-bundle-serving.md @@ -0,0 +1,6 @@ +--- +'@module-federation/modern-js': patch +'@module-federation/modern-js-v3': patch +--- + +Harden SSR `/bundles` and JSON asset serving against path traversal, and set `Content-Length` from UTF-8 byte length so non-ASCII federated chunks are not truncated. diff --git a/packages/modernjs-v3/src/cli/ssrPlugin.ts b/packages/modernjs-v3/src/cli/ssrPlugin.ts index 576b17708c7..e0446106c67 100644 --- a/packages/modernjs-v3/src/cli/ssrPlugin.ts +++ b/packages/modernjs-v3/src/cli/ssrPlugin.ts @@ -340,11 +340,22 @@ export const moduleFederationSSRPlugin = ( return; } try { + const requestPath = (req.url ?? '').split(/[?#]/)[0]; if ( - req.url?.includes('.json') && - !req.url?.includes('hot-update') + path.extname(requestPath) === '.json' && + !requestPath.includes('hot-update') ) { - const filepath = path.join(process.cwd(), `dist${req.url}`); + const distRoot = path.resolve(process.cwd(), 'dist'); + const relativePath = requestPath.replace(/^\/+/, ''); + const filepath = path.resolve(distRoot, relativePath); + const relativeToDist = path.relative(distRoot, filepath); + if ( + relativeToDist.startsWith('..') || + path.isAbsolute(relativeToDist) + ) { + next(); + return; + } fs.statSync(filepath); res.setHeader('Access-Control-Allow-Origin', '*'); res.setHeader( diff --git a/packages/modernjs-v3/src/server/fileCache.ts b/packages/modernjs-v3/src/server/fileCache.ts index 237a232d0e9..a6284fe5898 100644 --- a/packages/modernjs-v3/src/server/fileCache.ts +++ b/packages/modernjs-v3/src/server/fileCache.ts @@ -1,15 +1,6 @@ -import { access, lstat, readFile } from 'fs/promises'; +import { lstat, readFile } from 'fs/promises'; import { SizeLimitedCache } from '@module-federation/bridge-react/size-limited-cache'; -const pathExists = async (filepath: string): Promise => { - try { - await access(filepath); - return true; - } catch { - return false; - } -}; - export interface FileResult { content: string; lastModified: number; @@ -28,12 +19,8 @@ export class FileCache { * @returns FileResult or null if file doesn't exist */ async getFile(filepath: string): Promise { - // Check if file exists - if (!(await pathExists(filepath))) { - return null; - } - try { + // lstat alone is enough: ENOENT / access errors return null below. const stat = await lstat(filepath); const currentModified = stat.mtimeMs; @@ -53,9 +40,9 @@ export class FileCache { lastModified: currentModified, }; - this.cache.set(filepath, newEntry, { - size: stat.size || content.length, - }); + // Charge UTF-8 bytes (never 0 — SizeLimitedCache rejects non-positive sizes). + const size = Math.max(Buffer.byteLength(content, 'utf8'), 1); + this.cache.set(filepath, newEntry, { size }); return { content, diff --git a/packages/modernjs-v3/src/server/staticMiddleware.spec.ts b/packages/modernjs-v3/src/server/staticMiddleware.spec.ts index d37cd778b61..9ea140bcf93 100644 --- a/packages/modernjs-v3/src/server/staticMiddleware.spec.ts +++ b/packages/modernjs-v3/src/server/staticMiddleware.spec.ts @@ -1,36 +1,30 @@ +import path from 'node:path'; import { it, expect, describe, rs, beforeEach } from '@rstest/core'; import { createStaticMiddleware } from './staticMiddleware'; -// Mock dependencies -rs.mock('fs/promises', () => ({ - access: rs.fn(), -})); - rs.mock('./fileCache', () => ({ fileCache: { getFile: rs.fn(), }, })); -import { access } from 'fs/promises'; import { fileCache } from './fileCache'; describe('staticMiddleware', () => { let middleware: any; let mockContext: any; let nextSpy: any; + const pwd = '/test/path'; + const bundlesRoot = path.resolve(pwd, 'bundles'); beforeEach(() => { - // Reset all mocks rs.clearAllMocks(); - // Create middleware instance middleware = createStaticMiddleware({ assetPrefix: '', - pwd: '/test/path', + pwd, }); - // Setup mock context nextSpy = rs.fn(); mockContext = { req: { @@ -64,12 +58,13 @@ describe('staticMiddleware', () => { it('should process .js files', async () => { mockContext.req.path = '/bundles/test.js'; - (access as any).mockRejectedValue(new Error('ENOENT')); + (fileCache.getFile as any).mockResolvedValue(null); await middleware(mockContext, nextSpy); - // Should not return early due to extension check - expect(access).toHaveBeenCalled(); + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, 'test.js'), + ); }); }); @@ -80,7 +75,7 @@ describe('staticMiddleware', () => { await middleware(mockContext, nextSpy); expect(nextSpy).toHaveBeenCalledOnce(); - expect(access).not.toHaveBeenCalled(); + expect(fileCache.getFile).not.toHaveBeenCalled(); expect(mockContext.header).not.toHaveBeenCalled(); expect(mockContext.body).not.toHaveBeenCalled(); }); @@ -91,44 +86,64 @@ describe('staticMiddleware', () => { await middleware(mockContext, nextSpy); expect(nextSpy).toHaveBeenCalledOnce(); - expect(access).not.toHaveBeenCalled(); + expect(fileCache.getFile).not.toHaveBeenCalled(); }); it('should process paths starting with /bundles', async () => { mockContext.req.path = '/bundles/test.js'; - (access as any).mockRejectedValue(new Error('ENOENT')); + (fileCache.getFile as any).mockResolvedValue(null); + + await middleware(mockContext, nextSpy); + + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, 'test.js'), + ); + }); + }); + + describe('path traversal protection', () => { + it('should call next() for parent-directory escapes', async () => { + mockContext.req.path = '/bundles/../secret.js'; + + await middleware(mockContext, nextSpy); + + expect(nextSpy).toHaveBeenCalledOnce(); + expect(fileCache.getFile).not.toHaveBeenCalled(); + }); + + it('should call next() for nested parent-directory escapes', async () => { + mockContext.req.path = '/bundles/foo/../../etc/passwd.js'; await middleware(mockContext, nextSpy); - // Should proceed to file existence check - expect(access).toHaveBeenCalledWith('/test/path/bundles/test.js'); + expect(nextSpy).toHaveBeenCalledOnce(); + expect(fileCache.getFile).not.toHaveBeenCalled(); }); }); describe('file existence check', () => { it('should call next() when file does not exist', async () => { mockContext.req.path = '/bundles/nonexistent.js'; - (access as any).mockRejectedValue(new Error('ENOENT')); + (fileCache.getFile as any).mockResolvedValue(null); await middleware(mockContext, nextSpy); - expect(access).toHaveBeenCalledWith('/test/path/bundles/nonexistent.js'); + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, 'nonexistent.js'), + ); expect(nextSpy).toHaveBeenCalledOnce(); - expect(fileCache.getFile).not.toHaveBeenCalled(); expect(mockContext.header).not.toHaveBeenCalled(); expect(mockContext.body).not.toHaveBeenCalled(); }); it('should proceed to file cache when file exists', async () => { mockContext.req.path = '/bundles/existing.js'; - (access as any).mockResolvedValue(undefined); (fileCache.getFile as any).mockResolvedValue(null); await middleware(mockContext, nextSpy); - expect(access).toHaveBeenCalledWith('/test/path/bundles/existing.js'); expect(fileCache.getFile).toHaveBeenCalledWith( - '/test/path/bundles/existing.js', + path.resolve(bundlesRoot, 'existing.js'), ); }); }); @@ -142,29 +157,25 @@ describe('staticMiddleware', () => { }; mockContext.req.path = '/bundles/app.js'; - (access as any).mockResolvedValue(undefined); (fileCache.getFile as any).mockResolvedValue(mockFileResult); mockContext.body.mockReturnValue('response'); const result = await middleware(mockContext, nextSpy); - expect(access).toHaveBeenCalledWith('/test/path/bundles/app.js'); expect(fileCache.getFile).toHaveBeenCalledWith( - '/test/path/bundles/app.js', + path.resolve(bundlesRoot, 'app.js'), ); expect(nextSpy).not.toHaveBeenCalled(); - // Check headers expect(mockContext.header).toHaveBeenCalledWith( 'Content-Type', 'application/javascript', ); expect(mockContext.header).toHaveBeenCalledWith( 'Content-Length', - String(mockFileResult.content.length), + String(Buffer.byteLength(mockFileContent)), ); - // Check response expect(mockContext.body).toHaveBeenCalledWith( mockFileResult.content, 200, @@ -172,6 +183,27 @@ describe('staticMiddleware', () => { expect(result).toBe('response'); }); + it('should set Content-Length in bytes for non-ASCII content', async () => { + const mockFileContent = 'console.log("中文注释");'; + const mockFileResult = { + content: mockFileContent, + lastModified: Date.now(), + }; + + mockContext.req.path = '/bundles/non-ascii.js'; + (fileCache.getFile as any).mockResolvedValue(mockFileResult); + + await middleware(mockContext, nextSpy); + + expect(Buffer.byteLength(mockFileContent)).toBeGreaterThan( + mockFileContent.length, + ); + expect(mockContext.header).toHaveBeenCalledWith( + 'Content-Length', + String(Buffer.byteLength(mockFileContent)), + ); + }); + it('should handle empty file content', async () => { const mockFileResult = { content: '', @@ -179,7 +211,6 @@ describe('staticMiddleware', () => { }; mockContext.req.path = '/bundles/empty.js'; - (access as any).mockResolvedValue(undefined); (fileCache.getFile as any).mockResolvedValue(mockFileResult); mockContext.body.mockReturnValue('empty-response'); @@ -199,7 +230,7 @@ describe('staticMiddleware', () => { it('should handle custom asset prefix correctly', async () => { const customMiddleware = createStaticMiddleware({ assetPrefix: '/custom-prefix', - pwd: '/test/path', + pwd, }); mockContext.req.path = '/bundles/test.js'; @@ -213,7 +244,7 @@ describe('staticMiddleware', () => { it('should handle asset prefix removal correctly', async () => { const customMiddleware = createStaticMiddleware({ assetPrefix: '/prefix', - pwd: '/test/path', + pwd, }); const mockFileResult = { @@ -222,13 +253,13 @@ describe('staticMiddleware', () => { }; mockContext.req.path = '/prefix/bundles/test.js'; - (access as any).mockResolvedValue(undefined); (fileCache.getFile as any).mockResolvedValue(mockFileResult); await customMiddleware(mockContext, nextSpy); - // Should remove prefix from path - expect(access).toHaveBeenCalledWith('/test/path/bundles/test.js'); + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, 'test.js'), + ); }); }); }); diff --git a/packages/modernjs-v3/src/server/staticMiddleware.ts b/packages/modernjs-v3/src/server/staticMiddleware.ts index fd0b909fc54..d31b5c679ac 100644 --- a/packages/modernjs-v3/src/server/staticMiddleware.ts +++ b/packages/modernjs-v3/src/server/staticMiddleware.ts @@ -1,18 +1,25 @@ -import { access } from 'fs/promises'; import path from 'node:path'; import { fileCache } from './fileCache'; import type { MiddlewareHandler } from '@modern-js/server-runtime'; -const pathExists = async (filepath: string): Promise => { - try { - await access(filepath); - return true; - } catch { - return false; - } -}; - const bundlesAssetPrefix = '/bundles'; + +function isPathInsideRoot(root: string, candidate: string): boolean { + const relative = path.relative(root, candidate); + return ( + relative === '' || + (!relative.startsWith('..') && !path.isAbsolute(relative)) + ); +} + +function joinUrlPath(...parts: string[]): string { + const joined = parts + .filter((part) => part != null && part !== '') + .join('/') + .replace(/\/{2,}/g, '/'); + return joined.startsWith('/') ? joined : `/${joined}`; +} + // Remove domain name from assetPrefix if it exists // and remove trailing slash if it exists, if the url is a single slash, return it as empty string const removeHost = (url: string): string => { @@ -35,6 +42,7 @@ const createStaticMiddleware = (options: { pwd: string; }): MiddlewareHandler => { const { assetPrefix, pwd } = options; + const bundlesRoot = path.resolve(pwd, 'bundles'); return async (c, next) => { const pathname = c.req.path; @@ -44,16 +52,27 @@ const createStaticMiddleware = (options: { return next(); } - const prefixWithoutHost = removeHost(assetPrefix); - const prefixWithBundle = path.join(prefixWithoutHost, bundlesAssetPrefix); + const prefixWithoutHost = removeHost(assetPrefix).replace(/\/+$/, ''); + // URL prefixes must stay POSIX-style; path.join breaks on Windows (`\bundles`). + const prefixWithBundle = joinUrlPath(prefixWithoutHost, bundlesAssetPrefix); // Skip if the request is not for asset prefix + `/bundles` if (!pathname.startsWith(prefixWithBundle)) { return next(); } - const pathnameWithoutPrefix = pathname.replace(prefixWithBundle, ''); - const filepath = path.join(pwd, bundlesAssetPrefix, pathnameWithoutPrefix); - if (!(await pathExists(filepath))) { + const pathnameWithoutPrefix = pathname + .slice(prefixWithBundle.length) + .replace(/^\/+/, ''); + if ( + !pathnameWithoutPrefix || + pathnameWithoutPrefix.includes('\0') || + pathnameWithoutPrefix.split(/[/\\]/).some((segment) => segment === '..') + ) { + return next(); + } + + const filepath = path.resolve(bundlesRoot, pathnameWithoutPrefix); + if (!isPathInsideRoot(bundlesRoot, filepath)) { return next(); } @@ -63,7 +82,9 @@ const createStaticMiddleware = (options: { } c.header('Content-Type', 'application/javascript'); - c.header('Content-Length', String(fileResult.content.length)); + // File content is a UTF-8 string; Content-Length must be byte length or + // non-ASCII chunks are truncated by clients that honor the header. + c.header('Content-Length', String(Buffer.byteLength(fileResult.content))); return c.body(fileResult.content, 200); }; }; diff --git a/packages/modernjs/src/cli/ssrPlugin.ts b/packages/modernjs/src/cli/ssrPlugin.ts index bba9fb7f75f..2630f99bd75 100644 --- a/packages/modernjs/src/cli/ssrPlugin.ts +++ b/packages/modernjs/src/cli/ssrPlugin.ts @@ -369,11 +369,22 @@ export const moduleFederationSSRPlugin = ( return; } try { + const requestPath = (req.url ?? '').split(/[?#]/)[0]; if ( - req.url?.includes('.json') && - !req.url?.includes('hot-update') + path.extname(requestPath) === '.json' && + !requestPath.includes('hot-update') ) { - const filepath = path.join(process.cwd(), `dist${req.url}`); + const distRoot = path.resolve(process.cwd(), 'dist'); + const relativePath = requestPath.replace(/^\/+/, ''); + const filepath = path.resolve(distRoot, relativePath); + const relativeToDist = path.relative(distRoot, filepath); + if ( + relativeToDist.startsWith('..') || + path.isAbsolute(relativeToDist) + ) { + next(); + return; + } fs.statSync(filepath); res.setHeader('Access-Control-Allow-Origin', '*'); res.setHeader( diff --git a/packages/modernjs/src/server/fileCache.ts b/packages/modernjs/src/server/fileCache.ts index 237a232d0e9..a6284fe5898 100644 --- a/packages/modernjs/src/server/fileCache.ts +++ b/packages/modernjs/src/server/fileCache.ts @@ -1,15 +1,6 @@ -import { access, lstat, readFile } from 'fs/promises'; +import { lstat, readFile } from 'fs/promises'; import { SizeLimitedCache } from '@module-federation/bridge-react/size-limited-cache'; -const pathExists = async (filepath: string): Promise => { - try { - await access(filepath); - return true; - } catch { - return false; - } -}; - export interface FileResult { content: string; lastModified: number; @@ -28,12 +19,8 @@ export class FileCache { * @returns FileResult or null if file doesn't exist */ async getFile(filepath: string): Promise { - // Check if file exists - if (!(await pathExists(filepath))) { - return null; - } - try { + // lstat alone is enough: ENOENT / access errors return null below. const stat = await lstat(filepath); const currentModified = stat.mtimeMs; @@ -53,9 +40,9 @@ export class FileCache { lastModified: currentModified, }; - this.cache.set(filepath, newEntry, { - size: stat.size || content.length, - }); + // Charge UTF-8 bytes (never 0 — SizeLimitedCache rejects non-positive sizes). + const size = Math.max(Buffer.byteLength(content, 'utf8'), 1); + this.cache.set(filepath, newEntry, { size }); return { content, diff --git a/packages/modernjs/src/server/staticMiddleware.spec.ts b/packages/modernjs/src/server/staticMiddleware.spec.ts index d37cd778b61..9ea140bcf93 100644 --- a/packages/modernjs/src/server/staticMiddleware.spec.ts +++ b/packages/modernjs/src/server/staticMiddleware.spec.ts @@ -1,36 +1,30 @@ +import path from 'node:path'; import { it, expect, describe, rs, beforeEach } from '@rstest/core'; import { createStaticMiddleware } from './staticMiddleware'; -// Mock dependencies -rs.mock('fs/promises', () => ({ - access: rs.fn(), -})); - rs.mock('./fileCache', () => ({ fileCache: { getFile: rs.fn(), }, })); -import { access } from 'fs/promises'; import { fileCache } from './fileCache'; describe('staticMiddleware', () => { let middleware: any; let mockContext: any; let nextSpy: any; + const pwd = '/test/path'; + const bundlesRoot = path.resolve(pwd, 'bundles'); beforeEach(() => { - // Reset all mocks rs.clearAllMocks(); - // Create middleware instance middleware = createStaticMiddleware({ assetPrefix: '', - pwd: '/test/path', + pwd, }); - // Setup mock context nextSpy = rs.fn(); mockContext = { req: { @@ -64,12 +58,13 @@ describe('staticMiddleware', () => { it('should process .js files', async () => { mockContext.req.path = '/bundles/test.js'; - (access as any).mockRejectedValue(new Error('ENOENT')); + (fileCache.getFile as any).mockResolvedValue(null); await middleware(mockContext, nextSpy); - // Should not return early due to extension check - expect(access).toHaveBeenCalled(); + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, 'test.js'), + ); }); }); @@ -80,7 +75,7 @@ describe('staticMiddleware', () => { await middleware(mockContext, nextSpy); expect(nextSpy).toHaveBeenCalledOnce(); - expect(access).not.toHaveBeenCalled(); + expect(fileCache.getFile).not.toHaveBeenCalled(); expect(mockContext.header).not.toHaveBeenCalled(); expect(mockContext.body).not.toHaveBeenCalled(); }); @@ -91,44 +86,64 @@ describe('staticMiddleware', () => { await middleware(mockContext, nextSpy); expect(nextSpy).toHaveBeenCalledOnce(); - expect(access).not.toHaveBeenCalled(); + expect(fileCache.getFile).not.toHaveBeenCalled(); }); it('should process paths starting with /bundles', async () => { mockContext.req.path = '/bundles/test.js'; - (access as any).mockRejectedValue(new Error('ENOENT')); + (fileCache.getFile as any).mockResolvedValue(null); + + await middleware(mockContext, nextSpy); + + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, 'test.js'), + ); + }); + }); + + describe('path traversal protection', () => { + it('should call next() for parent-directory escapes', async () => { + mockContext.req.path = '/bundles/../secret.js'; + + await middleware(mockContext, nextSpy); + + expect(nextSpy).toHaveBeenCalledOnce(); + expect(fileCache.getFile).not.toHaveBeenCalled(); + }); + + it('should call next() for nested parent-directory escapes', async () => { + mockContext.req.path = '/bundles/foo/../../etc/passwd.js'; await middleware(mockContext, nextSpy); - // Should proceed to file existence check - expect(access).toHaveBeenCalledWith('/test/path/bundles/test.js'); + expect(nextSpy).toHaveBeenCalledOnce(); + expect(fileCache.getFile).not.toHaveBeenCalled(); }); }); describe('file existence check', () => { it('should call next() when file does not exist', async () => { mockContext.req.path = '/bundles/nonexistent.js'; - (access as any).mockRejectedValue(new Error('ENOENT')); + (fileCache.getFile as any).mockResolvedValue(null); await middleware(mockContext, nextSpy); - expect(access).toHaveBeenCalledWith('/test/path/bundles/nonexistent.js'); + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, 'nonexistent.js'), + ); expect(nextSpy).toHaveBeenCalledOnce(); - expect(fileCache.getFile).not.toHaveBeenCalled(); expect(mockContext.header).not.toHaveBeenCalled(); expect(mockContext.body).not.toHaveBeenCalled(); }); it('should proceed to file cache when file exists', async () => { mockContext.req.path = '/bundles/existing.js'; - (access as any).mockResolvedValue(undefined); (fileCache.getFile as any).mockResolvedValue(null); await middleware(mockContext, nextSpy); - expect(access).toHaveBeenCalledWith('/test/path/bundles/existing.js'); expect(fileCache.getFile).toHaveBeenCalledWith( - '/test/path/bundles/existing.js', + path.resolve(bundlesRoot, 'existing.js'), ); }); }); @@ -142,29 +157,25 @@ describe('staticMiddleware', () => { }; mockContext.req.path = '/bundles/app.js'; - (access as any).mockResolvedValue(undefined); (fileCache.getFile as any).mockResolvedValue(mockFileResult); mockContext.body.mockReturnValue('response'); const result = await middleware(mockContext, nextSpy); - expect(access).toHaveBeenCalledWith('/test/path/bundles/app.js'); expect(fileCache.getFile).toHaveBeenCalledWith( - '/test/path/bundles/app.js', + path.resolve(bundlesRoot, 'app.js'), ); expect(nextSpy).not.toHaveBeenCalled(); - // Check headers expect(mockContext.header).toHaveBeenCalledWith( 'Content-Type', 'application/javascript', ); expect(mockContext.header).toHaveBeenCalledWith( 'Content-Length', - String(mockFileResult.content.length), + String(Buffer.byteLength(mockFileContent)), ); - // Check response expect(mockContext.body).toHaveBeenCalledWith( mockFileResult.content, 200, @@ -172,6 +183,27 @@ describe('staticMiddleware', () => { expect(result).toBe('response'); }); + it('should set Content-Length in bytes for non-ASCII content', async () => { + const mockFileContent = 'console.log("中文注释");'; + const mockFileResult = { + content: mockFileContent, + lastModified: Date.now(), + }; + + mockContext.req.path = '/bundles/non-ascii.js'; + (fileCache.getFile as any).mockResolvedValue(mockFileResult); + + await middleware(mockContext, nextSpy); + + expect(Buffer.byteLength(mockFileContent)).toBeGreaterThan( + mockFileContent.length, + ); + expect(mockContext.header).toHaveBeenCalledWith( + 'Content-Length', + String(Buffer.byteLength(mockFileContent)), + ); + }); + it('should handle empty file content', async () => { const mockFileResult = { content: '', @@ -179,7 +211,6 @@ describe('staticMiddleware', () => { }; mockContext.req.path = '/bundles/empty.js'; - (access as any).mockResolvedValue(undefined); (fileCache.getFile as any).mockResolvedValue(mockFileResult); mockContext.body.mockReturnValue('empty-response'); @@ -199,7 +230,7 @@ describe('staticMiddleware', () => { it('should handle custom asset prefix correctly', async () => { const customMiddleware = createStaticMiddleware({ assetPrefix: '/custom-prefix', - pwd: '/test/path', + pwd, }); mockContext.req.path = '/bundles/test.js'; @@ -213,7 +244,7 @@ describe('staticMiddleware', () => { it('should handle asset prefix removal correctly', async () => { const customMiddleware = createStaticMiddleware({ assetPrefix: '/prefix', - pwd: '/test/path', + pwd, }); const mockFileResult = { @@ -222,13 +253,13 @@ describe('staticMiddleware', () => { }; mockContext.req.path = '/prefix/bundles/test.js'; - (access as any).mockResolvedValue(undefined); (fileCache.getFile as any).mockResolvedValue(mockFileResult); await customMiddleware(mockContext, nextSpy); - // Should remove prefix from path - expect(access).toHaveBeenCalledWith('/test/path/bundles/test.js'); + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, 'test.js'), + ); }); }); }); diff --git a/packages/modernjs/src/server/staticMiddleware.ts b/packages/modernjs/src/server/staticMiddleware.ts index fd0b909fc54..d31b5c679ac 100644 --- a/packages/modernjs/src/server/staticMiddleware.ts +++ b/packages/modernjs/src/server/staticMiddleware.ts @@ -1,18 +1,25 @@ -import { access } from 'fs/promises'; import path from 'node:path'; import { fileCache } from './fileCache'; import type { MiddlewareHandler } from '@modern-js/server-runtime'; -const pathExists = async (filepath: string): Promise => { - try { - await access(filepath); - return true; - } catch { - return false; - } -}; - const bundlesAssetPrefix = '/bundles'; + +function isPathInsideRoot(root: string, candidate: string): boolean { + const relative = path.relative(root, candidate); + return ( + relative === '' || + (!relative.startsWith('..') && !path.isAbsolute(relative)) + ); +} + +function joinUrlPath(...parts: string[]): string { + const joined = parts + .filter((part) => part != null && part !== '') + .join('/') + .replace(/\/{2,}/g, '/'); + return joined.startsWith('/') ? joined : `/${joined}`; +} + // Remove domain name from assetPrefix if it exists // and remove trailing slash if it exists, if the url is a single slash, return it as empty string const removeHost = (url: string): string => { @@ -35,6 +42,7 @@ const createStaticMiddleware = (options: { pwd: string; }): MiddlewareHandler => { const { assetPrefix, pwd } = options; + const bundlesRoot = path.resolve(pwd, 'bundles'); return async (c, next) => { const pathname = c.req.path; @@ -44,16 +52,27 @@ const createStaticMiddleware = (options: { return next(); } - const prefixWithoutHost = removeHost(assetPrefix); - const prefixWithBundle = path.join(prefixWithoutHost, bundlesAssetPrefix); + const prefixWithoutHost = removeHost(assetPrefix).replace(/\/+$/, ''); + // URL prefixes must stay POSIX-style; path.join breaks on Windows (`\bundles`). + const prefixWithBundle = joinUrlPath(prefixWithoutHost, bundlesAssetPrefix); // Skip if the request is not for asset prefix + `/bundles` if (!pathname.startsWith(prefixWithBundle)) { return next(); } - const pathnameWithoutPrefix = pathname.replace(prefixWithBundle, ''); - const filepath = path.join(pwd, bundlesAssetPrefix, pathnameWithoutPrefix); - if (!(await pathExists(filepath))) { + const pathnameWithoutPrefix = pathname + .slice(prefixWithBundle.length) + .replace(/^\/+/, ''); + if ( + !pathnameWithoutPrefix || + pathnameWithoutPrefix.includes('\0') || + pathnameWithoutPrefix.split(/[/\\]/).some((segment) => segment === '..') + ) { + return next(); + } + + const filepath = path.resolve(bundlesRoot, pathnameWithoutPrefix); + if (!isPathInsideRoot(bundlesRoot, filepath)) { return next(); } @@ -63,7 +82,9 @@ const createStaticMiddleware = (options: { } c.header('Content-Type', 'application/javascript'); - c.header('Content-Length', String(fileResult.content.length)); + // File content is a UTF-8 string; Content-Length must be byte length or + // non-ASCII chunks are truncated by clients that honor the header. + c.header('Content-Length', String(Buffer.byteLength(fileResult.content))); return c.body(fileResult.content, 200); }; }; From 54021d8e658d25b5b25a92bddbee2c53995e3390 Mon Sep 17 00:00:00 2001 From: Kevin Beier Date: Tue, 11 Aug 2026 23:52:34 +0200 Subject: [PATCH 2/2] fix(modernjs): refine SSR root boundary checks --- .../modernjs-v3/src/cli/ssrPlugin.spec.ts | 76 ++++++++++++++++++- packages/modernjs-v3/src/cli/ssrPlugin.ts | 3 +- .../modernjs-v3/src/server/fileCache.spec.ts | 27 ++++++- .../src/server/staticMiddleware.spec.ts | 22 ++++++ .../src/server/staticMiddleware.ts | 4 +- packages/modernjs/src/cli/ssrPlugin.spec.ts | 75 +++++++++++++++++- packages/modernjs/src/cli/ssrPlugin.ts | 3 +- .../modernjs/src/server/fileCache.spec.ts | 27 ++++++- .../src/server/staticMiddleware.spec.ts | 22 ++++++ .../modernjs/src/server/staticMiddleware.ts | 4 +- 10 files changed, 253 insertions(+), 10 deletions(-) diff --git a/packages/modernjs-v3/src/cli/ssrPlugin.spec.ts b/packages/modernjs-v3/src/cli/ssrPlugin.spec.ts index 47e8c2894b4..1f3b31dcd93 100644 --- a/packages/modernjs-v3/src/cli/ssrPlugin.spec.ts +++ b/packages/modernjs-v3/src/cli/ssrPlugin.spec.ts @@ -2,7 +2,21 @@ import path from 'path'; import { createRequire } from 'node:module'; import { mkdtemp, mkdir, rm, writeFile } from 'fs/promises'; import { tmpdir } from 'os'; -import { describe, expect, it, rs } from '@rstest/core'; +import { beforeEach, describe, expect, it, rs } from '@rstest/core'; + +const fsMocks = rs.hoisted(() => ({ + statSync: rs.fn(), + createReadStream: rs.fn(() => ({ pipe: rs.fn() })), +})); + +rs.mock('fs', () => ({ + default: fsMocks, + ...fsMocks, +})); + +beforeEach(() => { + rs.clearAllMocks(); +}); const nodeRequire = createRequire(__filename); @@ -29,12 +43,16 @@ const createPluginHarness = async () => { } as any; let rsbuildPlugin: any; + let devServerMiddleware: any; const api = { _internalRuntimePlugins: rs.fn(), _internalServerPlugins: rs.fn(), config: rs.fn((callback) => { const config = callback(); rsbuildPlugin = config.builderPlugins[0]; + const middlewares: any[] = []; + config.dev.setupMiddlewares(middlewares); + devServerMiddleware = middlewares[0]; }), getAppContext: rs.fn(() => ({ bundlerType: 'rspack' })), getConfig: rs.fn(() => ({ server: { ssr: true } })), @@ -74,7 +92,7 @@ const createPluginHarness = async () => { { name: 'node' }, ); - return { rsbuildApi }; + return { rsbuildApi, devServerMiddleware }; }; const runCompiler = (config: import('@rspack/core').Configuration) => @@ -188,4 +206,58 @@ describe('moduleFederationSSRPlugin', () => { await rm(outputDir, { force: true, recursive: true }); } }); + + it('serves JSON assets with query and hash suffixes, including ..-prefixed names', async () => { + const { devServerMiddleware } = await createPluginHarness(); + const response = { setHeader: rs.fn() }; + const stream = { pipe: rs.fn() }; + (fsMocks.createReadStream as any).mockReturnValue(stream); + + for (const requestPath of [ + '/..manifest.json?query=value#hash', + '/nested/..generated/app.json', + ]) { + const next = rs.fn(); + + await devServerMiddleware({ url: requestPath }, response, next); + + expect(next).not.toHaveBeenCalled(); + } + + expect(fsMocks.statSync).toHaveBeenNthCalledWith( + 1, + path.resolve(process.cwd(), 'dist', '..manifest.json'), + ); + expect(fsMocks.statSync).toHaveBeenNthCalledWith( + 2, + path.resolve(process.cwd(), 'dist', 'nested/..generated/app.json'), + ); + expect(stream.pipe).toHaveBeenCalledTimes(2); + expect(response.setHeader).toHaveBeenCalledWith( + 'Access-Control-Allow-Origin', + '*', + ); + }); + + it('passes through non-JSON and traversal requests', async () => { + const { devServerMiddleware } = await createPluginHarness(); + + for (const requestPath of [ + '/manifest.js?query=value#hash', + '/../outside.json?query=value#hash', + ]) { + const next = rs.fn(); + + await devServerMiddleware( + { url: requestPath }, + { setHeader: rs.fn() }, + next, + ); + + expect(next).toHaveBeenCalledOnce(); + } + + expect(fsMocks.statSync).not.toHaveBeenCalled(); + expect(fsMocks.createReadStream).not.toHaveBeenCalled(); + }); }); diff --git a/packages/modernjs-v3/src/cli/ssrPlugin.ts b/packages/modernjs-v3/src/cli/ssrPlugin.ts index e0446106c67..96be679f9e9 100644 --- a/packages/modernjs-v3/src/cli/ssrPlugin.ts +++ b/packages/modernjs-v3/src/cli/ssrPlugin.ts @@ -350,7 +350,8 @@ export const moduleFederationSSRPlugin = ( const filepath = path.resolve(distRoot, relativePath); const relativeToDist = path.relative(distRoot, filepath); if ( - relativeToDist.startsWith('..') || + relativeToDist === '..' || + relativeToDist.startsWith(`..${path.sep}`) || path.isAbsolute(relativeToDist) ) { next(); diff --git a/packages/modernjs-v3/src/server/fileCache.spec.ts b/packages/modernjs-v3/src/server/fileCache.spec.ts index 87b08665b55..b0cc2cc481a 100644 --- a/packages/modernjs-v3/src/server/fileCache.spec.ts +++ b/packages/modernjs-v3/src/server/fileCache.spec.ts @@ -15,7 +15,8 @@ import { FileCache } from './fileCache'; describe('modern serve static file cache', async () => { beforeEach(() => { - rs.mocked(readFile).mockClear(); + rs.mocked(readFile).mockReset(); + rs.mocked(readFile).mockResolvedValue('test'); }); it('should cache file', async () => { @@ -36,4 +37,28 @@ describe('modern serve static file cache', async () => { expect(readFile).toHaveBeenCalledTimes(3); }); + + it('accounts for UTF-8 bytes when evicting files', async () => { + rs.mocked(readFile).mockImplementation((filepath) => + Promise.resolve(filepath === 'unicode.txt' ? '你' : 'a'), + ); + const cache = new FileCache(3); + + await cache.getFile('unicode.txt'); + await cache.getFile('ascii.txt'); + await cache.getFile('unicode.txt'); + + expect(readFile).toHaveBeenCalledTimes(3); + }); + + it('caches empty files without rejecting their cache entry', async () => { + rs.mocked(readFile).mockResolvedValue(''); + const cache = new FileCache(1); + + const result = await cache.getFile('empty.txt'); + await cache.getFile('empty.txt'); + + expect(result?.content).toBe(''); + expect(readFile).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/modernjs-v3/src/server/staticMiddleware.spec.ts b/packages/modernjs-v3/src/server/staticMiddleware.spec.ts index 9ea140bcf93..73e0982e9c0 100644 --- a/packages/modernjs-v3/src/server/staticMiddleware.spec.ts +++ b/packages/modernjs-v3/src/server/staticMiddleware.spec.ts @@ -119,6 +119,28 @@ describe('staticMiddleware', () => { expect(nextSpy).toHaveBeenCalledOnce(); expect(fileCache.getFile).not.toHaveBeenCalled(); }); + + it('should allow a file whose name starts with two dots', async () => { + mockContext.req.path = '/bundles/..chunk.js'; + + await middleware(mockContext, nextSpy); + + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, '..chunk.js'), + ); + expect(nextSpy).toHaveBeenCalledOnce(); + }); + + it('should allow nested files whose name starts with two dots', async () => { + mockContext.req.path = '/bundles/..generated/app.js'; + + await middleware(mockContext, nextSpy); + + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, '..generated/app.js'), + ); + expect(nextSpy).toHaveBeenCalledOnce(); + }); }); describe('file existence check', () => { diff --git a/packages/modernjs-v3/src/server/staticMiddleware.ts b/packages/modernjs-v3/src/server/staticMiddleware.ts index d31b5c679ac..6d383a58fd8 100644 --- a/packages/modernjs-v3/src/server/staticMiddleware.ts +++ b/packages/modernjs-v3/src/server/staticMiddleware.ts @@ -8,7 +8,9 @@ function isPathInsideRoot(root: string, candidate: string): boolean { const relative = path.relative(root, candidate); return ( relative === '' || - (!relative.startsWith('..') && !path.isAbsolute(relative)) + (relative !== '..' && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative)) ); } diff --git a/packages/modernjs/src/cli/ssrPlugin.spec.ts b/packages/modernjs/src/cli/ssrPlugin.spec.ts index b8da455a0de..98b9ae3c14c 100644 --- a/packages/modernjs/src/cli/ssrPlugin.spec.ts +++ b/packages/modernjs/src/cli/ssrPlugin.spec.ts @@ -1,4 +1,19 @@ -import { describe, expect, it, rs } from '@rstest/core'; +import path from 'node:path'; +import { beforeEach, describe, expect, it, rs } from '@rstest/core'; + +const fsMocks = rs.hoisted(() => ({ + statSync: rs.fn(), + createReadStream: rs.fn(() => ({ pipe: rs.fn() })), +})); + +rs.mock('fs', () => ({ + default: fsMocks, + ...fsMocks, +})); + +beforeEach(() => { + rs.clearAllMocks(); +}); const createPluginHarness = async () => { const { moduleFederationSSRPlugin } = await import('./ssrPlugin'); @@ -18,12 +33,14 @@ const createPluginHarness = async () => { } as any; let rsbuildPlugin: any; + let devServerMiddleware: any; const api = { _internalRuntimePlugins: rs.fn(), _internalServerPlugins: rs.fn(), config: rs.fn((callback) => { const config = callback(); rsbuildPlugin = config.builderPlugins[0]; + devServerMiddleware = config.tools.devServer.before[0]; }), getAppContext: rs.fn(() => ({ bundlerType: 'rspack' })), getConfig: rs.fn(() => ({ server: { ssr: true } })), @@ -64,7 +81,7 @@ const createPluginHarness = async () => { { name: 'node' }, ); - return { rsbuildApi }; + return { rsbuildApi, devServerMiddleware }; }; describe('moduleFederationSSRPlugin', () => { @@ -91,4 +108,58 @@ describe('moduleFederationSSRPlugin', () => { ); } }, 10_000); + + it('serves JSON assets with query and hash suffixes, including ..-prefixed names', async () => { + const { devServerMiddleware } = await createPluginHarness(); + const response = { setHeader: rs.fn() }; + const stream = { pipe: rs.fn() }; + (fsMocks.createReadStream as any).mockReturnValue(stream); + + for (const requestPath of [ + '/..manifest.json?query=value#hash', + '/nested/..generated/app.json', + ]) { + const next = rs.fn(); + + await devServerMiddleware({ url: requestPath }, response, next); + + expect(next).not.toHaveBeenCalled(); + } + + expect(fsMocks.statSync).toHaveBeenNthCalledWith( + 1, + path.resolve(process.cwd(), 'dist', '..manifest.json'), + ); + expect(fsMocks.statSync).toHaveBeenNthCalledWith( + 2, + path.resolve(process.cwd(), 'dist', 'nested/..generated/app.json'), + ); + expect(stream.pipe).toHaveBeenCalledTimes(2); + expect(response.setHeader).toHaveBeenCalledWith( + 'Access-Control-Allow-Origin', + '*', + ); + }); + + it('passes through non-JSON and traversal requests', async () => { + const { devServerMiddleware } = await createPluginHarness(); + + for (const requestPath of [ + '/manifest.js?query=value#hash', + '/../outside.json?query=value#hash', + ]) { + const next = rs.fn(); + + await devServerMiddleware( + { url: requestPath }, + { setHeader: rs.fn() }, + next, + ); + + expect(next).toHaveBeenCalledOnce(); + } + + expect(fsMocks.statSync).not.toHaveBeenCalled(); + expect(fsMocks.createReadStream).not.toHaveBeenCalled(); + }); }); diff --git a/packages/modernjs/src/cli/ssrPlugin.ts b/packages/modernjs/src/cli/ssrPlugin.ts index 2630f99bd75..a9fbb53f91d 100644 --- a/packages/modernjs/src/cli/ssrPlugin.ts +++ b/packages/modernjs/src/cli/ssrPlugin.ts @@ -379,7 +379,8 @@ export const moduleFederationSSRPlugin = ( const filepath = path.resolve(distRoot, relativePath); const relativeToDist = path.relative(distRoot, filepath); if ( - relativeToDist.startsWith('..') || + relativeToDist === '..' || + relativeToDist.startsWith(`..${path.sep}`) || path.isAbsolute(relativeToDist) ) { next(); diff --git a/packages/modernjs/src/server/fileCache.spec.ts b/packages/modernjs/src/server/fileCache.spec.ts index 87b08665b55..b0cc2cc481a 100644 --- a/packages/modernjs/src/server/fileCache.spec.ts +++ b/packages/modernjs/src/server/fileCache.spec.ts @@ -15,7 +15,8 @@ import { FileCache } from './fileCache'; describe('modern serve static file cache', async () => { beforeEach(() => { - rs.mocked(readFile).mockClear(); + rs.mocked(readFile).mockReset(); + rs.mocked(readFile).mockResolvedValue('test'); }); it('should cache file', async () => { @@ -36,4 +37,28 @@ describe('modern serve static file cache', async () => { expect(readFile).toHaveBeenCalledTimes(3); }); + + it('accounts for UTF-8 bytes when evicting files', async () => { + rs.mocked(readFile).mockImplementation((filepath) => + Promise.resolve(filepath === 'unicode.txt' ? '你' : 'a'), + ); + const cache = new FileCache(3); + + await cache.getFile('unicode.txt'); + await cache.getFile('ascii.txt'); + await cache.getFile('unicode.txt'); + + expect(readFile).toHaveBeenCalledTimes(3); + }); + + it('caches empty files without rejecting their cache entry', async () => { + rs.mocked(readFile).mockResolvedValue(''); + const cache = new FileCache(1); + + const result = await cache.getFile('empty.txt'); + await cache.getFile('empty.txt'); + + expect(result?.content).toBe(''); + expect(readFile).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/modernjs/src/server/staticMiddleware.spec.ts b/packages/modernjs/src/server/staticMiddleware.spec.ts index 9ea140bcf93..73e0982e9c0 100644 --- a/packages/modernjs/src/server/staticMiddleware.spec.ts +++ b/packages/modernjs/src/server/staticMiddleware.spec.ts @@ -119,6 +119,28 @@ describe('staticMiddleware', () => { expect(nextSpy).toHaveBeenCalledOnce(); expect(fileCache.getFile).not.toHaveBeenCalled(); }); + + it('should allow a file whose name starts with two dots', async () => { + mockContext.req.path = '/bundles/..chunk.js'; + + await middleware(mockContext, nextSpy); + + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, '..chunk.js'), + ); + expect(nextSpy).toHaveBeenCalledOnce(); + }); + + it('should allow nested files whose name starts with two dots', async () => { + mockContext.req.path = '/bundles/..generated/app.js'; + + await middleware(mockContext, nextSpy); + + expect(fileCache.getFile).toHaveBeenCalledWith( + path.resolve(bundlesRoot, '..generated/app.js'), + ); + expect(nextSpy).toHaveBeenCalledOnce(); + }); }); describe('file existence check', () => { diff --git a/packages/modernjs/src/server/staticMiddleware.ts b/packages/modernjs/src/server/staticMiddleware.ts index d31b5c679ac..6d383a58fd8 100644 --- a/packages/modernjs/src/server/staticMiddleware.ts +++ b/packages/modernjs/src/server/staticMiddleware.ts @@ -8,7 +8,9 @@ function isPathInsideRoot(root: string, candidate: string): boolean { const relative = path.relative(root, candidate); return ( relative === '' || - (!relative.startsWith('..') && !path.isAbsolute(relative)) + (relative !== '..' && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative)) ); }