mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(routes): canonicalize the workspace before comparing it to a resolved path
validateSessionFilePath realpath-resolves the candidate path but compared it against the raw sessionWorkingDir. When the workspace is itself reached through a symlink the two sides live in different namespaces, so relative() reports a spurious `../` and every file in that workspace is judged an escape — reads and writes in the session are refused wholesale. That is not an exotic setup: os.tmpdir() hands back a symlinked path on macOS (/tmp -> /private/tmp), and symlinked project directories and bind-mounted case paths hit it too. Resolve both sides and compare canonical to canonical. This only makes the comparison honest — it does not widen it. The candidate keeps its own realpath, so a symlink pointing out of the workspace and a ../ traversal are still refused, and a workspace that cannot be resolved now fails closed. Three stubs in file-routes.test.ts used a blanket realpathSync.mockReturnValue(escapeTarget), which answers the same path for the workspace and the candidate; with both sides resolved that makes an escape look contained. They now use the input-aware mockImplementation idiom the rest of that file already uses, so the workspace resolves to itself and only the candidate escapes. Verified they still bite: removing the confinement check turns all of them red. Adds test/route-helpers-symlink-confinement.test.ts, which exercises the function against a real symlinked workspace on disk and pins the negative cases (../ escape, symlink-out, missing file) alongside the fix.
This commit is contained in:
@@ -63,19 +63,29 @@ export async function readJsonConfig<T>(filePath: string, logLabel: string, defa
|
|||||||
* Validates that a file path (possibly containing symlinks) resolves to a location
|
* Validates that a file path (possibly containing symlinks) resolves to a location
|
||||||
* within the given session working directory. Returns the resolved and relative paths,
|
* within the given session working directory. Returns the resolved and relative paths,
|
||||||
* or null if the path escapes the directory or doesn't exist.
|
* or null if the path escapes the directory or doesn't exist.
|
||||||
|
*
|
||||||
|
* BOTH sides are realpath-resolved before they are compared. Resolving only the
|
||||||
|
* candidate leaves the two paths in different namespaces whenever the workspace
|
||||||
|
* itself is reached through a symlink, and `relative()` then reports a spurious
|
||||||
|
* `../` for a file that is genuinely inside it — refusing every read and write in
|
||||||
|
* that session. A symlinked workspace is ordinary: `os.tmpdir()` returns one on
|
||||||
|
* macOS (`/tmp` -> `/private/tmp`), as do symlinked project dirs and bind-mounted
|
||||||
|
* case paths. Canonicalizing the base only makes the comparison honest; escapes
|
||||||
|
* are still refused, since the candidate keeps its own realpath.
|
||||||
*/
|
*/
|
||||||
export function validateSessionFilePath(
|
export function validateSessionFilePath(
|
||||||
sessionWorkingDir: string,
|
sessionWorkingDir: string,
|
||||||
filePath: string
|
filePath: string
|
||||||
): { resolvedPath: string; relativePath: string } | null {
|
): { resolvedPath: string; relativePath: string } | null {
|
||||||
const fullPath = resolve(sessionWorkingDir, filePath);
|
let resolvedWorkingDir: string;
|
||||||
let resolvedPath: string;
|
let resolvedPath: string;
|
||||||
try {
|
try {
|
||||||
resolvedPath = realpathSync(fullPath);
|
resolvedWorkingDir = realpathSync(sessionWorkingDir);
|
||||||
|
resolvedPath = realpathSync(resolve(sessionWorkingDir, filePath));
|
||||||
} catch {
|
} catch {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
const relativePath = relative(sessionWorkingDir, resolvedPath);
|
const relativePath = relative(resolvedWorkingDir, resolvedPath);
|
||||||
if (relativePath.startsWith('..') || isAbsolute(relativePath)) {
|
if (relativePath.startsWith('..') || isAbsolute(relativePath)) {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -0,0 +1,83 @@
|
|||||||
|
// Port: none (pure function over a real temp filesystem).
|
||||||
|
//
|
||||||
|
// `validateSessionFilePath` is the shared confinement gate for the file-serving
|
||||||
|
// and file-writing routes: it answers "does this path resolve to somewhere
|
||||||
|
// inside the session workspace". It realpath-resolves the CANDIDATE so a
|
||||||
|
// symlink cannot smuggle a path out of the workspace — but the workspace it
|
||||||
|
// compares against must be canonical too, or the two sides are expressed in
|
||||||
|
// different namespaces and `relative()` reports a spurious `../`.
|
||||||
|
//
|
||||||
|
// That is not exotic: a symlinked workspace is the norm on macOS, where
|
||||||
|
// `/tmp` is a symlink to `/private/tmp` and `os.tmpdir()` hands back the
|
||||||
|
// symlinked form, and it also covers symlinked project dirs and bind-mounted
|
||||||
|
// case paths. The effect is a workspace whose own files are all judged to be
|
||||||
|
// outside it, so every read and write in that session is refused.
|
||||||
|
import { mkdtempSync, mkdirSync, realpathSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
|
||||||
|
import { tmpdir } from 'node:os';
|
||||||
|
import { join } from 'node:path';
|
||||||
|
import { afterAll, describe, expect, it } from 'vitest';
|
||||||
|
|
||||||
|
import { validateSessionFilePath } from '../src/web/route-helpers.js';
|
||||||
|
|
||||||
|
// realpath the root itself so the fixture controls which side is symlinked,
|
||||||
|
// rather than inheriting whatever os.tmpdir() happens to be on this platform.
|
||||||
|
const root = realpathSync(mkdtempSync(join(tmpdir(), 'codeman-confinement-')));
|
||||||
|
const realWorkspace = join(root, 'real-workspace');
|
||||||
|
const linkedWorkspace = join(root, 'linked-workspace');
|
||||||
|
|
||||||
|
mkdirSync(join(realWorkspace, 'nested'), { recursive: true });
|
||||||
|
writeFileSync(join(realWorkspace, 'notes.md'), '# notes\n');
|
||||||
|
writeFileSync(join(realWorkspace, 'nested', 'deep.txt'), 'deep\n');
|
||||||
|
symlinkSync(realWorkspace, linkedWorkspace, 'dir');
|
||||||
|
|
||||||
|
afterAll(() => rmSync(root, { recursive: true, force: true }));
|
||||||
|
|
||||||
|
describe('validateSessionFilePath', () => {
|
||||||
|
it('accepts a file inside a workspace reached through a symlink', () => {
|
||||||
|
// The regression: the candidate is realpath'd to /…/real-workspace/notes.md
|
||||||
|
// while the base stays /…/linked-workspace, so a naive relative() yields
|
||||||
|
// '../real-workspace/notes.md' and the file is refused as an escape.
|
||||||
|
const result = validateSessionFilePath(linkedWorkspace, 'notes.md');
|
||||||
|
|
||||||
|
expect(result).not.toBeNull();
|
||||||
|
expect(result!.relativePath).toBe('notes.md');
|
||||||
|
expect(result!.resolvedPath).toBe(join(realWorkspace, 'notes.md'));
|
||||||
|
});
|
||||||
|
|
||||||
|
it('keeps the relative path usable for nested files under a symlinked workspace', () => {
|
||||||
|
// relativePath is what callers hand back to the client and re-join later,
|
||||||
|
// so an absolute or ../-prefixed value is a bug even when non-null.
|
||||||
|
const result = validateSessionFilePath(linkedWorkspace, 'nested/deep.txt');
|
||||||
|
|
||||||
|
expect(result).not.toBeNull();
|
||||||
|
expect(result!.relativePath).toBe(join('nested', 'deep.txt'));
|
||||||
|
});
|
||||||
|
|
||||||
|
it('accepts the same file through the canonical workspace path', () => {
|
||||||
|
const result = validateSessionFilePath(realWorkspace, 'notes.md');
|
||||||
|
|
||||||
|
expect(result).not.toBeNull();
|
||||||
|
expect(result!.relativePath).toBe('notes.md');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('still refuses a traversal escape from a symlinked workspace', () => {
|
||||||
|
// The point of canonicalizing the base is to make the comparison honest,
|
||||||
|
// NOT to loosen it: an escape must stay refused on both spellings.
|
||||||
|
writeFileSync(join(root, 'outside.txt'), 'outside\n');
|
||||||
|
|
||||||
|
expect(validateSessionFilePath(linkedWorkspace, '../outside.txt')).toBeNull();
|
||||||
|
expect(validateSessionFilePath(realWorkspace, '../outside.txt')).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('still refuses a symlink that points out of the workspace', () => {
|
||||||
|
// The candidate-side realpath must keep doing its job.
|
||||||
|
writeFileSync(join(root, 'secret.txt'), 'secret\n');
|
||||||
|
symlinkSync(join(root, 'secret.txt'), join(realWorkspace, 'escape.txt'));
|
||||||
|
|
||||||
|
expect(validateSessionFilePath(linkedWorkspace, 'escape.txt')).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns null for a path that does not exist', () => {
|
||||||
|
expect(validateSessionFilePath(linkedWorkspace, 'nope.md')).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -668,7 +668,9 @@ describe('file-routes', () => {
|
|||||||
|
|
||||||
it('rejects path traversal attempts', async () => {
|
it('rejects path traversal attempts', async () => {
|
||||||
// realpathSync resolves the symlink to a path outside workingDir
|
// realpathSync resolves the symlink to a path outside workingDir
|
||||||
mockedRealpathSync.mockReturnValue('/etc/passwd' as never);
|
mockedRealpathSync.mockImplementation(
|
||||||
|
(p: string) => (p === harness.ctx._session.workingDir ? p : '/etc/passwd') as never
|
||||||
|
);
|
||||||
|
|
||||||
const res = await harness.app.inject({
|
const res = await harness.app.inject({
|
||||||
method: 'GET',
|
method: 'GET',
|
||||||
@@ -773,7 +775,9 @@ describe('file-routes', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('rejects path traversal in raw file serving', async () => {
|
it('rejects path traversal in raw file serving', async () => {
|
||||||
mockedRealpathSync.mockReturnValue('/etc/shadow' as never);
|
mockedRealpathSync.mockImplementation(
|
||||||
|
(p: string) => (p === harness.ctx._session.workingDir ? p : '/etc/shadow') as never
|
||||||
|
);
|
||||||
|
|
||||||
const res = await harness.app.inject({
|
const res = await harness.app.inject({
|
||||||
method: 'GET',
|
method: 'GET',
|
||||||
@@ -868,7 +872,9 @@ describe('file-routes', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('rejects symlink targets that escape the session working directory', async () => {
|
it('rejects symlink targets that escape the session working directory', async () => {
|
||||||
mockedRealpathSync.mockReturnValue('/tmp/outside-workdir/link.log' as never);
|
mockedRealpathSync.mockImplementation(
|
||||||
|
(p: string) => (p === harness.ctx._session.workingDir ? p : '/tmp/outside-workdir/link.log') as never
|
||||||
|
);
|
||||||
|
|
||||||
const res = await harness.app.inject({
|
const res = await harness.app.inject({
|
||||||
method: 'GET',
|
method: 'GET',
|
||||||
|
|||||||
Reference in New Issue
Block a user