From ba6ab51118d02ca1af99373edbaf8291b0719d27 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Fri, 12 Jun 2026 00:39:23 +0200 Subject: [PATCH] confine editor.open/editor.save to the workspace (T-363) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The editor registry resolved buffer paths with a string join that passed absolute paths through verbatim and never normalized `..` — an unconfined read and write primitive over IPC while files.read was carefully guarded. Buffer paths now resolve through resolveUnderRootFollowingSymlinks: traversal, absolute escapes, and symlinks-out are rejected at open, and re-checked at save so a symlink swapped in under an open buffer's path can't redirect the write. D-80's extra read roots deliberately do not apply — a buffer is a write surface. Handlers map PathOutsideRoot to the same error files.read uses. Also merges a duplicate Added heading that had crept into the Unreleased changelog section. Co-Authored-By: Claude Fable 5 --- .pql/changelog/ticket_history/2026-06.sql | 2 ++ .pql/changelog/tickets/2026-06.sql | 6 ++++ CHANGELOG.md | 17 +++++---- lib/src/daemon/editor_commands.dart | 20 ++++++++++- lib/src/editor/registry.dart | 8 +++-- test/editor/registry_test.dart | 43 +++++++++++++++++++++++ 6 files changed, 87 insertions(+), 9 deletions(-) diff --git a/.pql/changelog/ticket_history/2026-06.sql b/.pql/changelog/ticket_history/2026-06.sql index 72895458..a7c2f8e8 100644 --- a/.pql/changelog/ticket_history/2026-06.sql +++ b/.pql/changelog/ticket_history/2026-06.sql @@ -3949,3 +3949,5 @@ INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, chang INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBSG6356MZJ2DCCCSBMBGM', 'status', 'in_progress', 'done', NULL, '2026-06-11 22:32:08', '2026-06-11 22:32:08', '2026-06-11 22:32:08', NULL, 'f3dbb31fb6f6f416a2d4d8ef37723316', 2) ON CONFLICT(hash) DO NOTHING; INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBR4636GSRJBWFJDAZ6ZA0', 'status', 'backlog', 'in_progress', NULL, '2026-06-11 22:32:29', '2026-06-11 22:32:29', '2026-06-11 22:32:29', NULL, '7b833a606da59ab523e0bb43f1753f6a', 2) ON CONFLICT(hash) DO NOTHING; INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBR4636GSRJBWFJDAZ6ZA0', 'status', 'in_progress', 'done', NULL, '2026-06-11 22:34:31', '2026-06-11 22:34:31', '2026-06-11 22:34:31', NULL, '254ae09f42b4da439a04d59d675d8f42', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBPQE4J4YBJX92812ZK6DR', 'status', 'backlog', 'in_progress', NULL, '2026-06-11 22:34:51', '2026-06-11 22:34:51', '2026-06-11 22:34:51', NULL, '14a7d3d6d6c71cae423ce12c71b6c540', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBPQE4J4YBJX92812ZK6DR', 'status', 'in_progress', 'done', NULL, '2026-06-11 22:39:04', '2026-06-11 22:39:04', '2026-06-11 22:39:04', NULL, 'ad1c483fe26d5e417b2e1cd8986114f3', 2) ON CONFLICT(hash) DO NOTHING; diff --git a/.pql/changelog/tickets/2026-06.sql b/.pql/changelog/tickets/2026-06.sql index b350f497..e8ec6dc5 100644 --- a/.pql/changelog/tickets/2026-06.sql +++ b/.pql/changelog/tickets/2026-06.sql @@ -4013,3 +4013,9 @@ Fix: apply the same glob filtering the search side uses before rewriting. Test: INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBR4636GSRJBWFJDAZ6ZA0', 'bug', '06FBHBGHNEQTAEPGNJKN42C1E8', 'search.replace silently ignores its include/exclude globs', 'lib/src/search/replace_engine.dart:124-143 — the include/exclude glob filters the user typed are accepted but never applied; replace will happily rewrite files outside the filter. Fix: apply the same glob filtering the search side uses before rewriting. Test: replace with an include glob touches only matching files; exclude glob is honored.', 'done', 'high', NULL, NULL, NULL, '2026-06-11 21:55:38', '2026-06-11 22:34:31', NULL, 'e86c024d6f8a6d9cd9b7273522b54511', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBPQE4J4YBJX92812ZK6DR', 'bug', '06FBHBGHNEQTAEPGNJKN42C1E8', 'editor.open/editor.save skip path confinement entirely', 'lib/src/editor/registry.dart:215-219 returns absolute paths verbatim — no .. normalization, no path_safety call — an unconfined read AND write primitive over IPC while files.read is carefully guarded. + +Fix: route editor.open/editor.save through path_safety like files.*. Tests: traversal and absolute-escape attempts rejected for both verbs. Longer-term the confinement should move to the dispatch layer (see the systemic ticket filed with this epic).', 'in_progress', 'high', NULL, NULL, NULL, '2026-06-11 21:55:26', '2026-06-11 22:34:51', NULL, 'ff2c4e85e11c02c88eff6f8e87348d34', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBPQE4J4YBJX92812ZK6DR', 'bug', '06FBHBGHNEQTAEPGNJKN42C1E8', 'editor.open/editor.save skip path confinement entirely', 'lib/src/editor/registry.dart:215-219 returns absolute paths verbatim — no .. normalization, no path_safety call — an unconfined read AND write primitive over IPC while files.read is carefully guarded. + +Fix: route editor.open/editor.save through path_safety like files.*. Tests: traversal and absolute-escape attempts rejected for both verbs. Longer-term the confinement should move to the dispatch layer (see the systemic ticket filed with this epic).', 'done', 'high', NULL, NULL, NULL, '2026-06-11 21:55:26', '2026-06-11 22:39:04', NULL, '50c42cb7af7715e9ea6417212be0ebdc', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); diff --git a/CHANGELOG.md b/CHANGELOG.md index 11e69869..64616644 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,12 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. below the result — connected only while the card is expanded. Commands with no followable file show a muted "nothing to follow" note. (T-325) +- **Double-tap-modifier shortcuts (e.g. double-Shift "Search Everywhere").** + The keymap can now bind a bare modifier and a double-tap sequence + (`shift shift`). All four presets (default, vim, vscode, jetbrains) map + double-Shift to the quick-open finder — JetBrains' "Search Everywhere" + gesture, aliased to clide's existing fuzzy file finder. (T-341) + ### Changed - **Each spawned subagent gets its own collapsing activity card.** A fan-out of @@ -68,13 +74,12 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. so the mode indicator and composer stayed stuck on "plan". The approval now syncs the tracked mode to `default`. (T-337) -### Added +### Security -- **Double-tap-modifier shortcuts (e.g. double-Shift "Search Everywhere").** - The keymap can now bind a bare modifier and a double-tap sequence - (`shift shift`). All four presets (default, vim, vscode, jetbrains) map - double-Shift to the quick-open finder — JetBrains' "Search Everywhere" - gesture, aliased to clide's existing fuzzy file finder. (T-341) +- **`editor.open` / `editor.save` are now workspace-confined.** Both verbs + accepted absolute paths and `..` traversal verbatim — an unconfined read + and write primitive over IPC. They now pass the same path-safety guard as + `files.read`, including a symlink re-check at save time. (T-363) ## [2.3.3] — 2026-06-11 diff --git a/lib/src/daemon/editor_commands.dart b/lib/src/daemon/editor_commands.dart index f107f38c..202c211f 100644 --- a/lib/src/daemon/editor_commands.dart +++ b/lib/src/daemon/editor_commands.dart @@ -13,6 +13,7 @@ import 'dart:io' show FileSystemException; import '../editor/buffer.dart' show Selection; import '../editor/registry.dart'; +import '../files/path_safety.dart' show PathOutsideRoot; import '../ipc/command_schema.dart'; import '../ipc/envelope.dart'; import '../ipc/errno_mapping.dart'; @@ -84,6 +85,13 @@ Future _open(IpcRequest req, EditorRegistry r) async { r.setSelection(buf.id, Selection.collapsed(_offsetForLine(buf.content, line))); } return IpcResponse.ok(id: req.id, data: buf.toJson()); + } on PathOutsideRoot { + // Same containment contract as files.read (T-363); a buffer is a + // write surface, so no D-80 extra-root widening here. + return IpcResponse.err( + id: req.id, + error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'path outside workspace: $path'), + ); } on FileSystemException catch (e) { final errno = e.osError?.errorCode; if (errno != null) { @@ -190,7 +198,17 @@ Future _setContent(IpcRequest req, EditorRegistry r) async { Future _save(IpcRequest req, EditorRegistry r) async { final id = _resolveId(req, r); if (id == null) return _notFound(req.id, 'no active buffer'); - final ok = await r.save(id); + final bool ok; + try { + ok = await r.save(id); + } on PathOutsideRoot { + // Defense in depth — open already validates, but a symlink can be + // swapped in under the buffer's path between open and save (T-363). + return IpcResponse.err( + id: req.id, + error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'path outside workspace'), + ); + } if (!ok) return _notFound(req.id, 'no such buffer: $id'); return IpcResponse.ok(id: req.id, data: {'id': id, 'saved': true}); } diff --git a/lib/src/editor/registry.dart b/lib/src/editor/registry.dart index d7929e23..5eafcd2f 100644 --- a/lib/src/editor/registry.dart +++ b/lib/src/editor/registry.dart @@ -9,6 +9,7 @@ library; import 'dart:convert'; import 'dart:io'; +import '../files/path_safety.dart'; import '../ipc/envelope.dart'; import '../panes/event_sink.dart'; import 'buffer.dart'; @@ -212,10 +213,13 @@ class EditorRegistry { events.emit(IpcEvent(subsystem: 'editor', kind: kind, timestamp: DateTime.now().toUtc(), data: data)); } + /// Resolve a buffer path to disk under the workspace root, with the + /// same traversal/symlink containment as files.* (T-363). A buffer is + /// a WRITE surface (save), so the D-80 extra read roots do not apply — + /// strictly workspace-confined. Throws [PathOutsideRoot] on escape. String _absolutePathOf(String repoRelative) { - if (repoRelative.startsWith('/')) return repoRelative; final sep = Platform.pathSeparator; - return '${workspaceRoot.absolute.path}$sep${repoRelative.replaceAll('/', sep)}'; + return resolveUnderRootFollowingSymlinks(workspaceRoot, repoRelative.replaceAll('/', sep)); } // Support JSON decode of Selection from IPC args. diff --git a/test/editor/registry_test.dart b/test/editor/registry_test.dart index e52c1810..e41b8a5f 100644 --- a/test/editor/registry_test.dart +++ b/test/editor/registry_test.dart @@ -2,6 +2,7 @@ import 'dart:io'; import 'package:clide/clide.dart'; import 'package:clide/src/editor/registry.dart'; +import 'package:clide/src/files/path_safety.dart' show PathOutsideRoot; import 'package:test/test.dart'; void main() { @@ -212,4 +213,46 @@ void main() { expect(changed.map((e) => e.data['id']), contains(readme.id)); }); }); + + // T-363: editor.open/save returned absolute paths verbatim and did no + // `..` normalization — an unconfined read AND write primitive over IPC + // while files.read was carefully guarded. + group('path confinement (T-363)', () { + test('open rejects .. traversal out of the workspace', () async { + final outside = await Directory.systemTemp.createTemp('clide-editor-outside-'); + addTearDown(() => outside.deleteSync(recursive: true)); + await File('${outside.path}/secret.txt').writeAsString('secret'); + final escape = '../${outside.path.split('/').last}/secret.txt'; + await expectLater(reg.open(escape), throwsA(isA())); + }); + + test('open rejects absolute paths outside the workspace', () async { + await expectLater(reg.open('/etc/hostname'), throwsA(isA())); + }); + + test('open accepts an absolute path inside the workspace', () async { + final buf = await reg.open('${sandbox.path}/README.md'); + expect(buf.content, contains('Hello')); + }); + + test('open rejects a symlink pointing outside the workspace', () async { + final outside = await Directory.systemTemp.createTemp('clide-editor-outside-'); + addTearDown(() => outside.deleteSync(recursive: true)); + await File('${outside.path}/secret.txt').writeAsString('secret'); + Link('${sandbox.path}/sneaky').createSync('${outside.path}/secret.txt'); + await expectLater(reg.open('sneaky'), throwsA(isA())); + }); + + test('save rejects a buffer whose path now symlinks outside', () async { + // Open a legitimate file, then swap a symlink in under its path. + final buf = await reg.open('victim.txt'); + reg.setContent(buf.id, 'attacker-controlled'); + final outside = await Directory.systemTemp.createTemp('clide-editor-outside-'); + addTearDown(() => outside.deleteSync(recursive: true)); + await File('${outside.path}/target.txt').writeAsString('original'); + Link('${sandbox.path}/victim.txt').createSync('${outside.path}/target.txt'); + await expectLater(reg.save(buf.id), throwsA(isA())); + expect(await File('${outside.path}/target.txt').readAsString(), 'original'); + }); + }); }