Update regex pattern and 403 handling in the Lob detector to fix false positives (#5188)

* lob: exclude underscores from keyPat to fix false positives

* lob: restrict keyPat to lowercase hex

* distinguish forbidden responses by their API error code

* remove payment_method_unverified from 403 case

---------

Co-authored-by: Shahzad Haider <[email protected]>
Co-authored-by: Shahzad Haider <[email protected]>

pkg/detectors/lob/lob.go

@@ -2,6 +2,8 @@ package lob
 
 import (
 	"context"
+	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -23,8 +25,9 @@ var _ detectors.Detector = (*Scanner)(nil)
 var (
 	defaultClient = common.SaneHttpClient()
 
-	// Make sure that your group is surrounded in boundary characters such as below to reduce false positives.
-	keyPat = regexp.MustCompile(`\b((live|test)_[a-zA-Z0-9_]{35})\b`)
+	// Secret keys are a live_/test_ prefix followed by 35 lowercase hex characters. Publishable keys
+	// ((live|test)_pub_ + 31 hex) are excluded, as they are intended to be embedded in client-side code.
+	keyPat = regexp.MustCompile(`\b((live|test)_[a-f0-9]{35})\b`)
 )
 
 func (s Scanner) getClient() *http.Client {
@@ -86,10 +89,27 @@ func (s Scanner) verify(ctx context.Context, key string) (bool, error) {
 	}
 	defer func() { _ = res.Body.Close() }()
 	switch res.StatusCode {
-	case http.StatusForbidden, http.StatusUnprocessableEntity:
-		// 403 indicates key is active but no billing method on file
+	case http.StatusUnprocessableEntity:
 		// 422 indicates key is active but request body is invalid
 		return true, nil
+	case http.StatusForbidden:
+		var body struct {
+			Error struct {
+				Code string `json:"code"`
+			} `json:"error"`
+		}
+		if err := json.NewDecoder(res.Body).Decode(&body); err != nil {
+			return false, errors.New("invalid API response")
+		}
+		switch body.Error.Code {
+		case "billing_address_required", "feature_limit_reached":
+			// The key authenticated and it is the account, not the key, that is rejected.
+			return true, nil
+		case "invalid_api_key":
+			return false, nil
+		default:
+			return false, fmt.Errorf("unexpected error code: <%s> in the API response body", body.Error.Code)
+		}
 	case http.StatusUnauthorized:
 		return false, nil
 	default:

pkg/detectors/lob/lob_test.go

@@ -3,6 +3,9 @@ package lob
 import (
 	"context"
 	"fmt"
+	"io"
+	"net/http"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -12,9 +15,10 @@ import (
 )
 
 var (
-	validPattern     = "live_0979969b3f6cc23ed67e9b650bfaf64f710"
-	validPatternTest = "test_0979969b3f6cc23ed67e9b650bfaf64f710"
-	invalidPattern   = "live_0979969b3f6cc23ed67e9b650bfaf64f71"
+	validPattern       = "live_0979969b3f6cc23ed67e9b650bfaf64f710"
+	validPatternTest   = "test_0979969b3f6cc23ed67e9b650bfaf64f710"
+	invalidPattern     = "live_0979969b3f6cc23ed67e9b650bfaf64f71"
+	publishablePattern = "live_pub_0979969b3f6cc23ed67e9b650bfaf64"
 )
 
 func TestLob_Pattern(t *testing.T) {
@@ -45,6 +49,21 @@ func TestLob_Pattern(t *testing.T) {
 			input: fmt.Sprintf("'%s'", invalidPattern),
 			want:  []string{},
 		},
+		{
+			name:  "snake_case identifier of key length",
+			input: "def test_calculate_total_price_with_tax_rate():",
+			want:  []string{},
+		},
+		{
+			name:  "camelCase identifier of key length",
+			input: "func test_someVeryLongCamelCaseFunctionNameXy() {}",
+			want:  []string{},
+		},
+		{
+			name:  "publishable key",
+			input: fmt.Sprintf("token = '%s'", publishablePattern),
+			want:  []string{},
+		},
 	}
 
 	for _, test := range tests {
@@ -89,3 +108,100 @@ func TestLob_Pattern(t *testing.T) {
 		})
 	}
 }
+
+type roundTripFunc func(*http.Request) (*http.Response, error)
+
+func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) }
+
+func TestLob_Verify(t *testing.T) {
+	lobError := func(code string) string {
+		return fmt.Sprintf(`{"error":{"message":"...","status_code":403,"code":%q}}`, code)
+	}
+
+	tests := []struct {
+		name        string
+		status      int
+		contentType string
+		body        string
+		want        bool
+		wantErr     bool
+	}{
+		{
+			name:   "active key",
+			status: http.StatusUnprocessableEntity,
+			body:   `{"error":{"message":"...","status_code":422,"code":"invalid"}}`,
+			want:   true,
+		},
+		{
+			name:   "invalid key",
+			status: http.StatusUnauthorized,
+			body:   `{"error":{"message":"...","status_code":401,"code":"invalid_api_key"}}`,
+			want:   false,
+		},
+		{
+			name:   "active key without a billing address",
+			status: http.StatusForbidden,
+			body:   lobError("billing_address_required"),
+			want:   true,
+		},
+		{
+			name:   "active key past its free request quota",
+			status: http.StatusForbidden,
+			body:   lobError("feature_limit_reached"),
+			want:   true,
+		},
+		{
+			name:   "invalid key reported as forbidden",
+			status: http.StatusForbidden,
+			body:   lobError("invalid_api_key"),
+			want:   false,
+		},
+		{
+			name:    "unrecognized error code",
+			status:  http.StatusForbidden,
+			body:    lobError("some_future_code"),
+			wantErr: true,
+		},
+		{
+			name:        "forbidden without an API response",
+			status:      http.StatusForbidden,
+			contentType: "text/html; charset=UTF-8",
+			body:        "<!DOCTYPE html><html><head><title>Attention Required!</title></head></html>",
+			wantErr:     true,
+		},
+		{
+			name:    "unexpected status",
+			status:  http.StatusInternalServerError,
+			body:    `{"error":{"message":"...","status_code":500,"code":"internal_server_error"}}`,
+			wantErr: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			s := Scanner{client: &http.Client{Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) {
+				username, _, ok := req.BasicAuth()
+				if !ok || username != validPattern {
+					t.Errorf("basic auth username = %q (ok=%t), want %q", username, ok, validPattern)
+				}
+				contentType := test.contentType
+				if contentType == "" {
+					contentType = "application/json; charset=utf-8"
+				}
+				return &http.Response{
+					StatusCode: test.status,
+					Header:     http.Header{"Content-Type": []string{contentType}},
+					Body:       io.NopCloser(strings.NewReader(test.body)),
+				}, nil
+			})}}
+
+			got, err := s.verify(context.Background(), validPattern)
+			if (err != nil) != test.wantErr {
+				t.Fatalf("error = %v, wantErr %t", err, test.wantErr)
+			}
+			if got != test.want {
+				t.Errorf("verified = %t, want %t", got, test.want)
+			}
+		})
+	}
+}