diff --git a/lib/builtin/tools_settings/src/path_preset_control.dart b/lib/builtin/tools_settings/src/path_preset_control.dart index 994650e8..c9ff4a81 100644 --- a/lib/builtin/tools_settings/src/path_preset_control.dart +++ b/lib/builtin/tools_settings/src/path_preset_control.dart @@ -77,6 +77,10 @@ class _PathPresetControlState extends State { d = d == '~' ? home : '$home${d.substring(1)}'; } if (d.isEmpty || !(d.startsWith('/') || RegExp(r'^[A-Za-z]:[/\\]').hasMatch(d))) return null; + // One dir per entry — an embedded PATH separator would smuggle extra + // (possibly empty → CWD) tokens into the joined PATH. + final body = RegExp(r'^[A-Za-z]:').hasMatch(d) ? d.substring(2) : d; + if (body.contains(':') || body.contains(';')) return null; while (d.length > 1 && d.endsWith('/')) { d = d.substring(0, d.length - 1); } @@ -293,7 +297,7 @@ class _PathPresetControlState extends State { onTap: () => _add(services, cwd), builder: (ctx, hovered, _) => Container( padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 5), - decoration: BoxDecoration(color: hovered ? tokens.listItemHoverBackground : tokens.buttonBackground, borderRadius: BorderRadius.circular(4)), + decoration: BoxDecoration(color: hovered ? tokens.buttonHoverBackground : tokens.buttonBackground, borderRadius: BorderRadius.circular(4)), child: ClideText(addLabel, color: tokens.buttonForeground, fontSize: clideFontCaption), ), ), diff --git a/lib/main.dart b/lib/main.dart index f6ea412b..2d9b5125 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -63,7 +63,7 @@ import 'package:clide/src/daemon/search_commands.dart'; import 'package:clide/src/editor/registry.dart' show EditorRegistry; import 'package:clide/src/git/client.dart'; import 'package:clide/src/cli/argv_dispatch.dart'; -import 'package:clide/src/env/path_preset.dart' show applyPathPreset, pathPresetKey, presetDirsFrom; +import 'package:clide/src/env/path_preset.dart' show applyPathPreset, pathPresetKey, presetDirsFrom, presetLookupRoot; import 'package:clide/src/env/shell_env.dart' show primeLoginShellPath, resolvedToolPath; import 'package:clide/src/env/supporter_binaries.dart'; import 'package:clide/widgets/src/icons/phosphor_glyphs.g.dart' show kPhosphorGlyphs; @@ -322,7 +322,7 @@ Future main() async { final settings = kernelSettings; final base = resolvedToolPath(); if (settings == null) return base; - return applyPathPreset(base, presetDirsFrom((k) => settings.get(k), cwd ?? workRoot.path)); + return applyPathPreset(base, presetDirsFrom((k) => settings.get(k), presetLookupRoot(cwd, workRoot.path))); }, ); // D-6 parity (T-219, D-83): make the tabs the user sees in the GUI diff --git a/lib/src/daemon/env_path_commands.dart b/lib/src/daemon/env_path_commands.dart index 6e5ba1b2..e7cc732a 100644 --- a/lib/src/daemon/env_path_commands.dart +++ b/lib/src/daemon/env_path_commands.dart @@ -174,6 +174,11 @@ Future _dispatch( } final absolute = d.startsWith('/') || RegExp(r'^[A-Za-z]:[/\\]').hasMatch(d); if (!absolute) return (out, 'not an absolute path: $raw'); + // One dir per entry: an embedded PATH separator would expand into extra + // tokens at join time — and a stray trailing ':' yields an EMPTY token, + // which POSIX shells resolve as CWD (the classic dot-in-PATH hazard). + final body = RegExp(r'^[A-Za-z]:').hasMatch(d) ? d.substring(2) : d; + if (body.contains(':') || body.contains(';')) return (out, 'entry contains a PATH separator: $raw'); while (d.length > 1 && d.endsWith('/')) { d = d.substring(0, d.length - 1); } diff --git a/lib/src/env/path_preset.dart b/lib/src/env/path_preset.dart index c0c33499..f0d6818e 100644 --- a/lib/src/env/path_preset.dart +++ b/lib/src/env/path_preset.dart @@ -29,8 +29,8 @@ const pathPresetKeyPrefix = 'app.env.pathPrepend.'; /// The settings key holding [workspaceRoot]'s preset — app layer, suffixed /// with the FNV-1a hash (the same one D-70 derives for the socket path) of the /// [presetRootFor]-resolved repo root, so all worktrees of a repo share one key. -String pathPresetKey(String workspaceRoot, {bool Function(String path)? isFile, String? Function(String path)? readFile}) => - '$pathPresetKeyPrefix${fnv1a64Hex(canonicalWorkspaceKey(presetRootFor(workspaceRoot, isFile: isFile, readFile: readFile)))}'; +String pathPresetKey(String workspaceRoot, {bool Function(String path)? isFile, String? Function(String path)? readFile, bool Function(String path)? isDir}) => + '$pathPresetKeyPrefix${fnv1a64Hex(canonicalWorkspaceKey(presetRootFor(workspaceRoot, isFile: isFile, readFile: readFile, isDir: isDir)))}'; /// The directory whose identity keys the preset: the MAIN repo root when /// [workspaceRoot] is a linked git worktree, else [workspaceRoot] itself @@ -42,11 +42,20 @@ String pathPresetKey(String workspaceRoot, {bool Function(String path)? isFile, /// that shape — a normal repo (`.git` directory), no `.git` at all, a /// submodule pointer — keys off [workspaceRoot] unchanged. /// -/// [isFile]/[readFile] are injectable for tests; defaults touch the real fs. -String presetRootFor(String workspaceRoot, {bool Function(String path)? isFile, String? Function(String path)? readFile}) { +/// The pointer content is REPO-controlled, so the resolved target is +/// validated before it is trusted: the candidate main root must actually +/// hold a `.git` directory (a genuine repo), else the pointer is ignored and +/// the workspace keys off itself. Without that check a crafted `.git` file +/// could alias an arbitrary path's preset key (same-user only — the preset +/// values themselves stay user-authored — but the boundary is cheap to hold). +/// +/// [isFile]/[readFile]/[isDir] are injectable for tests; defaults touch the +/// real fs. +String presetRootFor(String workspaceRoot, {bool Function(String path)? isFile, String? Function(String path)? readFile, bool Function(String path)? isDir}) { final root = _stripTrailingSep(workspaceRoot); final probe = isFile ?? _isFile; final read = readFile ?? _readFile; + final dirProbe = isDir ?? _isDir; final gitPointer = '$root/.git'; if (!probe(gitPointer)) return root; final content = read(gitPointer); @@ -58,7 +67,21 @@ String presetRootFor(String workspaceRoot, {bool Function(String path)? isFile, const marker = '/.git/worktrees/'; final idx = resolved.indexOf(marker); if (idx <= 0) return root; - return resolved.substring(0, idx); + final mainRoot = resolved.substring(0, idx); + if (!dirProbe('$mainRoot/.git')) return root; + return mainRoot; +} + +/// The root to key a spawn-time preset lookup on: [workspaceRoot] when [cwd] +/// is the workspace root or anywhere below it (a pane spawned in a subdir +/// must share the workspace's preset), else [cwd] itself (a spawn in an +/// unrelated directory keys off that directory's own repo). +String presetLookupRoot(String? cwd, String workspaceRoot) { + final ws = _stripTrailingSep(workspaceRoot); + if (cwd == null || cwd.isEmpty) return ws; + final c = _stripTrailingSep(cwd); + if (c == ws || c.startsWith('$ws/')) return ws; + return c; } /// Read [workspaceRoot]'s preset through an injected settings [read] (key → @@ -69,8 +92,9 @@ List presetDirsFrom( String workspaceRoot, { bool Function(String path)? isFile, String? Function(String path)? readFile, + bool Function(String path)? isDir, }) { - final raw = read(pathPresetKey(workspaceRoot, isFile: isFile, readFile: readFile)); + final raw = read(pathPresetKey(workspaceRoot, isFile: isFile, readFile: readFile, isDir: isDir)); if (raw is! List) return const []; return [ for (final e in raw) @@ -81,11 +105,17 @@ List presetDirsFrom( /// Pure prepend: [preset] dirs (de-duplicated, order kept) ahead of [base], /// with base entries that repeat a preset dir dropped so the preset always /// wins. An empty preset returns [base] unchanged. +/// +/// Contract: one directory per entry. An entry containing [sep] is malformed +/// (the CLI/UI reject it at input time; this guards stored values that +/// predate the check) and is skipped — joined verbatim it would smuggle +/// extra tokens into PATH, and a trailing separator yields an EMPTY token, +/// which POSIX shells resolve as CWD. String applyPathPreset(String base, List preset, {String sep = ':'}) { final dirs = []; for (final d in preset) { final t = d.trim(); - if (t.isNotEmpty && !dirs.contains(t)) dirs.add(t); + if (t.isNotEmpty && !t.contains(sep) && !dirs.contains(t)) dirs.add(t); } if (dirs.isEmpty) return base; final baseParts = base.isEmpty ? const [] : base.split(sep); @@ -108,6 +138,8 @@ List missingLoginShellDirs({required String? loginPath, required String bool _isFile(String path) => FileSystemEntity.typeSync(path) == FileSystemEntityType.file; +bool _isDir(String path) => FileSystemEntity.typeSync(path) == FileSystemEntityType.directory; + String? _readFile(String path) { try { return File(path).readAsStringSync(); diff --git a/test/builtin/claude/session_orchestrator_test.dart b/test/builtin/claude/session_orchestrator_test.dart index 55098c5a..6a81e91b 100644 --- a/test/builtin/claude/session_orchestrator_test.dart +++ b/test/builtin/claude/session_orchestrator_test.dart @@ -53,6 +53,19 @@ void main() { expect(spawnedArgs.single, isNot(contains('--effort'))); }); + test('the workspace PATH preset reaches the spawned session env (D-106)', () async { + final envs = ?>[]; + final preset = ClaudeSessionOrchestrator( + processFactory: ({required sessionArgs, required cwd, env}) async { + envs.add(env); + return _FakeProc(); + }, + pathPresetFor: (cwd) => cwd == '/repo' ? const ['/opt/go/bin'] : const [], + ); + await preset.spawn(SpawnSpec(id: 'p1', role: 'primary', sessionId: 'p1-uuid', cwd: '/repo')); + expect(envs.single?['PATH'], startsWith('/opt/go/bin:'), reason: 'preset dirs lead the delta PATH'); + }); + test('a fresh session gets the skills nudge; resume + fork do not (T-490)', () async { String appendPrompt(List args) { final i = args.indexOf('--append-system-prompt'); diff --git a/test/builtin/tools_settings/path_preset_control_test.dart b/test/builtin/tools_settings/path_preset_control_test.dart index 3dc2fb14..86c8b76a 100644 --- a/test/builtin/tools_settings/path_preset_control_test.dart +++ b/test/builtin/tools_settings/path_preset_control_test.dart @@ -81,6 +81,29 @@ void main() { expect(dirs(f.tempDir.path), isEmpty); }); + testWidgets('an entry containing a PATH separator is rejected (CWD-token guard)', (tester) async { + await tester.runAsync(() => f.services.settings.setProjectDir(f.tempDir)); + await pump(tester); + await tester.pump(); + await tester.enterText(find.byType(EditableText), '/opt/go/bin:'); + await tester.tap(find.text('Add entry')); + await tester.pump(); + expect(find.textContaining('absolute path'), findsOneWidget); + expect(dirs(f.tempDir.path), isEmpty); + }); + + testWidgets('~/ expands against HOME and Enter submits (parity with the CLI verb)', (tester) async { + final home = Platform.environment['HOME']; + if (home == null || home.isEmpty) return; // no HOME in this environment — the guard path is CLI-tested + await tester.runAsync(() => f.services.settings.setProjectDir(f.tempDir)); + await pump(tester); + await tester.pump(); + await tester.enterText(find.byType(EditableText), '~/go/bin/'); + await tester.testTextInput.receiveAction(TextInputAction.done); + await tester.pump(); + expect(dirs(f.tempDir.path), ['$home/go/bin']); + }); + testWidgets('an existing dir renders without the missing tag', (tester) async { await tester.runAsync(() async { await f.services.settings.setProjectDir(f.tempDir); diff --git a/test/daemon/env_path_commands_test.dart b/test/daemon/env_path_commands_test.dart index 073baee9..3d4f26dd 100644 --- a/test/daemon/env_path_commands_test.dart +++ b/test/daemon/env_path_commands_test.dart @@ -32,7 +32,7 @@ void main() { String? loginPath; var processPath = '/usr/bin:/bin'; - void wire({String? cwd = '/repo', bool withStore = true}) { + void wire({String? cwd = '/repo', bool withStore = true, String? home = '/home/u'}) { store = _FakeStore(); published = []; existingDirs = {}; @@ -45,7 +45,7 @@ void main() { publisher: () => (p, c, data) => published.add((publisher: p, channel: c, data: data)), workspaceCwd: () => cwd, - home: () => '/home/u', + home: () => home, dirExists: (dir) => existingDirs.contains(dir), loginPath: () => loginPath, processPath: () => processPath, @@ -91,6 +91,26 @@ void main() { expect(empty.error?.hint, contains('clear')); }); + test('add and remove apply the same guards as set (relative + separator)', () async { + wire(); + for (final action in ['set', 'add', 'remove']) { + final rel = await run([action, 'go/bin']); + expect(rel.ok, isFalse, reason: '$action relative'); + expect(rel.error?.message, contains('not an absolute path')); + final sep = await run([action, '/a:']); + expect(sep.ok, isFalse, reason: '$action separator'); + expect(sep.error?.message, contains('PATH separator')); + } + expect(store.byCwd, isEmpty, reason: 'nothing was written'); + }); + + test('~ with no HOME errors instead of storing a broken entry', () async { + wire(home: null); + final r = await run(['set', '~/go/bin']); + expect(r.ok, isFalse); + expect(r.error?.message, contains('cannot expand')); + }); + test('a leading-dash entry is rejected by the schema (T-104 guard)', () async { wire(); final r = await d.dispatch( diff --git a/test/src/env/path_preset_test.dart b/test/src/env/path_preset_test.dart index 1e3fc668..5b9c0283 100644 --- a/test/src/env/path_preset_test.dart +++ b/test/src/env/path_preset_test.dart @@ -34,6 +34,11 @@ void main() { test('honours a custom separator', () { expect(applyPathPreset(r'C:\bin', [r'C:\go'], sep: ';'), r'C:\go;C:\bin'); }); + + test('skips a malformed entry containing the separator (would smuggle a CWD token)', () { + expect(applyPathPreset('/usr/bin', ['/a:', '/ok']), '/ok:/usr/bin'); + expect(applyPathPreset('/usr/bin', ['/a::/b']), '/usr/bin'); + }); }); group('missingLoginShellDirs', () { @@ -65,20 +70,45 @@ void main() { test('an in-repo .worktrees worktree resolves to the main repo root (absolute gitdir)', () { expect( - presetRootFor('/repo/.worktrees/fix', isFile: (p) => p == '/repo/.worktrees/fix/.git', readFile: (p) => 'gitdir: /repo/.git/worktrees/fix\n'), + presetRootFor( + '/repo/.worktrees/fix', + isFile: (p) => p == '/repo/.worktrees/fix/.git', + readFile: (p) => 'gitdir: /repo/.git/worktrees/fix\n', + isDir: (p) => p == '/repo/.git', + ), '/repo', ); }); test('a relative gitdir pointer resolves against the worktree root', () { expect( - presetRootFor('/repo/.worktrees/fix', isFile: (p) => p == '/repo/.worktrees/fix/.git', readFile: (p) => 'gitdir: ../../.git/worktrees/fix'), + presetRootFor( + '/repo/.worktrees/fix', + isFile: (p) => p == '/repo/.worktrees/fix/.git', + readFile: (p) => 'gitdir: ../../.git/worktrees/fix', + isDir: (p) => p == '/repo/.git', + ), '/repo', ); }); test('a worktree outside the repo still resolves to the main root', () { - expect(presetRootFor('/tmp/wt', isFile: (p) => p == '/tmp/wt/.git', readFile: (_) => 'gitdir: /srv/repos/main/.git/worktrees/wt'), '/srv/repos/main'); + expect( + presetRootFor( + '/tmp/wt', + isFile: (p) => p == '/tmp/wt/.git', + readFile: (_) => 'gitdir: /srv/repos/main/.git/worktrees/wt', + isDir: (p) => p == '/srv/repos/main/.git', + ), + '/srv/repos/main', + ); + }); + + test('a pointer whose target is not a real repo is ignored (repo-controlled content)', () { + expect( + presetRootFor('/evil', isFile: (p) => p == '/evil/.git', readFile: (_) => 'gitdir: /home/u/victim/.git/worktrees/x', isDir: (_) => false), + '/evil', + ); }); test('a gitdir pointer without the worktrees marker (submodule-style) keys off itself', () { @@ -91,7 +121,15 @@ void main() { }); test('backslashed gitdir (Windows-written pointer) still matches', () { - expect(presetRootFor('/repo/.worktrees/x', isFile: (p) => p == '/repo/.worktrees/x/.git', readFile: (_) => r'gitdir: /repo/.git\worktrees\x'), '/repo'); + expect( + presetRootFor( + '/repo/.worktrees/x', + isFile: (p) => p == '/repo/.worktrees/x/.git', + readFile: (_) => r'gitdir: /repo/.git\worktrees\x', + isDir: (p) => p == '/repo/.git', + ), + '/repo', + ); }); test('resolves a REAL worktree layout on disk (no injected probes)', () { @@ -116,7 +154,12 @@ void main() { test('a worktree and its main repo share one key; trailing slash is unified', () { final main = pathPresetKey('/repo', isFile: (_) => false, readFile: (_) => null); final slash = pathPresetKey('/repo/', isFile: (_) => false, readFile: (_) => null); - final wt = pathPresetKey('/repo/.worktrees/fix', isFile: (p) => p == '/repo/.worktrees/fix/.git', readFile: (_) => 'gitdir: /repo/.git/worktrees/fix'); + final wt = pathPresetKey( + '/repo/.worktrees/fix', + isFile: (p) => p == '/repo/.worktrees/fix/.git', + readFile: (_) => 'gitdir: /repo/.git/worktrees/fix', + isDir: (p) => p == '/repo/.git', + ); expect(slash, main); expect(wt, main); expect(pathPresetKey('/other', isFile: (_) => false, readFile: (_) => null), isNot(main));