read user-scope Claude config files via a read allow-list (D-80)
The reader opened repo-local .claude markdown but rejected user-scope files under ~/.claude with "path outside workspace" — that dir is global, outside the repo, and files.read was repo-confined (T-102). Per D-76 the Claude config surface is clide-managed, so files.read now resolves a path under an allow-list: the workspace root plus trusted extra read roots (FilesService.extraReadRoots), wired in main.dart to ~/.claude when present. Reads widen; writes stay repo-confined, and the symlink re-check still refuses a config-root symlink that escapes. Off- root paths and `..` traversal are rejected as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -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/<user>/.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 + <repo>/.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);
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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 `<repo>/.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.
|
||||
|
||||
---
|
||||
|
||||
+11
-1
@@ -166,7 +166,17 @@ Future<void> 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 = <Directory>[];
|
||||
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).
|
||||
|
||||
@@ -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<Directory> extraReadRoots;
|
||||
|
||||
FileWatcher? _watcher;
|
||||
|
||||
Future<void> 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'));
|
||||
}
|
||||
|
||||
@@ -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`, `<repo>/.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<Directory> 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<Directory> 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
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<PathOutsideRoot>()));
|
||||
});
|
||||
|
||||
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<PathOutsideRoot>()),
|
||||
);
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user