From 5de80e542e3a846290ba65d765edbef2ad5a884a Mon Sep 17 00:00:00 2001 From: Vaibhav Srivastava Date: Tue, 25 Aug 2026 21:45:00 +0530 Subject: [PATCH] feat: surface mid-stream parse errors via ParseWithErrors Parse discarded the error when a fragment failed to parse and let the deferred close(out) end the stream, so an abandoned parse was indistinguishable from a well-formed patch that simply ended. Consumers scanning patch content therefore reported success over the part they had managed to read (gitleaks#1338). Add ParseWithErrors returning a buffered, always-closed error channel carrying the error that ended the parse. Parse delegates to it and drains the channel, so its contract is unchanged and the parse goroutine is never left blocked on an unread send. Refs gitleaks/gitleaks#1338 --- gitdiff/parser.go | 47 +++++++++-- gitdiff/parser_errors_test.go | 146 ++++++++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 6 deletions(-) create mode 100644 gitdiff/parser_errors_test.go diff --git a/gitdiff/parser.go b/gitdiff/parser.go index 5ffa9bd..48eb495 100644 --- a/gitdiff/parser.go +++ b/gitdiff/parser.go @@ -15,20 +15,54 @@ const commitPrefix = "commit" // Parse parses a patch with changes to one or more files. Any content before // the first file is returned as the second value. If an error occurs while // parsing, it returns all files parsed before the error. +// +// A fragment that fails to parse ends the stream silently: the returned +// channel closes exactly as it would at the end of a well-formed patch, so a +// caller ranging over it cannot tell a complete patch from one abandoned +// part-way through. Callers that need that distinction should use +// [ParseWithErrors]. func Parse(r io.Reader) (<-chan *File, error) { + files, errs := ParseWithErrors(r) + // Drained so the parse goroutine is never left blocked on an unread + // send; Parse's contract is unchanged, errors and all. + go func() { + for range errs { + } + }() + return files, nil +} + +// ParseWithErrors is [Parse] with the mid-stream parse error surfaced. +// +// The returned error channel is buffered and yields at most one value: the +// error that ended the parse, if one did. It is closed when parsing finishes, +// so a caller that ranges over the file channel and then reads the error +// channel sees nil for a patch that ended normally. +// +// This exists because the alternative is silence. A fragment that fails to +// parse -- a malformed hunk header, say -- previously stopped the parse and +// closed the file channel with no signal, which reads to a caller as a patch +// that simply ended. Consumers that scan patch content therefore reported +// success over the part they managed to read (see gitleaks#1338), which is the +// worst shape of failure: nothing surfaces, and the answer is the reassuring +// one. +func ParseWithErrors(r io.Reader) (<-chan *File, <-chan error) { p := newParser(r) out := make(chan *File) + errCh := make(chan error, 1) if err := p.Next(); err != nil { close(out) - if err == io.EOF { - return out, nil + if err != io.EOF { + errCh <- err } - return out, err + close(errCh) + return out, errCh } - go func(out chan *File, r io.Reader) { + go func(out chan *File, errCh chan error, r io.Reader) { defer close(out) + defer close(errCh) ph := &PatchHeader{} for { @@ -55,6 +89,7 @@ func Parse(r io.Reader) (<-chan *File, error) { } { n, err := fn(file) if err != nil { + errCh <- err return } if n > 0 { @@ -65,9 +100,9 @@ func Parse(r io.Reader) (<-chan *File, error) { file.PatchHeader = ph out <- file } - }(out, r) + }(out, errCh, r) - return out, nil + return out, errCh } // TODO(bkeyes): consider exporting the parser type with configuration diff --git a/gitdiff/parser_errors_test.go b/gitdiff/parser_errors_test.go new file mode 100644 index 0000000..3e22055 --- /dev/null +++ b/gitdiff/parser_errors_test.go @@ -0,0 +1,146 @@ +package gitdiff + +import ( + "io" + "strings" + "testing" + "time" +) + +// A hunk header whose old-line count is 2^64-1. parseRange fails on it, which +// is the report behind gitleaks#1338. +const malformedHunkPatch = `diff --git a/a.txt b/a.txt +index 1111111..2222222 100644 +--- a/a.txt ++++ b/a.txt +@@ -231,18446744073709551615 +231,1 @@ ++secret = "AKIAIOSFODNN7EXAMPLE" +` + +const wellFormedPatch = `diff --git a/a.txt b/a.txt +index 1111111..2222222 100644 +--- a/a.txt ++++ b/a.txt +@@ -1,0 +1,1 @@ ++hello = "world" +` + +// collect drains both channels and returns the files seen and the final error. +func collect(t *testing.T, patch string) ([]*File, error) { + t.Helper() + + files, errs := ParseWithErrors(strings.NewReader(patch)) + var seen []*File + for f := range files { + seen = append(seen, f) + } + select { + case err := <-errs: + return seen, err + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for the error channel; it must close when parsing ends") + return nil, nil + } +} + +// TestParseWithErrorsSurfacesAMalformedHunkHeader is the whole point: without +// it the channel closes exactly as it would at the end of a good patch, and a +// caller reports success over the part it managed to read. +func TestParseWithErrorsSurfacesAMalformedHunkHeader(t *testing.T) { + _, err := collect(t, malformedHunkPatch) + + if err == nil { + t.Fatal("a patch that could not be parsed reported no error; truncation is indistinguishable from a clean end") + } +} + +// TestParseWithErrorsReportsNoErrorForAGoodPatch is the pairing. A signal that +// fires on everything is not a signal, and here it would make every ordinary +// patch look truncated. +func TestParseWithErrorsReportsNoErrorForAGoodPatch(t *testing.T) { + files, err := collect(t, wellFormedPatch) + + if err != nil { + t.Fatalf("well-formed patch reported an error: %v", err) + } + if len(files) != 1 { + t.Fatalf("expected 1 file, got %d", len(files)) + } +} + +func TestParseWithErrorsOnEmptyInput(t *testing.T) { + files, err := collect(t, "") + + if err != nil { + t.Fatalf("empty input reported an error: %v", err) + } + if len(files) != 0 { + t.Fatalf("expected no files, got %d", len(files)) + } +} + +// TestParseWithErrorsClosesTheErrorChannel keeps the contract usable: a caller +// that reads the error channel after draining files must not block when the +// patch was fine. +func TestParseWithErrorsClosesTheErrorChannel(t *testing.T) { + files, errs := ParseWithErrors(strings.NewReader(wellFormedPatch)) + for range files { + } + + select { + case _, open := <-errs: + if open { + // A value is fine; draining again must then see it closed. + if _, stillOpen := <-errs; stillOpen { + t.Fatal("error channel yielded more than one value") + } + } + case <-time.After(5 * time.Second): + t.Fatal("error channel neither yielded nor closed") + } +} + +// TestParseKeepsItsOldContract guards the compatibility half: Parse must still +// return files and no error, and must not leave the parse goroutine blocked +// sending an error nobody reads. +func TestParseKeepsItsOldContract(t *testing.T) { + for name, patch := range map[string]string{ + "well-formed": wellFormedPatch, + "malformed": malformedHunkPatch, + } { + t.Run(name, func(t *testing.T) { + files, err := Parse(strings.NewReader(patch)) + if err != nil { + t.Fatalf("Parse returned an immediate error: %v", err) + } + done := make(chan struct{}) + go func() { + for range files { + } + close(done) + }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("Parse's file channel never closed; the parse goroutine is stuck") + } + }) + } +} + +func TestParseWithErrorsPropagatesAReadFailure(t *testing.T) { + _, errs := ParseWithErrors(failingReader{}) + + select { + case err := <-errs: + if err == nil { + t.Fatal("a failing reader produced no error") + } + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for a read failure to surface") + } +} + +type failingReader struct{} + +func (failingReader) Read([]byte) (int, error) { return 0, io.ErrUnexpectedEOF }