diff --git a/app/api/controller/githook/post_receive.go b/app/api/controller/githook/post_receive.go index 151e9a1b5..ebc29f267 100644 --- a/app/api/controller/githook/post_receive.go +++ b/app/api/controller/githook/post_receive.go @@ -62,9 +62,6 @@ func (c *Controller) PostReceive( c.handlePRMessaging(ctx, repo, in.PostReceiveInput, &out) err = c.postReceiveExtender.Extend(ctx, rgit, session, repo, in, &out) - if out.Error != nil { - return out, nil - } if err != nil { return hook.Output{}, fmt.Errorf("failed to extend post-receive hook: %w", err) } @@ -249,7 +246,7 @@ func (c *Controller) suggestPullRequest( // this is a new PR! out.Messages = append(out.Messages, - fmt.Sprintf("Create a new PR for branch %q", branchName), + fmt.Sprintf("Create a pull request for %q by visiting:", branchName), " "+c.urlProvider.GenerateUICompareURL(repo.Path, repo.DefaultBranch, branchName), ) } diff --git a/app/api/controller/githook/pre_receive.go b/app/api/controller/githook/pre_receive.go index 6dcdf3275..cacceffca 100644 --- a/app/api/controller/githook/pre_receive.go +++ b/app/api/controller/githook/pre_receive.go @@ -66,11 +66,6 @@ func (c *Controller) PreReceive( return output, nil } - err = c.scanSecrets(ctx, rgit, repo, in, &output) - if output.Error != nil || err != nil { - return output, err - } - if c.blockPullReqRefUpdate(refUpdates) { output.Error = ptr.String(usererror.ErrPullReqRefsCantBeModified.Error()) return output, nil @@ -92,10 +87,12 @@ func (c *Controller) PreReceive( return hook.Output{}, fmt.Errorf("failed to check protection rules: %w", err) } - err = c.preReceiveExtender.Extend(ctx, rgit, session, repo, in, &output) - if output.Error != nil { - return output, nil + err = c.scanSecrets(ctx, rgit, repo, in, &output) + if err != nil { + return hook.Output{}, err } + + err = c.preReceiveExtender.Extend(ctx, rgit, session, repo, in, &output) if err != nil { return hook.Output{}, fmt.Errorf("failed to extend pre-receive hook: %w", err) } diff --git a/app/api/controller/githook/pre_receive_scan_secrets.go b/app/api/controller/githook/pre_receive_scan_secrets.go index 90ee956c1..8ea10f652 100644 --- a/app/api/controller/githook/pre_receive_scan_secrets.go +++ b/app/api/controller/githook/pre_receive_scan_secrets.go @@ -17,6 +17,7 @@ package githook import ( "context" "fmt" + "time" "github.com/harness/gitness/app/services/settings" "github.com/harness/gitness/git" @@ -29,12 +30,9 @@ import ( "github.com/rs/zerolog/log" ) -type scanSecretsResult struct { - findings []api.Finding -} - -func (r *scanSecretsResult) HasResults() bool { - return len(r.findings) > 0 +type secretFinding struct { + api.Finding + Ref string } func (c *Controller) scanSecrets( @@ -60,7 +58,8 @@ func (c *Controller) scanSecrets( } // scan for secrets - scanResult, err := scanSecretsInternal( + startTime := time.Now() + findings, err := scanSecretsInternal( ctx, rgit, repo, @@ -70,12 +69,13 @@ func (c *Controller) scanSecrets( return fmt.Errorf("failed to scan for git leaks: %w", err) } - if !scanResult.HasResults() { - return nil - } + // always print result (handles both no results and results found) + printScanSecretsFindings(output, findings, len(in.RefUpdates) > 1, time.Since(startTime)) - // pretty print output - printScanSecretsFindings(output, scanResult.findings) + // block the push if any secrets were found + if len(findings) > 0 { + output.Error = ptr.String("Changes blocked by security scan results") + } return nil } @@ -84,9 +84,9 @@ func scanSecretsInternal(ctx context.Context, rgit RestrictedGIT, repo *types.Repository, in types.GithookPreReceiveInput, -) (scanSecretsResult, error) { +) ([]secretFinding, error) { var baseRevFallBack *string - res := scanSecretsResult{} + findings := []secretFinding{} for _, refUpdate := range in.RefUpdates { ctx := logging.NewContext(ctx, loggingWithRefUpdate(refUpdate)) @@ -112,7 +112,7 @@ func scanSecretsInternal(ctx context.Context, refUpdate, ) if err != nil { - return scanSecretsResult{}, fmt.Errorf("failed to get fallback sha: %w", err) + return nil, fmt.Errorf("failed to get fallback sha: %w", err) } if fallbackAvailable { @@ -140,7 +140,7 @@ func scanSecretsInternal(ctx context.Context, Rev: rev, }) if err != nil { - return scanSecretsResult{}, fmt.Errorf("failed to detect secret leaks: %w", err) + return nil, fmt.Errorf("failed to detect secret leaks: %w", err) } if len(scanSecretsOut.Findings) == 0 { @@ -150,8 +150,17 @@ func scanSecretsInternal(ctx context.Context, log.Debug().Msgf("found %d new secrets", len(scanSecretsOut.Findings)) - res.findings = append(res.findings, scanSecretsOut.Findings...) + for _, finding := range scanSecretsOut.Findings { + findings = append(findings, secretFinding{ + Finding: finding, + Ref: refUpdate.Ref, + }) + } } - return res, nil + if len(findings) > 0 { + log.Ctx(ctx).Debug().Msgf("found total of %d new secrets", len(findings)) + } + + return findings, nil } diff --git a/app/api/controller/githook/print.go b/app/api/controller/githook/print.go index 48fb4ffb4..69281b1a8 100644 --- a/app/api/controller/githook/print.go +++ b/app/api/controller/githook/print.go @@ -16,70 +16,75 @@ package githook import ( "fmt" + "time" - "github.com/harness/gitness/git/api" "github.com/harness/gitness/git/hook" "github.com/fatih/color" - "github.com/gotidy/ptr" ) var ( - colorScanHeader = color.New(color.BgRed, color.FgHiWhite, color.Bold) - colorScanSummary = color.New(color.FgHiRed, color.Bold) + colorScanHeader = color.New(color.FgHiWhite, color.Underline) + colorScanSummary = color.New(color.FgHiRed, color.Bold) + colorScanSummaryNoFindings = color.New(color.FgHiGreen, color.Bold) ) -func printScanSecretsFindings(out *hook.Output, findings []api.Finding) { +func printScanSecretsFindings( + output *hook.Output, + findings []secretFinding, + multipleRefs bool, + duration time.Duration, +) { findingsCnt := len(findings) - out.Messages = append( - out.Messages, + + // no results? output success and continue + if findingsCnt == 0 { + output.Messages = append( + output.Messages, + colorScanSummaryNoFindings.Sprintf("No secrets found")+ + fmt.Sprintf(" in %s", duration.Round(time.Millisecond)), + "", "", // add two empty lines for making it visually more consumable + ) + return + } + + output.Messages = append( + output.Messages, colorScanHeader.Sprintf( - " Detected leaked %s ", + "Push contains %s:", stringSecretOrSecrets(findingsCnt > 1), ), + "", // add empty line for making it visually more consumable ) for _, finding := range findings { - out.Messages = append( - out.Messages, - fmt.Sprintf(" Commit: %s", finding.Commit), - fmt.Sprintf(" File: %s", finding.File), - ) - if finding.StartLine == finding.EndLine { - out.Messages = append( - out.Messages, - fmt.Sprintf(" Line: %d", finding.StartLine), - ) - } else { - out.Messages = append( - out.Messages, - fmt.Sprintf(" Lines: %d-%d", finding.StartLine, finding.EndLine), - ) + headerTxt := fmt.Sprintf("%s in %s:%d", finding.RuleID, finding.File, finding.StartLine) + if finding.StartLine != finding.EndLine { + headerTxt += fmt.Sprintf("-%d", finding.EndLine) } - out.Messages = append( - out.Messages, - fmt.Sprintf(" Details: %s", finding.Description), - fmt.Sprintf(" Secret: %s", finding.Match), - fmt.Sprintf(" RuleID: %s", finding.RuleID), - fmt.Sprintf(" Author: %s", finding.Author), - fmt.Sprintf(" Date: %s", finding.Date), - "", + if multipleRefs { + headerTxt += fmt.Sprintf(" [%s]", finding.Ref) + } + + output.Messages = append( + output.Messages, + fmt.Sprintf(" %s", headerTxt), + fmt.Sprintf(" Secret: %s", finding.Secret), + fmt.Sprintf(" Commit: %s", finding.Commit), + fmt.Sprintf(" Details: %s", finding.Description), + "", // add empty line for making it visually more consumable ) } - out.Messages = append(out.Messages, "") - - out.Messages = append( - out.Messages, + output.Messages = append( + output.Messages, colorScanSummary.Sprintf( "%d %s found", findingsCnt, stringSecretOrSecrets(findingsCnt > 1), - ), + )+fmt.Sprintf(" in %s", FMTDuration(time.Millisecond)), + "", "", // add two empty lines for making it visually more consumable ) - - // block the commit - out.Error = ptr.String("Changes blocked by security scan results") } func stringSecretOrSecrets(plural bool) string { @@ -88,3 +93,18 @@ func stringSecretOrSecrets(plural bool) string { } return "secret" } + +func FMTDuration(d time.Duration) string { + const secondsRounding = time.Second / time.Duration(10) + switch { + case d <= time.Millisecond: + // keep anything under a millisecond untouched + case d < time.Second: + d = d.Round(time.Millisecond) // round under a second to millisecondss + case d < time.Minute: + d = d.Round(secondsRounding) // round under a minute to .1 precision + default: + d = d.Round(time.Second) // keep rest at second precision + } + return d.String() +} diff --git a/app/api/controller/githook/update.go b/app/api/controller/githook/update.go index d0da67a72..2a371a80d 100644 --- a/app/api/controller/githook/update.go +++ b/app/api/controller/githook/update.go @@ -39,9 +39,6 @@ func (c *Controller) Update( output := hook.Output{} err = c.updateExtender.Extend(ctx, rgit, session, repo, in, &output) - if output.Error != nil { - return output, nil - } if err != nil { return hook.Output{}, fmt.Errorf("failed to extend update hook: %w", err) } diff --git a/git/hook/cli_core.go b/git/hook/cli_core.go index 944e62e11..366fb460c 100644 --- a/git/hook/cli_core.go +++ b/git/hook/cli_core.go @@ -115,6 +115,15 @@ func handleServerHookOutput(out Output, err error) error { return fmt.Errorf("an error occurred when calling the server: %w", err) } + // remove any preceding empty lines + for len(out.Messages) > 0 && out.Messages[0] == "" { + out.Messages = out.Messages[1:] + } + // remove any trailing empty lines + for len(out.Messages) > 0 && out.Messages[len(out.Messages)-1] == "" { + out.Messages = out.Messages[:len(out.Messages)-1] + } + // print messages before any error if len(out.Messages) > 0 { // add empty line before and after to make it easier readable