diff --git a/__tests__/security.test.ts b/__tests__/security.test.ts index 8cc9d7aa..fd075828 100644 --- a/__tests__/security.test.ts +++ b/__tests__/security.test.ts @@ -244,6 +244,44 @@ describe('Symlink escape prevention (#527)', () => { expect(validatePathWithinRoot(root, 'escapedir/secret.txt')).toBeNull(); }); + it('rejects a missing leaf below an out-of-root directory symlink', () => { + if (!link(path.join(root, 'escapedir'), path.join(outside, 'pkg'))) return; + expect(validatePathWithinRoot(root, 'escapedir/not-created-yet/file.txt')).toBeNull(); + }); + + // A directory link that needs no privilege on Windows (a junction), so the + // cases below run there without Developer Mode; a symlink elsewhere. Each + // test removes its own link, so nothing recursive ever walks through one. + const dirLink = (linkPath: string, target: string): void => + fs.symlinkSync(target, linkPath, process.platform === 'win32' ? 'junction' : 'dir'); + + it.runIf(process.platform === 'win32')('rejects a missing leaf below an out-of-root directory junction', () => { + const junction = path.join(root, 'escapejunction'); + dirLink(junction, path.join(outside, 'pkg')); + try { + expect(validatePathWithinRoot(root, 'escapejunction/not-created-yet/file.txt')).toBeNull(); + } finally { + fs.unlinkSync(junction); + } + }); + + it('still allows a missing leaf below a directory link that stays WITHIN the root', () => { + fs.mkdirSync(path.join(root, 'src', 'realdir')); + const inLink = path.join(root, 'src', 'linkdir'); + dirLink(inLink, path.join(root, 'src', 'realdir')); + try { + expect(validatePathWithinRoot(root, 'src/linkdir/new/file.ts')).toBe( + path.join(root, 'src', 'linkdir', 'new', 'file.ts') + ); + } finally { + fs.unlinkSync(inLink); + } + }); + + it('rejects every path under a root that does not exist', () => { + expect(validatePathWithinRoot(path.join(root, 'gone'), 'src/in.ts')).toBeNull(); + }); + it('still allows an in-repo symlink that stays WITHIN the root (no over-blocking)', () => { if (!link(path.join(root, 'src', 'inlink.ts'), path.join(root, 'src', 'in.ts'))) return; expect(validatePathWithinRoot(root, 'src/inlink.ts')).not.toBeNull(); @@ -263,6 +301,18 @@ describe('Symlink escape prevention (#527)', () => { expect(validatePathWithinRoot(root, 'escapedir/secret.txt', { allowSymlinkEscape: true })).not.toBeNull(); }); + it('allowSymlinkEscape still allows a missing leaf through an out-of-root DIR link (indexing read)', () => { + const escapeLink = path.join(root, 'escapedir'); + dirLink(escapeLink, path.join(outside, 'pkg')); + try { + expect( + validatePathWithinRoot(root, 'escapedir/not-created-yet/file.txt', { allowSymlinkEscape: true }) + ).not.toBeNull(); + } finally { + fs.unlinkSync(escapeLink); + } + }); + it('allowSymlinkEscape STILL rejects a lexical ../ traversal (guard not waived)', () => { expect( validatePathWithinRoot(root, `../${path.basename(outside)}/pkg/secret.txt`, { allowSymlinkEscape: true }) diff --git a/__tests__/ui-trails.test.ts b/__tests__/ui-trails.test.ts index a669cb1c..8f6b13e2 100644 --- a/__tests__/ui-trails.test.ts +++ b/__tests__/ui-trails.test.ts @@ -18,6 +18,7 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import * as http from 'http'; +import { execFileSync } from 'child_process'; import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; @@ -26,11 +27,15 @@ import { createGraphApi, startUiServer, type GraphApi, type UiServerHandle } fro import { encodeResolvedRun, isTrailId, + listStoredTrails, parseTrail, slugify, TRAILS_RELATIVE_DIR, + type StoredTrail, type WireTrailHop, } from '../src/ui-server/api'; +import { writeStoredTrail } from '../src/ui-server/api/trail-store'; +import { resetTrailAuthor, trailAuthor } from '../src/ui-server/api/trails'; interface Res { status: number; @@ -115,6 +120,66 @@ function trailsDir(): string { return path.join(projectRoot, TRAILS_RELATIVE_DIR); } +function storedTrail(id: string): StoredTrail { + return { + version: 1, + id, + name: 'Must stay inside', + note: '', + author: 'test', + createdAt: new Date(0).toISOString(), + updatedAt: new Date(0).toISOString(), + hops: [ + { + dir: 'start', + name: 'start', + qualifiedName: 'start', + kind: 'function', + file: 'src/start.ts', + line: 1, + id: 'function:start', + }, + ], + }; +} + +/** Remove a link itself, never what it points at, when there is one. */ +function removeLink(link: string): void { + try { + if (fs.lstatSync(link).isSymbolicLink()) fs.unlinkSync(link); + } catch { + // Nothing there. + } +} + +/** + * Whether this machine can create a link of `type`. A Windows junction needs no + * privilege; a symbolic link there needs Developer Mode or admin rights. Where + * one can't be made, its tests are skipped, with the reason in their name. + */ +function canCreateLink(type: 'file' | 'dir' | 'junction'): boolean { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-link-probe-')); + const target = path.join(dir, 'target'); + const link = path.join(dir, 'link'); + try { + if (type === 'file') fs.writeFileSync(target, ''); + else fs.mkdirSync(target); + fs.symlinkSync(target, link, type); + return true; + } catch { + return false; + } finally { + removeLink(link); + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +/** A `.codegraph/ui` link can be a junction or a directory symlink on Windows. */ +const DIR_LINKS = (process.platform === 'win32' ? ['junction', 'dir'] : ['dir']) as Array< + 'junction' | 'dir' +>; +const NO_LINK = ' (skipped: this link needs Developer Mode or admin rights on Windows)'; + /** Re-index in place, the way a `codegraph sync` would after an edit. */ async function reindex(): Promise { const cg = CodeGraph.openSync(projectRoot); @@ -216,6 +281,153 @@ describe('trail ids', () => { }); }); +describe('trail write containment', () => { + for (const type of DIR_LINKS) { + const canLink = canCreateLink(type); + const kind = type === 'junction' ? 'junction' : 'symlink'; + + it.skipIf(!canLink)( + `refuses a missing trail directory below a ${kind} that leaves the project${canLink ? '' : NO_LINK}`, + () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-trail-symlink-')); + const project = path.join(base, 'project'); + const outside = path.join(base, 'outside'); + const uiLink = path.join(project, '.codegraph', 'ui'); + fs.mkdirSync(path.join(project, '.codegraph'), { recursive: true }); + fs.mkdirSync(outside, { recursive: true }); + fs.symlinkSync(outside, uiLink, type); + + try { + expect(() => writeStoredTrail(project, storedTrail('must-stay-inside'))).toThrow( + /outside the project/ + ); + expect(fs.existsSync(path.join(outside, 'trails'))).toBe(false); + } finally { + removeLink(uiLink); + fs.rmSync(base, { recursive: true, force: true }); + } + } + ); + + it.skipIf(!canLink)( + `allows a project root reached through a ${kind} when the real path stays inside it${canLink ? '' : NO_LINK}`, + () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-trail-root-link-')); + const project = path.join(base, 'project'); + const alias = path.join(base, 'project-link'); + fs.mkdirSync(path.join(project, '.codegraph'), { recursive: true }); + fs.symlinkSync(project, alias, type); + + try { + expect(() => writeStoredTrail(alias, storedTrail('safe-root-link'))).not.toThrow(); + expect( + fs.existsSync(path.join(project, TRAILS_RELATIVE_DIR, 'safe-root-link.json')) + ).toBe(true); + } finally { + removeLink(alias); + fs.rmSync(base, { recursive: true, force: true }); + } + } + ); + } +}); + +describe('trail read containment', () => { + const fileLinks = canCreateLink('file'); + + it.skipIf(!fileLinks)( + `skips a trail file symlinked outside the project instead of reading it${fileLinks ? '' : NO_LINK}`, + () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-trail-read-link-')); + const project = path.join(base, 'project'); + const outside = path.join(base, 'outside'); + const dir = path.join(project, TRAILS_RELATIVE_DIR); + const linked = path.join(dir, 'linked.json'); + fs.mkdirSync(dir, { recursive: true }); + fs.mkdirSync(outside, { recursive: true }); + fs.writeFileSync( + path.join(outside, 'secret.json'), + JSON.stringify({ ...storedTrail('outside'), note: 'must not be read' }) + ); + fs.symlinkSync(path.join(outside, 'secret.json'), linked, 'file'); + + try { + expect(listStoredTrails(project)).toEqual({ trails: [], skipped: 1 }); + } finally { + removeLink(linked); + fs.rmSync(base, { recursive: true, force: true }); + } + } + ); + + it.runIf(process.platform !== 'win32')( + 'skips a FIFO in the trails directory without blocking on it', + () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-trail-read-fifo-')); + const project = path.join(base, 'project'); + const dir = path.join(project, TRAILS_RELATIVE_DIR); + fs.mkdirSync(dir, { recursive: true }); + // A FIFO with no writer: a blocking open would hang here until one showed up. + execFileSync('mkfifo', [path.join(dir, 'blocked.json')]); + + try { + // Run current TS source in a child: a synchronous open blocks Vitest's + // own timeout, but the parent's execFileSync timeout can kill the child. + const script = ` + const fs = require('fs'); + const ts = require('typescript'); + require.extensions['.ts'] = (module, filename) => { + const source = fs.readFileSync(filename, 'utf8'); + const { outputText } = ts.transpileModule(source, { + compilerOptions: { module: ts.ModuleKind.CommonJS, target: ts.ScriptTarget.ES2022 }, + }); + module._compile(outputText, filename); + }; + const { listStoredTrails } = require(process.argv[1]); + process.stdout.write(JSON.stringify(listStoredTrails(process.argv[2]))); + `; + const result = execFileSync(process.execPath, [ + '-e', script, path.resolve(__dirname, '../src/ui-server/api/trail-store.ts'), project, + ], { + cwd: path.resolve(__dirname, '..'), + encoding: 'utf8', + timeout: 5_000, + killSignal: 'SIGKILL', + }); + expect(JSON.parse(result)).toEqual({ trails: [], skipped: 1 }); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }, + 10_000 + ); +}); + +describe('trail author cache', () => { + it('keeps git user names separate for projects served by one process', () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-trail-authors-')); + const first = path.join(base, 'first'); + const second = path.join(base, 'second'); + + try { + for (const [dir, author] of [ + [first, 'Alice Project'], + [second, 'Bob Project'], + ] as const) { + fs.mkdirSync(dir, { recursive: true }); + execFileSync('git', ['init', '-q'], { cwd: dir }); + execFileSync('git', ['config', 'user.name', author], { cwd: dir }); + } + resetTrailAuthor(); + expect(trailAuthor(first)).toBe('Alice Project'); + expect(trailAuthor(second)).toBe('Bob Project'); + } finally { + resetTrailAuthor(); + fs.rmSync(base, { recursive: true, force: true }); + } + }); +}); + describe('parseTrail', () => { it('rejects a file that is not a trail rather than half-reading it', () => { expect(parseTrail('x', 'not json')).toBeNull(); @@ -225,6 +437,12 @@ describe('parseTrail', () => { expect(parseTrail('x', '{"name":"","hops":[{"qualifiedName":"a"}]}')).toBeNull(); }); + it('rejects a declared format version this build does not understand', () => { + expect( + parseTrail('future', JSON.stringify({ ...storedTrail('future'), version: 2 })) + ).toBeNull(); + }); + it('takes its id from the FILE, not from the field inside it', () => { const trail = parseTrail('on-disk', '{"name":"a","id":"remembered","hops":[{"name":"f"}]}'); expect(trail?.id).toBe('on-disk'); diff --git a/docs/viewer-launch-changelog.md b/docs/viewer-launch-changelog.md index c8195357..70b17271 100644 --- a/docs/viewer-launch-changelog.md +++ b/docs/viewer-launch-changelog.md @@ -215,6 +215,10 @@ These describe `codegraph ui` and its screens. They were taken out of `## [Unrel - A viewer trail now holds up to 64 hops everywhere — the trail bar, saved trails and "Read as flow". Thanks @inth3shadows for the report and @danusha2345. (#1976) +- **Saved trails stay inside your project.** When `.codegraph/ui` was a link to a folder somewhere else, saving a trail wrote it into that folder, even though the save then reported an error, and a trail file that was itself a link to a file outside the project was listed and opened like one of yours. Both are now refused, and links that stay inside the project keep working. Thanks @danusha2345. + +- **Saved trails record the right author when one process serves several projects.** An app embedding the viewer for more than one project wrote the first project's git user name on every trail it saved; each project now uses its own. Thanks @danusha2345. + - **`codegraph ui` on Windows moves on from a port another program holds.** Windows refuses a port that another program keeps for itself, or one inside a range it reserves, with a different error than a port that is simply in use, so the viewer gave up at the first such port instead of trying the next one, and blamed ports below 1024 needing elevated privileges — a rule Windows doesn't have. It now moves on to the next free port, and when none is left, or the port you pinned with `--port` is refused, it says what Windows did and how to list the reserved ranges. Thanks @ijbranch for the report and @sx4im. (#2299) - **A file's outside imports are listed in more languages.** The file screen lists the imports that name nothing in your project, like a third-party package or a system header. In C and C++, Objective-C, JavaScript and TypeScript, Python, C#, Lua, R, Solidity, Svelte and Vue, an import that named no file had been linked to its own import statement, so it was listed nowhere. Re-index after upgrading. diff --git a/src/ui-server/api/trail-store.ts b/src/ui-server/api/trail-store.ts index e6c5e19b..60dce867 100644 --- a/src/ui-server/api/trail-store.ts +++ b/src/ui-server/api/trail-store.ts @@ -28,7 +28,7 @@ */ import * as fs from 'fs'; -import * as path from 'path'; +import { randomUUID } from 'crypto'; import { CODEGRAPH_DIR } from '../../directory'; import { resolveProjectFile } from '../security'; import { ApiError, badRequest } from './respond'; @@ -162,6 +162,9 @@ export function parseTrail(id: string, text: string): StoredTrail | null { } if (typeof raw !== 'object' || raw === null) return null; const value = raw as Record; + // Unversioned hand-written trails predate the stored format marker and are + // treated as v1. A declared future/unknown version must not be guessed at. + if (value.version !== undefined && value.version !== TRAIL_FORMAT_VERSION) return null; if (typeof value.name !== 'string' || value.name.trim() === '') return null; if (!Array.isArray(value.hops) || value.hops.length === 0) return null; @@ -230,7 +233,15 @@ export function listStoredTrails(projectRoot: string): StoredTrailList { skipped += 1; continue; } - const trail = readTrailFile(path.join(dir, name), id); + let trail: StoredTrail | null = null; + try { + // Use the same per-file chokepoint as direct reads. Checking only the + // directory lets `trails/x.json -> /outside/secret.json` escape it. + trail = readStoredTrail(projectRoot, id); + } catch { + // A linked or otherwise refused entry is an unreadable trail, not a reason + // to hide every valid trail beside it. + } if (trail) trails.push(trail); else skipped += 1; } @@ -241,14 +252,25 @@ export function listStoredTrails(projectRoot: string): StoredTrailList { } function readTrailFile(absolute: string, id: string): StoredTrail | null { + let fd: number | null = null; try { - const stat = fs.statSync(absolute); + // O_NOFOLLOW closes the race between path validation and the actual open: + // replacing a checked file with a symlink must fail rather than follow it. + // O_NONBLOCK keeps a FIFO with no writer from parking the whole server on + // this open; a regular file ignores it, and `isFile()` below rejects the rest. + fd = fs.openSync( + absolute, + fs.constants.O_RDONLY | (fs.constants.O_NOFOLLOW ?? 0) | (fs.constants.O_NONBLOCK ?? 0) + ); + const stat = fs.fstatSync(fd); // A file too big to be a trail is skipped rather than read: this directory // is inside the project, and something else may one day put a log in it. if (!stat.isFile() || stat.size > MAX_TRAIL_FILE_BYTES) return null; - return parseTrail(id, fs.readFileSync(absolute, 'utf-8')); + return parseTrail(id, fs.readFileSync(fd, 'utf-8')); } catch { return null; + } finally { + if (fd !== null) fs.closeSync(fd); } } @@ -263,9 +285,9 @@ export function readStoredTrail(projectRoot: string, id: string): StoredTrail | * Write a trail, atomically. * * Temp file beside the target then `rename`, so a reader either sees the - * previous trail or the new one and never a partial file. The temp name carries - * the pid: two `codegraph ui` processes on one project is unusual but not - * forbidden, and two writers sharing a temp name would corrupt each other's. + * previous trail or the new one and never a partial file. A random suffix plus + * exclusive creation means neither another viewer nor a pre-planted symlink can + * capture the temporary write. */ export function writeStoredTrail(projectRoot: string, trail: StoredTrail): void { const dir = trailsDirectory(projectRoot); @@ -274,10 +296,18 @@ export function writeStoredTrail(projectRoot: string, trail: StoredTrail): void } catch (err) { throw writeFailure(err); } + // The directory did not necessarily exist during the first check. Resolve it + // again now that mkdir completed, so an existing parent symlink cannot turn a + // not-yet-existing trail directory into an out-of-project write. + trailsDirectory(projectRoot); const target = trailPath(projectRoot, trail.id); - const temp = `${target}.${process.pid}.tmp`; + const temp = `${target}.${process.pid}.${randomUUID()}.tmp`; try { - fs.writeFileSync(temp, `${JSON.stringify(trail, null, 2)}\n`, 'utf-8'); + fs.writeFileSync(temp, `${JSON.stringify(trail, null, 2)}\n`, { + encoding: 'utf-8', + flag: 'wx', + mode: 0o600, + }); fs.renameSync(temp, target); } catch (err) { try { diff --git a/src/ui-server/api/trails.ts b/src/ui-server/api/trails.ts index f9f4d74f..51cfa98e 100644 --- a/src/ui-server/api/trails.ts +++ b/src/ui-server/api/trails.ts @@ -39,7 +39,9 @@ */ import { execFileSync } from 'child_process'; +import * as fs from 'fs'; import * as os from 'os'; +import * as path from 'path'; import type { CodeGraph } from '../../index'; import type { Node } from '../../types'; import { ApiError, badRequest, notFound } from './respond'; @@ -434,21 +436,32 @@ function parseSaveRequest(body: unknown): { name: string; note: string; hops: Sa * * Git's `user.name` first, because a trail is a thing one person wrote for * others to read and that is the name they already sign work with in this - * project; the OS user is the fallback. Read ONCE per process — `git config` is + * project; the OS user is the fallback. Read once per PROJECT — `git config` is * a subprocess, and a save should not pay for it twice — and never sent * anywhere: it goes into a file inside the user's own `.codegraph/`. */ -let cachedAuthor: string | null = null; +const cachedAuthors = new Map(); export function trailAuthor(projectRoot: string): string { - if (cachedAuthor !== null) return cachedAuthor; - cachedAuthor = gitUserName(projectRoot) ?? osUserName() ?? ''; - return cachedAuthor; + const key = authorCacheKey(projectRoot); + const cached = cachedAuthors.get(key); + if (cached !== undefined) return cached; + const author = gitUserName(projectRoot) ?? osUserName() ?? ''; + cachedAuthors.set(key, author); + return author; } /** Test seam: forget the cached author. */ export function resetTrailAuthor(): void { - cachedAuthor = null; + cachedAuthors.clear(); +} + +function authorCacheKey(projectRoot: string): string { + try { + return fs.realpathSync(projectRoot); + } catch { + return path.resolve(projectRoot); + } } function gitUserName(projectRoot: string): string | null { diff --git a/src/utils.ts b/src/utils.ts index ff8428cf..c07e8c7d 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -123,7 +123,8 @@ export function lexicalPathWithinRoot(projectRoot: string, filePath: string): st * @param options.allowSymlinkEscape - Follow in-root symlinks out of the root * (indexing read path only); defaults to the strict, leak-safe behavior. * @returns The resolved absolute path (realpath when it exists), or null if it - * escapes the root + * escapes the root. A path that does not exist yet is judged by its nearest + * existing ancestor, and every path under a root that does not exist is null. */ export function validatePathWithinRoot( projectRoot: string, @@ -143,23 +144,43 @@ export function validatePathWithinRoot( // The indexing read path (allowSymlinkEscape) skips only this rejection so // it stays consistent with the directory walk, which already followed the // in-root symlink to enumerate these files (#935). + let realRoot: string; + try { + realRoot = fs.realpathSync(normalizedRoot); + } catch { + return null; + } + try { - const realRoot = fs.realpathSync(normalizedRoot); const realResolved = fs.realpathSync(resolved); if (options?.allowSymlinkEscape) { return realResolved; } return isWithinDir(realResolved, realRoot) ? realResolved : null; } catch (err) { - // ENOENT: the path doesn't exist yet (a file about to be written, or an - // index entry for a since-deleted file) — no symlink to follow, and the - // lexical check already passed, so allow the lexical path. Any other - // resolution failure (ELOOP, EACCES, …) is treated as unsafe → reject. - if ((err as NodeJS.ErrnoException).code === 'ENOENT') { - return resolved; - } - return null; + // A missing leaf may still sit below an EXISTING symlink. Resolve the + // nearest existing ancestor before allowing a write path: otherwise + // `project/.codegraph/ui -> /outside` plus a missing `trails/` directory + // passes the lexical check and mkdir follows the symlink out of the root. + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') return null; } + + let ancestor = path.dirname(resolved); + while (isWithinDir(ancestor, normalizedRoot)) { + try { + const realAncestor = fs.realpathSync(ancestor); + if (options?.allowSymlinkEscape || isWithinDir(realAncestor, realRoot)) { + return resolved; + } + return null; + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') return null; + const parent = path.dirname(ancestor); + if (parent === ancestor) return null; + ancestor = parent; + } + } + return null; } /**