From 816e60d02837f3c9b248223de2b2b948e3b79e73 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Tue, 5 May 2026 15:02:30 +0200 Subject: [PATCH] reject path traversal in files.read and files.ls (T-78) Both handlers concatenated the request path onto the workspace root without validating containment, letting `path: "../../../etc/passwd"` escape the workspace. resolveUnderRoot normalizes the path and checks containment under root.absolute.path before any filesystem access. Co-Authored-By: Claude --- .pql/pql-plan.json | 13 ++++-- CHANGELOG.md | 6 +++ lib/src/daemon/files_commands.dart | 16 +++++++- lib/src/files/path_safety.dart | 51 +++++++++++++++++++++++ test/files/path_safety_test.dart | 66 ++++++++++++++++++++++++++++++ 5 files changed, 148 insertions(+), 4 deletions(-) create mode 100644 lib/src/files/path_safety.dart create mode 100644 test/files/path_safety_test.dart diff --git a/.pql/pql-plan.json b/.pql/pql-plan.json index 3c62785e..99a122b9 100644 --- a/.pql/pql-plan.json +++ b/.pql/pql-plan.json @@ -1,5 +1,5 @@ { - "exported_at": "2026-05-05T12:59:14Z", + "exported_at": "2026-05-05T13:02:30Z", "decisions": [ { "id": "D-1", @@ -2633,10 +2633,10 @@ "id": "T-78", "type": "bug", "title": "files.read path traversal — validate paths stay under workspace root", - "status": "backlog", + "status": "in_progress", "priority": "high", "created_at": "2026-05-05 12:58:59", - "updated_at": "2026-05-05 12:58:59" + "updated_at": "2026-05-05 12:59:50" }, { "id": "T-79", @@ -3926,6 +3926,13 @@ "old_value": "in_progress", "new_value": "done", "changed_at": "2026-05-05 12:59:05" + }, + { + "ticket_id": "T-78", + "field": "status", + "old_value": "backlog", + "new_value": "in_progress", + "changed_at": "2026-05-05 12:59:50" } ] } diff --git a/CHANGELOG.md b/CHANGELOG.md index ac9c7e73..1435a5e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,12 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. is suppressed at the painter level since synthetic bold (with no Bold.ttf registered) shifts glyph advance widths. +### Security + +- `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. + ### Changed - Inline terminal emulator based on xterm.dart v4.0.0 — replaces the diff --git a/lib/src/daemon/files_commands.dart b/lib/src/daemon/files_commands.dart index 9b57d4d4..76fffe3b 100644 --- a/lib/src/daemon/files_commands.dart +++ b/lib/src/daemon/files_commands.dart @@ -6,6 +6,7 @@ import 'dart:io'; import '../files/ignore.dart'; import '../files/listing.dart'; +import '../files/path_safety.dart'; import '../files/watcher.dart'; import '../ipc/envelope.dart'; import '../ipc/schema_v1.dart'; @@ -71,7 +72,13 @@ void registerFilesCommands(DaemonDispatcher d, FilesService files) { if (path == null || path.isEmpty) { return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'files.read requires a path')); } - final file = File('${files.root.absolute.path}/$path'); + final String absPath; + try { + absPath = resolveUnderRoot(files.root, path); + } on PathOutsideRoot { + return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'path outside workspace: $path')); + } + final file = File(absPath); if (!file.existsSync()) { return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'file not found: $path')); } @@ -81,6 +88,13 @@ void registerFilesCommands(DaemonDispatcher d, FilesService files) { d.register('files.ls', (req) async { final dir = (req.args['path'] as String?) ?? ''; + if (dir.isNotEmpty) { + try { + resolveUnderRoot(files.root, dir); + } on PathOutsideRoot { + return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'path outside workspace: $dir')); + } + } final entries = await listDir( root: files.root, dir: dir, diff --git a/lib/src/files/path_safety.dart b/lib/src/files/path_safety.dart new file mode 100644 index 00000000..d3e4574b --- /dev/null +++ b/lib/src/files/path_safety.dart @@ -0,0 +1,51 @@ +/// Workspace-relative path validation. Rejects paths that resolve +/// outside the workspace root (path traversal via `..`, absolute +/// paths, symlink-out attempts). +library; + +import 'dart:io'; + +class PathOutsideRoot implements Exception { + PathOutsideRoot(this.requested, this.resolved, this.root); + final String requested; + final String resolved; + final String root; + + @override + String toString() => 'path outside workspace root: $requested → $resolved (root: $root)'; +} + +/// Resolve [relative] against [root] and verify the result is +/// contained within [root]. Returns the absolute, normalized path. +/// Throws [PathOutsideRoot] on traversal attempts. +String resolveUnderRoot(Directory root, String relative) { + final rootPath = _normalize(root.absolute.path); + final joined = _normalize('$rootPath${Platform.pathSeparator}$relative'); + + // Containment check: joined must equal rootPath, or start with + // rootPath + separator. Equality covers `relative == ''` (the + // root itself); the separator check prevents `/repo` matching + // `/repository`. + if (joined != rootPath && !joined.startsWith('$rootPath${Platform.pathSeparator}')) { + throw PathOutsideRoot(relative, joined, rootPath); + } + return joined; +} + +String _normalize(String path) { + // Use Uri to collapse `..` and `.` segments without hitting the + // filesystem (Directory(...).resolveSymbolicLinksSync would also + // resolve symlinks, which we don't want here — symlink handling + // belongs at the filesystem-access layer, not the path layer). + final segments = []; + for (final raw in path.split(Platform.pathSeparator)) { + if (raw.isEmpty || raw == '.') continue; + if (raw == '..') { + if (segments.isNotEmpty) segments.removeLast(); + continue; + } + segments.add(raw); + } + final prefix = path.startsWith(Platform.pathSeparator) ? Platform.pathSeparator : ''; + return '$prefix${segments.join(Platform.pathSeparator)}'; +} diff --git a/test/files/path_safety_test.dart b/test/files/path_safety_test.dart new file mode 100644 index 00000000..7168a789 --- /dev/null +++ b/test/files/path_safety_test.dart @@ -0,0 +1,66 @@ +import 'dart:io'; + +import 'package:clide/src/files/path_safety.dart'; +import 'package:test/test.dart'; + +void main() { + late Directory root; + + setUp(() { + root = Directory.systemTemp.createTempSync('clide_path_safety_'); + }); + + tearDown(() { + if (root.existsSync()) root.deleteSync(recursive: true); + }); + + group('resolveUnderRoot', () { + test('plain relative path resolves under root', () { + final out = resolveUnderRoot(root, 'file.txt'); + expect(out, '${root.absolute.path}/file.txt'); + }); + + test('nested relative path resolves under root', () { + final out = resolveUnderRoot(root, 'src/main.dart'); + expect(out, '${root.absolute.path}/src/main.dart'); + }); + + test('empty relative path resolves to root itself', () { + final out = resolveUnderRoot(root, ''); + expect(out, root.absolute.path); + }); + + test('rejects ../etc/passwd traversal', () { + expect(() => resolveUnderRoot(root, '../../../etc/passwd'), + throwsA(isA())); + }); + + test('rejects traversal that lands at filesystem root', () { + expect(() => resolveUnderRoot(root, '../'), + throwsA(isA())); + }); + + test('rejects sibling-directory traversal', () { + expect(() => resolveUnderRoot(root, '../sibling/file'), + throwsA(isA())); + }); + + test('allows internal `..` that stays under root', () { + final out = resolveUnderRoot(root, 'a/b/../c'); + expect(out, '${root.absolute.path}/a/c'); + }); + + test('rejects path that prefix-matches root but is outside', () { + // Sibling dir whose name starts with the root's last segment. + // resolveUnderRoot must not be fooled by string-prefix matching. + final twin = Directory('${root.parent.path}/${root.uri.pathSegments.where((s) => s.isNotEmpty).last}_twin'); + try { + twin.createSync(); + expect(() => resolveUnderRoot(root, '../${twin.uri.pathSegments.where((s) => s.isNotEmpty).last}/file'), + throwsA(isA())); + } finally { + if (twin.existsSync()) twin.deleteSync(recursive: true); + } + }); + }); +}