Skip to content

fix(sso): default tokenEndpointAuthentication to client_secret_post#3627

Open
minijeong-log wants to merge 4 commits intosimstudioai:stagingfrom
minijeong-log:fix/sso-token-endpoint-auth-default
Open

fix(sso): default tokenEndpointAuthentication to client_secret_post#3627
minijeong-log wants to merge 4 commits intosimstudioai:stagingfrom
minijeong-log:fix/sso-token-endpoint-auth-default

Conversation

@minijeong-log
Copy link
Copy Markdown

Summary

  • Default tokenEndpointAuthentication to 'client_secret_post' when not explicitly set in register-sso-provider.ts
  • Prevents invalid_client errors when client secrets contain Base64 special characters (+, =, /)

Root Cause

better-auth's SSO plugin (index.mjs:595) defaults to client_secret_basic when tokenEndpointAuthentication is undefined. In this mode, credentials are Base64-encoded without URL-encoding first, violating RFC 6749 §2.3.1. OIDC providers decode + as space, causing secret mismatch.

Changes

packages/db/scripts/register-sso-provider.ts: Fall back to 'client_secret_post' instead of passing undefined through to better-auth.

Fixes #3626

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • Other

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@cursor
Copy link
Copy Markdown

cursor bot commented Mar 17, 2026

PR Summary

Medium Risk
Touches SSO/OIDC configuration in the DB registration script; while small, it affects how client secrets are sent to IdPs and could change behavior for providers relying on the previous default.

Overview
Updates the register-sso-provider.ts DB registration script to support an optional SSO_OIDC_TOKEN_ENDPOINT_AUTH env var and to default tokenEndpointAuthentication to client_secret_post when writing OIDC provider configs.

This avoids passing undefined through to the underlying auth library (which would fall back to client_secret_basic), preventing invalid_client issues with secrets containing characters like +.

Written by Cursor Bugbot for commit f3c1335. This will update automatically on new commits. Configure here.

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 17, 2026

@minijeong-log is attempting to deploy a commit to the Sim Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 17, 2026

Greptile Summary

This PR fixes an invalid_client SSO authentication error caused by better-auth's default client_secret_basic mode, which Base64-encodes credentials without URL-encoding — causing OIDC providers to misinterpret + characters in secrets as spaces (violating RFC 6749 §2.3.1). The fix defaults tokenEndpointAuthentication to 'client_secret_post' (credentials sent in the request body, avoiding encoding issues entirely), and adds a new SSO_OIDC_TOKEN_ENDPOINT_AUTH environment variable so operators can explicitly choose the auth method.

  • Adds SSO_OIDC_TOKEN_ENDPOINT_AUTH env var parsing to buildSSOConfigFromEnv(), validated against the two accepted values (client_secret_post | client_secret_basic), with safe fallback to undefined
  • In registerSSOProvider(), applies ?? 'client_secret_post' default (using nullish coalescing as suggested in the previous thread) instead of passing undefined through to better-auth
  • Documents the new env var in the script header comment

Confidence Score: 5/5

Safe to merge — targeted one-line logic fix with clear rationale and no side effects on unrelated flows.

The change is minimal and well-motivated. The previous P2 suggestion (use ?? over ||) has been addressed. All remaining feedback is P2 style improvements only (no-warning silent fallback on invalid env var), which do not block merge.

No files require special attention.

Important Files Changed

Filename Overview
packages/db/scripts/register-sso-provider.ts Correctly defaults tokenEndpointAuthentication to client_secret_post to prevent invalid_client errors from Base64-special-char secrets; also adds env var parsing with validation and updated documentation.

Sequence Diagram

sequenceDiagram
    participant Env as Environment Vars
    participant Script as register-sso-provider.ts
    participant BA as better-auth SSO plugin
    participant IDP as OIDC Provider

    Note over Script: buildSSOConfigFromEnv()
    Env->>Script: SSO_OIDC_TOKEN_ENDPOINT_AUTH (optional)
    Script->>Script: Validate: client_secret_post or client_secret_basic or undefined

    Note over Script: registerSSOProvider()
    Script->>Script: tokenEndpointAuthentication ?? 'client_secret_post'
    Script->>BA: Register OIDC provider (explicit client_secret_post)

    BA->>IDP: POST /token (clientId + secret in request body)
    IDP-->>BA: 200 OK - token response

    Note over BA,IDP: BEFORE this fix
    BA--xIDP: Authorization: Basic base64(clientId:secret+special)<br/>IDP decodes '+' as space → secret mismatch → invalid_client
Loading

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.
Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

Reviews (2): Last reviewed commit: "fix(sso): validate SSO_OIDC_TOKEN_ENDPOI..." | Re-trigger Greptile

Comment on lines +513 to +514
tokenEndpointAuthentication:
ssoConfig.oidcConfig.tokenEndpointAuthentication || 'client_secret_post',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Prefer nullish coalescing (??) over logical OR (||)

tokenEndpointAuthentication is typed as 'client_secret_post' | 'client_secret_basic' | undefined. Both valid values are truthy strings, so || happens to work here, but ?? is semantically more precise — it only falls back when the value is null or undefined, rather than any falsy value. This makes the intent clearer and is safer if the type ever evolves.

Suggested change
tokenEndpointAuthentication:
ssoConfig.oidcConfig.tokenEndpointAuthentication || 'client_secret_post',
tokenEndpointAuthentication:
ssoConfig.oidcConfig.tokenEndpointAuthentication ?? 'client_secret_post',

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

better-auth's SSO plugin does not URL-encode credentials before Base64
encoding in client_secret_basic mode (RFC 6749 §2.3.1). When the client
secret contains special characters (+, =, /), OIDC providers decode them
incorrectly, causing invalid_client errors.

Default to client_secret_post when tokenEndpointAuthentication is not
explicitly set to avoid this upstream encoding issue.

Fixes simstudioai#3626
@minijeong-log minijeong-log force-pushed the fix/sso-token-endpoint-auth-default branch from a24f271 to 4ba09f4 Compare March 17, 2026 10:42
…hentication

- Use ?? instead of || for semantic correctness
- Add SSO_OIDC_TOKEN_ENDPOINT_AUTH env var so users can explicitly
  set client_secret_basic when their provider requires it
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Signed-off-by: Mini Jeong <mini.jeong@navercorp.com>
Replace unsafe `as` type cast with runtime validation to ensure only
'client_secret_post' or 'client_secret_basic' are accepted. Invalid
values (typos, empty strings) now fall back to undefined, letting the
downstream ?? fallback apply correctly.

Signed-off-by: Mini Jeong <mini.jeong@navercorp.com>
@waleedlatif1 waleedlatif1 deleted the branch simstudioai:staging April 3, 2026 23:01
@minijeong-log
Copy link
Copy Markdown
Author

@waleedlatif1
Hi, I noticed this PR was closed without being merged. Could you help me understand why this fix wasn't included?

The related issue (#3626) is still open, and I'm running into this invalid_client error in my environment. Is there a planned fix coming through a different approach, or is there a workaround I should use in the meantime?

Any context would be greatly appreciated!

@waleedlatif1 waleedlatif1 reopened this Apr 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants