From 842f9cfbc5758f0a0e188612798072078f84ecaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Sibiril?= <81782+aureliensibiril@users.noreply.github.com> Date: Sun, 17 May 2026 17:22:49 +0200 Subject: [PATCH] =?UTF-8?q?Stop=20leaking=20customer=20email=20domain=20in?= =?UTF-8?q?=20cassette=20guard=20=E2=86=92=20Add=20PKCE=20coverage=20tests?= =?UTF-8?q?=20for=20entropy=20and=20replay?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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> --- .../drivers/cassette_safety_test.go | 22 ++-- pkg/accessreview/drivers/monday.go | 4 +- pkg/connector/oauth2_test.go | 117 ++++++++++++++++++ pkg/connector/providers.go | 20 ++- 4 files changed, 146 insertions(+), 17 deletions(-) diff --git a/pkg/accessreview/drivers/cassette_safety_test.go b/pkg/accessreview/drivers/cassette_safety_test.go index c1c2c076d..eacce9d0e 100644 --- a/pkg/accessreview/drivers/cassette_safety_test.go +++ b/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), ) } }) diff --git a/pkg/accessreview/drivers/monday.go b/pkg/accessreview/drivers/monday.go index 9a186fa41..4daeef15a 100644 --- a/pkg/accessreview/drivers/monday.go +++ b/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 diff --git a/pkg/connector/oauth2_test.go b/pkg/connector/oauth2_test.go index 825d4a1a2..4fba0dce8 100644 --- a/pkg/connector/oauth2_test.go +++ b/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") +} diff --git a/pkg/connector/providers.go b/pkg/connector/providers.go index 32b41fb1b..7a375d37a 100644 --- a/pkg/connector/providers.go +++ b/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)