Use first non-empty group if `secretGroup` isn't set (#1459)

* feat(detect): secret is first non-empty group

* test: fix errors

- update capture groups
- update expected data

detect/detect.go

@@ -291,24 +291,31 @@ func (d *Detector) detectRule(fragment Fragment, rule config.Rule) []report.Find
 			continue
 		}
 
-		// by default if secret group is not set, we will check to see if there
-		// are any capture groups. If there are, we will use the first capture to start
-		groups := rule.Regex.FindStringSubmatch(secret)
-		if rule.SecretGroup == 0 {
-			// if len(groups) == 2 that means there is only one capture group
-			// the first element in groups is the full match, the second is the
-			// first capture group
-			if len(groups) == 2 {
-				secret = groups[1]
-				finding.Secret = secret
-			}
-		} else {
-			if len(groups) <= rule.SecretGroup || len(groups) == 0 {
-				// Config validation should prevent this
-				continue
+		// Set the value of |secret|, if the pattern contains at least one capture group.
+		// (The first element is the full match, hence we check >= 2.)
+		groups := rule.Regex.FindStringSubmatch(finding.Secret)
+		if len(groups) >= 2 {
+			if rule.SecretGroup > 0 {
+				if len(groups) <= rule.SecretGroup {
+					// Config validation should prevent this
+					continue
+				}
+				finding.Secret = groups[rule.SecretGroup]
+			} else {
+				// If |secretGroup| is not set, we will use the first suitable capture group.
+				if len(groups) == 2 {
+					// Use the only group.
+					finding.Secret = groups[1]
+				} else {
+					// Use the first non-empty group.
+					for _, s := range groups[1:] {
+						if len(s) > 0 {
+							finding.Secret = s
+							break
+						}
+					}
+				}
 			}
-			secret = groups[rule.SecretGroup]
-			finding.Secret = secret
 		}
 
 		// check if the regexTarget is defined in the allowlist "regexes" entry
@@ -354,7 +361,7 @@ func (d *Detector) detectRule(fragment Fragment, rule config.Rule) []report.Find
 			// secret contains both digits and alphabetical characters.
 			// TODO: this should be replaced with stop words
 			if strings.HasPrefix(rule.RuleID, "generic") {
-				if !containsDigit(secret) {
+				if !containsDigit(finding.Secret) {
 					continue
 				}
 			}

report/sarif_test.go

@@ -76,7 +76,7 @@ func TestWriteSarif(t *testing.T) {
 			}
 			want, err := os.ReadFile(test.expected)
 			require.NoError(t, err)
-			assert.Equal(t, want, got)
+			assert.Equal(t, string(want), string(got))
 		})
 	}
 }

testdata/config/simple.toml

@@ -79,13 +79,13 @@ title = "gitleaks config"
 [[rules]]
     id = "slack"
     description = "Slack"
-    regex = '''xox[baprs]-([0-9a-zA-Z]{10,48})?'''
+    regex = '''xox[baprs]-(?:[0-9a-zA-Z]{10,48})?'''
     tags = ["key", "Slack"]
 
 [[rules]]
     id = "apkey"
     description = "Asymmetric Private Key"
-    regex = '''-----BEGIN ((EC|PGP|DSA|RSA|OPENSSH) )?PRIVATE KEY( BLOCK)?-----'''
+    regex = '''-----BEGIN (?:(?:EC|PGP|DSA|RSA|OPENSSH) )?PRIVATE KEY(?: BLOCK)?-----'''
     tags = ["key", "AsymmetricPrivateKey"]
 
 [[rules]]

testdata/expected/report/sarif_simple.sarif

@@ -104,14 +104,14 @@
        "id": "slack",
        "name": "Slack",
        "shortDescription": {
-        "text": "xox[baprs]-([0-9a-zA-Z]{10,48})?"
+        "text": "xox[baprs]-(?:[0-9a-zA-Z]{10,48})?"
        }
       },
       {
        "id": "apkey",
        "name": "Asymmetric Private Key",
        "shortDescription": {
-        "text": "-----BEGIN ((EC|PGP|DSA|RSA|OPENSSH) )?PRIVATE KEY( BLOCK)?-----"
+        "text": "-----BEGIN (?:(?:EC|PGP|DSA|RSA|OPENSSH) )?PRIVATE KEY(?: BLOCK)?-----"
        }
       },
       {