From caec9f1ad5a36d80df34cfafff1e6795f779178e 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?Rename=20access-review=20migration=20with=20ran?= =?UTF-8?q?dom=20time=20=E2=86=92=20Validate=20PagerDuty=20subdomain=20on?= =?UTF-8?q?=20OAuth=20callback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename access-review migration with random time - Move PagerDuty token-response handling to its own file - Strip OAuth error_description from log and redirect - Validate PagerDuty subdomain on OAuth callback Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> --- pkg/connector/oauth2.go | 26 +++------- pkg/connector/pagerduty.go | 42 ++++++++++++++++ pkg/coredata/migrations/20260508T729406Z.sql | 27 ++++++++++ pkg/server/api/console/v1/resolver.go | 53 ++++++++++++++++---- 4 files changed, 120 insertions(+), 28 deletions(-) create mode 100644 pkg/connector/pagerduty.go create mode 100644 pkg/coredata/migrations/20260508T729406Z.sql diff --git a/pkg/connector/oauth2.go b/pkg/connector/oauth2.go index 47c1aef25..9466c3b2d 100644 --- a/pkg/connector/oauth2.go +++ b/pkg/connector/oauth2.go @@ -331,29 +331,11 @@ func (c *OAuth2Connector) CompleteWithState(ctx context.Context, r *http.Request return conn, &payload.Data, err } - // PagerDuty Scoped OAuth includes the customer's subdomain in the - // token response body. We parse it here so the OAuth callback - // handler can write it to PagerDutyConnectorSettings without - // having to issue a second decode against a now-closed body. - if payload.Data.Provider == PagerDutyProvider { - var pd struct { - Subdomain string `json:"subdomain"` - } - if err := json.Unmarshal(body, &pd); err == nil && pd.Subdomain != "" { - if payload.Data.ProviderMetadata == nil { - payload.Data.ProviderMetadata = map[string]string{} - } - payload.Data.ProviderMetadata["subdomain"] = pd.Subdomain - } - } + AbsorbPagerDutyTokenResponse(&payload.Data, body) return &oauth2Conn, &payload.Data, nil } -// PagerDutyProvider is the canonical string used by PagerDuty in the -// state token's `provider` field. -const PagerDutyProvider = "PAGERDUTY" - func basicAuthHeader(clientID, clientSecret string) string { credentials := clientID + ":" + clientSecret return "Basic " + base64.StdEncoding.EncodeToString([]byte(credentials)) @@ -425,6 +407,12 @@ 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: diff --git a/pkg/connector/pagerduty.go b/pkg/connector/pagerduty.go new file mode 100644 index 000000000..dbf0dc0ef --- /dev/null +++ b/pkg/connector/pagerduty.go @@ -0,0 +1,42 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +// OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +package connector + +import "encoding/json" + +const ( + PagerDutyProvider = "PAGERDUTY" +) + +// AbsorbPagerDutyTokenResponse extracts the customer subdomain that +// PagerDuty's Scoped OAuth surfaces in the token-exchange response body +// and stuffs it into state.ProviderMetadata. The OAuth callback handler +// later writes that value to PagerDutyConnectorSettings. No-op when the +// state's provider is not PagerDuty or when the body has no subdomain. +func AbsorbPagerDutyTokenResponse(state *OAuth2State, body []byte) { + if state == nil || state.Provider != PagerDutyProvider { + return + } + var pd struct { + Subdomain string `json:"subdomain"` + } + if err := json.Unmarshal(body, &pd); err != nil || pd.Subdomain == "" { + return + } + if state.ProviderMetadata == nil { + state.ProviderMetadata = map[string]string{} + } + state.ProviderMetadata["subdomain"] = pd.Subdomain +} diff --git a/pkg/coredata/migrations/20260508T729406Z.sql b/pkg/coredata/migrations/20260508T729406Z.sql new file mode 100644 index 000000000..abc09da3d --- /dev/null +++ b/pkg/coredata/migrations/20260508T729406Z.sql @@ -0,0 +1,27 @@ +-- Copyright (c) 2026 Probo Inc . +-- +-- Permission to use, copy, modify, and/or distribute this software for any +-- purpose with or without fee is hereby granted, provided that the above +-- copyright notice and this permission notice appear in all copies. +-- +-- THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +-- REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +-- AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +-- INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +-- LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +-- OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +-- PERFORMANCE OF THIS SOFTWARE. + +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'GITLAB'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'BITBUCKET'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'HEROKU'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'PAGERDUTY'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'ASANA'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'SNYK'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'NETLIFY'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'RAMP'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'CLICKUP'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'VERCEL'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'MONDAY'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'LEVER'; +ALTER TYPE connector_provider ADD VALUE IF NOT EXISTS 'DEEL'; diff --git a/pkg/server/api/console/v1/resolver.go b/pkg/server/api/console/v1/resolver.go index d8f8751ba..df2364869 100644 --- a/pkg/server/api/console/v1/resolver.go +++ b/pkg/server/api/console/v1/resolver.go @@ -205,12 +205,28 @@ func handleConnectorComplete( Connection: connection, } - // PagerDuty Scoped OAuth surfaces the customer's subdomain - // in the token response body; CompleteWithState parsed it - // into state.ProviderMetadata. Persist it on the connector + // PagerDuty Scoped OAuth surfaces the customer's subdomain as + // a `subdomain` query parameter on the redirect URL (not in + // the token response body). Persist it on the connector // settings so the driver and name resolver can read it. if connectorProvider == coredata.ConnectorProviderPagerDuty { - if subdomain := state.ProviderMetadata["subdomain"]; subdomain != "" { + subdomain := query.Get("subdomain") + if subdomain == "" { + // Fall back to ProviderMetadata for older OAuth flows + // that may have surfaced the subdomain through the + // token response body. + subdomain = state.ProviderMetadata["subdomain"] + } + // The subdomain comes from an attacker-influenceable + // callback parameter; refuse anything that isn't a valid + // DNS label so it cannot be smuggled into URLs or logs. + if subdomain != "" && !isValidPagerDutySubdomain(subdomain) { + logger.WarnCtx(r.Context(), "rejecting invalid pagerduty subdomain", + log.String("provider", string(connectorProvider)), + ) + subdomain = "" + } + if subdomain != "" { createReq.PagerDutySettings = &coredata.PagerDutyConnectorSettings{ Subdomain: subdomain, } @@ -276,7 +292,6 @@ func handleConnectorOAuth2Error( query url.Values, ) { oauthErr := query.Get("error") - oauthErrDesc := query.Get("error_description") provider := "unknown" redirectURL := baseURL.String() @@ -291,18 +306,17 @@ func handleConnectorOAuth2Error( } } + // Provider error_description fields routinely carry PII (user emails, + // account names) and must never reach logs or the client redirect URL. + // Forward only the standardized error code. logger.WarnCtx(r.Context(), "OAuth2 callback returned error", log.String("provider", provider), log.String("error", oauthErr), - log.String("error_description", oauthErrDesc), ) parsedURL, _ := url.Parse(redirectURL) q := parsedURL.Query() q.Set("error", oauthErr) - if oauthErrDesc != "" { - q.Set("error_description", oauthErrDesc) - } parsedURL.RawQuery = q.Encode() safeRedirect.Redirect(w, r, parsedURL.String(), "/", http.StatusSeeOther) @@ -345,6 +359,27 @@ func fetchVercelUserID(ctx context.Context, accessToken string) (string, error) return body.User.ID, nil } +// isValidPagerDutySubdomain reports whether s is a single DNS label +// (RFC 1035 §2.3.1). PagerDuty subdomains are tenant identifiers that +// will be embedded in API URLs; the OAuth callback is the only place +// where a malformed value can enter the system. +func isValidPagerDutySubdomain(s string) bool { + if s == "" || len(s) > 63 { + return false + } + for _, c := range s { + switch { + case c >= 'a' && c <= 'z': + case c >= 'A' && c <= 'Z': + case c >= '0' && c <= '9': + case c == '-': + default: + return false + } + } + return true +} + func (r *Resolver) ProboService(ctx context.Context, tenantID gid.TenantID) *probo.TenantService { return r.probo.WithTenant(tenantID) }