From 31d40ad8ce46ca6eccfd76223517a52364eb0995 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Mon, 18 May 2026 09:53:20 +0200 Subject: [PATCH] harden IPC: reject -prefixed git refs, cap files.read / git.log (T-104) Three security fixes the consultant flagged: * git.checkout, git.push now reject branch/remote arguments starting with `-` via a top-level validateGitRef helper. `git push` also gets a `--` option terminator; checkout can't use `--` without changing semantics (it would be parsed as a pathspec), so the validator is the only line of defence there. * files.read caps responses at 10 MB so a single call can't OOM the UI on a multi-gigabyte log. * git.log caps `count` at 1000; git.diff / git.stage cap paths at 256. Excess is a userError rather than burning subprocess time. The bigger typed-schema framework (item 1 in T-104) is split out as T-120 since it needs design discussion alongside T-99. Co-Authored-By: Claude --- .pql/changelog/ticket_history/2026-05.sql | 2 ++ .pql/changelog/tickets/2026-05.sql | 19 ++++++++++ CHANGELOG.md | 7 ++++ lib/src/daemon/files_commands.dart | 19 ++++++++++ lib/src/daemon/git_commands.dart | 38 +++++++++++++++++++- lib/src/git/client.dart | 11 +++++- lib/src/git/operations.dart | 28 +++++++++++++++ test/daemon/files_commands_test.dart | 16 +++++++++ test/daemon/git_commands_test.dart | 41 ++++++++++++++++++++++ test/git/operations_test.dart | 42 +++++++++++++++++++++++ 10 files changed, 221 insertions(+), 2 deletions(-) diff --git a/.pql/changelog/ticket_history/2026-05.sql b/.pql/changelog/ticket_history/2026-05.sql index 18404f9a..9a2d9b2a 100644 --- a/.pql/changelog/ticket_history/2026-05.sql +++ b/.pql/changelog/ticket_history/2026-05.sql @@ -1809,3 +1809,5 @@ INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, 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-118', 'status', 'backlog', 'done', NULL, '2026-05-17 20:53:41', '2026-05-17 20:53:41', '2026-05-17 20:53:41', NULL, 'ff0126655805fed0ebe45f313c5ceff7', 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-114', 'status', 'in_progress', 'done', NULL, '2026-05-18 07:04:38', '2026-05-18 07:04:38', '2026-05-18 07:04:38', NULL, '8cf198f8c824fa8f46277b7e22e68f0c', 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-111', 'status', 'in_progress', 'done', NULL, '2026-05-18 07:43:45', '2026-05-18 07:43:45', '2026-05-18 07:43:45', NULL, '792efcf7c772de7cda7069ea3fc3ac0a', 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-104', 'status', 'backlog', 'in_progress', NULL, '2026-05-18 07:49:44', '2026-05-18 07:49:44', '2026-05-18 07:49:44', NULL, 'bf7f2746c252f8b8893f4fdc06d459b8', 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-104', 'status', 'in_progress', 'done', NULL, '2026-05-18 07:53:10', '2026-05-18 07:53:10', '2026-05-18 07:53:10', NULL, 'ce2b66658977edc3fddc78079d7852e5', 1) ON CONFLICT(hash) DO NOTHING; diff --git a/.pql/changelog/tickets/2026-05.sql b/.pql/changelog/tickets/2026-05.sql index b23d1adf..e64a2fea 100644 --- a/.pql/changelog/tickets/2026-05.sql +++ b/.pql/changelog/tickets/2026-05.sql @@ -1847,3 +1847,22 @@ Coordinate with T-99 (IPC implementation) if the CLI verb needs new dispatch wir Source: consultants.md "UX — Findings — [Major] Drag-resize handles".', 'done', 'medium', NULL, NULL, NULL, '2026-05-17 18:48:14', '2026-05-18 07:43:45', NULL, '594d1cbcc8daa115246e2398d9ba9506', 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-119', 'task', 'T-99', 'clide panel resize CLI verb (split from T-111)', 'Once T-99 lands an IPC dispatch path, register a ''panel.resize'' command that takes and args and calls LayoutArrangement.setSize / setEditorRatio. Keyboard parity already landed in T-111; this completes user/Claude parity per D-6.', 'backlog', 'low', NULL, NULL, NULL, '2026-05-18 07:43:50', '2026-05-18 07:43:50', NULL, '29df32cde1a30168a27b763f5e6ce81d', 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-120', 'task', 'T-97', 'typed IPC command schema framework (split from T-104)', 'T-104 covered the spot-fixes (argv-injection rejection, size/count caps on specific commands). What remains is the framework piece: a typed schema per IPC command — branch/remote/path/etc. with regex/charset constraints — applied at DaemonDispatcher dispatch time rather than scattered through individual handlers. + +This needs design before code: +- Where the schema lives (per-handler? a central registry?) +- How it composes with the IpcRequest envelope +- Whether it generates the existing handler boilerplate or wraps it +- Coordination with T-99''s IPC architecture decision (if we go socket-server, the schema becomes the wire contract) + +Source: consultants.md "Security — Findings — [Major]" item 1.', 'backlog', 'medium', NULL, NULL, NULL, '2026-05-18 07:53:08', '2026-05-18 07:53:08', NULL, 'c2673e06e042bb418de9a99e8ab21c2b', 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-104', 'task', 'T-97', 'schema-validate IPC argument surface; reject -prefixed values', '`DaemonDispatcher.dispatch` (`dispatcher.dart:26`) and `IpcRequest.fromJson` (`envelope.dart:49`) do no schema validation. No size limit on `files.read`, no count cap on `git.log`. `git diff`/`stage` use `--` separators (good), but `checkout(branch)` and `push(remote, branch)` (`git_commands.dart:240`) do not — argument injection (e.g. `git checkout --upload-pack=...`) is possible. + +**Fix:** +1. Typed schema per IPC command (`branch`, `remote`, `path`, etc. with regex / charset constraints). +2. Size + count bounds on `files.read`, `git.log`, `git.diff`. +3. Either reject `-`-prefixed values for branch/remote/path args, or use `--` everywhere (including `checkout` and `push`). + +**Acceptance:** unit tests cover the rejection path for each injection vector listed. + +Source: consultants.md "Security — Findings — [Major]".', 'done', 'high', NULL, NULL, NULL, '2026-05-17 18:47:42', '2026-05-18 07:53:10', NULL, '688f888ba89cefe636ece09f32bea8ab', 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 dc844cf6..cb7b818d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,6 +91,13 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. mounted via IndexedStack so switching tabs preserves their state (PTY connections, scroll position, etc.). +### Security + +- IPC: `git.checkout`, `git.push` reject branch/remote args starting + with `-` (closes the `--upload-pack=...` argv-injection vector). + `files.read` rejects files over 10 MB. `git.log` caps `count` at + 1000; `git.diff` / `git.stage` cap paths at 256. (T-104.) + ### Fixed - `TerminalView.onTapUp` now actually fires on primary tap — was diff --git a/lib/src/daemon/files_commands.dart b/lib/src/daemon/files_commands.dart index b5c9a805..6b9dfe8f 100644 --- a/lib/src/daemon/files_commands.dart +++ b/lib/src/daemon/files_commands.dart @@ -13,6 +13,11 @@ import '../ipc/schema_v1.dart'; import '../panes/event_sink.dart'; import 'dispatcher.dart'; +/// Cap on `files.read` response size. UI doesn't render multi-MB +/// blobs usefully and a single uncapped call can OOM. Range/stream +/// reads will land as a separate command (T-104 follow-up). +const int _filesReadMaxBytes = 10 * 1024 * 1024; + /// Daemon-side state for the `files` subsystem. Holds one /// [FileWatcher] rooted at the workspace and a resolved [IgnoreSet]. class FilesService { @@ -84,6 +89,20 @@ void registerFilesCommands(DaemonDispatcher d, FilesService files) { if (!file.existsSync()) { return IpcResponse.err(id: req.id, error: IpcError(code: IpcExitCode.toolError, kind: IpcErrorKind.toolError, message: 'file not found: $path')); } + // Cap response size so a single IPC call can't OOM the UI on a + // multi-gigabyte log file. Caller can paginate / stream via a + // future range-read variant when that ships. + final length = file.lengthSync(); + if (length > _filesReadMaxBytes) { + return IpcResponse.err( + id: req.id, + error: IpcError( + code: IpcExitCode.toolError, + kind: IpcErrorKind.toolError, + message: 'file too large: $path ($length bytes; cap $_filesReadMaxBytes)', + ), + ); + } final content = file.readAsStringSync(); return IpcResponse.ok(id: req.id, data: {'path': path, 'content': content}); }); diff --git a/lib/src/daemon/git_commands.dart b/lib/src/daemon/git_commands.dart index 24c3caeb..0a4cc972 100644 --- a/lib/src/daemon/git_commands.dart +++ b/lib/src/daemon/git_commands.dart @@ -12,6 +12,16 @@ import '../ipc/schema_v1.dart'; import '../panes/event_sink.dart'; import 'dispatcher.dart'; +/// Cap on `git.log --count` to keep a single query from spinning git +/// up on multi-million-commit repos. UI's history pane paginates; +/// callers asking for more should be using ranges instead. +const int _gitLogMaxCount = 1000; + +/// Cap on `git.diff` and `git.stage` paths-list length so a single +/// IPC request can't queue up an unbounded fan-out of subprocess +/// arguments. +const int _gitPathsMaxCount = 256; + void registerGitCommands( DaemonDispatcher d, GitClient git, @@ -30,6 +40,8 @@ void registerGitCommands( try { final staged = req.args['staged'] as bool? ?? false; final paths = _pathList(req.args['paths']); + final tooMany = _tooManyPaths(req.id, paths); + if (tooMany != null) return tooMany; final diffs = await git.diff(staged: staged, paths: paths); return IpcResponse.ok(id: req.id, data: { 'staged': staged, @@ -53,6 +65,8 @@ void registerGitCommands( ), ); } + final tooMany = _tooManyPaths(req.id, paths); + if (tooMany != null) return tooMany; try { await git.stage(paths); _emitChanged(events); @@ -190,8 +204,18 @@ void registerGitCommands( }); d.register('git.log', (req) async { + final count = (req.args['count'] as num?)?.toInt() ?? 20; + if (count > _gitLogMaxCount) { + return IpcResponse.err( + id: req.id, + error: IpcError( + code: IpcExitCode.userError, + kind: IpcErrorKind.userError, + message: 'git.log count $count exceeds cap $_gitLogMaxCount', + ), + ); + } try { - final count = (req.args['count'] as num?)?.toInt() ?? 20; final entries = await git.log(count: count); return IpcResponse.ok(id: req.id, data: { 'entries': [for (final e in entries) e.toJson()], @@ -264,6 +288,18 @@ List _pathList(Object? raw) { return const []; } +IpcResponse? _tooManyPaths(String id, List paths) { + if (paths.length <= _gitPathsMaxCount) return null; + return IpcResponse.err( + id: id, + error: IpcError( + code: IpcExitCode.userError, + kind: IpcErrorKind.userError, + message: 'paths length ${paths.length} exceeds cap $_gitPathsMaxCount', + ), + ); +} + void _emitChanged(DaemonEventSink events) { events.emit(IpcEvent( subsystem: 'git', diff --git a/lib/src/git/client.dart b/lib/src/git/client.dart index 82a2e886..3894cdd1 100644 --- a/lib/src/git/client.dart +++ b/lib/src/git/client.dart @@ -9,7 +9,7 @@ import 'dart:io'; import '../../kernel/src/toolchain_paths.dart'; import 'diff.dart' show GitDiff, parseDiffOutput; -import 'operations.dart' show GitException, GitLogEntry; +import 'operations.dart' show GitException, GitLogEntry, validateGitRef; import 'status.dart'; class GitClient { @@ -191,8 +191,13 @@ class GitClient { } Future push({String? remote, String? branch, bool setUpstream = false}) async { + if (remote != null) validateGitRef(remote, kind: 'remote'); + if (branch != null) validateGitRef(branch, kind: 'branch'); final args = ['push']; if (setUpstream) args.add('-u'); + // `--` terminates option parsing — belt-and-suspenders alongside + // the ref validator above. + args.add('--'); if (remote != null) args.add(remote); if (branch != null) args.add(branch); final r = await _run(args); @@ -201,6 +206,10 @@ class GitClient { } Future checkout(String branch) async { + // `git checkout -- name` means pathspec, not branch — see the + // matching note in `operations.dart#gitCheckout`. validateGitRef + // is the only defence here. + validateGitRef(branch, kind: 'branch'); final r = await _run(['checkout', branch]); if (r.exitCode != 0) throw GitException('git checkout failed', stderr: r.stderr as String); } diff --git a/lib/src/git/operations.dart b/lib/src/git/operations.dart index f5f8f79a..9f3b44b9 100644 --- a/lib/src/git/operations.dart +++ b/lib/src/git/operations.dart @@ -37,6 +37,21 @@ class GitException implements Exception { String toString() => 'GitException: $message'; } +/// Validate a string about to be passed to git as a branch name, +/// remote name, or similar ref-shaped positional argument. Rejects +/// empty values and anything starting with `-`, which would otherwise +/// be parsed as an option flag by git (the classic +/// `--upload-pack=evil` argv-injection vector). Throws [GitException] +/// — callers convert it to the right IPC error kind. +void validateGitRef(String? value, {required String kind}) { + if (value == null || value.isEmpty) { + throw GitException('$kind is required'); + } + if (value.startsWith('-')) { + throw GitException('$kind cannot start with "-" (looks like an option flag): $value'); + } +} + class GitLogEntry { const GitLogEntry({ required this.hash, @@ -204,8 +219,14 @@ Future gitPush( String? branch, bool setUpstream = false, }) async { + if (remote != null) validateGitRef(remote, kind: 'remote'); + if (branch != null) validateGitRef(branch, kind: 'branch'); final args = ['push']; if (setUpstream) args.add('-u'); + // `--` terminates option parsing — belt-and-suspenders alongside + // the ref validator above. Without it a future caller that bypasses + // the validator could still inject `--upload-pack=...`. + args.add('--'); if (remote != null) args.add(remote); if (branch != null) args.add(branch); final r = await Process.run(gitBin, args, workingDirectory: workDir.path); @@ -236,7 +257,14 @@ Future> gitBranches(Directory workDir) async } /// Checkout a branch. +/// +/// `git checkout` overloads positionals: `-- ` means "restore +/// pathspec ``", not "checkout branch ``". So this can't +/// use `--` as an option terminator without changing semantics — the +/// [validateGitRef] guard against `-`-prefixed values is the only +/// argv-injection defence here. Use `gitSwitch` if/when we adopt it. Future gitCheckout(Directory workDir, String branch) async { + validateGitRef(branch, kind: 'branch'); final r = await Process.run( gitBin, ['checkout', branch], diff --git a/test/daemon/files_commands_test.dart b/test/daemon/files_commands_test.dart index e3ea2ae5..91cc7cc5 100644 --- a/test/daemon/files_commands_test.dart +++ b/test/daemon/files_commands_test.dart @@ -110,6 +110,22 @@ void main() { expect(r.error!.message, contains('not found')); }); + test('files.read rejects a file over the size cap', () async { + // Cap is 10 MB; write 11 MB of zeros and confirm rejection rather + // than reading it into memory. + final big = File('${sandbox.path}/huge.bin'); + final chunk = List.filled(1024 * 1024, 0); + final sink = big.openWrite(); + for (var i = 0; i < 11; i++) { + sink.add(chunk); + } + await sink.flush(); + await sink.close(); + final r = await call('files.read', const {'path': 'huge.bin'}); + expect(r.ok, isFalse); + expect(r.error!.message, contains('too large')); + }); + test('files.ls with a path outside the root is rejected', () async { final r = await call('files.ls', const {'path': '../escape'}); expect(r.ok, isFalse); diff --git a/test/daemon/git_commands_test.dart b/test/daemon/git_commands_test.dart index 6f1cfee1..8cdc30d6 100644 --- a/test/daemon/git_commands_test.dart +++ b/test/daemon/git_commands_test.dart @@ -294,4 +294,45 @@ void main() { final r = await call('git.stage', {'paths': 'new.txt'}); expect(r.ok, isTrue); }); + + test('git.checkout rejects a -prefixed branch (argv-injection guard)', () async { + final r = await call('git.checkout', {'branch': '--upload-pack=evil'}); + expect(r.ok, isFalse); + expect(r.error?.message, contains('branch')); + }); + + test('git.push rejects a -prefixed remote', () async { + final r = await call('git.push', {'remote': '--upload-pack=evil', 'branch': 'main'}); + expect(r.ok, isFalse); + expect(r.error?.message, contains('remote')); + }); + + test('git.push rejects a -prefixed branch', () async { + final r = await call('git.push', {'remote': 'origin', 'branch': '--exec=evil'}); + expect(r.ok, isFalse); + expect(r.error?.message, contains('branch')); + }); + + test('git.log over the count cap fails as userError', () async { + final r = await call('git.log', {'count': 100000}); + expect(r.ok, isFalse); + expect(r.error?.kind, IpcErrorKind.userError); + expect(r.error?.message, contains('exceeds cap')); + }); + + test('git.diff with too many paths fails as userError', () async { + final paths = [for (var i = 0; i < 300; i++) 'file_$i.txt']; + final r = await call('git.diff', {'paths': paths}); + expect(r.ok, isFalse); + expect(r.error?.kind, IpcErrorKind.userError); + expect(r.error?.message, contains('exceeds cap')); + }); + + test('git.stage with too many paths fails as userError', () async { + final paths = [for (var i = 0; i < 300; i++) 'file_$i.txt']; + final r = await call('git.stage', {'paths': paths}); + expect(r.ok, isFalse); + expect(r.error?.kind, IpcErrorKind.userError); + expect(r.error?.message, contains('exceeds cap')); + }); } diff --git a/test/git/operations_test.dart b/test/git/operations_test.dart index 501ca000..2e3ca3dc 100644 --- a/test/git/operations_test.dart +++ b/test/git/operations_test.dart @@ -227,6 +227,48 @@ void main() { } on GitException catch (_) {} }); + test('gitPush rejects a -prefixed remote (argv-injection guard)', () async { + try { + await gitPush(sandbox, remote: '--upload-pack=evil', branch: 'main'); + fail('expected GitException'); + } on GitException catch (e) { + expect(e.message, contains('remote')); + } + }); + + test('gitPush rejects a -prefixed branch', () async { + try { + await gitPush(sandbox, remote: 'origin', branch: '--exec=evil'); + fail('expected GitException'); + } on GitException catch (e) { + expect(e.message, contains('branch')); + } + }); + + test('gitCheckout rejects a -prefixed branch', () async { + try { + await gitCheckout(sandbox, '--upload-pack=evil'); + fail('expected GitException'); + } on GitException catch (e) { + expect(e.message, contains('branch')); + } + }); + + test('gitCheckout rejects an empty branch', () async { + try { + await gitCheckout(sandbox, ''); + fail('expected GitException'); + } on GitException catch (e) { + expect(e.message, contains('branch')); + } + }); + + test('validateGitRef accepts plain refs', () { + expect(() => validateGitRef('main', kind: 'branch'), returnsNormally); + expect(() => validateGitRef('feature/foo', kind: 'branch'), returnsNormally); + expect(() => validateGitRef('origin', kind: 'remote'), returnsNormally); + }); + test('gitPull against no remote throws GitException', () async { try { await gitPull(sandbox);