From 8a0c64c22d5ebfbba4b4b2d8294ecfd072ac7e7a Mon Sep 17 00:00:00 2001 From: kkdev92 <112151103+kkdev92@users.noreply.github.com> Date: Mon, 28 Sep 2026 14:04:28 +0900 Subject: [PATCH] fix: follow links in the existing part of a path that does not exist yet validatePathInsideWorkspace resolved a path through realpath only when the path itself or its parent existed. When neither did - a save directory with folders still to be created - it compared the path as written, resolving nothing. Two things followed: - A link in the existing part of the path that leads out of the workspace went unseen. writeAtomic validates and then creates the folders with mkdir -p, so the folders and the image were created on the far side of the link. - In a workspace opened through a link (a symlinked home directory, macOS's /var, an 8.3 short name on Windows) the workspace side is a real path and the target side was not, so new nested folders were refused as outside. Resolve the nearest ancestor that exists and append the missing segments to it, at any depth. A relative path is now taken from the workspace root on every branch; before, the first attempt resolved it from the process's working directory. The only caller builds absolute paths. The extension runs only in trusted workspaces, so this is a check that did less than it claimed, not an exposure to an untrusted workspace. The new tests build a real tree with a link out of the workspace and a workspace reached through a link. Two of them fail on the previous code: new folders under the outward link were accepted, and new folders under the linked workspace were refused. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 ++ src/security/path-validator.ts | 55 ++++++---- .../security/path-validator-workspace.test.ts | 100 ++++++++++++++++++ 3 files changed, 146 insertions(+), 18 deletions(-) create mode 100644 test/security/path-validator-workspace.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 0264025..fca05aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- The check that keeps saved images inside the workspace now follows links in + the part of the path that already exists when the folders below it do not + exist yet. A save directory whose existing part was a link leading out of the + workspace, followed by folders still to be created, passed the check, so the + folders and the image were created on the far side of the link. The same gap + refused new nested folders in a workspace opened through a link. + ## [0.9.0] - 2026-09-18 **Breaking: VS Code 1.138 or later is now required**, up from 1.137, in step with diff --git a/src/security/path-validator.ts b/src/security/path-validator.ts index a039d20..ea23a20 100644 --- a/src/security/path-validator.ts +++ b/src/security/path-validator.ts @@ -28,24 +28,8 @@ export async function validatePathInsideWorkspace( // Normalize both paths const normalizedWorkspace = normalizePath(realWorkspaceRoot); - // Check if target path exists - let realTargetPath: string; - try { - // If target exists, resolve its real path - realTargetPath = await fs.realpath(targetPath); - } catch { - // Target doesn't exist yet - validate parent directory - const parentDir = path.dirname(targetPath); - try { - const realParentDir = await fs.realpath(parentDir); - realTargetPath = path.join(realParentDir, path.basename(targetPath)); - } catch { - // Parent doesn't exist - check the constructed path - // This is acceptable for new directories that will be created - const resolvedTarget = path.resolve(workspaceRoot, targetPath); - realTargetPath = resolvedTarget; - } - } + // Resolve the target through links as far as it exists + const realTargetPath = await resolveExistingPrefix(targetPath, workspaceRoot); const normalizedTarget = normalizePath(realTargetPath); @@ -72,6 +56,41 @@ export async function validatePathInsideWorkspace( } } +/** + * Resolve a path through links as far as it exists, then append the rest. + * + * `realpath` fails on a path that does not exist yet, so the nearest ancestor + * that does exist is resolved instead and the missing segments are appended to + * it. Resolving less than that — only the parent, or nothing once the parent is + * missing too — lets a link in the existing part lead out of the workspace + * unseen, while a workspace that is itself reached through a link stops looking + * like it contains its own new folders. + * + * @param targetPath - The path to resolve; a relative path is taken from the workspace root + * @param workspaceRoot - The workspace root path + * @returns The real path of the existing part, followed by the missing segments + */ +async function resolveExistingPrefix(targetPath: string, workspaceRoot: string): Promise { + const absolute = path.isAbsolute(targetPath) ? targetPath : path.join(workspaceRoot, targetPath); + const missing: string[] = []; + let current = absolute; + + for (;;) { + try { + const real = await fs.realpath(current); + return path.join(real, ...missing.reverse()); + } catch { + const parent = path.dirname(current); + if (parent === current) { + // Not even the root resolved: compare the path as given + return path.resolve(absolute); + } + missing.push(path.basename(current)); + current = parent; + } + } +} + /** * Check if a path contains parent directory traversal (..) * diff --git a/test/security/path-validator-workspace.test.ts b/test/security/path-validator-workspace.test.ts new file mode 100644 index 0000000..2d80595 --- /dev/null +++ b/test/security/path-validator-workspace.test.ts @@ -0,0 +1,100 @@ +import * as fs from 'fs/promises'; +import * as os from 'os'; +import * as path from 'path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { PathValidationError } from '../../src/core/errors'; +import { validatePathInsideWorkspace } from '../../src/security/path-validator'; + +// A real directory tree rather than mocks: what this function exists for — a +// link that leads out of the workspace — only shows up on a real file system. +// Directory links are junctions, which Windows creates without privileges; +// elsewhere the type argument is ignored. +describe('validatePathInsideWorkspace', () => { + let root: string; + let workspace: string; + let outside: string; + let linkOut: string; + let linkedWorkspace: string; + + beforeAll(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'clipshot-containment-')); + workspace = path.join(root, 'workspace'); + outside = path.join(root, 'outside'); + await fs.mkdir(path.join(workspace, 'images'), { recursive: true }); + await fs.mkdir(outside); + await fs.writeFile(path.join(workspace, 'images', 'inside.png'), ''); + await fs.writeFile(path.join(outside, 'secret.txt'), ''); + + linkOut = path.join(workspace, 'link-out'); + await fs.symlink(outside, linkOut, 'junction'); + + linkedWorkspace = path.join(root, 'workspace-link'); + await fs.symlink(workspace, linkedWorkspace, 'junction'); + }); + + afterAll(async () => { + await fs.rm(root, { recursive: true, force: true }); + }); + + describe('inside the workspace', () => { + it('accepts an existing file', async () => { + await expect( + validatePathInsideWorkspace(path.join(workspace, 'images', 'inside.png'), workspace) + ).resolves.toBe(true); + }); + + it('accepts a file that does not exist yet in an existing folder', async () => { + await expect( + validatePathInsideWorkspace(path.join(workspace, 'images', 'new.png'), workspace) + ).resolves.toBe(true); + }); + + it('accepts folders that do not exist yet, several levels deep', async () => { + await expect( + validatePathInsideWorkspace(path.join(workspace, 'new', 'deeper', 'image.png'), workspace) + ).resolves.toBe(true); + }); + + // A workspace opened through a link (a symlinked home directory, macOS's + // /var, an 8.3 short name on Windows) resolves to a different string than + // the path the caller builds from it. The check has to compare real paths + // on both sides, including for folders that do not exist yet. + it('accepts new folders under a workspace that is itself reached through a link', async () => { + await expect( + validatePathInsideWorkspace( + path.join(linkedWorkspace, 'new', 'deeper', 'image.png'), + linkedWorkspace + ) + ).resolves.toBe(true); + }); + }); + + describe('outside the workspace', () => { + it('rejects an absolute path outside', async () => { + await expect( + validatePathInsideWorkspace(path.join(outside, 'secret.txt'), workspace) + ).rejects.toBeInstanceOf(PathValidationError); + }); + + it('rejects an existing file reached through a link that leads outside', async () => { + await expect( + validatePathInsideWorkspace(path.join(linkOut, 'secret.txt'), workspace) + ).rejects.toBeInstanceOf(PathValidationError); + }); + + it('rejects a new file directly under a link that leads outside', async () => { + await expect( + validatePathInsideWorkspace(path.join(linkOut, 'new.png'), workspace) + ).rejects.toBeInstanceOf(PathValidationError); + }); + + // The case the folder-creating callers reach: `writeAtomic` and `ensureDir` + // validate first and then `mkdir -p`, so accepting this would create the + // folders — and write the image — on the far side of the link. + it('rejects new folders under a link that leads outside', async () => { + await expect( + validatePathInsideWorkspace(path.join(linkOut, 'new', 'deeper', 'image.png'), workspace) + ).rejects.toBeInstanceOf(PathValidationError); + }); + }); +});