diff --git a/.pql/changelog/ticket_history/2026-06.sql b/.pql/changelog/ticket_history/2026-06.sql index fb6397cf..931ffaf0 100644 --- a/.pql/changelog/ticket_history/2026-06.sql +++ b/.pql/changelog/ticket_history/2026-06.sql @@ -1 +1,2 @@ INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-194', 'status', 'backlog', 'done', NULL, '2026-06-01 06:49:47', '2026-06-01 06:49:47', '2026-06-01 06:49:47', NULL, '09bfe6f27513d7d16f7139543961b4f8', 1) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-195', 'status', 'backlog', 'done', NULL, '2026-06-01 07:08:16', '2026-06-01 07:08:16', '2026-06-01 07:08:16', NULL, 'c2fb7c6ede9e970a8186b5932141f472', 1) ON CONFLICT(hash) DO NOTHING; diff --git a/.pql/changelog/tickets/2026-06.sql b/.pql/changelog/tickets/2026-06.sql index 43c30f22..465fe5c8 100644 --- a/.pql/changelog/tickets/2026-06.sql +++ b/.pql/changelog/tickets/2026-06.sql @@ -1 +1,2 @@ INSERT INTO tickets (id, type, parent_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-194', 'bug', 'T-132', 'Markdown reader 404s on absolute paths handed to files.read', 'The sidebar markdown reader (and any files.read caller) was handed an absolute path (e.g. the Claude Config tab publishing a skill''s absolute SKILL.md manifestPath). resolveUnderRoot joined the absolute path onto the workspace root (/repo + /repo/x = /repo/repo/x), which does not exist, so the reader showed ''file not found'' for a file that exists. Fix: resolveUnderRoot normalizes an absolute input as-is and relies on the existing containment check (so absolute-under-root resolves, absolute-outside still throws PathOutsideRoot). Found live opening .claude/skills/frame0-wireframe/SKILL.md.', 'done', 'high', NULL, NULL, NULL, '2026-06-01 06:49:25', '2026-06-01 06:49:47', NULL, '01a3a891c9786f7afb7a1b95e8ab0b2f', 1) ON CONFLICT(id) DO UPDATE SET type=excluded.type, parent_id=excluded.parent_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 (id, type, parent_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-195', 'story', 'T-132', 'Reader can''t open user-scope Claude config files (~/.claude skills/agents/commands)', 'Clicking a user-scope skill in the Claude Config tab (e.g. /home//.claude/skills/peon-ping-toggle/SKILL.md) shows ''path outside workspace''. The markdown reader loads via files.read, which is workspace-confined for security (T-102); but user-scope Claude config files legitimately live under ~/.claude, outside the repo. Per D-76, Claude''s config surface (skills/agents/commands under ~/.claude AND the repo .claude) is clide-managed and safe to surface. Proposed: a scoped trusted-read — a new IPC verb (e.g. claude.read-config-file) that permits reads ONLY under the resolved Claude config roots (~/.claude + /.claude), still rejecting arbitrary paths. Route the Config tab''s skill/agent/command markdown clicks through it, or have the markdown reader fall back to it when the path is under a Claude config root but outside the workspace. Alternative: the Config tab already reads SKILL.md during scan (claude_config.dart) and could publish content directly via a load-by-content channel rather than a path (the reader currently only reads by path). Repo-scope skills already work after T-194; this covers the user-scope half.', 'done', 'medium', NULL, NULL, NULL, '2026-06-01 06:56:05', '2026-06-01 07:08:16', NULL, '7b46d0491c01bcc056c430a7af48f734', 1) ON CONFLICT(id) DO UPDATE SET type=excluded.type, parent_id=excluded.parent_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 2bd07f66..e3eecced 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,11 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. ### Fixed +- The markdown reader can now open user-scope Claude config files (skills / + agents / commands under `~/.claude`), not just repo-local ones. `files.read` + gained a read allow-list covering the workspace plus the trusted Claude config + roots; writes stay repo-confined and off-root paths are still rejected. (T-195, + D-80) - The markdown reader opens files given an absolute path again (e.g. a skill's `SKILL.md` from the Claude Config tab). `resolveUnderRoot` no longer doubles an absolute path onto the workspace root; absolute-under-root resolves, while diff --git a/governance/README.md b/governance/README.md index 9b801fcc..b67c6e9f 100644 --- a/governance/README.md +++ b/governance/README.md @@ -121,6 +121,7 @@ You might also want, project-permitting: - [D-77: Drive Claude via the stream-json control protocol; teams become a clide-owned coordination layer](decisions/architecture.md#d-77-drive-claude-via-the-stream-json-control-protocol-teams-become-a-clide-owned-coordination-layer) — _architecture_ - [D-78: Claude permission/prompt transport is the stdio control channel](decisions/architecture.md#d-78-claude-permissionprompt-transport-is-the-stdio-control-channel) — _architecture_ - [D-79: Workspace content search is a pure-Dart in-process engine, outside pql](decisions/architecture.md#d-79-workspace-content-search-is-a-pure-dart-in-process-engine-outside-pql) — _architecture_ +- [D-80: `files.read` allows trusted Claude config roots beyond the workspace](decisions/architecture.md#d-80-filesread-allows-trusted-claude-config-roots-beyond-the-workspace) — _architecture_ ## Open questions diff --git a/governance/decisions/architecture.md b/governance/decisions/architecture.md index dad98fdb..75a150e8 100644 --- a/governance/decisions/architecture.md +++ b/governance/decisions/architecture.md @@ -357,3 +357,14 @@ Core, rendering, IPC, kernel, panel manager. - **Raised by:** 2026-05-31 — during /whats-next refinement of the search/nav batch (T-51/T-52/T-53). Refinement surfaced that `pql search` structurally can't satisfy find-in-files; the user probed tree-sitter (ruled out — it's a parser, not a grepper) and the performance ceiling, then chose the fastest *reasonable* Dart option (isolate pool + literal fast-path + streaming) over a ripgrep dependency. --- + +### D-80: `files.read` allows trusted Claude config roots beyond the workspace +- **Date:** 2026-06-01 +- **Status:** accepted +- **Decision:** `files.read` accepts an **allow-list of read roots**: the workspace root (as before) **plus** the resolved Claude config directories — the global `~/.claude` and the repo-local `/.claude` (the latter already lives under the workspace). A path is readable when it is contained by **any** allowed root; everything else is still rejected with `path outside workspace`. This widens **reads only** — writes (`search.replace`, future `files.write`) stay confined to the workspace root. +- **Rationale:** Per [D-76](#d-76-claudeconfig--claudes-config-surface-is-clides-app-settings-builtin-owned-watched-probe-cached-per-version) Claude's config surface (skills/agents/commands under `~/.claude` + the repo `.claude`) is clide-managed and surfaced in the Config tab. Opening a surfaced skill's `SKILL.md` in the markdown reader is a legitimate, expected action, but `~/.claude` is global and outside the repo — the original [T-102] confinement rejected it (`path outside workspace`). Extending the allow-list to exactly the config roots the app already reads is the user's chosen model ("the `.claude` dir is in the workspace") and is simpler than a separate trusted-read verb. +- **Security boundary:** This is a *bounded* widening, not a hole. Only the explicitly-listed config roots are added; arbitrary off-repo paths and `..` traversal are still rejected, and the symlink re-check ([resolveUnderRootsFollowingSymlinks]) re-verifies the real path is contained by one of the allowed roots (so a symlink under a config root pointing to `/etc/shadow` is still refused). The roots are the user's own trusted Claude config (same trust level as pql's data per D-3), and writes are unaffected. +- **Cross-reference:** amends the read side of [D-4](#d-4-ignore-file-strategy)/T-102's "repo-is-the-workspace" confinement; builds on [T-194](#) (absolute-under-root reads). Implemented by T-195 — `FilesService.extraReadRoots`, wired in `main.dart` to `~/.claude` when present. +- **Raised by:** 2026-06-01 — after T-194 fixed repo-scope skill reads, the user hit `path outside workspace` opening a *user-scope* skill (`~/.claude/skills/peon-ping-toggle/SKILL.md`) and said the `.claude` dir should be in the workspace. Chose extending the read allow-list over a separate trusted-read verb. + +--- diff --git a/lib/main.dart b/lib/main.dart index c93d1bf6..8d00d538 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -166,7 +166,17 @@ Future main() async { final eventSink = _BusEventSink(events); final paneRegistry = PaneRegistry(events: eventSink); registerPaneCommands(dispatcher, paneRegistry); - final filesService = FilesService(root: workRoot, events: eventSink); + // Trusted read-only roots beyond the workspace: the global Claude + // config dir (~/.claude), so the reader can open user-scope skill / + // agent / command markdown the Config tab surfaces (D-80, T-195). + // The repo-local .claude is already under workRoot. + final extraReadRoots = []; + final claudeHome = Platform.environment['HOME']; + if (claudeHome != null) { + final globalClaude = Directory('$claudeHome/.claude'); + if (globalClaude.existsSync()) extraReadRoots.add(globalClaude); + } + final filesService = FilesService(root: workRoot, events: eventSink, extraReadRoots: extraReadRoots); registerFilesCommands(dispatcher, filesService); // Search reuses the files service's resolved ignore set so the // grep honours the same ignore_files: layering (D-4 / D-79). diff --git a/lib/src/daemon/files_commands.dart b/lib/src/daemon/files_commands.dart index c042d9b3..64718d0c 100644 --- a/lib/src/daemon/files_commands.dart +++ b/lib/src/daemon/files_commands.dart @@ -26,6 +26,7 @@ class FilesService { required this.root, required this.events, IgnoreSet? ignore, + this.extraReadRoots = const [], }) : ignore = ignore ?? _defaultIgnore(root); /// Build from the current working directory, walking up to the git @@ -39,6 +40,10 @@ class FilesService { final IgnoreSet ignore; final DaemonEventSink events; + /// Trusted read-only roots outside the workspace that `files.read` + /// also accepts (the Claude config dirs, D-80). Writes ignore these. + final List extraReadRoots; + FileWatcher? _watcher; Future startWatching() async { @@ -81,8 +86,9 @@ void registerFilesCommands(DaemonDispatcher d, FilesService files) { final String absPath; try { // 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); + // symlink can't be read (T-102). Accepts the workspace root plus + // the trusted extra read roots (Claude config dirs, D-80). + absPath = resolveUnderRootsFollowingSymlinks(files.root, files.extraReadRoots, path); } on PathOutsideRoot { return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'path outside workspace: $path')); } diff --git a/lib/src/files/path_safety.dart b/lib/src/files/path_safety.dart index 3357b8ef..cd7f436c 100644 --- a/lib/src/files/path_safety.dart +++ b/lib/src/files/path_safety.dart @@ -70,6 +70,50 @@ String resolveUnderRootFollowingSymlinks(Directory root, String relative) { return realPath; } +/// Like [resolveUnderRoot], but an **absolute** [path] is also accepted +/// when it falls under any of [extraReadRoots] — trusted read-only roots +/// such as the Claude config dirs (`~/.claude`, `/.claude`) that +/// clide surfaces but which may live outside the workspace (D-80). +/// Relative paths always resolve under the primary [root]. Throws +/// [PathOutsideRoot] when the path is contained by none of the roots. +/// +/// This widens *reads* only; writes stay confined to the workspace via +/// the single-root variant. +String resolveUnderRoots(Directory root, List extraReadRoots, String path) { + if (!path.startsWith(Platform.pathSeparator)) { + return resolveUnderRoot(root, path); // relative → workspace-relative + } + final norm = _normalize(path); + for (final r in [root, ...extraReadRoots]) { + final rp = _normalize(r.absolute.path); + if (norm == rp || norm.startsWith('$rp${Platform.pathSeparator}')) return norm; + } + throw PathOutsideRoot(path, norm, _normalize(root.absolute.path)); +} + +/// Symlink-following multi-root resolver — the [resolveUnderRoots] +/// analogue of [resolveUnderRootFollowingSymlinks]. Re-verifies the real +/// (symlink-resolved) path is contained by one of the allowed roots. +String resolveUnderRootsFollowingSymlinks(Directory root, List extraReadRoots, String path) { + final pathResolved = resolveUnderRoots(root, extraReadRoots, path); + if (FileSystemEntity.typeSync(pathResolved, followLinks: false) == FileSystemEntityType.notFound) { + return pathResolved; + } + final realPath = File(pathResolved).resolveSymbolicLinksSync(); + for (final r in [root, ...extraReadRoots]) { + final String realRoot; + try { + realRoot = Directory(r.absolute.path).resolveSymbolicLinksSync(); + } catch (_) { + continue; // a non-existent allowed root can't contain anything + } + if (realPath == realRoot || realPath.startsWith('$realRoot${Platform.pathSeparator}')) { + return realPath; + } + } + throw PathOutsideRoot(path, realPath, _normalize(root.absolute.path)); +} + String _normalize(String path) { // Use Uri to collapse `..` and `.` segments without hitting the // filesystem (Directory(...).resolveSymbolicLinksSync would also diff --git a/test/daemon/files_commands_test.dart b/test/daemon/files_commands_test.dart index 7bb0355d..701426e0 100644 --- a/test/daemon/files_commands_test.dart +++ b/test/daemon/files_commands_test.dart @@ -107,6 +107,26 @@ void main() { expect(r.data['content'], 'hi'); }); + test('files.read reads an absolute path under an extra read root (D-80)', () async { + final extra = await Directory.systemTemp.createTemp('clide-extra-claude-'); + addTearDown(() async => extra.existsSync() ? extra.deleteSync(recursive: true) : null); + File('${extra.path}/SKILL.md').writeAsStringSync('# peon'); + final svc = FilesService(root: sandbox, events: RecordingEventSink(), ignore: IgnoreSet.builtin(), extraReadRoots: [extra]); + final d = DaemonDispatcher(); + registerFilesCommands(d, svc); + addTearDown(svc.shutdown); + + final r = await d.dispatch(IpcRequest(id: '1', cmd: 'files.read', args: {'path': '${extra.absolute.path}/SKILL.md'})); + expect(r.ok, isTrue); + expect(r.data['content'], '# peon'); + }); + + test('files.read still rejects an absolute path outside all roots', () async { + final r = await call('files.read', const {'path': '/etc/passwd'}); + expect(r.ok, isFalse); + expect(r.error!.message, contains('outside workspace')); + }); + test('files.read without a path returns toolError', () async { final r = await call('files.read', const {}); expect(r.ok, isFalse); diff --git a/test/files/path_safety_test.dart b/test/files/path_safety_test.dart index 4f73ea10..ebfe8485 100644 --- a/test/files/path_safety_test.dart +++ b/test/files/path_safety_test.dart @@ -136,4 +136,45 @@ void main() { ); }); }); + + group('resolveUnderRoots (extra read roots, D-80)', () { + late Directory extra; + setUp(() => extra = Directory.systemTemp.createTempSync('clide_extra_root_')); + tearDown(() => extra.existsSync() ? extra.deleteSync(recursive: true) : null); + + test('a relative path still resolves under the primary root', () { + expect(resolveUnderRoots(root, [extra], 'file.txt'), '${root.absolute.path}/file.txt'); + }); + + test('an absolute path under the primary root is accepted', () { + final abs = '${root.absolute.path}/.claude/x.md'; + expect(resolveUnderRoots(root, [extra], abs), abs); + }); + + test('an absolute path under an extra read root is accepted', () { + final abs = '${extra.absolute.path}/skills/peon/SKILL.md'; + expect(resolveUnderRoots(root, [extra], abs), abs); + }); + + test('an absolute path outside every root is rejected', () { + expect(() => resolveUnderRoots(root, [extra], '/etc/passwd'), throwsA(isA())); + }); + + test('following symlinks: a real file under an extra root resolves', () { + File('${extra.path}/SKILL.md').writeAsStringSync('# skill'); + final out = resolveUnderRootsFollowingSymlinks(root, [extra], '${extra.absolute.path}/SKILL.md'); + expect(out, endsWith('/SKILL.md')); + }); + + test('following symlinks: a symlink under an extra root pointing outside is rejected', () { + final outside = Directory.systemTemp.createTempSync('clide_extra_leak_'); + addTearDown(() => outside.existsSync() ? outside.deleteSync(recursive: true) : null); + File('${outside.path}/secret.txt').writeAsStringSync('payload'); + Link('${extra.path}/leak').createSync('${outside.path}/secret.txt'); + expect( + () => resolveUnderRootsFollowingSymlinks(root, [extra], '${extra.absolute.path}/leak'), + throwsA(isA()), + ); + }); + }); }