Rename access-review migration with random time → Validate PagerDuty subdomain on OAuth callback

- 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

@@ -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
+	AbsorbPagerDutyTokenResponse(&payload.Data, body)
-	// 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
-		}
-	}
 
 	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:

pkg/connector/pagerduty.go

@@ -0,0 +1,42 @@
+// Copyright (c) 2026 Probo Inc <hello@getprobo.com>.
+//
+// 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
+}

pkg/coredata/migrations/20260508T729406Z.sql

@@ -0,0 +1,27 @@
+-- Copyright (c) 2026 Probo Inc <hello@getprobo.com>.
+--
+-- 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';

pkg/server/api/console/v1/resolver.go

@@ -205,12 +205,28 @@ func handleConnectorComplete(
 				Connection:     connection,
 			}
 
-			// PagerDuty Scoped OAuth surfaces the customer's subdomain
+			// PagerDuty Scoped OAuth surfaces the customer's subdomain as
-			// in the token response body; CompleteWithState parsed it
+			// a `subdomain` query parameter on the redirect URL (not in
-			// into state.ProviderMetadata. Persist it on the connector
+			// 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)
 }