From 06b08b7388bd237ad88ce94d3efff64c42803ebe Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sun, 17 May 2026 21:01:56 +0200 Subject: [PATCH] reject symlinks pointing outside the workspace (T-102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveUnderRoot already blocked path-layer traversal but explicitly did NOT follow symlinks — a repo symlink config -> /etc/shadow passed the containment check because the link path was under root. clide would then read the target. Add resolveUnderRootFollowingSymlinks: resolves any symlinks at the target and re-verifies containment against the resolved real root. The split keeps pure path math testable without filesystem access. files.read and files.ls now route through it. Tests cover: plain non-symlink passthrough, non-existent target (returns path-layer result so caller surfaces not-found cleanly), single-hop and chained symlinks whose targets escape the workspace, and tolerance of symlinks in the root path itself (macOS /tmp). Also adds the T-101 CHANGELOG entry that the docs commit missed. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 6 +++ lib/src/daemon/files_commands.dart | 6 ++- lib/src/files/path_safety.dart | 33 +++++++++++++++ test/daemon/files_commands_test.dart | 25 ++++++++++++ test/files/path_safety_test.dart | 60 ++++++++++++++++++++++++++++ 5 files changed, 128 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6792a398..7981585a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -95,6 +95,9 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. - `TreeSitterService` and `TreeSitterLib` accept injectable FFI + asset loaders for fake-driven tests; production paths unchanged. - Tidied test imports flagged by `unnecessary_import`. +- `README.md` rewritten to match current architecture; `docs/initial-plan.md` + bannered as historical; new `docs/architecture.md` describes today's + shape (T-101). - Terminal panes now render bold attributes with a real bold weight — bundled JetBrainsMono Bold + BoldItalic are registered with the `JetBrainsMono` family at `weight: 700`. The painter's bold @@ -158,6 +161,9 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. and clide would run it on auto-fired `git.status`. Dugite now resolves against the install dir + `CLIDE_DUGITE_DIR` env override only (T-98). +- `files.read` and `files.ls` now reject symlinks whose targets live + outside the workspace — closes a path-safety bypass via in-repo + symlinks (T-102). - `files.read` and `files.ls` now reject paths that resolve outside the workspace root. Previously a relative path containing `..` could read arbitrary files via path traversal. diff --git a/lib/src/daemon/files_commands.dart b/lib/src/daemon/files_commands.dart index 76fffe3b..b5c9a805 100644 --- a/lib/src/daemon/files_commands.dart +++ b/lib/src/daemon/files_commands.dart @@ -74,7 +74,9 @@ void registerFilesCommands(DaemonDispatcher d, FilesService files) { } final String absPath; try { - absPath = resolveUnderRoot(files.root, path); + // Follow symlinks + re-check containment so a `config -> /etc/shadow` + // symlink under the workspace can't be read (T-102). + absPath = resolveUnderRootFollowingSymlinks(files.root, path); } on PathOutsideRoot { return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'path outside workspace: $path')); } @@ -90,7 +92,7 @@ void registerFilesCommands(DaemonDispatcher d, FilesService files) { final dir = (req.args['path'] as String?) ?? ''; if (dir.isNotEmpty) { try { - resolveUnderRoot(files.root, dir); + resolveUnderRootFollowingSymlinks(files.root, dir); } on PathOutsideRoot { return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'path outside workspace: $dir')); } diff --git a/lib/src/files/path_safety.dart b/lib/src/files/path_safety.dart index d3e4574b..b31dba46 100644 --- a/lib/src/files/path_safety.dart +++ b/lib/src/files/path_safety.dart @@ -18,6 +18,12 @@ class PathOutsideRoot implements Exception { /// Resolve [relative] against [root] and verify the result is /// contained within [root]. Returns the absolute, normalized path. /// Throws [PathOutsideRoot] on traversal attempts. +/// +/// Path-layer check only — does NOT follow symlinks. Callers that +/// read or list the filesystem should use [resolveUnderRootFollowingSymlinks] +/// instead, which adds a second containment check against the real +/// path. The two-step split exists so pure path math can be tested +/// without touching disk (T-102). String resolveUnderRoot(Directory root, String relative) { final rootPath = _normalize(root.absolute.path); final joined = _normalize('$rootPath${Platform.pathSeparator}$relative'); @@ -32,6 +38,33 @@ String resolveUnderRoot(Directory root, String relative) { return joined; } +/// Like [resolveUnderRoot] but also resolves any symlinks at the +/// target and re-verifies containment against the real path. Use this +/// for any operation that will read/list/write the filesystem — the +/// path-layer check alone does not defend against a symlink under the +/// workspace whose target lives outside (T-102, e.g. `config -> +/// /etc/shadow`). +/// +/// Returns the **resolved real path** (with symlinks followed) when +/// the target exists. When the target does not exist, returns the +/// path-layer result so callers surface a clean "not found" error from +/// their filesystem op (rather than this layer throwing first). +/// +/// Symlinks in the workspace root path itself are tolerated: both +/// sides of the containment check are resolved. +String resolveUnderRootFollowingSymlinks(Directory root, String relative) { + final pathResolved = resolveUnderRoot(root, relative); + if (FileSystemEntity.typeSync(pathResolved, followLinks: false) == FileSystemEntityType.notFound) { + return pathResolved; + } + final realRoot = Directory(root.absolute.path).resolveSymbolicLinksSync(); + final realPath = File(pathResolved).resolveSymbolicLinksSync(); + if (realPath != realRoot && !realPath.startsWith('$realRoot${Platform.pathSeparator}')) { + throw PathOutsideRoot(relative, realPath, realRoot); + } + return realPath; +} + String _normalize(String path) { // Use Uri to collapse `..` and `.` segments without hitting the // filesystem (Directory(...).resolveSymbolicLinksSync would also diff --git a/test/daemon/files_commands_test.dart b/test/daemon/files_commands_test.dart index 616b56fe..e3ea2ae5 100644 --- a/test/daemon/files_commands_test.dart +++ b/test/daemon/files_commands_test.dart @@ -116,6 +116,31 @@ void main() { expect(r.error!.message, contains('outside workspace')); }); + test('files.read rejects a symlink whose target is outside the workspace (T-102)', () async { + final outside = await Directory.systemTemp.createTemp('clide_t102_read_'); + addTearDown(() async { + if (await outside.exists()) await outside.delete(recursive: true); + }); + File('${outside.path}/secret.txt').writeAsStringSync('payload'); + Link('${sandbox.path}/leak').createSync('${outside.path}/secret.txt'); + + final r = await call('files.read', const {'path': 'leak'}); + expect(r.ok, isFalse); + expect(r.error!.message, contains('outside workspace')); + }); + + test('files.ls rejects a symlinked subdir whose target is outside (T-102)', () async { + final outside = await Directory.systemTemp.createTemp('clide_t102_ls_'); + addTearDown(() async { + if (await outside.exists()) await outside.delete(recursive: true); + }); + Link('${sandbox.path}/leak-dir').createSync(outside.path); + + final r = await call('files.ls', const {'path': 'leak-dir'}); + expect(r.ok, isFalse); + expect(r.error!.message, contains('outside workspace')); + }); + test('files.watch is idempotent: a second call still acks subscription', () async { final r1 = await call('files.watch', const {}); final r2 = await call('files.watch', const {}); diff --git a/test/files/path_safety_test.dart b/test/files/path_safety_test.dart index b4a7ade2..8e477c5b 100644 --- a/test/files/path_safety_test.dart +++ b/test/files/path_safety_test.dart @@ -64,4 +64,64 @@ void main() { expect(e.toString(), allOf(contains('r'), contains('/abs'), contains('/root'))); }); }); + + group('resolveUnderRootFollowingSymlinks (T-102)', () { + test('plain non-symlink file passes through with the resolved real path', () { + final f = File('${root.path}/plain.txt')..writeAsStringSync('hello'); + final out = resolveUnderRootFollowingSymlinks(root, 'plain.txt'); + // Real-path may differ from root.path on hosts where systemTemp + // is itself a symlink (macOS /tmp -> /private/tmp). Compare via + // resolveSymbolicLinksSync on both sides. + expect(out, f.resolveSymbolicLinksSync()); + }); + + test('non-existent target returns the path-layer result (caller surfaces not-found)', () { + final out = resolveUnderRootFollowingSymlinks(root, 'never-existed.txt'); + expect(out, endsWith('/never-existed.txt')); + }); + + test('rejects a symlink under the workspace whose target lives outside', () async { + // Create an outside file the symlink will point at. + final outside = await Directory.systemTemp.createTemp('clide_t102_outside_'); + addTearDown(() async { + if (await outside.exists()) await outside.delete(recursive: true); + }); + final secret = File('${outside.path}/secret.txt')..writeAsStringSync('payload'); + + // Plant a symlink inside the workspace that targets the outside file. + final link = Link('${root.path}/leak')..createSync(secret.path); + expect(link.existsSync(), isTrue); + + expect( + () => resolveUnderRootFollowingSymlinks(root, 'leak'), + throwsA(isA()), + ); + }); + + test('tolerates symlinks in the workspace root path itself', () { + // Where systemTemp is itself a symlink (macOS), the realPath of a + // file under root won't startWith root.absolute.path — but + // resolveUnderRootFollowingSymlinks resolves the root too, so + // the containment check still passes. + File('${root.path}/under-root.txt').writeAsStringSync('ok'); + // No throw is the assertion. + resolveUnderRootFollowingSymlinks(root, 'under-root.txt'); + }); + + test('rejects a symlink-to-symlink chain whose final target is outside', () async { + final outside = await Directory.systemTemp.createTemp('clide_t102_chain_'); + addTearDown(() async { + if (await outside.exists()) await outside.delete(recursive: true); + }); + final secret = File('${outside.path}/secret.txt')..writeAsStringSync('payload'); + // a -> b (under root) -> /outside/secret.txt + Link('${root.path}/b').createSync(secret.path); + Link('${root.path}/a').createSync('${root.path}/b'); + + expect( + () => resolveUnderRootFollowingSymlinks(root, 'a'), + throwsA(isA()), + ); + }); + }); }