From 682083cb122a8f60ba23183299d177d84b954582 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 00:09:05 -0700 Subject: [PATCH 1/3] fix: hash a hunk past the scanner's line limit bufio.Scanner refuses a token longer than 64KB. hunkHash never checked Err(), so on a longer line it hashed only what it had read and stopped. Two hunks differing solely beyond that point therefore hashed alike, and a hunk matching the approval-time diff is dropped as already reviewed -- so an approval could survive a change nobody saw. hunkBlocks does check Err() and declines the hunk, which is why this only shows up here. Split the body by hand instead. There is no limit to exceed, so the failure mode is gone rather than pushed further out, and trailing carriage returns are dropped so a CRLF file hashes like any other. Coverage badge regenerated. --- README.md | 2 +- internal/git/diff.go | 16 +++++++++++----- internal/git/diff_test.go | 28 +++++++++++++++++++++++++++- 3 files changed, 39 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 90857ad..312f015 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-82.6%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-82.7%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) diff --git a/internal/git/diff.go b/internal/git/diff.go index 95c74c0..ba244ee 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -1,7 +1,6 @@ package git import ( - "bufio" "bytes" "crypto/sha256" "fmt" @@ -231,6 +230,8 @@ func getGitDiff(data DiffContext, executor gitCommandExecutor) ([]*diff.FileDiff func hunkHash(hunk *diff.Hunk) [32]byte { // Generate a hash for a hunk based on its added and removed lines. + // Split by hand: bufio.Scanner refuses a token past 64KB and stops there, so + // hashing what it read lets two hunks differing only beyond that collide. var lines []byte data := hunk.Body @@ -238,10 +239,15 @@ func hunkHash(hunk *diff.Hunk) [32]byte { return sha256.Sum256(nil) } - scanner := bufio.NewScanner(bytes.NewReader(data)) - - for scanner.Scan() { - line := scanner.Text() + for len(data) > 0 { + line := data + if i := bytes.IndexByte(data, '\n'); i >= 0 { + line, data = data[:i], data[i+1:] + } else { + data = nil + } + // Trailing carriage returns belong to the line ending, not the content. + line = bytes.TrimSuffix(line, []byte("\r")) if len(line) == 0 { continue } diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index 59a2244..a01a201 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "strings" "testing" "github.com/multimediallc/codeowners-plus/pkg/codeowners" @@ -129,7 +130,7 @@ Binary files a/assets/img/offline.png and b/assets/img/offline.png differ`, expectedErr: false, expectedFiles: 2, expectedHunks: map[string]int{ - "file1.go": 1, + "file1.go": 1, "assets/img/offline.png": 0, }, }, @@ -806,3 +807,28 @@ func TestDiffOfDiffs(t *testing.T) { } } } + +// A hunk matching the approval-time diff is dropped as already reviewed, so a +// collision past the old 64KB read limit retained an approval over unseen change. +func TestHunkHashReadsLinesPastScannerLimit(t *testing.T) { + long := strings.Repeat("x", 70*1024) + first := &diff.Hunk{Body: []byte("+keep()\n+" + long + "A")} + second := &diff.Hunk{Body: []byte("+keep()\n+" + long + "B")} + + if hunkHash(first) == hunkHash(second) { + t.Error("hunks differing past 64KB must not hash alike") + } + same := &diff.Hunk{Body: []byte("+keep()\n+" + long + "A")} + if hunkHash(first) != hunkHash(same) { + t.Error("identical over-long hunks must still hash alike") + } + // Context lines stay excluded however long the hunk is. + withContext := &diff.Hunk{Body: []byte(" ctx\n+keep()\n+" + long + "A")} + if hunkHash(first) != hunkHash(withContext) { + t.Error("context lines must not enter the hash") + } + crlf := &diff.Hunk{Body: []byte("+keep()\r\n+" + long + "A")} + if hunkHash(first) != hunkHash(crlf) { + t.Error("a carriage return in the line ending must not change the hash") + } +} From 2eef233381a91d766fbdb077bc48ad435b4b0355 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 12:29:48 -0700 Subject: [PATCH 2/3] fix: keep a carriage return that is content, not a line ending Review catch: the carriage return was stripped from every line, including an unterminated final line where the CR is part of the content rather than a terminator. A hunk ending "+value\r" therefore hashed the same as one ending "+value", so it could be subtracted as already reviewed. That is the same fail-open direction this PR set out to close. Strip the CR only where it precedes a newline. A CRLF file still hashes like its LF twin, and the new test goes red without the change. --- internal/git/diff.go | 6 +++--- internal/git/diff_test.go | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/internal/git/diff.go b/internal/git/diff.go index ba244ee..fab7ca2 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -242,12 +242,12 @@ func hunkHash(hunk *diff.Hunk) [32]byte { for len(data) > 0 { line := data if i := bytes.IndexByte(data, '\n'); i >= 0 { - line, data = data[:i], data[i+1:] + // Only a CR before a newline is a line ending; an unterminated line + // ending in CR carries it as content. + line, data = bytes.TrimSuffix(data[:i], []byte("\r")), data[i+1:] } else { data = nil } - // Trailing carriage returns belong to the line ending, not the content. - line = bytes.TrimSuffix(line, []byte("\r")) if len(line) == 0 { continue } diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index a01a201..0c45186 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -810,6 +810,20 @@ func TestDiffOfDiffs(t *testing.T) { // A hunk matching the approval-time diff is dropped as already reviewed, so a // collision past the old 64KB read limit retained an approval over unseen change. +func TestHunkHashKeepsAnUnterminatedCarriageReturn(t *testing.T) { + withCR := &diff.Hunk{Body: []byte("+keep()\n+value\r")} + without := &diff.Hunk{Body: []byte("+keep()\n+value")} + if hunkHash(withCR) == hunkHash(without) { + t.Error("a CR that is content, not a line ending, must change the hash") + } + + crlf := &diff.Hunk{Body: []byte("+keep()\r\n+value\r\n")} + lf := &diff.Hunk{Body: []byte("+keep()\n+value\n")} + if hunkHash(crlf) != hunkHash(lf) { + t.Error("a CRLF file must still hash like its LF twin") + } +} + func TestHunkHashReadsLinesPastScannerLimit(t *testing.T) { long := strings.Repeat("x", 70*1024) first := &diff.Hunk{Body: []byte("+keep()\n+" + long + "A")} From 96c77b339df15343af38dea319c96d4b2c520649 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 13:50:35 -0700 Subject: [PATCH 3/3] fix: separate the lines a hunk hash is built from Review catch, and the same fail-open class as the rest of this PR. The kept lines were concatenated with nothing between them, so two lines hashed as their joined form: "+total = x\n+y\n" == "+total = x+y\n" "+foo\n-bar\n" == "+foo-bar\n" Any added line containing a plus or minus collides with the two-line hunk split at that character, and the author controls both sides of the diff. Write a separator after each line. Two more while in here. The "\ No newline at end of file" marker was skipped as a context line, so a hunk that only gained a trailing newline hashed into the approval set; it now counts. And the accumulation buffer had no bound once the scanner went away, so the lines stream into the digest instead: 1 allocation rather than a buffer that grows with the hunk. The two standalone tests are now rows in the existing table, which already had the right shape, and the three collisions above are rows too. Each new row fails without its fix. --- internal/git/diff.go | 44 +++++++++++----------- internal/git/diff_test.go | 78 +++++++++++++++++++++------------------ 2 files changed, 63 insertions(+), 59 deletions(-) diff --git a/internal/git/diff.go b/internal/git/diff.go index fab7ca2..045862d 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -228,36 +228,34 @@ func getGitDiff(data DiffContext, executor gitCommandExecutor) ([]*diff.FileDiff return gitDiff, nil } -func hunkHash(hunk *diff.Hunk) [32]byte { - // Generate a hash for a hunk based on its added and removed lines. - // Split by hand: bufio.Scanner refuses a token past 64KB and stops there, so - // hashing what it read lets two hunks differing only beyond that collide. - var lines []byte - data := hunk.Body +const ( + addedLine = '+' + removedLine = '-' + noNewlineAtEOF = '\\' +) - if len(data) == 0 { - return sha256.Sum256(nil) +func splitLine(data []byte) (line, rest []byte) { + i := bytes.IndexByte(data, '\n') + if i < 0 { + return data, nil } + return bytes.TrimSuffix(data[:i], []byte("\r")), data[i+1:] +} - for len(data) > 0 { - line := data - if i := bytes.IndexByte(data, '\n'); i >= 0 { - // Only a CR before a newline is a line ending; an unterminated line - // ending in CR carries it as content. - line, data = bytes.TrimSuffix(data[:i], []byte("\r")), data[i+1:] - } else { - data = nil - } +func hunkHash(hunk *diff.Hunk) [32]byte { + sum := sha256.New() + var line []byte + + for data := hunk.Body; len(data) > 0; { + line, data = splitLine(data) if len(line) == 0 { continue } switch line[0] { - case '+', '-': - // Include the line type and content - lines = append(lines, line...) - default: - // Skip context lines + case addedLine, removedLine, noNewlineAtEOF: + _, _ = sum.Write(line) + _, _ = sum.Write([]byte{'\n'}) } } - return sha256.Sum256(lines) + return [32]byte(sum.Sum(nil)) } diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index 0c45186..4b7d711 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -388,6 +388,48 @@ func TestHunkHash(t *testing.T) { hunk2Body: []byte(``), expectedSame: true, }, + { + name: "two lines vs their concatenation", + hunkBody: []byte("+total = x\n+y\n"), + hunk2Body: []byte("+total = x+y\n"), + expectedSame: false, + }, + { + name: "added then removed vs one joined line", + hunkBody: []byte("+foo\n-bar\n"), + hunk2Body: []byte("+foo-bar\n"), + expectedSame: false, + }, + { + name: "a trailing newline is a change", + hunkBody: []byte("+value\n\\ No newline at end of file\n"), + hunk2Body: []byte("+value\n"), + expectedSame: false, + }, + { + name: "differ only past the old 64KB scanner limit", + hunkBody: []byte("+keep()\n+" + strings.Repeat("x", 70*1024) + "A"), + hunk2Body: []byte("+keep()\n+" + strings.Repeat("x", 70*1024) + "B"), + expectedSame: false, + }, + { + name: "identical over-long hunks", + hunkBody: []byte("+keep()\n+" + strings.Repeat("x", 70*1024) + "A"), + hunk2Body: []byte("+keep()\n+" + strings.Repeat("x", 70*1024) + "A"), + expectedSame: true, + }, + { + name: "an unterminated CR is content", + hunkBody: []byte("+keep()\n+value\r"), + hunk2Body: []byte("+keep()\n+value"), + expectedSame: false, + }, + { + name: "a CRLF file hashes like its LF twin", + hunkBody: []byte("+keep()\r\n+value\r\n"), + hunk2Body: []byte("+keep()\n+value\n"), + expectedSame: true, + }, } for _, tc := range tt { @@ -810,39 +852,3 @@ func TestDiffOfDiffs(t *testing.T) { // A hunk matching the approval-time diff is dropped as already reviewed, so a // collision past the old 64KB read limit retained an approval over unseen change. -func TestHunkHashKeepsAnUnterminatedCarriageReturn(t *testing.T) { - withCR := &diff.Hunk{Body: []byte("+keep()\n+value\r")} - without := &diff.Hunk{Body: []byte("+keep()\n+value")} - if hunkHash(withCR) == hunkHash(without) { - t.Error("a CR that is content, not a line ending, must change the hash") - } - - crlf := &diff.Hunk{Body: []byte("+keep()\r\n+value\r\n")} - lf := &diff.Hunk{Body: []byte("+keep()\n+value\n")} - if hunkHash(crlf) != hunkHash(lf) { - t.Error("a CRLF file must still hash like its LF twin") - } -} - -func TestHunkHashReadsLinesPastScannerLimit(t *testing.T) { - long := strings.Repeat("x", 70*1024) - first := &diff.Hunk{Body: []byte("+keep()\n+" + long + "A")} - second := &diff.Hunk{Body: []byte("+keep()\n+" + long + "B")} - - if hunkHash(first) == hunkHash(second) { - t.Error("hunks differing past 64KB must not hash alike") - } - same := &diff.Hunk{Body: []byte("+keep()\n+" + long + "A")} - if hunkHash(first) != hunkHash(same) { - t.Error("identical over-long hunks must still hash alike") - } - // Context lines stay excluded however long the hunk is. - withContext := &diff.Hunk{Body: []byte(" ctx\n+keep()\n+" + long + "A")} - if hunkHash(first) != hunkHash(withContext) { - t.Error("context lines must not enter the hash") - } - crlf := &diff.Hunk{Body: []byte("+keep()\r\n+" + long + "A")} - if hunkHash(first) != hunkHash(crlf) { - t.Error("a carriage return in the line ending must not change the hash") - } -}