From 995769c4b65439724943e3abaa920951f3995707 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:50 +0200 Subject: [PATCH] =?UTF-8?q?Drop=20dead=20OAuth2=20TokenExtraParams=20plumb?= =?UTF-8?q?ing=20=E2=86=92=20Reject=20non-numeric=20ClickUp=20timestamps?= =?UTF-8?q?=20via=20strconv?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop dead OAuth2 TokenExtraParams plumbing - Drop Deel-only x-client-id header from basic-form - Follow Bitbucket workspace pagination cursor - Reject non-numeric ClickUp timestamps via strconv Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> --- pkg/accessreview/drivers/clickup.go | 7 +- pkg/accessreview/drivers/organizations.go | 104 ++++++++++++---------- pkg/connector/oauth2.go | 22 +---- pkg/connector/oauth2_test.go | 100 --------------------- pkg/connector/providers.go | 15 +--- 5 files changed, 65 insertions(+), 183 deletions(-) diff --git a/pkg/accessreview/drivers/clickup.go b/pkg/accessreview/drivers/clickup.go index 1a2b3ffc0..9198afb6a 100644 --- a/pkg/accessreview/drivers/clickup.go +++ b/pkg/accessreview/drivers/clickup.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" "net/url" + "strconv" "time" "go.probo.inc/probo/pkg/coredata" @@ -147,8 +148,10 @@ func parseClickUpTime(raw string) (time.Time, error) { return t, nil } - var ms int64 - if _, err := fmt.Sscanf(raw, "%d", &ms); err != nil { + // strconv.ParseInt rejects trailing non-digit garbage that fmt.Sscanf + // would silently truncate (e.g. "123abc" → 123). + ms, err := strconv.ParseInt(raw, 10, 64) + if err != nil { return time.Time{}, fmt.Errorf("cannot parse clickup time %q: %w", raw, err) } return time.UnixMilli(ms).UTC(), nil diff --git a/pkg/accessreview/drivers/organizations.go b/pkg/accessreview/drivers/organizations.go index 233c7f78a..46d0ff0a3 100644 --- a/pkg/accessreview/drivers/organizations.go +++ b/pkg/accessreview/drivers/organizations.go @@ -163,59 +163,67 @@ func ListGitLabOrganizations(ctx context.Context, httpClient *http.Client) ([]Or // ListBitbucketOrganizations fetches the workspaces the authenticated // Bitbucket user belongs to. The legacy /2.0/workspaces endpoint was // sunset by CHANGE-2770 (April 2026); /2.0/user/workspaces is the -// supported cross-workspace replacement (CHANGE-3022). +// supported cross-workspace replacement (CHANGE-3022). Bitbucket pages +// via an absolute `next` URL on each response; follow until exhausted. func ListBitbucketOrganizations(ctx context.Context, httpClient *http.Client) ([]Organization, error) { - req, err := http.NewRequestWithContext( - ctx, - http.MethodGet, - "https://api.bitbucket.org/2.0/user/workspaces?pagelen=100", - nil, - ) - if err != nil { - return nil, fmt.Errorf("cannot create bitbucket organizations request: %w", err) - } - req.Header.Set("Accept", "application/json") + pageURL := "https://api.bitbucket.org/2.0/user/workspaces?pagelen=100" + result := make([]Organization, 0) - resp, err := httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("cannot fetch bitbucket organizations: %w", err) - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("cannot fetch bitbucket organizations: unexpected status %d", resp.StatusCode) - } - - // We tolerate both shapes (flat and nested under `workspace`) since - // Atlassian has shipped variants of similar endpoints with both. - var body struct { - Values []struct { - Slug string `json:"slug"` - Name string `json:"name"` - Workspace struct { - Slug string `json:"slug"` - Name string `json:"name"` - } `json:"workspace"` - } `json:"values"` - } - if err := json.NewDecoder(resp.Body).Decode(&body); err != nil { - return nil, fmt.Errorf("cannot decode bitbucket organizations response: %w", err) - } - - result := make([]Organization, 0, len(body.Values)) - for _, v := range body.Values { - slug, name := v.Slug, v.Name - if slug == "" { - slug = v.Workspace.Slug - name = v.Workspace.Name + for range maxPaginationPages { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, pageURL, nil) + if err != nil { + return nil, fmt.Errorf("cannot create bitbucket organizations request: %w", err) } - displayName := name - if displayName == "" { - displayName = slug + req.Header.Set("Accept", "application/json") + + resp, err := httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("cannot fetch bitbucket organizations: %w", err) } - result = append(result, Organization{Slug: slug, DisplayName: displayName}) + + if resp.StatusCode != http.StatusOK { + _ = resp.Body.Close() + return nil, fmt.Errorf("cannot fetch bitbucket organizations: unexpected status %d", resp.StatusCode) + } + + // We tolerate both shapes (flat and nested under `workspace`) since + // Atlassian has shipped variants of similar endpoints with both. + var body struct { + Values []struct { + Slug string `json:"slug"` + Name string `json:"name"` + Workspace struct { + Slug string `json:"slug"` + Name string `json:"name"` + } `json:"workspace"` + } `json:"values"` + Next string `json:"next"` + } + if err := json.NewDecoder(resp.Body).Decode(&body); err != nil { + _ = resp.Body.Close() + return nil, fmt.Errorf("cannot decode bitbucket organizations response: %w", err) + } + _ = resp.Body.Close() + + for _, v := range body.Values { + slug, name := v.Slug, v.Name + if slug == "" { + slug = v.Workspace.Slug + name = v.Workspace.Name + } + displayName := name + if displayName == "" { + displayName = slug + } + result = append(result, Organization{Slug: slug, DisplayName: displayName}) + } + + if body.Next == "" { + return result, nil + } + pageURL = body.Next } - return result, nil + return nil, fmt.Errorf("cannot list all bitbucket organizations: %w", ErrPaginationLimitReached) } // ListHerokuOrganizations fetches the teams the authenticated Heroku diff --git a/pkg/connector/oauth2.go b/pkg/connector/oauth2.go index 9466c3b2d..200d89290 100644 --- a/pkg/connector/oauth2.go +++ b/pkg/connector/oauth2.go @@ -23,7 +23,6 @@ import ( "encoding/json" "fmt" "io" - "maps" "net/http" "net/url" "strings" @@ -58,11 +57,6 @@ type ( // to the authorize URL; CompleteWithState replays the verifier // on the token exchange. RequiresPKCE bool - // TokenExtraParams are merged into the token-exchange request - // body (form-encoded for post-form / basic-form, JSON for - // basic-json). Used for provider-specific extras such as - // Lever's `audience` parameter. - TokenExtraParams map[string]string // AuthURLParams are operator-supplied placeholders substituted // into the static provider AuthURL by ApplyProviderDefaults // (for example Vercel's "{integration_slug}"). Empty for the @@ -344,8 +338,7 @@ func basicAuthHeader(clientID, clientSecret string) string { // buildTokenRequest creates the HTTP request for the token exchange, branching // on c.TokenEndpointAuth to support different provider requirements. When // codeVerifier is non-empty (PKCE-enabled providers), it is replayed as -// `code_verifier` in the request body. TokenExtraParams are merged into the -// body in every branch. +// `code_verifier` in the request body. func (c *OAuth2Connector) buildTokenRequest(ctx context.Context, code, redirectURI, codeVerifier string) (*http.Request, error) { switch c.TokenEndpointAuth { case "basic-json": @@ -358,7 +351,6 @@ func (c *OAuth2Connector) buildTokenRequest(ctx context.Context, code, redirectU if codeVerifier != "" { body["code_verifier"] = codeVerifier } - maps.Copy(body, c.TokenExtraParams) jsonBody, err := json.Marshal(body) if err != nil { return nil, fmt.Errorf("cannot marshal token request body: %w", err) @@ -389,9 +381,6 @@ func (c *OAuth2Connector) buildTokenRequest(ctx context.Context, code, redirectU if codeVerifier != "" { formData.Set("code_verifier", codeVerifier) } - for k, v := range c.TokenExtraParams { - formData.Set(k, v) - } req, err := http.NewRequestWithContext( ctx, @@ -407,12 +396,6 @@ func (c *OAuth2Connector) buildTokenRequest(ctx context.Context, code, redirectU req.Header.Set("Accept", "application/json") req.Header.Set("User-Agent", "Probo Connector") req.Header.Set("Authorization", basicAuthHeader(c.ClientID, c.ClientSecret)) - // Deel rejects token-exchange requests that omit the x-client-id - // header even when the credentials are correctly Base64-encoded - // in the Authorization header. Sending it for every basic-form - // provider is harmless — providers that don't expect it ignore - // the header. - req.Header.Set("x-client-id", c.ClientID) return req, nil default: @@ -426,9 +409,6 @@ func (c *OAuth2Connector) buildTokenRequest(ctx context.Context, code, redirectU if codeVerifier != "" { formData.Set("code_verifier", codeVerifier) } - for k, v := range c.TokenExtraParams { - formData.Set(k, v) - } req, err := http.NewRequestWithContext( ctx, diff --git a/pkg/connector/oauth2_test.go b/pkg/connector/oauth2_test.go index 59e9bb2e0..18d1e8ee0 100644 --- a/pkg/connector/oauth2_test.go +++ b/pkg/connector/oauth2_test.go @@ -694,106 +694,6 @@ func TestInitiateWithState_PKCE(t *testing.T) { }) } -// TestBuildTokenRequest_TokenExtraParams verifies that TokenExtraParams are -// merged into the token-exchange body in all three auth branches. This -// powers Lever's required `audience=https://api.lever.co/v1/` parameter -// without any per-provider branching in the OAuth2 core. -func TestBuildTokenRequest_TokenExtraParams(t *testing.T) { - t.Parallel() - - t.Run("post-form merges audience into form body", func(t *testing.T) { - t.Parallel() - - c := &OAuth2Connector{ - ClientID: "lever-client-id", - ClientSecret: "lever-client-secret", - TokenURL: "https://auth.lever.co/oauth/token", - TokenExtraParams: map[string]string{ - "audience": "https://api.lever.co/v1/", - }, - } - - req, err := c.buildTokenRequest( - context.Background(), - "the-code", - "https://example.com/cb", - "", - ) - require.NoError(t, err) - - body, err := io.ReadAll(req.Body) - require.NoError(t, err) - - // Raw body check: the URL-encoded value must be present - // verbatim (catches any double-encoding regressions). - assert.Contains(t, string(body), "audience=https%3A%2F%2Fapi.lever.co%2Fv1%2F") - - form, err := url.ParseQuery(string(body)) - require.NoError(t, err) - assert.Equal(t, "https://api.lever.co/v1/", form.Get("audience")) - assert.Equal(t, "the-code", form.Get("code")) - assert.Equal(t, "authorization_code", form.Get("grant_type")) - }) - - t.Run("basic-form merges extra params into form body", func(t *testing.T) { - t.Parallel() - - c := &OAuth2Connector{ - ClientID: "id", - ClientSecret: "secret", - TokenURL: "https://provider.example.com/oauth/token", - TokenEndpointAuth: "basic-form", - TokenExtraParams: map[string]string{ - "audience": "https://api.lever.co/v1/", - }, - } - - req, err := c.buildTokenRequest( - context.Background(), - "the-code", - "https://example.com/cb", - "", - ) - require.NoError(t, err) - - body, err := io.ReadAll(req.Body) - require.NoError(t, err) - - form, err := url.ParseQuery(string(body)) - require.NoError(t, err) - assert.Equal(t, "https://api.lever.co/v1/", form.Get("audience")) - }) - - t.Run("basic-json merges extra params into JSON body", func(t *testing.T) { - t.Parallel() - - c := &OAuth2Connector{ - ClientID: "id", - ClientSecret: "secret", - TokenURL: "https://provider.example.com/oauth/token", - TokenEndpointAuth: "basic-json", - TokenExtraParams: map[string]string{ - "audience": "https://api.lever.co/v1/", - }, - } - - req, err := c.buildTokenRequest( - context.Background(), - "the-code", - "https://example.com/cb", - "", - ) - require.NoError(t, err) - - body, err := io.ReadAll(req.Body) - require.NoError(t, err) - - var jsonBody map[string]string - require.NoError(t, json.Unmarshal(body, &jsonBody)) - assert.Equal(t, "https://api.lever.co/v1/", jsonBody["audience"]) - }) -} - // TestApplyProviderDefaults_AuthURLTemplating verifies that operator-supplied // AuthURLParams (for example Vercel's "{integration_slug}") are substituted // into the static provider AuthURL when the connector is initialized. diff --git a/pkg/connector/providers.go b/pkg/connector/providers.go index e2f06b8c7..361ce492c 100644 --- a/pkg/connector/providers.go +++ b/pkg/connector/providers.go @@ -39,10 +39,6 @@ type providerDefinition struct { // request and replays the verifier on the token exchange. Default // false; existing providers are unaffected. RequiresPKCE bool - // TokenExtraParams are merged into the token-exchange request body - // (form-encoded for "post-form"/"basic-form", JSON for "basic-json"). - // Used by providers like Lever that require an `audience` parameter. - TokenExtraParams map[string]string } // providerDefinitions maps provider names to their static OAuth2 definitions. @@ -167,19 +163,14 @@ func ApplyProviderDefaults(provider string, redirectURI string, c *OAuth2Connect c.SupportsIncrementalAuth = def.SupportsIncrementalAuth c.RequiresPKCE = def.RequiresPKCE - // Deep copy ExtraAuthParams and TokenExtraParams so per-connector - // mutations (e.g. incremental auth, scope overrides) cannot alias - // back into the shared providerDefinitions map. + // Deep copy ExtraAuthParams 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) - c.TokenExtraParams = tokenExtra - } // Resolve operator-supplied placeholders in the static AuthURL // (for example Vercel's "{integration_slug}"). Providers without