diff --git a/packages/kernel-platforms/CHANGELOG.md b/packages/kernel-platforms/CHANGELOG.md index d4ab7eeb3..fedb3268c 100644 --- a/packages/kernel-platforms/CHANGELOG.md +++ b/packages/kernel-platforms/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING:** Vend the `fs` capability as an exo taking absolute path segments, replacing the `node:fs` lookalike record of functions ([#1057](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1057)) + - Call it as `await E(fs).readFile(['srv', 'data', 'x'])`. Methods share one flat namespace, so `promises.readFile` is now `readFile`, and a segment may not be empty, `.`, `..`, or contain a path separator. + - `existsSync` and every other synchronous operation are gone. A narrowed method forwards through `E()`, so nothing synchronous can survive narrowing. + - Config is `{ root: ['srv', 'data'], methods: ['readFile'] }`, replacing `{ rootDir, promises: { readFile } }`. An empty `root` is rejected rather than denoting the whole filesystem, and a platform prefix is a leading segment, so a Windows drive is `['C:', 'srv']`. + - A trailing argument must now be Passable, so `readFile(path, { signal })` is rejected where the bare `node:fs` function accepted it. + ### Removed - **BREAKING:** Remove the `fetch` platform capability and its exports (`fetchConfigStruct`, `FetchCapability`, `FetchConfig`, `makeHostCaveat`, `makeCaveatedFetch`) ([#942](https://github.com/MetaMask/ocap-kernel/pull/942)) diff --git a/packages/kernel-platforms/src/capabilities/fs/browser.test.ts b/packages/kernel-platforms/src/capabilities/fs/browser.test.ts index 4bdeb85d2..a77e9d58d 100644 --- a/packages/kernel-platforms/src/capabilities/fs/browser.test.ts +++ b/packages/kernel-platforms/src/capabilities/fs/browser.test.ts @@ -7,11 +7,11 @@ import type { FsConfig } from './types.ts'; describe('fs browser capability', () => { describe('capabilityFactory', () => { it.each([ - { name: 'readFile', config: { rootDir: '/root', methods: ['readFile'] } }, - { name: 'access', config: { rootDir: '/root', methods: ['access'] } }, + { name: 'readFile', config: { root: ['root'], methods: ['readFile'] } }, + { name: 'access', config: { root: ['root'], methods: ['access'] } }, { name: 'all operations', - config: { rootDir: '/root', methods: ['readFile', 'access'] }, + config: { root: ['root'], methods: ['readFile', 'access'] }, }, ] as { name: string; config: FsConfig }[])( 'throws not implemented error for $name', @@ -23,7 +23,7 @@ describe('fs browser capability', () => { ); it('creates capability with no operations', () => { - const config: FsConfig = { rootDir: '/root' }; + const config: FsConfig = { root: ['root'] }; const capability = capabilityFactory(config); expect(capability[GET_INTERFACE_GUARD]()).toBeDefined(); diff --git a/packages/kernel-platforms/src/capabilities/fs/browser.ts b/packages/kernel-platforms/src/capabilities/fs/browser.ts index 5eb69ea16..4cff8f5a9 100644 --- a/packages/kernel-platforms/src/capabilities/fs/browser.ts +++ b/packages/kernel-platforms/src/capabilities/fs/browser.ts @@ -8,4 +8,5 @@ export const { configStruct, capabilityFactory } = makeFsSpecification({ makeReadFile: () => notImplemented('readFile'), makeAccess: () => notImplemented('access'), makePathCaveat: () => () => undefined, + toPath: (segments) => `/${segments.join('/')}`, }); diff --git a/packages/kernel-platforms/src/capabilities/fs/nodejs.test.ts b/packages/kernel-platforms/src/capabilities/fs/nodejs.test.ts index 422617090..1f020e850 100644 --- a/packages/kernel-platforms/src/capabilities/fs/nodejs.test.ts +++ b/packages/kernel-platforms/src/capabilities/fs/nodejs.test.ts @@ -1,10 +1,9 @@ import { lstatSync, Stats } from 'node:fs'; import fs from 'node:fs/promises'; -import { relative } from 'node:path'; import { describe, expect, it, vi, beforeEach } from 'vitest'; import { capabilityFactory } from './nodejs.ts'; -import type { FsConfig, PathLike } from './types.ts'; +import type { FsConfig } from './types.ts'; /* eslint-disable n/no-sync */ @@ -23,119 +22,19 @@ vi.mock('node:fs', () => ({ lstatSync: vi.fn(), })); -// Mock path -vi.mock('node:path', () => ({ - relative: vi.fn(), -})); - // Mock factories const createMockStats = (isSymlink: boolean): Stats => ({ isSymbolicLink: () => isSymlink, }) as unknown as Stats; -const createMockRelative = (returnValue: string) => - vi.mocked(relative).mockReturnValue(returnValue); - const createMockLstatSync = (isSymlink: boolean) => vi.mocked(lstatSync).mockReturnValue(createMockStats(isSymlink)); describe('fs nodejs capability', () => { beforeEach(() => { vi.clearAllMocks(); - // Default mocks createMockLstatSync(false); - createMockRelative('subdir/file.txt'); - }); - - describe('caveat functions', () => { - describe('makeNoSymlinksCaveat', () => { - const createSymlinkCaveat = () => (path: PathLike) => { - const pathString = path.toString(); - const stats = lstatSync(pathString); - if (stats.isSymbolicLink()) { - throw new Error(`Symlinks are prohibited: ${pathString}`); - } - }; - - it('throws error for symlinks', () => { - createMockLstatSync(true); - const caveat = createSymlinkCaveat(); - - expect(() => caveat('/symlink/path')).toThrow( - 'Symlinks are prohibited: /symlink/path', - ); - expect(lstatSync).toHaveBeenCalledWith('/symlink/path'); - }); - - it.each([ - { - name: 'string', - input: '/path', - }, - { - name: 'Buffer', - input: Buffer.from('/path'), - }, - ])('accepts $name paths', ({ input }) => { - createMockLstatSync(false); - const caveat = createSymlinkCaveat(); - - expect(caveat(input)).toBeUndefined(); - expect(lstatSync).toHaveBeenCalledWith(input.toString()); - }); - }); - - describe('makeRootCaveat', () => { - const createRootCaveat = () => (path: PathLike) => { - const pathString = path.toString(); - const relativePath = relative('/root', pathString); - if (relativePath.startsWith('..')) { - throw new Error(`Path ${pathString} is outside allowed root /root`); - } - }; - - it('accepts paths within root directory', () => { - createMockRelative('subdir/file.txt'); - const caveat = createRootCaveat(); - - expect(caveat('/root/subdir/file.txt')).toBeUndefined(); - expect(relative).toHaveBeenCalledWith('/root', '/root/subdir/file.txt'); - }); - - it.each([ - { - name: 'outside root', - input: '/outside/file.txt', - relativeReturn: '../../outside/file.txt', - expectedError: 'Path /outside/file.txt is outside allowed root /root', - }, - { - name: 'with root prefix but outside', - input: '/root-other/file.txt', - relativeReturn: '../../root-other/file.txt', - expectedError: - 'Path /root-other/file.txt is outside allowed root /root', - }, - ])( - 'throws error for paths $name', - ({ input, relativeReturn, expectedError }) => { - createMockRelative(relativeReturn); - const caveat = createRootCaveat(); - - expect(() => caveat(input)).toThrow(expectedError); - expect(relative).toHaveBeenCalledWith('/root', input.toString()); - }, - ); - - it('accepts root directory itself', () => { - createMockRelative(''); - const caveat = createRootCaveat(); - - expect(caveat('/root')).toBeUndefined(); - expect(relative).toHaveBeenCalledWith('/root', '/root'); - }); - }); }); describe('capabilityFactory', () => { @@ -143,17 +42,15 @@ describe('fs nodejs capability', () => { { operation: 'readFile', mockFn: fs.readFile, - validArgs: ['/root/file.txt'], mockReturn: 'file content', - additionalArgs: ['/root/file.txt', { encoding: 'utf8' }], + additionalArg: { encoding: 'utf8' }, additionalMockReturn: Buffer.from('file content'), }, { operation: 'access', mockFn: fs.access, - validArgs: ['/root/file.txt'], mockReturn: undefined, - additionalArgs: ['/root/file.txt', 0o644], + additionalArg: 0o644, additionalMockReturn: undefined, }, ])( @@ -161,61 +58,67 @@ describe('fs nodejs capability', () => { ({ operation, mockFn, - validArgs, mockReturn, - additionalArgs, + additionalArg, additionalMockReturn, }) => { type TestCapability = Record; const makeCapability = (): TestCapability => { const config: FsConfig = { - rootDir: '/root', + root: ['root'], methods: [operation], }; return capabilityFactory(config) as unknown as TestCapability; }; - it('returns expected result for valid path', async () => { + it('joins segments into a path for the underlying operation', async () => { vi.mocked(mockFn).mockResolvedValue(mockReturn as never); - const result = await makeCapability()[operation]?.(...validArgs); - expect(mockFn).toHaveBeenCalledWith(...validArgs); + const result = await makeCapability()[operation]?.([ + 'root', + 'file.txt', + ]); + + expect(mockFn).toHaveBeenCalledWith('/root/file.txt'); expect(result).toBe(mockReturn); }); + it('throws error for a path outside the root', async () => { + await expect( + makeCapability()[operation]?.(['outside', 'file.txt']), + ).rejects.toThrow('is outside allowed root'); + expect(mockFn).not.toHaveBeenCalled(); + }); + + it('throws error for a symlink', async () => { + createMockLstatSync(true); + + await expect( + makeCapability()[operation]?.(['root', 'file.txt']), + ).rejects.toThrow('Symlinks are prohibited: /root/file.txt'); + expect(mockFn).not.toHaveBeenCalled(); + }); + it.each([ - { - name: 'outside root', - relativeReturn: '../../outside/file.txt', - isSymlink: false, - expectedError: `Path ${validArgs[0]} is outside allowed root /root`, - }, - { - name: 'symlink', - relativeReturn: '/root/file.txt', - isSymlink: true, - expectedError: `Symlinks are prohibited: ${validArgs[0]}`, - }, - ])( - 'throws error for path $name', - async ({ relativeReturn, isSymlink, expectedError }) => { - createMockRelative(relativeReturn); - createMockLstatSync(isSymlink); - - await expect( - makeCapability()[operation]?.(...validArgs), - ).rejects.toThrow(expectedError); - expect(mockFn).not.toHaveBeenCalled(); - }, - ); + { name: 'parent segments', segments: ['root', '..', '..', 'etc'] }, + { name: 'an embedded traversal', segments: ['root', '../../etc'] }, + ])('throws error for $name', async ({ segments }) => { + await expect(makeCapability()[operation]?.(segments)).rejects.toThrow( + 'contains an invalid segment', + ); + expect(mockFn).not.toHaveBeenCalled(); + }); it('handles additional arguments correctly', async () => { vi.mocked(mockFn).mockResolvedValue(additionalMockReturn as never); - const result = await makeCapability()[operation]?.(...additionalArgs); + const result = await makeCapability()[operation]?.( + ['root', 'file.txt'], + additionalArg, + ); - expect(mockFn).toHaveBeenCalledWith(...additionalArgs); + expect(mockFn).toHaveBeenCalledWith('/root/file.txt', additionalArg); expect(result).toBe(additionalMockReturn); }); }, diff --git a/packages/kernel-platforms/src/capabilities/fs/nodejs.ts b/packages/kernel-platforms/src/capabilities/fs/nodejs.ts index d1836c494..a93841b97 100644 --- a/packages/kernel-platforms/src/capabilities/fs/nodejs.ts +++ b/packages/kernel-platforms/src/capabilities/fs/nodejs.ts @@ -1,55 +1,51 @@ import { lstatSync } from 'node:fs'; import fs from 'node:fs/promises'; -import { relative } from 'node:path'; +import { resolve, sep } from 'node:path'; -import { makeFsSpecification } from './shared.ts'; -import type { PathLike, SyncPathCaveat } from './types.ts'; +import { makeFsSpecification, makeRootCaveat } from './shared.ts'; +import type { PathSegments, SegmentsCaveat } from './types.ts'; /** - * Node.js specific symlink caveat factory using node:fs + * Joins absolute segments into a Node.js path. + * + * `resolve` rather than a join on `sep` so a leading drive segment lands as a + * drive. Callers have already rejected separators and traversals, so there is + * nothing left for it to normalize away. * - * @returns A caveat function that validates a path against symlinks + * @param segments - The segments to join + * @returns The corresponding absolute path */ -const makeNoSymlinksCaveat = (): SyncPathCaveat => { - return (path: PathLike): void => { - const pathString = path.toString(); - // eslint-disable-next-line n/no-sync - const stats = lstatSync(pathString); - if (stats.isSymbolicLink()) { - throw new Error(`Symlinks are prohibited: ${pathString}`); - } - }; -}; +const toPath = (segments: PathSegments): string => resolve(sep, ...segments); /** - * Node.js specific root directory caveat factory using node:path + * Node.js specific symlink caveat factory using node:fs * - * @param rootDir - The root directory to validate paths against - * @returns A caveat function that validates a path against the root directory + * @returns A caveat function that validates segments against symlinks */ -const makeRootCaveat = (rootDir: string): SyncPathCaveat => { - return (path: PathLike): void => { - const pathString = path.toString(); - const relativePath = relative(rootDir, pathString); - if (relativePath.startsWith('..')) { - throw new Error(`Path ${pathString} is outside allowed root ${rootDir}`); +const makeNoSymlinksCaveat = (): SegmentsCaveat => { + return (segments: PathSegments): void => { + const path = toPath(segments); + // eslint-disable-next-line n/no-sync + const stats = lstatSync(path); + if (stats.isSymbolicLink()) { + throw new Error(`Symlinks are prohibited: ${path}`); } }; }; /** - * Node.js specific path caveat factory using node:path tools + * Node.js specific path caveat factory * - * @param rootDir - The root directory to validate paths against - * @returns A caveat function that validates a path against configured constraints + * @param root - The root the segments must extend + * @returns A caveat function that validates segments against configured constraints */ -const makeNodejsPathCaveat = (rootDir: string): SyncPathCaveat => { +const makeNodejsPathCaveat = (root: PathSegments): SegmentsCaveat => { + const withinRoot = makeRootCaveat(root); const noSymlinks = makeNoSymlinksCaveat(); - const withinRoot = makeRootCaveat(rootDir); - return harden((path: PathLike) => { - noSymlinks(path); - withinRoot(path); + return harden((segments: PathSegments) => { + withinRoot(segments); + noSymlinks(segments); }); }; @@ -57,4 +53,5 @@ export const { configStruct, capabilityFactory } = makeFsSpecification({ makeReadFile: () => fs.readFile, makeAccess: () => fs.access, makePathCaveat: makeNodejsPathCaveat, + toPath, }); diff --git a/packages/kernel-platforms/src/capabilities/fs/shared.test.ts b/packages/kernel-platforms/src/capabilities/fs/shared.test.ts index 693264128..6c5e6817b 100644 --- a/packages/kernel-platforms/src/capabilities/fs/shared.test.ts +++ b/packages/kernel-platforms/src/capabilities/fs/shared.test.ts @@ -3,28 +3,37 @@ import { getInterfaceGuardPayload, M } from '@endo/patterns'; import type { MethodGuard } from '@endo/patterns'; import { describe, expect, it, vi } from 'vitest'; -import { makeCaveatedFsOperation, makeFsSpecification } from './shared.ts'; +import { + assertPlainSegments, + makeCaveatedFsOperation, + makeFsSpecification, + makeRootCaveat, +} from './shared.ts'; import type { ReadFile, Access, - SyncPathCaveat, + SegmentsCaveat, FsCapability, } from './types.ts'; +const toPath = (segments: string[]): string => `/${segments.join('/')}`; + describe('makeCaveatedFsOperation', () => { + const makeCaveated = ( + operation: (...args: never[]) => Promise, + caveat: SegmentsCaveat, + ) => makeCaveatedFsOperation({ operation, caveat, toPath }); + it('applies caveat before operation', async () => { const mockOperation = vi.fn().mockResolvedValue('result'); const mockCaveat = vi.fn().mockReturnValue(undefined); - const caveatedOperation = makeCaveatedFsOperation( - mockOperation, - mockCaveat, - ); + const caveatedOperation = makeCaveated(mockOperation, mockCaveat); - const result = await caveatedOperation('/path', 'arg2', 'arg3'); + const result = await caveatedOperation(['srv', 'x'], 'arg2', 'arg3'); - expect(mockCaveat).toHaveBeenCalledWith('/path'); - expect(mockOperation).toHaveBeenCalledWith('/path', 'arg2', 'arg3'); + expect(mockCaveat).toHaveBeenCalledWith(['srv', 'x']); + expect(mockOperation).toHaveBeenCalledWith('/srv/x', 'arg2', 'arg3'); expect(result).toBe('result'); }); @@ -34,15 +43,11 @@ describe('makeCaveatedFsOperation', () => { throw new Error('Path not allowed'); }); - const caveatedOperation = makeCaveatedFsOperation( - mockOperation, - mockCaveat, - ); + const caveatedOperation = makeCaveated(mockOperation, mockCaveat); - await expect(caveatedOperation('/path')).rejects.toThrow( + await expect(caveatedOperation(['srv', 'x'])).rejects.toThrow( 'Path not allowed', ); - expect(mockCaveat).toHaveBeenCalledWith('/path'); expect(mockOperation).not.toHaveBeenCalled(); }); @@ -50,14 +55,59 @@ describe('makeCaveatedFsOperation', () => { const mockOperation = vi.fn().mockResolvedValue(undefined); const mockCaveat = vi.fn().mockReturnValue(undefined); - const caveatedOperation = makeCaveatedFsOperation( - mockOperation, - mockCaveat, + const caveatedOperation = makeCaveated(mockOperation, mockCaveat); + + expect(await caveatedOperation(['srv', 'x'])).toBeUndefined(); + expect(mockOperation).toHaveBeenCalledWith('/srv/x'); + }); + + it.each([ + { name: 'a parent traversal', segments: ['srv', '..', 'etc'] }, + { name: 'a bare dot', segments: ['srv', '.', 'x'] }, + { name: 'an embedded forward slash', segments: ['srv', 'data/../../etc'] }, + { name: 'an embedded backslash', segments: ['srv', 'data\\..\\..\\etc'] }, + { name: 'an empty segment', segments: ['srv', '', 'x'] }, + ])('rejects $name before the operation runs', async ({ segments }) => { + const mockOperation = vi.fn(); + const caveatedOperation = makeCaveated(mockOperation, vi.fn()); + + await expect(caveatedOperation(segments)).rejects.toThrow( + 'contains an invalid segment', ); + expect(mockOperation).not.toHaveBeenCalled(); + }); +}); + +describe('assertPlainSegments', () => { + it('accepts a drive-prefixed root', () => { + expect(() => assertPlainSegments(['C:', 'srv'], 'root')).not.toThrow(); + }); - expect(await caveatedOperation('/path')).toBeUndefined(); - expect(mockCaveat).toHaveBeenCalledWith('/path'); - expect(mockOperation).toHaveBeenCalledWith('/path'); + it('names what it was checking', () => { + expect(() => assertPlainSegments(['srv', '..'], 'root')).toThrow( + 'root contains an invalid segment: ".."', + ); + }); +}); + +describe('makeRootCaveat', () => { + it.each([ + { name: 'the root itself', segments: ['srv', 'data'] }, + { name: 'a path under the root', segments: ['srv', 'data', 'x', 'y'] }, + ])('accepts $name', ({ segments }) => { + expect(() => makeRootCaveat(['srv', 'data'])(segments)).not.toThrow(); + }); + + it.each([ + { name: 'a sibling of the root', segments: ['srv', 'other'] }, + { name: 'a prefix of the root', segments: ['srv'] }, + { name: 'a disjoint path', segments: ['etc', 'passwd'] }, + // `['srv', 'data']` must not admit `/srv/database`. + { name: 'a longer first segment', segments: ['srv', 'database', 'x'] }, + ])('rejects $name', ({ segments }) => { + expect(() => makeRootCaveat(['srv', 'data'])(segments)).toThrow( + 'is outside allowed root', + ); }); }); @@ -65,7 +115,7 @@ describe('makeFsSpecification', () => { const createMockSpecification = () => { const mockReadFile: ReadFile = vi.fn(); const mockAccess: Access = vi.fn(); - const mockPathCaveat: SyncPathCaveat = vi.fn(); + const mockPathCaveat: SegmentsCaveat = vi.fn(); const makeReadFile = vi.fn(() => mockReadFile); const makeAccess = vi.fn(() => mockAccess); @@ -74,6 +124,7 @@ describe('makeFsSpecification', () => { makeReadFile, makeAccess, makePathCaveat: () => mockPathCaveat, + toPath, }), mockReadFile, mockAccess, @@ -112,7 +163,7 @@ describe('makeFsSpecification', () => { ])('exposes exactly the methods named by $methods', ({ methods }) => { const { specification } = createMockSpecification(); const capability = specification.capabilityFactory({ - rootDir: '/root', + root: ['root'], methods: [...methods], }); @@ -123,7 +174,7 @@ describe('makeFsSpecification', () => { it('exposes no methods when the config omits the method list', () => { const { specification } = createMockSpecification(); - const capability = specification.capabilityFactory({ rootDir: '/root' }); + const capability = specification.capabilityFactory({ root: ['root'] }); expect(guardedMethodNames(capability)).toStrictEqual([]); }); @@ -132,7 +183,7 @@ describe('makeFsSpecification', () => { const { specification, makeReadFile, makeAccess } = createMockSpecification(); specification.capabilityFactory({ - rootDir: '/root', + root: ['root'], methods: ['readFile'], }); @@ -145,27 +196,40 @@ describe('makeFsSpecification', () => { createMockSpecification(); vi.mocked(mockReadFile).mockResolvedValue('contents' as never); const capability = specification.capabilityFactory({ - rootDir: '/root', + root: ['root'], methods: ['readFile'], }); - expect(await capability.readFile?.('/root/file.txt')).toBe('contents'); - expect(mockPathCaveat).toHaveBeenCalledWith('/root/file.txt'); + expect(await capability.readFile?.(['root', 'file.txt'])).toBe('contents'); + expect(mockPathCaveat).toHaveBeenCalledWith(['root', 'file.txt']); expect(mockReadFile).toHaveBeenCalledWith('/root/file.txt'); }); + it('rejects a root the config cannot address', () => { + const { specification } = createMockSpecification(); + + expect(() => + specification.capabilityFactory({ root: ['srv', '..'] }), + ).toThrow('root contains an invalid segment: ".."'); + }); + // Asserted by the operation not being reached rather than by the rejection // value: `mock-endoify` stubs out `assert`, so a guard violation rejects with // `undefined` and `rejects.toThrow()` would pass vacuously. - it('does not forward a readFile path that is not a string', async () => { + it.each([ + { name: 'a bare string', segments: '/root/file.txt' }, + { name: 'an array holding a non-string', segments: ['root', 42] }, + ])('does not forward a readFile path that is $name', async ({ segments }) => { const { specification, mockReadFile, mockPathCaveat } = createMockSpecification(); const capability = specification.capabilityFactory({ - rootDir: '/root', + root: ['root'], methods: ['readFile'], }); - await capability.readFile?.(42 as unknown as string).catch(() => undefined); + await capability + .readFile?.(segments as unknown as string[]) + .catch(() => undefined); expect(mockReadFile).not.toHaveBeenCalled(); expect(mockPathCaveat).not.toHaveBeenCalled(); @@ -174,26 +238,26 @@ describe('makeFsSpecification', () => { it('does not forward an access mode that is not a number', async () => { const { specification, mockAccess } = createMockSpecification(); const capability = specification.capabilityFactory({ - rootDir: '/root', + root: ['root'], methods: ['access'], }); await capability - .access?.('/root/file.txt', 'r' as unknown as number) + .access?.(['root', 'file.txt'], 'r' as unknown as number) .catch(() => undefined); expect(mockAccess).not.toHaveBeenCalled(); }); - it('guards a readFile path as a string', () => { + it('guards a readFile path as a string array', () => { const { specification } = createMockSpecification(); const capability = specification.capabilityFactory({ - rootDir: '/root', + root: ['root'], methods: ['readFile'], }); expect(methodGuards(capability).readFile).toStrictEqual( - M.callWhen(M.string()).optional(M.any()).returns(M.any()), + M.callWhen(M.arrayOf(M.string())).optional(M.any()).returns(M.any()), ); }); }); diff --git a/packages/kernel-platforms/src/capabilities/fs/shared.ts b/packages/kernel-platforms/src/capabilities/fs/shared.ts index c4b528310..f2ed0959f 100644 --- a/packages/kernel-platforms/src/capabilities/fs/shared.ts +++ b/packages/kernel-platforms/src/capabilities/fs/shared.ts @@ -3,8 +3,8 @@ import { M } from '@endo/patterns'; import type { MethodGuard } from '@endo/patterns'; import type { - PathLike, - SyncPathCaveat, + PathSegments, + SegmentsCaveat, ReadFile, Access, FsConfig, @@ -15,38 +15,92 @@ import type { import { fsConfigStruct } from './types.ts'; import { makeCapabilitySpecification } from '../../specification.ts'; +// The guard can only require strings, so `['srv', 'data/../../etc']` reaches +// here intact and would resolve to `/etc` on the way to a syscall. Rejecting +// these is what makes a prefix check sufficient. +const plainSegment = /^[^/\\]+$/u; + +/** + * Asserts that every segment addresses exactly one path component. + * + * @param segments - The segments to check + * @param label - What is being checked, for the error message + */ +export const assertPlainSegments = ( + segments: PathSegments, + label: string, +): void => { + const bad = segments.find( + (segment) => + segment === '.' || segment === '..' || !plainSegment.test(segment), + ); + if (bad !== undefined) { + throw new Error( + `${label} contains an invalid segment: ${JSON.stringify(bad)}`, + ); + } +}; + /** - * Cross-platform FS operation wrapper with validation + * Wraps a path-taking FS operation as a segments-taking one, with validation. * - * @param operation - The underlying operation to wrap - * @param syncPathCaveat - The caveat to apply to path arguments + * @param options - The operation and the restrictions to apply to it + * @param options.operation - The underlying operation to wrap + * @param options.caveat - The caveat to apply to the segments argument + * @param options.toPath - Converts segments to a platform path * @returns The operation restricted by the provided caveat */ -export const makeCaveatedFsOperation = < - Operation extends (...args: never[]) => Promise, ->( - operation: Operation, - syncPathCaveat: SyncPathCaveat, -): Operation => { - return harden(async (...args: Parameters) => { +export const makeCaveatedFsOperation = ({ + operation, + caveat, + toPath, +}: { + operation: (...args: never[]) => Promise; + caveat: SegmentsCaveat; + toPath: (segments: PathSegments) => string; +}): ((segments: PathSegments, ...rest: unknown[]) => Promise) => { + return harden(async (segments: PathSegments, ...rest: unknown[]) => { try { - // Assuming first argument is always the path - syncPathCaveat(args[0] as unknown as PathLike); + assertPlainSegments(segments, 'path'); + caveat(segments); // We don't need async caveats yet, but we could await one here. } catch (cause) { const message = cause instanceof Error ? cause.message : 'Caveat failed'; throw new Error(`fs.${operation.name}: ${message}`, { cause }); } - return operation(...args); - }) as Operation; + return operation(...([toPath(segments), ...rest] as unknown as never[])); + }); +}; + +/** + * Builds a caveat requiring segments to fall under a root. + * + * @param root - The root the segments must extend + * @returns A caveat that rejects segments outside the root + */ +export const makeRootCaveat = (root: PathSegments): SegmentsCaveat => { + return (segments: PathSegments): void => { + if ( + segments.length < root.length || + root.some((segment, index) => segments[index] !== segment) + ) { + throw new Error( + `Path ${JSON.stringify(segments)} is outside allowed root ${JSON.stringify(root)}`, + ); + } + }; }; // Written out per method rather than via `makeDefaultExo`, whose // `defaultGuards: 'passable'` leaves an empty guard map: narrowing conjoins a // delta onto a per-argument guard, so there has to be one to conjoin onto. const fsMethodGuards: Record = harden({ - readFile: M.callWhen(M.string()).optional(M.any()).returns(M.any()), - access: M.callWhen(M.string()).optional(M.number()).returns(M.undefined()), + readFile: M.callWhen(M.arrayOf(M.string())) + .optional(M.any()) + .returns(M.any()), + access: M.callWhen(M.arrayOf(M.string())) + .optional(M.number()) + .returns(M.undefined()), }); /* eslint-disable @typescript-eslint/explicit-function-return-type */ @@ -57,22 +111,26 @@ const fsMethodGuards: Record = harden({ * @param config.makeReadFile - The factory returning a read file operation * @param config.makeAccess - The factory returning an access operation * @param config.makePathCaveat - Factory function to create path caveats + * @param config.toPath - Converts segments to a platform path * @returns The capability specification */ export const makeFsSpecification = ({ makeReadFile, makeAccess, makePathCaveat, + toPath, }: { makeReadFile: () => ReadFile; makeAccess: () => Access; - makePathCaveat: (rootDir: string) => SyncPathCaveat; + makePathCaveat: (root: PathSegments) => SegmentsCaveat; + toPath: (segments: PathSegments) => string; }) => makeCapabilitySpecification( fsConfigStruct, (config: FsConfig): FsCapability => { - const { rootDir, methods = [] } = config; - const caveat = makePathCaveat(rootDir); + const { root, methods = [] } = config; + assertPlainSegments(root, 'root'); + const caveat = makePathCaveat(root); const makeOperation = { readFile: makeReadFile, access: makeAccess }; const guards: Partial> = {}; @@ -80,10 +138,11 @@ export const makeFsSpecification = ({ {}; for (const name of methods) { guards[name] = fsMethodGuards[name]; - operations[name] = makeCaveatedFsOperation( - makeOperation[name](), + operations[name] = makeCaveatedFsOperation({ + operation: makeOperation[name](), caveat, - ); + toPath, + }) as FsMethods[FsMethodName]; } return makeExo( diff --git a/packages/kernel-platforms/src/capabilities/fs/types.test.ts b/packages/kernel-platforms/src/capabilities/fs/types.test.ts index b10d49354..ba93a327c 100644 --- a/packages/kernel-platforms/src/capabilities/fs/types.test.ts +++ b/packages/kernel-platforms/src/capabilities/fs/types.test.ts @@ -7,38 +7,48 @@ import { superstructValidationError } from '../../../test/utils.ts'; describe('fs types', () => { describe('fsConfigStruct', () => { it.each([ - { name: 'minimal config with rootDir', config: { rootDir: '/root' } }, + { name: 'minimal config with root', config: { root: ['root'] } }, + { + name: 'config with a multi-segment root', + config: { root: ['srv', 'data'] }, + }, + { + name: 'config with a drive-prefixed root', + config: { root: ['C:', 'srv'] }, + }, { name: 'config with one method', - config: { rootDir: '/root', methods: ['readFile'] }, + config: { root: ['root'], methods: ['readFile'] }, }, { name: 'config with every method', - config: { rootDir: '/root', methods: ['readFile', 'access'] }, + config: { root: ['root'], methods: ['readFile', 'access'] }, }, { name: 'config with an empty method list', - config: { rootDir: '/root', methods: [] }, + config: { root: ['root'], methods: [] }, }, - { name: 'config with empty string rootDir', config: { rootDir: '' } }, ])('validates $name', ({ config }) => { expect(() => fsConfigStruct.create(config)).not.toThrow(); }); it.each([ - { name: 'config without rootDir', config: {} }, - { name: 'config with non-string rootDir', config: { rootDir: 123 } }, + { name: 'config without root', config: {} }, + { name: 'config with a non-array root', config: { root: 123 } }, + { name: 'config with a non-string segment', config: { root: [123] } }, + // An empty root would denote the whole filesystem. + { name: 'config with an empty root', config: { root: [] } }, { name: 'config with an unknown method', - config: { rootDir: '/root', methods: ['writeFile'] }, + config: { root: ['root'], methods: ['writeFile'] }, }, { name: 'config with a non-array methods', - config: { rootDir: '/root', methods: 'readFile' }, + config: { root: ['root'], methods: 'readFile' }, }, { name: 'config with additional properties', - config: { rootDir: '/root', extraProp: 'value' }, + config: { root: ['root'], extraProp: 'value' }, }, ])('rejects $name', ({ config }) => { expect(() => fsConfigStruct.create(config)).toThrow( @@ -47,18 +57,18 @@ describe('fs types', () => { }); it('allows undefined properties', () => { - const config: FsConfig = { rootDir: '/root' }; + const config: FsConfig = { root: ['root'] }; const validated = fsConfigStruct.create(config); - expect(validated).toStrictEqual({ rootDir: '/root' }); + expect(validated).toStrictEqual({ root: ['root'] }); }); it('preserves the method list', () => { - const config: FsConfig = { rootDir: '/root', methods: ['readFile'] }; + const config: FsConfig = { root: ['root'], methods: ['readFile'] }; const validated = fsConfigStruct.create(config); expect(validated).toStrictEqual({ - rootDir: '/root', + root: ['root'], methods: ['readFile'], }); }); diff --git a/packages/kernel-platforms/src/capabilities/fs/types.ts b/packages/kernel-platforms/src/capabilities/fs/types.ts index 5bff4c177..7b897031f 100644 --- a/packages/kernel-platforms/src/capabilities/fs/types.ts +++ b/packages/kernel-platforms/src/capabilities/fs/types.ts @@ -3,19 +3,19 @@ import { array, enums, exactOptional, + nonempty, object, string, } from '@metamask/superstruct'; import type { Infer } from '@metamask/superstruct'; -import type { PathLike } from 'node:fs'; import type { readFile, access } from 'node:fs/promises'; -export type { PathLike }; +// An absolute path, as in `['srv', 'data', 'x']`. Any platform prefix is a +// leading segment, so a Windows drive is `['C:', 'srv']`. +export type PathSegments = string[]; -// Throws if the path argument violates expectations (async version). -export type PathCaveat = (path: PathLike) => Promise; -// Throws if the path argument violates expectations (sync version). -export type SyncPathCaveat = (path: PathLike) => void; +// Throws if the segments argument violates expectations. +export type SegmentsCaveat = (segments: PathSegments) => void; export type ReadFile = typeof readFile; export type Access = typeof access; @@ -25,7 +25,7 @@ export const fsMethodNames = ['readFile', 'access'] as const; export type FsMethodName = (typeof fsMethodNames)[number]; export const fsConfigStruct = object({ - rootDir: string(), + root: nonempty(array(string())), methods: exactOptional(array(enums(fsMethodNames))), }); @@ -33,10 +33,13 @@ export type FsConfig = Infer; export type FsMethods = { readFile: ( - path: string, + segments: PathSegments, options?: Parameters[1], ) => ReturnType; - access: (path: string, mode?: Parameters[1]) => ReturnType; + access: ( + segments: PathSegments, + mode?: Parameters[1], + ) => ReturnType; }; export type FsCapability = Guarded>; diff --git a/packages/kernel-platforms/src/capabilities/index.test.ts b/packages/kernel-platforms/src/capabilities/index.test.ts index 41dd9bb94..81716fa2b 100644 --- a/packages/kernel-platforms/src/capabilities/index.test.ts +++ b/packages/kernel-platforms/src/capabilities/index.test.ts @@ -5,7 +5,7 @@ import { platformConfigStruct } from './index.ts'; describe('platformConfigStruct', () => { it.each([ { name: 'empty config', config: {} }, - { name: 'config with fs capability', config: { fs: { rootDir: '/tmp' } } }, + { name: 'config with fs capability', config: { fs: { root: ['tmp'] } } }, ])('validates $name', ({ config }) => { expect(() => platformConfigStruct.create(config)).not.toThrow(); }); diff --git a/packages/kernel-platforms/src/factory.test.ts b/packages/kernel-platforms/src/factory.test.ts index 536fb2422..09ae9d4c7 100644 --- a/packages/kernel-platforms/src/factory.test.ts +++ b/packages/kernel-platforms/src/factory.test.ts @@ -17,13 +17,13 @@ describe('makePlatformFactory', () => { it.each([ { name: 'single capability', - config: { fs: { rootDir: '/tmp' } }, + config: { fs: { root: ['tmp'] } }, expectedCapabilities: ['fs'] as const, expectedOptions: {}, }, { name: 'with options', - config: { fs: { rootDir: '/tmp' } }, + config: { fs: { root: ['tmp'] } }, expectedCapabilities: ['fs'] as const, expectedOptions: { fs: { timeout: 5000 } }, }, @@ -53,7 +53,7 @@ describe('makePlatformFactory', () => { it('creates platform with partial config', async () => { const mockFactories = createMockFactories(); const platformFactory = makePlatformFactory(mockFactories); - const config = { fs: { rootDir: '/tmp' } }; + const config = { fs: { root: ['tmp'] } }; const platform = await platformFactory(config); @@ -64,7 +64,7 @@ describe('makePlatformFactory', () => { const factories = { fs: vi.fn() }; const platformFactory = makePlatformFactory(factories); const config = { - fs: { rootDir: '/tmp' }, + fs: { root: ['tmp'] }, unknown: {}, } as Partial; await expect(platformFactory(config)).rejects.toThrow( diff --git a/packages/kernel-platforms/src/platform-test.ts b/packages/kernel-platforms/src/platform-test.ts index 1142ac8fd..561d33008 100644 --- a/packages/kernel-platforms/src/platform-test.ts +++ b/packages/kernel-platforms/src/platform-test.ts @@ -14,7 +14,7 @@ export const createPlatformTestSuite = ( it.each([ { name: 'fs capability', - config: { fs: { rootDir: '/tmp' } }, + config: { fs: { root: ['tmp'] } }, expectedFs: { type: 'object' }, }, ])('creates platform with $name', async ({ config, expectedFs }) => { @@ -23,7 +23,7 @@ export const createPlatformTestSuite = ( }); it('creates platform with partial config', async () => { - const config = { fs: { rootDir: '/tmp' } }; + const config = { fs: { root: ['tmp'] } }; const platform = await makePlatform(config); expect(platform.fs).toBeDefined(); diff --git a/packages/kernel-platforms/src/specification.test.ts b/packages/kernel-platforms/src/specification.test.ts index fbb3d13c0..b2519f721 100644 --- a/packages/kernel-platforms/src/specification.test.ts +++ b/packages/kernel-platforms/src/specification.test.ts @@ -26,7 +26,7 @@ describe('makeCapabilitySpecification', () => { mockCapabilityFactory, ); - const validConfig: FsConfig = { rootDir: '/tmp' }; + const validConfig: FsConfig = { root: ['tmp'] }; expect(() => specification.configStruct.create(validConfig)).not.toThrow(); }); @@ -37,7 +37,7 @@ describe('makeCapabilitySpecification', () => { mockCapabilityFactory, ); - const invalidConfig = { rootDir: 123 }; + const invalidConfig = { root: 123 }; expect(() => specification.configStruct.create(invalidConfig)).toThrow( superstructValidationError, ); @@ -50,7 +50,7 @@ describe('makeCapabilitySpecification', () => { mockCapabilityFactory, ); - const config: FsConfig = { rootDir: '/tmp' }; + const config: FsConfig = { root: ['tmp'] }; const options = { timeout: 5000 }; const result = specification.capabilityFactory(config, options); @@ -66,7 +66,7 @@ describe('makeCapabilitySpecification', () => { mockCapabilityFactory, ); - const config: FsConfig = { rootDir: '/tmp' }; + const config: FsConfig = { root: ['tmp'] }; const result = specification.capabilityFactory(config); diff --git a/packages/ocap-kernel/src/types.test.ts b/packages/ocap-kernel/src/types.test.ts index e8ddcb391..eb519e01d 100644 --- a/packages/ocap-kernel/src/types.test.ts +++ b/packages/ocap-kernel/src/types.test.ts @@ -123,7 +123,7 @@ describe('isVatConfig', () => { config: { bundleSpec: 'bundle.js', platformConfig: { - fs: { rootDir: '/tmp' }, + fs: { root: ['tmp'] }, }, }, expected: true, @@ -135,7 +135,7 @@ describe('isVatConfig', () => { creationOptions: { foo: 'bar' }, parameters: { baz: 123 }, platformConfig: { - fs: { rootDir: '/tmp', methods: ['readFile'] }, + fs: { root: ['tmp'], methods: ['readFile'] }, }, }, expected: true, @@ -159,7 +159,7 @@ describe('isVatConfig', () => { config: { bundleSpec: 'bundle.js', platformConfig: { - fs: { rootDir: 123 }, + fs: { root: 123 }, }, }, }, diff --git a/packages/ocap-kernel/src/vats/VatSupervisor.test.ts b/packages/ocap-kernel/src/vats/VatSupervisor.test.ts index 2c103fd15..c3c5d0e9d 100644 --- a/packages/ocap-kernel/src/vats/VatSupervisor.test.ts +++ b/packages/ocap-kernel/src/vats/VatSupervisor.test.ts @@ -255,7 +255,7 @@ describe('VatSupervisor', () => { describe('platform configuration', () => { it('accepts makePlatform and platformOptions parameters', async () => { const makePlatform = vi.fn().mockResolvedValue({}); - const platformOptions = { fs: { rootDir: '/tmp' } }; + const platformOptions = { fs: { root: ['tmp'] } }; const { supervisor } = await makeVatSupervisor({ makePlatform,