fix: Support for more kinds of binary headers #1
Conversation
k0ral
force-pushed
the
master
branch
2 times, most recently
from
abac1eb
to
b92baca
Compare
| @@ -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)) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 alwaysreturn false, false, nil.
Not sure I understand your proposal, as types don't match in your sentence :) . Did you mean:
ParseBinaryMarkershould always returnfalse, false, nil?- or
ParseBinaryFragmentsshould always return0, nil?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
If parseBinaryMarker always returned false, false, nil, then:
f.IsBinarywould always befalse- ... which would essentially treat all files as non-binary
- ... which would be incorrect, right ? :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull-request has been updated to add ^$.
|
LGTM now. Thanks for the contribution @k0ral! |
* wait the zombies * close the reader * defer order * partially update tests
On my side,
git logproduces the following output for binary files:(Example taken from celery/celery@d6e4683.)
go-gitdiffcurrently fails to parse such headers. This change is meant to fix that.