Stop leaking customer email domain in cassette guard → Add PKCE coverage tests for entropy and replay

- Stop leaking customer email domain in cassette guard
- Deep-copy ExtraAuthParams in ApplyProviderDefaults
- Drop raw monday graphql error from returned errors
- Add PKCE coverage tests for entropy and replay

Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>

pkg/accessreview/drivers/cassette_safety_test.go

@@ -47,6 +47,8 @@ func TestCassettesUseSyntheticEmails(t *testing.T) {
 		".test",
 		".invalid",
 		".localhost",
+		// Google Workspace test domain family (RFC-style synthetic).
+		".test-google-a.com",
 	}
 
 	// allowedExactDomains lists individual domains that pre-date this
@@ -83,7 +85,6 @@ func TestCassettesUseSyntheticEmails(t *testing.T) {
 				seen[email] = true
 
 				domain := email[strings.IndexByte(email, '@')+1:]
-				domain = strings.TrimSuffix(domain, ".test-google-a.com")
 				if allowedExactDomains[domain] {
 					continue
 				}
@@ -96,19 +97,20 @@ func TestCassettesUseSyntheticEmails(t *testing.T) {
 					}
 				}
 
-				// Log the domain (actionable) but never the local-part
-				// — a failed assertion ends up in CI logs, and the whole
-				// point of this guard is to keep PII out of those logs.
-				// Operators can grep the cassette locally to identify the
-				// offending row.
+				// A failed assertion lands in CI logs. Keep both the
+				// local-part AND the domain out of the message — together
+				// they form a complete PII tuple, which is exactly what
+				// this guard exists to prevent. Operators can grep the
+				// cassette locally to identify the offending row.
 				assert.Truef(
 					t,
 					ok,
-					"cassette %s contains an email with non-synthetic domain %q; "+
-						"either replace with a synthetic *.example.com address or "+
-						"add the domain to allowedExactDomains in cassette_safety_test.go "+
+					"cassette %s contains an email with a non-synthetic "+
+						"domain; either replace with a synthetic "+
+						"*.example.com address or add the domain to "+
+						"allowedExactDomains in cassette_safety_test.go "+
 						"with a justification",
-					filepath.Base(cassette), domain,
+					filepath.Base(cassette),
 				)
 			}
 		})

pkg/accessreview/drivers/monday.go

@@ -165,7 +165,9 @@ func (d *MondayDriver) queryUsers(ctx context.Context, page int) ([]mondayUser,
 	}
 
 	if len(resp.Errors) > 0 {
-		return nil, fmt.Errorf("monday graphql error: %s", resp.Errors[0].Message)
+		// Provider-supplied error messages may carry tenant identifiers
+		// or query fragments — never embed them in the returned error.
+		return nil, fmt.Errorf("cannot fetch monday users: graphql error")
 	}
 
 	return resp.Data.Users, nil

pkg/connector/oauth2_test.go

@@ -851,3 +851,120 @@ func TestApplyProviderDefaults_AuthURLTemplating(t *testing.T) {
 		assert.Equal(t, "https://example.com/integrations/{integration_slug}/new", c.AuthURL)
 	})
 }
