Skip to content

fix: Support for more kinds of binary headers #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 19, 2022
Merged

fix: Support for more kinds of binary headers #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion gitdiff/binary.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@ import (
"fmt"
"io"
"io/ioutil"
"regexp"
"strconv"
"strings"
)

var binaryRegexp = regexp.MustCompile(`^Binary files (/dev/null|a/(.+)|"a/(.+)") and (/dev/null|b/(.+)|"b/(.+)") differ\s*$`)

func (p *parser) ParseBinaryFragments(f *File) (n int, err error) {
isBinary, hasData, err := p.ParseBinaryMarker()
if err != nil || !isBinary {
Expand Down Expand Up @@ -56,7 +59,9 @@ func (p *parser) ParseBinaryMarker() (isBinary bool, hasData bool, err error) {
case "Binary files differ\n":
case "Files differ\n":
default:
return false, false, nil
if !binaryRegexp.MatchString(p.Line(0)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should optimize this to strings.Contains(p.Line(0), "Binary files") instead of using a regex. Gitleaks really shouldn't be inspecting any binary data.

Or perhaps maybe func (p *parser) ParseBinaryFragments(f *File) (n int, err error) { should always return false, false, nil.

WDYT @k0ral?

@weineran might you have thoughts on this?

Copy link
Author

@k0ral k0ral Feb 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should optimize this to strings.Contains(p.Line(0), "Binary files")

This sounds quite lax to me, how confident are we that the string "Binary files" can't appear anywhere else in a diff ?
Also, I realize now that MatchString is actually a "contains" rather than a "exactly matches", so for optimization sake, I guess we should enclose the pattern with ^$ to allow an early return, shouldn't we ?

Or perhaps maybe func (p *parser) ParseBinaryFragments(f *File) (n int, err error) { should always return false, false, nil.

Not sure I understand your proposal, as types don't match in your sentence :) . Did you mean:

  • ParseBinaryMarker should always return false, false, nil ?
  • or ParseBinaryFragments should always return 0, nil ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParseBinaryMarker should always return false, false, nil ?

Whoops, this is what I meant. Good catch 😉

I guess we should enclose the pattern with ^$ to allow an early return, shouldn't we ?

That makes sense to me

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If parseBinaryMarker always returned false, false, nil, then:

  • f.IsBinary would always be false
  • ... which would essentially treat all files as non-binary
  • ... which would be incorrect, right ? :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're catching me before having some caffeine in my system! ☕

If parseBinaryMarker always returned false, false, nil, then:
... which would be incorrect, right ? :)

You are right, that would be incorrect. What we are aiming for is if a binary file has been detected, treat that file as if it has no data. In other words, gitleaks should never scan binary files (for now). However, that logic might be better for another PR as this PR is a fix.

I think once I guess we should enclose the pattern with ^$ to allow an early return, shouldn't we ? has been added these changes look good to me.

Copy link
Author

@k0ral k0ral Feb 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull-request has been updated to add ^$.

return false, false, nil
}
}

if err = p.Next(); err != nil && err != io.EOF {
Expand Down
20 changes: 20 additions & 0 deletions gitdiff/binary_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,26 @@ func TestParseBinaryMarker(t *testing.T) {
IsBinary: false,
HasData: false,
},
"binaryPatchCreated": {
Input: "Binary files /dev/null and b/path/to/file.ext differ\n",
IsBinary: true,
HasData: false,
},
"binaryPatchModified": {
Input: "Binary files a/path/to/file.ext and b/path/to/file.ext differ\n",
IsBinary: true,
HasData: false,
},
"binaryPatchModifiedQuoted": {
Input: "Binary files \"a/path/to/file.ext\" and \"b/path/to/file.ext\" differ\n",
IsBinary: true,
HasData: false,
},
"binaryPatchDeleted": {
Input: "Binary files a/path/to/file.ext and /dev/null differ\n",
IsBinary: true,
HasData: false,
},
}

for name, test := range tests {
Expand Down