diff --git a/packages/devframe/src/utils/remote-assets.test.ts b/packages/devframe/src/utils/remote-assets.test.ts index 5fcda185..1a39d97c 100644 --- a/packages/devframe/src/utils/remote-assets.test.ts +++ b/packages/devframe/src/utils/remote-assets.test.ts @@ -1,10 +1,10 @@ import type { AddressInfo } from 'node:net' import type { MockInstance } from 'vitest' -import type { RemoteAssets, RemoteAssetsErrorMessage, RemoteAssetsStore } from '../types/remote-assets' +import type { RemoteAssets, RemoteAssetsErrorMessage, RemoteAssetsProviderCustom, RemoteAssetsStore } from '../types/remote-assets' import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, writeFileSync } from 'node:fs' import { createServer } from 'node:http' import { tmpdir } from 'node:os' -import { join } from 'node:path' +import { dirname, join } from 'node:path' import { pathToFileURL } from 'node:url' import { H3, toNodeHandler } from 'h3' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -188,6 +188,43 @@ describe('resolveStaticAssetsSource (remote store)', () => { expect(existsSync(join(target, 'package.json'))).toBe(false) }) + it('rejects unsafe provider-listed paths before fetching or writing them', async () => { + const calls: string[] = [] + const fetchImpl: typeof globalThis.fetch = async (input) => { + const url = String(input) + calls.push(url) + return url.endsWith('/dist/assets/app.js') ? new Response('console.log("app")') : new Response('should never be served') + } + const provider: RemoteAssetsProviderCustom = { + fileUrl: (pkg, version, filePath) => `https://mirror.example.com/${pkg}@${version}/${filePath}`, + // A compromised (or merely buggy) custom provider — every entry below is + // unsafe or out of scope except the one normal nested asset. + listFiles: async () => [ + 'package.json', // ordinary file outside the selected prefix — stays ignored + 'dist/assets/app.js', // a normal nested asset — still materializes + 'dist/../evil-traversal.txt', // prefixed traversal entry + '/outside/evil-absolute.txt', // absolute path entry + 'dist/evil\\..\\..\\evil-backslash.txt', // backslash traversal entry — rejected on every platform + 'dist-confusable/evil-prefix.txt', // prefix-confusion entry — outside the selected prefix + ], + } + const store = storeFor({ fetch: fetchImpl }, makeTmp(), { provider }) + const target = makeTmp() + + await store.materialize(target) + + // The one normal nested asset still materializes. + expect(readFileSync(join(target, 'assets/app.js'), 'utf8')).toBe('console.log("app")') + // Nothing else was fetched... + expect(calls).toEqual([expect.stringContaining('/dist/assets/app.js')]) + // ...or written, inside or outside the target directory. + expect(existsSync(join(target, 'package.json'))).toBe(false) + expect(existsSync(join(target, 'evil-traversal.txt'))).toBe(false) + expect(existsSync(join(dirname(target), 'evil-traversal.txt'))).toBe(false) + expect(existsSync(join(target, 'evil-backslash.txt'))).toBe(false) + expect(existsSync(join(dirname(target), 'evil-prefix.txt'))).toBe(false) + }) + it('supports the unpkg provider URL scheme', async () => { const calls: string[] = [] const fetchImpl: typeof globalThis.fetch = async (input) => { diff --git a/packages/devframe/src/utils/remote-assets.ts b/packages/devframe/src/utils/remote-assets.ts index 5852c926..f936ba99 100644 --- a/packages/devframe/src/utils/remote-assets.ts +++ b/packages/devframe/src/utils/remote-assets.ts @@ -11,7 +11,7 @@ import { createRequire } from 'node:module' import { Readable } from 'node:stream' import { lookup } from 'mrmime' import { createDebug } from 'obug' -import { dirname, extname, join, normalize, sep } from 'pathe' +import { dirname, extname, isAbsolute, join, normalize, resolve, sep } from 'pathe' import { diagnostics } from '../node/diagnostics' const debugFetch = createDebug('devframe:remote-assets:fetch') @@ -189,6 +189,27 @@ function candidatePaths(prefix: string, cleaned: string): string[] { return candidates } +/** + * Resolve the safe destination for a provider-listed `filePath` beneath + * `prefix`, materializing into the already-resolved `root`, or `null` when + * the entry is unsafe or lies outside the selected `prefix`. A compromised + * provider is an untrusted boundary even though the built-in jsDelivr/unpkg + * listings never emit any of this — reject an absolute path, a backslash + * (never normalized into a separator; Windows-style traversal stays rejected + * on every platform), and any suffix whose resolved destination would land + * outside `root`. `target === root` is deliberately unsafe too: it names the + * directory itself, never a writable file. + */ +function materializeTarget(filePath: string, prefix: string, root: string): string | null { + if (filePath.includes('\\') || isAbsolute(filePath) || !filePath.startsWith(prefix)) + return null + const suffix = filePath.slice(prefix.length) + if (!suffix || isAbsolute(suffix)) + return null + const target = resolve(root, suffix) + return target === root || !target.startsWith(root + sep) ? null : target +} + function createStore(assets: RemoteAssets, cacheDir: string): RemoteAssetsStore { const normalized = { ...assets, path: assets.path ?? 'dist' } const { provider, name: providerName } = resolveProvider(assets) @@ -340,8 +361,11 @@ function createStore(assets: RemoteAssets, cacheDir: string): RemoteAssetsStore catch (error) { return fail(errText(error), error) } - for (const filePath of files.filter(f => f.startsWith(prefix))) { - const target = join(targetDir, filePath.slice(prefix.length)) + const root = resolve(targetDir) + for (const filePath of files) { + const target = materializeTarget(filePath, prefix, root) + if (target == null) + continue const url = provider.fileUrl(normalized.package, normalized.version, filePath) let res: Response try { diff --git a/plans/README.md b/plans/README.md index 1bb65f2c..feec7f28 100644 --- a/plans/README.md +++ b/plans/README.md @@ -9,7 +9,7 @@ Generated by the improve skill on 2026-09-01 at commit `2d978f84`. Execute in th | 001 | Pin privileged GitHub Actions dependencies | P1 | S | - | TODO | | 002 | Require authentication on route-based MCP | P1 | M | 001 | TODO | | 003 | Enforce shared-state exposure policy on direct MCP reads | P1 | S | 002 | TODO | -| 004 | Contain remote asset materialization | P1 | S | - | TODO | +| 004 | Contain remote asset materialization | P1 | S | - | DONE | | 005 | Block Data Inspector prototype-chain writes | P1 | S | - | DONE | | 006 | Validate request-derived authentication-link origins | P1 | M | - | TODO | | 007 | Reject pre-existing symlink escapes from filesystem roots | P2 | M | - | TODO |