feat: surface mid-stream parse errors via ParseWithErrors

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

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 }