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 <noreply@anthropic.com>
This commit is contained in:
+10
-3
@@ -1,5 +1,5 @@
|
|||||||
{
|
{
|
||||||
"exported_at": "2026-05-05T12:59:14Z",
|
"exported_at": "2026-05-05T13:02:30Z",
|
||||||
"decisions": [
|
"decisions": [
|
||||||
{
|
{
|
||||||
"id": "D-1",
|
"id": "D-1",
|
||||||
@@ -2633,10 +2633,10 @@
|
|||||||
"id": "T-78",
|
"id": "T-78",
|
||||||
"type": "bug",
|
"type": "bug",
|
||||||
"title": "files.read path traversal — validate paths stay under workspace root",
|
"title": "files.read path traversal — validate paths stay under workspace root",
|
||||||
"status": "backlog",
|
"status": "in_progress",
|
||||||
"priority": "high",
|
"priority": "high",
|
||||||
"created_at": "2026-05-05 12:58:59",
|
"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",
|
"id": "T-79",
|
||||||
@@ -3926,6 +3926,13 @@
|
|||||||
"old_value": "in_progress",
|
"old_value": "in_progress",
|
||||||
"new_value": "done",
|
"new_value": "done",
|
||||||
"changed_at": "2026-05-05 12:59:05"
|
"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"
|
||||||
}
|
}
|
||||||
]
|
]
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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
|
is suppressed at the painter level since synthetic bold (with no
|
||||||
Bold.ttf registered) shifts glyph advance widths.
|
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
|
### Changed
|
||||||
|
|
||||||
- Inline terminal emulator based on xterm.dart v4.0.0 — replaces the
|
- Inline terminal emulator based on xterm.dart v4.0.0 — replaces the
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import 'dart:io';
|
|||||||
|
|
||||||
import '../files/ignore.dart';
|
import '../files/ignore.dart';
|
||||||
import '../files/listing.dart';
|
import '../files/listing.dart';
|
||||||
|
import '../files/path_safety.dart';
|
||||||
import '../files/watcher.dart';
|
import '../files/watcher.dart';
|
||||||
import '../ipc/envelope.dart';
|
import '../ipc/envelope.dart';
|
||||||
import '../ipc/schema_v1.dart';
|
import '../ipc/schema_v1.dart';
|
||||||
@@ -71,7 +72,13 @@ void registerFilesCommands(DaemonDispatcher d, FilesService files) {
|
|||||||
if (path == null || path.isEmpty) {
|
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'));
|
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()) {
|
if (!file.existsSync()) {
|
||||||
return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'file not found: $path'));
|
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 {
|
d.register('files.ls', (req) async {
|
||||||
final dir = (req.args['path'] as String?) ?? '';
|
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(
|
final entries = await listDir(
|
||||||
root: files.root,
|
root: files.root,
|
||||||
dir: dir,
|
dir: dir,
|
||||||
|
|||||||
@@ -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 = <String>[];
|
||||||
|
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)}';
|
||||||
|
}
|
||||||
@@ -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<PathOutsideRoot>()));
|
||||||
|
});
|
||||||
|
|
||||||
|
test('rejects traversal that lands at filesystem root', () {
|
||||||
|
expect(() => resolveUnderRoot(root, '../'),
|
||||||
|
throwsA(isA<PathOutsideRoot>()));
|
||||||
|
});
|
||||||
|
|
||||||
|
test('rejects sibling-directory traversal', () {
|
||||||
|
expect(() => resolveUnderRoot(root, '../sibling/file'),
|
||||||
|
throwsA(isA<PathOutsideRoot>()));
|
||||||
|
});
|
||||||
|
|
||||||
|
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<PathOutsideRoot>()));
|
||||||
|
} finally {
|
||||||
|
if (twin.existsSync()) twin.deleteSync(recursive: true);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user