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 }