From 2afee00cb08ada40ba885d609b442d90ec90432a Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Tue, 27 Oct 2026 18:18:41 +0000 Subject: [PATCH] fix(viewer): a file with % in its name opens as itself (#2933) codegraph ui couldn't show a file whose name contains a %: 100%.ts was refused as an unusable path, and a%20b.ts was served as changed on disk with no lines. resolveProjectFile, the chokepoint for every file the viewer reads, percent-decoded its path. But that path is either one the index stored or a query value the URL parser had already decoded once, so this was a second decode. It threw on 100%.ts and turned a%20b.ts into a b.ts, while resolveRequestedFile looked the file up in the index by the name as given: the bytes of a b.ts were hashed against a%20b.ts's record. The chokepoint no longer decodes. It refuses raw control bytes, and on Windows a backslash, then checks containment as before. Raw request paths (the viewer's own assets) are still decoded where they are read. The File view's /api/file/, whose path the HTTP layer also decodes once, had the same double decode and is fixed by the same change. The POSIX half of PR #2009 (backslashes kept in names on POSIX) is left out, as it needs a wider change and a POSIX run. Validation on Windows 11, with a fixture holding src/a%20b.ts, src/a b.ts and src/100%.ts: - codegraph ui over HTTP, /api/source with the query a browser sends: main serves a%20b.ts as drift=true with no lines and answers 403 for 100%.ts; the fix serves all three files as themselves, drift=false. - Tests: 2 new tests (the chokepoint, and /api/source plus /api/file/ over HTTP) fail on main's source and pass with the fix. Two existing cases that fed an encoded ..%2f and %00 straight to the chokepoint now go through the URL parser's single decode, as requests do, and are still refused. Reported in PR #2009 by @shrankr; this reimplements the viewer half of the fix independently. Co-Authored-By: Claude Opus 5.5 --- __tests__/ui-server-api.test.ts | 48 +++++++++++++++++++++++++++++++++ __tests__/ui-server.test.ts | 34 +++++++++++++++++++++-- docs/viewer-launch-changelog.md | 2 ++ src/ui-server/api/source.ts | 4 ++- src/ui-server/security.ts | 31 ++++++++++++++++----- 5 files changed, 110 insertions(+), 9 deletions(-) diff --git a/__tests__/ui-server-api.test.ts b/__tests__/ui-server-api.test.ts index a750e31e6..a47f9e258 100644 --- a/__tests__/ui-server-api.test.ts +++ b/__tests__/ui-server-api.test.ts @@ -1233,6 +1233,54 @@ describe('GET /api/nodes', () => { }); }); +// #2009: the source of `100%.ts` was refused, and `a%20b.ts` was read from +// `a b.ts` and so shown as changed on disk with no lines. +describe('GET /api/source and /api/file/ — file names with % in them', () => { + it('serves each file as itself', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-ui-percent-')); + const files: Record = { + 'src/a%20b.ts': 'export function percentNamed() { return "A"; }', + 'src/a b.ts': 'export function spaceNamed() { return "B"; }', + 'src/100%.ts': 'export function hundredPercent() { return "C"; }', + }; + fs.mkdirSync(path.join(root, 'src')); + for (const [name, text] of Object.entries(files)) fs.writeFileSync(path.join(root, name), `${text}\n`); + const indexed = await CodeGraph.init(root, { index: true }); + indexed.close(); + + const percentApi = createGraphApi({ projectRoot: root }); + const percentServer = await startUiServer({ + projectRoot: root, + viewerDir, + port: 0, + api: percentApi.handler, + }); + try { + for (const [name, text] of Object.entries(files)) { + // The query a browser sends for this name: `%` itself arrives encoded. + const res = await requestOn(percentServer.port, `/api/source?${new URLSearchParams({ file: name })}`); + expect(res.status, name).toBe(200); + const body = JSON.parse(res.body); + expect(body.file, name).toBe(name); + expect(body.drift, name).toBe(false); + expect(body.lines, name).toEqual([text]); + + // The File view carries the path in the URL path instead of the query. + const encodedPath = name.split('/').map(encodeURIComponent).join('/'); + const fileView = await requestOn(percentServer.port, `/api/file/${encodedPath}`); + expect(fileView.status, name).toBe(200); + const view = JSON.parse(fileView.body); + expect(view.file.path, name).toBe(name); + expect(view.drift, name).toBe(false); + } + } finally { + percentApi.close(); + await percentServer.close(); + fs.rmSync(root, { recursive: true, force: true }); + } + }); +}); + describe('an index that is not there', () => { it('answers with the same guidance the CLI prints, not a stack trace', async () => { const emptyRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-ui-noindex-')); diff --git a/__tests__/ui-server.test.ts b/__tests__/ui-server.test.ts index 928e945ff..fb50fe8ca 100644 --- a/__tests__/ui-server.test.ts +++ b/__tests__/ui-server.test.ts @@ -493,12 +493,41 @@ describe('resolveProjectFile — the source read chokepoint', () => { ); }); + /** A `file=` value as the API receives it: decoded once by the URL parser. */ + const fromQuery = (encoded: string): string => new URLSearchParams(`file=${encoded}`).get('file') ?? ''; + it('refuses traversal out of the project', () => { - for (const escape of ['../secret.txt', 'src/../../secret.txt', '..%2fsecret.txt']) { + for (const escape of [ + '../secret.txt', + 'src/../../secret.txt', + fromQuery('..%2fsecret.txt'), + fromQuery('src%2F..%2F..%2Fsecret.txt'), + ]) { expect(() => resolveProjectFile(projectRoot, escape), escape).toThrow(PathRefusalError); } }); + // #2009: the path is never decoded a second time, so a `%` in a file's name + // is part of the name. Decoding again refused `100%.ts` and read `a b.ts` + // in place of `a%20b.ts`. A literal `%2f` stays a name, not a separator. + it('reads a file whose name contains % as that file', () => { + const names = ['a%20b.ts', 'a b.ts', '100%.ts', '..%2fsecret.txt']; + for (const name of names) fs.writeFileSync(path.join(projectRoot, 'src', name), `// ${name}\n`); + try { + for (const name of names) { + expect(resolveProjectFile(projectRoot, `src/${name}`), name).toBe( + fs.realpathSync(path.join(projectRoot, 'src', name)) + ); + } + } finally { + for (const name of names) fs.rmSync(path.join(projectRoot, 'src', name), { force: true }); + } + }); + + it.runIf(process.platform === 'win32')('still refuses a backslash on Windows', () => { + expect(() => resolveProjectFile(projectRoot, 'src\\auth.ts')).toThrow(PathRefusalError); + }); + it('refuses an absolute path', () => { expect(() => resolveProjectFile(projectRoot, path.join(tempDir, 'secret.txt'))).toThrow( PathRefusalError @@ -511,7 +540,8 @@ describe('resolveProjectFile — the source read chokepoint', () => { }); it('refuses a NUL byte', () => { - expect(() => resolveProjectFile(projectRoot, 'src/auth.ts%00.png')).toThrow(PathRefusalError); + expect(() => resolveProjectFile(projectRoot, 'src/auth.ts\u0000.png')).toThrow(PathRefusalError); + expect(() => resolveProjectFile(projectRoot, fromQuery('src/auth.ts%00.png'))).toThrow(PathRefusalError); }); // `/etc` resolves to a non-existent `C:\etc` on Windows, so the sensitive-path diff --git a/docs/viewer-launch-changelog.md b/docs/viewer-launch-changelog.md index 4a123114f..e5f988183 100644 --- a/docs/viewer-launch-changelog.md +++ b/docs/viewer-launch-changelog.md @@ -219,6 +219,8 @@ These describe `codegraph ui` and its screens. They were taken out of `## [Unrel - **`codegraph ui` won't open a sensitive folder by another name.** A link or junction to a folder like `~/.ssh` or `~/.config`, a differently capitalized name on Windows and macOS, or a `\\?\` spelling on Windows got past the check that keeps such a folder from being opened, and the viewer then showed the files inside. They are now refused like the folder itself. Thanks @mixxer for finding it. +- **A file with `%` in its name opens as itself.** The viewer decoded a file's name a second time before reading it, so `100%.ts` was refused, and `a%20b.ts` was shown as changed on disk, with no lines, because `a b.ts` was read in its place. Every file now opens as the file it names. Thanks @shrankr. + - **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) diff --git a/src/ui-server/api/source.ts b/src/ui-server/api/source.ts index a81ce031e..3ff1ce697 100644 --- a/src/ui-server/api/source.ts +++ b/src/ui-server/api/source.ts @@ -114,7 +114,9 @@ export function resolveRequestedFile( requested: string ): { record: FileRecord; storedPath: string; absolute: string } { const posix = toRequestPath(requested); - // Refusals happen here, ahead of everything. + // Refusals happen here, ahead of everything. The read and the index lookup + // below use the same string, so a `%` in a file's name means the same file + // to both (#2009). const absolute = resolveProjectFile(projectRoot, posix); const found = findIndexedFile(cg, posix); diff --git a/src/ui-server/security.ts b/src/ui-server/security.ts index e99529e0c..fe144ef81 100644 --- a/src/ui-server/security.ts +++ b/src/ui-server/security.ts @@ -244,6 +244,9 @@ export function resolveStaticAsset(rootDir: string, urlPath: string): string | n * — treating it as a separator everywhere is the safe direction, and no built * asset name contains one). * + * For a RAW request path only. A source path handed to + * {@link resolveProjectFile} has already been decoded. + * * @returns the decoded path, or `null` if it is unusable. */ function decodePath(urlPath: string): string | null { @@ -258,6 +261,18 @@ function decodePath(urlPath: string): string | null { return decoded; } +/** + * Whether a project-relative source path is one we would read at all: no raw + * control byte (NUL truncates a path in some syscalls), and on Windows no + * backslash, a separator there that indexed paths never use. Nothing is + * decoded: a `%` in the path is part of a file name. + */ +function isUsableSourcePath(relativePath: string): boolean { + // eslint-disable-next-line no-control-regex -- rejecting raw control bytes IS the point + if (/[\x00-\x1f\x7f]/.test(relativePath)) return false; + return !(process.platform === 'win32' && relativePath.includes('\\')); +} + /** * Resolve a project-relative source path to an absolute path that is safe to * read and hand to the browser. @@ -268,6 +283,11 @@ function decodePath(urlPath: string): string | null { * what keeps `/api/source?path=../../.ssh/id_rsa` from being a credential leak * over a port the user opened to read their own code. * + * The path is taken as it is stored in the index, or as a query value the URL + * parser has already decoded once, and is never decoded again. Decoding it a + * second time refused `100%.ts` and read `a b.ts` for `a%20b.ts`, while the + * index lookup used the name as given (#2009). + * * @throws {PathRefusalError} when the root is a sensitive system directory, or * the path escapes the root by traversal or symlink. */ @@ -275,8 +295,7 @@ export function resolveProjectFile(projectRoot: string, relativePath: string): s if (typeof relativePath !== 'string' || relativePath.trim() === '') { throw new PathRefusalError('No file path was given.'); } - const decoded = decodePath(relativePath); - if (decoded === null) { + if (!isUsableSourcePath(relativePath)) { throw new PathRefusalError(`Refusing to read an unusable path: ${relativePath}`); } @@ -286,13 +305,13 @@ export function resolveProjectFile(projectRoot: string, relativePath: string): s const rootError = validateProjectPath(projectRoot); if (rootError) throw new PathRefusalError(rootError); - if (path.isAbsolute(decoded)) { - throw new PathRefusalError(`Refusing to read an absolute path: ${decoded}`); + if (path.isAbsolute(relativePath)) { + throw new PathRefusalError(`Refusing to read an absolute path: ${relativePath}`); } - const absolute = validatePathWithinRoot(projectRoot, decoded); + const absolute = validatePathWithinRoot(projectRoot, relativePath); if (!absolute) { - throw new PathRefusalError(`Refusing to read a path outside the project: ${decoded}`); + throw new PathRefusalError(`Refusing to read a path outside the project: ${relativePath}`); } return absolute; }