Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 18 additions & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
"strings"
"sync"
"syscall"

"time"
"github.com/alecthomas/kingpin/v2"
"github.com/fatih/color"
"github.com/felixge/fgprof"
Expand Down Expand Up @@ -61,6 +61,7 @@ var (
results = cli.Flag("results", "Specifies which type(s) of results to output: verified (confirmed valid by API), unknown (verification failed due to error), unverified (detected but not verified), filtered_unverified (unverified but would have been filtered out). Defaults to verified,unverified,unknown.").String()
noColor = cli.Flag("no-color", "Disable colorized output").Bool()
noColour = cli.Flag("no-colour", "Alias for --no-color").Hidden().Bool()
logSync func() error //Package-level variable for sync function
Copy link
Contributor

Choose a reason for hiding this comment

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

Burying this var amongst all the cli.Flag vars that surround it is going to make it far more difficult to find than it needs to be. Can you put it either at the beginning or the end? I think it would be ok to take it out of the grouped declaration entirely (declaring it using its own var keyword) if gofmt lets you do that.

(That being said, I don't think you need to use a package var for this - read my other comments for details.)


allowVerificationOverlap = cli.Flag("allow-verification-overlap", "Allow verification of similar credentials across detectors").Bool()
filterUnverified = cli.Flag("filter-unverified", "Only output first unverified result per chunk per detector if there are more than one results.").Bool()
Expand Down Expand Up @@ -354,6 +355,7 @@ func main() {
logFormat = log.WithJSONSink
}
logger, sync := log.New("trufflehog", logFormat(os.Stderr, log.WithGlobalRedaction()))
logSync = sync //
// make it the default logger for contexts
context.SetDefaultLogger(logger)

Expand Down Expand Up @@ -413,6 +415,21 @@ func run(state overseer.State) {
} else {
logger.Info("cleaned temporary artifacts")
}

// Flush logs with timeout to prevent hanging
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this, because sync is already called: It's deferred on line 367 of this file. If that call is hanging, then the hang should be fixed there, not fixed by putting a timer on an os.Exit here. Or, that call should be removed entirely, so that this code is the only sync code. And if you do that, is there a way to pass the sync function through the overseer state, rather than relying on a package var? Mutable globals (like package vars) make the code more difficult to understand and maintain because it obscures entanglement.

if logSync != nil {
done := make(chan struct{})
go func() {
_ = logSync()
close(done)
}()

select {
case <-done:
case <-time.After(100 * time.Millisecond):
Copy link

Choose a reason for hiding this comment

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

Magic number for log flush timeout

Low Severity

The 100 * time.Millisecond log flush timeout is a magic number used inline. In contrast, the related PR changes in gitparse.go define similar timeout values as named constants with descriptive comments (e.g., defaultWaitDelay = 5 * time.Second). For consistency and maintainability, this timeout value would benefit from being a named constant with documentation explaining why 100ms was chosen.

Fix in Cursor Fix in Web

logger.Info("Log flush timed out, exiting")
}
}
os.Exit(0)
}()

Expand Down
27 changes: 22 additions & 5 deletions pkg/gitparse/gitparse.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ const (

// defaultMaxCommitSize is the maximum size for a commit. Larger commits will be cut off.
defaultMaxCommitSize int64 = 2 * 1024 * 1024 * 1024 // 2GB

// defaultWaitDelay is the default time to wait after context cancellation before forcefully killing git processes.
defaultWaitDelay = 5 * time.Second
)

// contentWriter defines a common interface for writing, reading, and managing diff content.
Expand Down Expand Up @@ -124,6 +127,7 @@ type Parser struct {
maxDiffSize int64
maxCommitSize int64
dateFormat string
waitDelay time.Duration

useCustomContentWriter bool
}
Expand Down Expand Up @@ -204,6 +208,14 @@ func WithMaxCommitSize(maxCommitSize int64) Option {
}
}

// WithWaitDelay sets the waitDelay option. This specifies how long to wait after
// context cancellation before forcefully killing git processes.
func WithWaitDelay(waitDelay time.Duration) Option {
return func(parser *Parser) {
parser.waitDelay = waitDelay
}
}

