Improve secret scanning output (#1170)

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),
 	)
 }

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)
 	}

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
 }

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)
+	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,
+		headerTxt := fmt.Sprintf("%s in %s:%d", finding.RuleID, finding.File, finding.StartLine)
+		if finding.StartLine != finding.EndLine {
+			headerTxt += fmt.Sprintf("-%d", finding.EndLine)
+		}
+		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("  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),
-			)
-		}
-		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),
-			"",
+			"", // 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()
+}

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)
 	}

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