@anshugarg15checks…fix/s2s-oauth2-no-shared-secret-in-connection → master13 files · +421 −83review: @rohanprabhureview: @sarthakreview: @acsrujanupdated 3d agoThe S2S_OAUTH2 flow merged the auth-config credentials (client_id, client_secret, JWT signing keys) into a single blob that was both used for the token exchange and templated into the persisted connection data. Because the persist-time templating ran the user-controlled credential object against a secret-bearing variable map:
leak: "{{client_secret}}" expanded to the real secret and was stored/returned under an arbitrary (non-secret-named) key, andconnection.val — redundant with the auth config and reachable on any surface that reads connection data.This is unique to S2S: OAuth2/OAuth1/DCR keep client_secret in the auth config and read it at exchange/refresh time (refreshAccessToken.ts → getDecryptedAuthConfigPayload), so it never lands in connection data.
connection.valcreateConnectedAccount, submitLinkSessionInput): the full merge is used only for the outbound exchange; persistCredentials (connection-level / user-supplied values only) is what gets persisted, verbatim (no persist-time templating). Exchange behaviour is unchanged.buildActiveConnectionData no longer writes client_id/client_secret — they appear in connection.val only when they are connection-level (bring-your-own-app providers like Bitwarden, where the user supplies them).connectionDataScheme: client_id/client_secret are now optional on the ACTIVE S2S schema — absent for shared-app configs, present only for BYO providers / pre-existing rows. (SAML / OAuth / DCR schemas are untouched.)refresh_s2s_oauth2_access_token.ts + callers): re-source client_id/client_secret from the decrypted auth config (authConfigCredentials, read from both stores — dataTypedEncrypted and encryptedSharedCredentials), merged with connection-level values. The link-token path now does the same, since the secret is no longer staged.getStringTemplateVariables/buildTemplateVariables copies were consolidated into one exported getS2STemplateVariables in s2s_oauth2_utils.ts.The leak required a specific data flow: (secret present in a template-variable map) → (templating applied to user-controlled fields) → (result persisted and returned). This PR severs every leg of that flow, and the barriers are independent — any one of them alone defeats the attack:
persistCredentials, which is built at the call site from connection-level layers only (userCredentials / staged values). The auth-config client_secret is excluded before persistence — it lives in the full credentials blob that is used solely for the outbound exchange. So connection.val cannot contain the secret to begin with.visitObjectAndTemplate was removed from the persist path; persistCredentials is stored verbatim. A smuggled leak: "{{client_secret}}" is therefore inert text — there is no resolver step on persisted data, and no current S2S toolkit stores a templated connection field (token-endpoint templating happens only in the outbound exchange).buildActiveConnectionData no longer force-writes client_id/client_secret; for a shared-app config they are not in connection.val at all. For BYO providers they are present, but only because the user supplied them themselves (connection_field) — reading those back is not a privilege escalation.Consequently the secret never reaches any read surface — masked or raw, GET / list / create response, logs, or proxy. This is strictly stronger than name-based masking or scrubbing the templating map (the prior approach), which left the secret in connection.val and depended on the redactor. Here there is simply nothing to redact: the secret was never written.
The outbound token exchange still uses the full credentials (including the secret) — but that request goes to the provider, is never persisted, and is never returned to the caller.
S2S_OAUTH2 (the createConnectedAccount/submitLinkSessionInput/refreshAccessToken branches) or lives in S2S-only files. The schema change touches only S2SOauth2ActiveConnectionDataSchema. The shared eval-field helper (getDataToResolveTemplatesWithEvalFields, used by SAML/OAuth2/DCR) and mustacheUtils are not modified.val): refresh merges authConfigCredentials with connectionVal (connection-level wins), so they keep refreshing unchanged. authConfigCredentials is optional (defaults {}) as a safety net.client_secret is connection_field): the secret stays in connection.val and is sourced from there at refresh — correct and unchanged.Two intentional behaviour changes (called out for reviewers):
client_id in their connection data, so it won't appear in the connection API response (client_secret was already masked/redacted). BYO providers still include it.reinitiateRedirectableAccount) now decrypts the auth config on every S2S refresh; if that decryption fails, refresh errors where a legacy row (secret in val) previously would not have needed it. Low risk (auth-config decryption is standard).create_s2s_oauth2_connected_account.test.ts — new: the exchange still receives the real secret, but client_id/client_secret are never persisted, a smuggled {{client_secret}} is inert, and no persisted field equals the secret.refresh_s2s_oauth2_access_token.test.ts — new: refresh sources the secret from the auth config when it is absent from connection data, and the refreshed data still doesn't carry it.pnpm check-types clean; Oxlint 0 errors; oxfmt clean.🤖 Generated with Claude Code