// Option is used for adding options to Config.
type Option func(*Parser)

Expand All @@ -213,6 +225,7 @@ func NewParser(options ...Option) *Parser {
dateFormat: defaultDateFormat,
maxDiffSize: defaultMaxDiffSize,
maxCommitSize: defaultMaxCommitSize,
waitDelay: defaultWaitDelay,
}
for _, option := range options {
option(parser)
Expand Down Expand Up @@ -253,7 +266,7 @@ func (c *Parser) RepoPath(
args = append(args, "--", ".", ":(exclude)"+glob)
}

cmd := exec.Command("git", args...)
cmd := exec.CommandContext(ctx, "git", args...)
absPath, err := filepath.Abs(source)
if err == nil {
if !isBare {
Expand All @@ -273,26 +286,27 @@ func (c *Parser) RepoPath(
}
}

return c.executeCommand(ctx, cmd, false)
return c.executeCommand(ctx, cmd, false, c.waitDelay)
}

// Staged parses the output of the `git diff` command for the `source` path.
func (c *Parser) Staged(ctx context.Context, source string) (chan *Diff, error) {
// Provide the --cached flag to diff to get the diff of the staged changes.
args := []string{"-C", source, "diff", "-p", "--cached", "--full-history", "--diff-filter=AM", "--date=iso-strict"}

cmd := exec.Command("git", args...)
cmd := exec.CommandContext(ctx, "git", args...)

absPath, err := filepath.Abs(source)
if err == nil {
cmd.Env = append(cmd.Env, "GIT_DIR="+filepath.Join(absPath, ".git"))
}

return c.executeCommand(ctx, cmd, true)
return c.executeCommand(ctx, cmd, true, c.waitDelay)
}

// executeCommand runs an exec.Cmd, reads stdout and stderr, and waits for the Cmd to complete.
func (c *Parser) executeCommand(ctx context.Context, cmd *exec.Cmd, isStaged bool) (chan *Diff, error) {
// waitDelay specifies how long to wait after context cancellation before forcefully killing the process.
func (c *Parser) executeCommand(ctx context.Context, cmd *exec.Cmd, isStaged bool, waitDelay time.Duration) (chan *Diff, error) {
diffChan := make(chan *Diff, 64)

stdOut, err := cmd.StdoutPipe()
Expand All @@ -304,6 +318,9 @@ func (c *Parser) executeCommand(ctx context.Context, cmd *exec.Cmd, isStaged boo
return diffChan, err
}

// Set WaitDelay to allow the command additional time to exit after context cancellation
cmd.WaitDelay = waitDelay

err = cmd.Start()
if err != nil {
return diffChan, err
Expand Down
20 changes: 20 additions & 0 deletions pkg/gitparse/gitparse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1478,6 +1478,26 @@ func TestMaxCommitSize(t *testing.T) {
}

}
func TestWaitDelay(t *testing.T) {
// Test that WithWaitDelay sets the waitDelay correctly
customDelay := 10 * time.Second
parser := NewParser(WithWaitDelay(customDelay))
if parser.waitDelay != customDelay {
t.Errorf("waitDelay not set correctly. Got: %v, expected: %v", parser.waitDelay, customDelay)
}

// Test that default waitDelay is used when not specified
defaultParser := NewParser()
if defaultParser.waitDelay != defaultWaitDelay {
t.Errorf("default waitDelay not set correctly. Got: %v, expected: %v", defaultParser.waitDelay, defaultWaitDelay)
}

// Test that zero value is allowed (caller can set to 0 if they want no delay)
zeroDelayParser := NewParser(WithWaitDelay(0))
if zeroDelayParser.waitDelay != 0 {
t.Errorf("zero waitDelay not set correctly. Got: %v, expected: 0", zeroDelayParser.waitDelay)
}
}

const commitLog = `commit e50b135fd29e91b2fbb25923797f5ecffe59f359
Author: lionzxy <nikita@kulikof.ru>
Expand Down
Loading