reject symlinks pointing outside the workspace (T-102)
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) <noreply@anthropic.com>
This commit is contained in:
@@ -95,6 +95,9 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit.
|
|||||||
- `TreeSitterService` and `TreeSitterLib` accept injectable FFI + asset
|
- `TreeSitterService` and `TreeSitterLib` accept injectable FFI + asset
|
||||||
loaders for fake-driven tests; production paths unchanged.
|
loaders for fake-driven tests; production paths unchanged.
|
||||||
- Tidied test imports flagged by `unnecessary_import`.
|
- 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 —
|
- Terminal panes now render bold attributes with a real bold weight —
|
||||||
bundled JetBrainsMono Bold + BoldItalic are registered with the
|
bundled JetBrainsMono Bold + BoldItalic are registered with the
|
||||||
`JetBrainsMono` family at `weight: 700`. The painter's bold
|
`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
|
and clide would run it on auto-fired `git.status`. Dugite now
|
||||||
resolves against the install dir + `CLIDE_DUGITE_DIR` env override
|
resolves against the install dir + `CLIDE_DUGITE_DIR` env override
|
||||||
only (T-98).
|
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
|
- `files.read` and `files.ls` now reject paths that resolve outside
|
||||||
the workspace root. Previously a relative path containing `..`
|
the workspace root. Previously a relative path containing `..`
|
||||||
could read arbitrary files via path traversal.
|
could read arbitrary files via path traversal.
|
||||||
|
|||||||
@@ -74,7 +74,9 @@ void registerFilesCommands(DaemonDispatcher d, FilesService files) {
|
|||||||
}
|
}
|
||||||
final String absPath;
|
final String absPath;
|
||||||
try {
|
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 {
|
} on PathOutsideRoot {
|
||||||
return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'path outside workspace: $path'));
|
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?) ?? '';
|
final dir = (req.args['path'] as String?) ?? '';
|
||||||
if (dir.isNotEmpty) {
|
if (dir.isNotEmpty) {
|
||||||
try {
|
try {
|
||||||
resolveUnderRoot(files.root, dir);
|
resolveUnderRootFollowingSymlinks(files.root, dir);
|
||||||
} on PathOutsideRoot {
|
} on PathOutsideRoot {
|
||||||
return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'path outside workspace: $dir'));
|
return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'path outside workspace: $dir'));
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -18,6 +18,12 @@ class PathOutsideRoot implements Exception {
|
|||||||
/// Resolve [relative] against [root] and verify the result is
|
/// Resolve [relative] against [root] and verify the result is
|
||||||
/// contained within [root]. Returns the absolute, normalized path.
|
/// contained within [root]. Returns the absolute, normalized path.
|
||||||
/// Throws [PathOutsideRoot] on traversal attempts.
|
/// 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) {
|
String resolveUnderRoot(Directory root, String relative) {
|
||||||
final rootPath = _normalize(root.absolute.path);
|
final rootPath = _normalize(root.absolute.path);
|
||||||
final joined = _normalize('$rootPath${Platform.pathSeparator}$relative');
|
final joined = _normalize('$rootPath${Platform.pathSeparator}$relative');
|
||||||
@@ -32,6 +38,33 @@ String resolveUnderRoot(Directory root, String relative) {
|
|||||||
return joined;
|
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) {
|
String _normalize(String path) {
|
||||||
// Use Uri to collapse `..` and `.` segments without hitting the
|
// Use Uri to collapse `..` and `.` segments without hitting the
|
||||||
// filesystem (Directory(...).resolveSymbolicLinksSync would also
|
// filesystem (Directory(...).resolveSymbolicLinksSync would also
|
||||||
|
|||||||
@@ -116,6 +116,31 @@ void main() {
|
|||||||
expect(r.error!.message, contains('outside workspace'));
|
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 {
|
test('files.watch is idempotent: a second call still acks subscription', () async {
|
||||||
final r1 = await call('files.watch', const {});
|
final r1 = await call('files.watch', const {});
|
||||||
final r2 = await call('files.watch', const {});
|
final r2 = await call('files.watch', const {});
|
||||||
|
|||||||
@@ -64,4 +64,64 @@ void main() {
|
|||||||
expect(e.toString(), allOf(contains('r'), contains('/abs'), contains('/root')));
|
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<PathOutsideRoot>()),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
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<PathOutsideRoot>()),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user