harden IPC: reject -prefixed git refs, cap files.read / git.log (T-104)
test / unit + widget + golden + a11y (push) Failing after 31s
test / integration_test (xvfb) (push) Has been skipped
test / bundle smoke (xvfb 5s) (push) Has been skipped
test / daemon subprocess + web WASM smoke (push) Has been skipped
test / dart doc (lib API) (push) Failing after 1m0s
test / unit + widget + golden + a11y (push) Failing after 31s
test / integration_test (xvfb) (push) Has been skipped
test / bundle smoke (xvfb 5s) (push) Has been skipped
test / daemon subprocess + web WASM smoke (push) Has been skipped
test / dart doc (lib API) (push) Failing after 1m0s
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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});
|
||||
});
|
||||
|
||||
@@ -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<String> _pathList(Object? raw) {
|
||||
return const [];
|
||||
}
|
||||
|
||||
IpcResponse? _tooManyPaths(String id, List<String> 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',
|
||||
|
||||
+10
-1
@@ -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<String> 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<void> 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);
|
||||
}
|
||||
|
||||
@@ -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<String> 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<List<({String name, bool current})>> gitBranches(Directory workDir) async
|
||||
}
|
||||
|
||||
/// Checkout a branch.
|
||||
///
|
||||
/// `git checkout` overloads positionals: `-- <name>` means "restore
|
||||
/// pathspec `<name>`", not "checkout branch `<name>`". 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<void> gitCheckout(Directory workDir, String branch) async {
|
||||
validateGitRef(branch, kind: 'branch');
|
||||
final r = await Process.run(
|
||||
gitBin,
|
||||
['checkout', branch],
|
||||
|
||||
Reference in New Issue
Block a user