From d314686cc68f76ae96cf204c836608e9ca1a0c30 Mon Sep 17 00:00:00 2001 From: Kevin Moore Date: Tue, 6 Oct 2026 23:04:53 +0000 Subject: [PATCH] fix: throw FormatException on malformed commit/tree output and add fuzz workflow - Throw `FormatException` in `TreeEntry.fromLsTree` when a line does not match `_lsTreeRegEx` instead of `StateError` from `.single`. - Validate required single headers (`tree`, `author`, `committer`, and `commit` in rev-list mode) and trailing newline in `Commit._parse` via `scanner.error` (`FormatException`) instead of throwing null-check `TypeError`, `StateError`, or `AssertionError`. - Pass `--no-sign` in `test/tag_test.dart` lightweight tag test so global `tag.gpgSign=true` does not force an annotated tag editor prompt. - Add unit test regressions in `test/parse_test.dart`, fuzz harness in `test/fuzz/git_parser_fuzz.dart`, and `.github/workflows/fuzz.yaml`. --- .github/workflows/fuzz.yaml | 16 ++++ CHANGELOG.md | 2 + lib/src/commit.dart | 56 +++++++---- lib/src/tree_entry.dart | 6 +- test/fuzz/git_parser_fuzz.dart | 32 +++++++ test/parse_test.dart | 164 +++++++++++++++++++++++++++++++++ test/tag_test.dart | 1 + 7 files changed, 258 insertions(+), 19 deletions(-) create mode 100644 .github/workflows/fuzz.yaml create mode 100644 test/fuzz/git_parser_fuzz.dart diff --git a/.github/workflows/fuzz.yaml b/.github/workflows/fuzz.yaml new file mode 100644 index 00000000..ec8ac9ca --- /dev/null +++ b/.github/workflows/fuzz.yaml @@ -0,0 +1,16 @@ +name: Fuzz + +on: + push: + branches: [main] + pull_request: + schedule: + - cron: '0 6 * * 1' + +jobs: + fuzz: + uses: kevmoo/fuzz.dart/.github/workflows/fuzz.yaml@main + with: + target: test/fuzz/git_parser_fuzz.dart + instrument-packages: string_scanner,source_span + max-total-time: 30 diff --git a/CHANGELOG.md b/CHANGELOG.md index b886919e..6e61a787 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## 2.3.3-wip +- `Commit.parse`, `Commit.parseRawRevList`, and `TreeEntry.fromLsTree` now + consistently throw `FormatException` on malformed input. - Require Dart 3.9 ## 2.3.2 diff --git a/lib/src/commit.dart b/lib/src/commit.dart index b2096490..2d56e57d 100644 --- a/lib/src/commit.dart +++ b/lib/src/commit.dart @@ -2,6 +2,7 @@ import 'dart:collection'; import 'package:string_scanner/string_scanner.dart'; +import 'top_level.dart'; import 'util.dart'; /// Represents a Git commit object. @@ -73,9 +74,11 @@ class Commit { // at all, or might be empty. scanner.scan(RegExp(r'\r?\n')); - var message = ''; + String? commitSha; + String message; if (isRevParse) { + commitSha = _singleHeader(headers, 'commit', scanner, requireSha: true); final msgLines = []; while (scanner.scan(RegExp(r' ([^\r\n]*)(?:\r?\n|$)'))) { @@ -85,28 +88,27 @@ class Commit { } } - if (msgLines.isNotEmpty) { - message = msgLines.join('\n'); - } + message = msgLines.join('\n'); } else { - message = scanner.rest; + if (headers.containsKey('commit')) { + scanner.error('Unexpected "commit" header.'); + } + final rest = scanner.rest; scanner.position = scanner.string.length; - assert(message.endsWith('\n')); - final originalMessageLength = message.length; - message = message.trim(); - // message should be trimmed by git, so the only diff after trim - // should be 1 character - the removed new line - assert(message.length + 1 == originalMessageLength); + if (!rest.endsWith('\n')) { + scanner.error('Commit message must end with a newline.'); + } + message = rest.replaceFirst(RegExp(r'\r?\n$'), ''); } - final treeSha = headers['tree']!.single; - final author = headers['author']!.single; - final committer = headers['committer']!.single; - final commitSha = headers.containsKey('commit') - ? headers['commit']!.single - : null; + final treeSha = _singleHeader(headers, 'tree', scanner, requireSha: true); + final author = _singleHeader(headers, 'author', scanner); + final committer = _singleHeader(headers, 'committer', scanner); final parents = headers['parent'] ?? []; + if (!parents.every(isValidSha)) { + scanner.error('Invalid SHA1 value in "parent" header.'); + } final endSpot = scanner.position; @@ -117,4 +119,24 @@ class Commit { commit: Commit._(treeSha, author, committer, message, content, parents), ); } + + static String _singleHeader( + Map> headers, + String name, + StringScanner scanner, { + bool requireSha = false, + }) { + final values = headers[name]; + if (values == null || values.isEmpty) { + scanner.error('Missing required "$name" header.'); + } + if (values.length > 1) { + scanner.error('Duplicate "$name" header.'); + } + final value = values.single; + if (requireSha && !isValidSha(value)) { + scanner.error('Invalid SHA1 value in "$name" header: "$value".'); + } + return value; + } } diff --git a/lib/src/tree_entry.dart b/lib/src/tree_entry.dart index b9ca9c2a..d78cd624 100644 --- a/lib/src/tree_entry.dart +++ b/lib/src/tree_entry.dart @@ -33,8 +33,10 @@ class TreeEntry { } factory TreeEntry.fromLsTree(String value) { - // TODO: should catch and re-throw a descriptive error - final match = _lsTreeRegEx.allMatches(value).single; + final match = _lsTreeRegEx.firstMatch(value); + if (match == null) { + throw FormatException('Could not parse ls-tree line.', value); + } return TreeEntry(match[1]!, match[2]!, match[3]!, match[4]!); } diff --git a/test/fuzz/git_parser_fuzz.dart b/test/fuzz/git_parser_fuzz.dart new file mode 100644 index 00000000..edf9cb8f --- /dev/null +++ b/test/fuzz/git_parser_fuzz.dart @@ -0,0 +1,32 @@ +import 'dart:convert'; +import 'dart:typed_data'; + +import 'package:git/git.dart'; + +void fuzzTarget(Uint8List bytes) { + final input = utf8.decode(bytes, allowMalformed: true); + + try { + Commit.parse(input); + } on FormatException { + // Expected on malformed commit object. + } + + try { + Commit.parseRawRevList(input); + } on FormatException { + // Expected on malformed rev-list output. + } + + try { + TreeEntry.fromLsTree(input); + } on FormatException { + // Expected on malformed ls-tree line. + } + + try { + TreeEntry.fromLsTreeOutput(input); + } on FormatException { + // Expected on malformed ls-tree output. + } +} diff --git a/test/parse_test.dart b/test/parse_test.dart index 64f17542..d7181422 100644 --- a/test/parse_test.dart +++ b/test/parse_test.dart @@ -50,6 +50,170 @@ bcd1284d805951a16e765cea5b2273a464ee2d86''', check(result.first).has((e) => e.name, 'name').equals('.gitignore'); check(result).every((e) => e.isNotNull()); }); + + group('TreeEntry.fromLsTree malformed input', () { + for (final invalid in [ + '', + '\n', + 'invalid', + '100644 invalid bcd1284d805951a16e765cea5b2273a464ee2d86\tfile.txt', + '100644 blob invalidsha\tfile.txt', + '100644 blob bcd1284d805951a16e765cea5b2273a464ee2d86\t', + ]) { + test('throws FormatException on "$invalid"', () { + check(() => TreeEntry.fromLsTree(invalid)).throws(); + }); + } + + test('fromLsTreeOutput throws FormatException on blank line', () { + check(() => TreeEntry.fromLsTreeOutput('\n\n')).throws(); + }); + }); + + group('Commit.parse and Commit.parseRawRevList', () { + const validSha = 'bcd1284d805951a16e765cea5b2273a464ee2d86'; + + test('Commit.parse parses valid commit and preserves whitespace', () { + final commit = Commit.parse( + 'tree $validSha\n' + 'author Alice 1700000000 +0000\n' + 'committer Bob 1700000000 +0000\n' + '\n' + ' leading and trailing whitespace \n', + ); + check(commit.treeSha).equals(validSha); + check(commit.message).equals(' leading and trailing whitespace '); + }); + + test('Commit.parse throws FormatException on missing headers', () { + check(() => Commit.parse('')).throws(); + check( + () => Commit.parse( + 'author Alice \ncommitter Bob \n\nmsg\n', + ), + ).throws(); + check( + () => Commit.parse('tree $validSha\ncommitter Bob \n\nmsg\n'), + ).throws(); + check( + () => Commit.parse('tree $validSha\nauthor Alice \n\nmsg\n'), + ).throws(); + }); + + test('Commit.parse throws FormatException on duplicate headers', () { + check( + () => Commit.parse( + 'tree $validSha\n' + 'tree $validSha\n' + 'author Alice \n' + 'committer Bob \n\n' + 'msg\n', + ), + ).throws(); + check( + () => Commit.parse( + 'tree $validSha\n' + 'author Alice \n' + 'author Alice \n' + 'committer Bob \n\n' + 'msg\n', + ), + ).throws(); + check( + () => Commit.parse( + 'tree $validSha\n' + 'author Alice \n' + 'committer Bob \n' + 'committer Bob \n\n' + 'msg\n', + ), + ).throws(); + }); + + test('Commit.parse throws FormatException on unexpected commit header', () { + check( + () => Commit.parse( + 'commit $validSha\n' + 'tree $validSha\n' + 'author Alice \n' + 'committer Bob \n\n' + 'msg\n', + ), + ).throws(); + }); + + test('Commit.parse throws FormatException on missing trailing newline', () { + check( + () => Commit.parse( + 'tree $validSha\n' + 'author Alice \n' + 'committer Bob \n\n' + 'msg without trailing newline', + ), + ).throws(); + }); + + test( + 'Commit.parseRawRevList throws FormatException on missing/dupe headers', + () { + check( + () => Commit.parseRawRevList( + 'tree $validSha\n' + 'author Alice \n' + 'committer Bob \n\n' + ' msg\n', + ), + ).throws(); + check( + () => Commit.parseRawRevList( + 'commit $validSha\n' + 'commit $validSha\n' + 'tree $validSha\n' + 'author Alice \n' + 'committer Bob \n\n' + ' msg\n', + ), + ).throws(); + check( + () => Commit.parseRawRevList( + 'commit $validSha\n' + 'author Alice \n' + 'committer Bob \n\n' + ' msg\n', + ), + ).throws(); + }, + ); + + test('throws FormatException on invalid SHA-1 in headers', () { + check( + () => Commit.parse( + 'tree not-a-sha\n' + 'author Alice \n' + 'committer Bob \n\n' + 'msg\n', + ), + ).throws(); + check( + () => Commit.parse( + 'tree $validSha\n' + 'parent not-a-sha\n' + 'author Alice \n' + 'committer Bob \n\n' + 'msg\n', + ), + ).throws(); + check( + () => Commit.parseRawRevList( + 'commit not-a-sha\n' + 'tree $validSha\n' + 'author Alice \n' + 'committer Bob \n\n' + ' msg\n', + ), + ).throws(); + }); + }); } const _showRefOutput = '''ff1c31c454c4128a98dcd610d203820eeeb91923 HEAD diff --git a/test/tag_test.dart b/test/tag_test.dart index a040744f..8d1936b7 100644 --- a/test/tag_test.dart +++ b/test/tag_test.dart @@ -15,6 +15,7 @@ void main() { await runGit([ 'tag', + '--no-sign', givenTagName, branchRef.sha, ], processWorkingDir: testDir.path);