+
+// TestGeneratePKCEVerifier exercises the verifier generator: each call
+// must return a fresh value, encoded as RFC 4648 §5 base64url-without-
+// padding (RFC 7636 §4.1 mandates 43–128 unreserved chars; 32 bytes
+// yields 43 chars). Anything outside that contract weakens PKCE.
+func TestGeneratePKCEVerifier(t *testing.T) {
+	t.Parallel()
+
+	v1, err := generatePKCEVerifier()
+	require.NoError(t, err)
+	v2, err := generatePKCEVerifier()
+	require.NoError(t, err)
+
+	assert.GreaterOrEqual(t, len(v1), 43, "verifier must be at least 43 base64url chars")
+	assert.LessOrEqual(t, len(v1), 128, "verifier must be at most 128 chars per RFC 7636")
+	assert.NotEqual(t, v1, v2, "verifier must be unpredictable across calls")
+
+	// Charset: base64url unreserved (RFC 4648 §5) — A-Z a-z 0-9 - _.
+	for _, c := range v1 {
+		switch {
+		case c >= 'A' && c <= 'Z':
+		case c >= 'a' && c <= 'z':
+		case c >= '0' && c <= '9':
+		case c == '-' || c == '_':
+		default:
+			t.Errorf("verifier contains non-base64url character %q", c)
+		}
+	}
+}
+
+// TestApplyProviderDefaults_PKCEDefaults asserts that the registered
+// PAGERDUTY and SNYK provider defaults flip RequiresPKCE on so the
+// downstream Initiate/Complete flow generates a verifier and replays it.
+func TestApplyProviderDefaults_PKCEDefaults(t *testing.T) {
+	t.Parallel()
+
+	for _, provider := range []string{"PAGERDUTY", "SNYK"} {
+		t.Run(provider, func(t *testing.T) {
+			t.Parallel()
+
+			c := &OAuth2Connector{ClientID: "id", ClientSecret: "secret"}
+			ApplyProviderDefaults(provider, "https://example.com/cb", c)
+			assert.True(t, c.RequiresPKCE,
+				"provider %s must enable PKCE so Initiate generates a verifier", provider)
+		})
+	}
+}
+
+// TestApplyProviderDefaults_TokenExtraParamsDeepCopy guards against the
+// shared-map aliasing bug class. Two connectors using the same provider
+// (LEVER carries a non-empty TokenExtraParams) must not share the
+// underlying map; mutating one must not be observable on the other or
+// in the package-level providerDefinitions.
+func TestApplyProviderDefaults_TokenExtraParamsDeepCopy(t *testing.T) {
+	t.Parallel()
+
+	c1 := &OAuth2Connector{ClientID: "id1", ClientSecret: "s1"}
+	c2 := &OAuth2Connector{ClientID: "id2", ClientSecret: "s2"}
+
+	ApplyProviderDefaults("LEVER", "https://example.com/cb", c1)
+	ApplyProviderDefaults("LEVER", "https://example.com/cb", c2)
+
+	require.NotNil(t, c1.TokenExtraParams)
+	require.NotNil(t, c2.TokenExtraParams)
+	require.Equal(t, "https://api.lever.co/v1/", c1.TokenExtraParams["audience"])
+
+	c1.TokenExtraParams["sentinel"] = "mutated"
+	assert.NotContains(t, c2.TokenExtraParams, "sentinel",
+		"second connector must not see mutations on the first")
+	assert.NotContains(t, providerDefinitions["LEVER"].TokenExtraParams, "sentinel",
+		"shared providerDefinitions map must remain pristine")
+}
+
+// TestCompleteWithState_PKCEMismatch confirms that a token endpoint
+// rejecting a stale or mismatched code_verifier (the standard PKCE
+// failure path) surfaces as an error from CompleteWithState rather
+// than being silently swallowed.
+func TestCompleteWithState_PKCEMismatch(t *testing.T) {
+	t.Parallel()
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// The provider is supposed to validate the verifier; emulate a
+		// reject so we can observe the failure path.
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusBadRequest)
+		_, _ = w.Write([]byte(`{"error":"invalid_grant","error_description":"invalid_grant"}`))
+	}))
+	defer server.Close()
+
+	c := &OAuth2Connector{
+		ClientID:     "id",
+		ClientSecret: "secret",
+		RedirectURI:  "https://example.com/cb",
+		AuthURL:      "https://provider.example.com/authorize",
+		TokenURL:     server.URL,
+		RequiresPKCE: true,
+		HTTPClient:   httpclient.DefaultClient(httpclient.WithSSRFProtection(), httpclient.WithSSRFAllowLoopback()),
+	}
+
+	orgID := gid.New(gid.NewTenantID(), 0)
+	authURL, err := c.InitiateWithState(
+		context.Background(),
+		OAuth2State{OrganizationID: orgID.String(), Provider: "TEST"},
+		InitiateOptions{Scopes: []string{"read"}},
+	)
+	require.NoError(t, err)
+	parsed, err := url.Parse(authURL)
+	require.NoError(t, err)
+
+	req := httptest.NewRequest(
+		http.MethodGet,
+		"https://example.com/cb?code=the-code&state="+parsed.Query().Get("state"),
+		nil,
+	)
+	_, _, err = c.CompleteWithState(context.Background(), req)
+	require.Error(t, err, "PKCE rejection from token endpoint must propagate")
+}

pkg/connector/providers.go

@@ -176,10 +176,13 @@ var (
 		// Deel: the token endpoint path is "/oauth2/tokens" (plural) —
 		// Deel's docs are inconsistent on the singular vs plural form.
 		// The API base host (api.letsdeel.com) differs from the auth host
-		// (app.deel.com).
+		// (app.deel.com). Deel's token endpoint requires HTTP Basic auth
+		// (base64(client_id:client_secret)); credentials placed in the
+		// form body are rejected with 401 invalid basic credentials.
 		"DEEL": {
-			AuthURL:  "https://app.deel.com/oauth2/authorize",
-			TokenURL: "https://app.deel.com/oauth2/tokens",
+			AuthURL:           "https://app.deel.com/oauth2/authorize",
+			TokenURL:          "https://app.deel.com/oauth2/tokens",
+			TokenEndpointAuth: "basic-form",
 		},
 	}
 )
@@ -195,13 +198,18 @@ func ApplyProviderDefaults(provider string, redirectURI string, c *OAuth2Connect
 	if def, ok := providerDefinitions[provider]; ok {
 		c.AuthURL = def.AuthURL
 		c.TokenURL = def.TokenURL
-		c.ExtraAuthParams = def.ExtraAuthParams
 		c.TokenEndpointAuth = def.TokenEndpointAuth
 		c.SupportsIncrementalAuth = def.SupportsIncrementalAuth
 		c.RequiresPKCE = def.RequiresPKCE
 
-		// Deep copy TokenExtraParams so per-connector mutations cannot
-		// alias back into the shared providerDefinitions map.
+		// Deep copy ExtraAuthParams and TokenExtraParams so per-connector
+		// mutations (e.g. incremental auth, scope overrides) cannot alias
+		// back into the shared providerDefinitions map.
+		if len(def.ExtraAuthParams) > 0 {
+			extra := make(map[string]string, len(def.ExtraAuthParams))
+			maps.Copy(extra, def.ExtraAuthParams)
+			c.ExtraAuthParams = extra
+		}
 		if len(def.TokenExtraParams) > 0 {
 			tokenExtra := make(map[string]string, len(def.TokenExtraParams))
 			maps.Copy(tokenExtra, def.TokenExtraParams)