From cb1cbfa4ccba1ce13f7fea419a6fc37dcbdc2f15 Mon Sep 17 00:00:00 2001 From: Bereket Engida <86073083+Bekacru@users.noreply.github.com> Date: Tue, 9 Jun 2026 19:46:12 -0700 Subject: [PATCH] fix: address bug findings across packages (#9974) --- .../admin-protected-field-permissions.md | 5 + .changeset/api-key-create-fresh-session.md | 5 + .changeset/api-key-verify-stale-write.md | 5 + .changeset/electron-require-s256-pkce.md | 5 + .../facebook-access-token-app-binding.md | 5 + .../generic-oauth-require-account-id.md | 5 + .changeset/google-enforce-hosted-domain.md | 5 + .changeset/jwks-cache-per-source.md | 5 + .../oauth-continue-postlogin-revalidate.md | 5 + .changeset/oauth-introspect-require-azp.md | 5 + .../oauth-provider-client-grant-types.md | 5 + .../organization-invitation-team-scope.md | 5 + .changeset/paypal-verify-id-token.md | 5 + .changeset/reddit-require-real-email.md | 5 + .../refresh-token-stale-account-cookie.md | 5 + .changeset/scim-existing-user-linking.md | 5 + .changeset/siwe-bind-message-to-nonce.md | 7 + .changeset/sso-oidc-endpoint-resolution.md | 5 + .changeset/sso-providerid-namespace.md | 8 + .../verify-remote-introspection-audience.md | 5 + .cspell/tech-terms.txt | 1 + .github/workflows/auto-changeset.yml | 25 +- .gitignore | 2 + docs/content/docs/plugins/admin.mdx | 4 +- docs/content/docs/plugins/siwe.mdx | 6 +- docs/content/docs/plugins/sso.mdx | 8 +- knip.jsonc | 3 +- packages/api-key/src/api-key.test.ts | 190 +++++++++++ packages/api-key/src/routes/create-api-key.ts | 10 +- packages/api-key/src/routes/verify-api-key.ts | 21 +- .../src/api/routes/account.test.ts | 129 +++++++ .../better-auth/src/api/routes/account.ts | 20 +- .../better-auth/src/api/routes/callback.ts | 7 +- .../better-auth/src/context/create-context.ts | 2 +- .../better-auth/src/cookies/cookies.test.ts | 55 +++ packages/better-auth/src/cookies/index.ts | 12 +- .../better-auth/src/oauth2/link-account.ts | 16 +- .../src/plugins/admin/access/statement.ts | 2 + .../src/plugins/admin/admin.test.ts | 276 +++++++++++++++ .../src/plugins/admin/error-codes.ts | 4 + .../better-auth/src/plugins/admin/routes.ts | 151 ++++++++- .../src/plugins/email-otp/email-otp.test.ts | 120 +++++++ .../src/plugins/email-otp/routes.ts | 6 +- .../generic-oauth/generic-oauth.test.ts | 164 +++++++++ .../src/plugins/generic-oauth/routes.ts | 25 +- .../last-login-method.test.ts | 47 ++- .../organization/routes/crud-invites.test.ts | 229 +++++++++++++ .../organization/routes/crud-invites.ts | 32 ++ .../plugins/organization/routes/crud-team.ts | 43 ++- .../src/plugins/organization/team.test.ts | 127 +++++++ .../better-auth/src/plugins/siwe/index.ts | 58 ++++ .../src/plugins/siwe/parse-message.ts | 102 ++++++ .../better-auth/src/plugins/siwe/siwe.test.ts | 320 +++++++++++++----- packages/better-auth/src/social.test.ts | 192 +++++++++++ packages/cli/src/commands/info.ts | 4 + packages/cli/test/info.test.ts | 40 +++ packages/core/src/env/env-impl.test.ts | 61 ++++ packages/core/src/env/env-impl.ts | 3 +- packages/core/src/oauth2/verify.test.ts | 195 +++++++++++ packages/core/src/oauth2/verify.ts | 73 +++- .../src/social-providers/facebook.test.ts | 207 +++++++++++ .../core/src/social-providers/facebook.ts | 77 ++++- .../core/src/social-providers/google.test.ts | 189 +++++++++++ packages/core/src/social-providers/google.ts | 28 +- packages/core/src/social-providers/paypal.ts | 95 +++++- packages/core/src/social-providers/reddit.ts | 6 +- packages/electron/src/error-codes.ts | 1 + packages/electron/src/index.ts | 21 +- packages/electron/src/routes.ts | 61 ++-- packages/electron/test/electron.test.ts | 160 +++++++-- packages/oauth-provider/src/authorize.ts | 13 + .../oauth-provider/src/client-resource.ts | 14 + packages/oauth-provider/src/continue.ts | 18 +- .../oauth-provider/src/introspect.test.ts | 143 ++++++++ packages/oauth-provider/src/introspect.ts | 40 ++- packages/oauth-provider/src/oauth.test.ts | 135 ++++++++ packages/oauth-provider/src/revoke.test.ts | 5 + packages/oauth-provider/src/token.test.ts | 216 ++++++++++++ packages/oauth-provider/src/token.ts | 9 + packages/oauth-provider/src/utils/index.ts | 37 ++ packages/scim/src/index.ts | 2 +- packages/scim/src/routes.ts | 121 ++++++- packages/scim/src/scim-users.test.ts | 66 +++- packages/scim/src/scim.management.test.ts | 41 +++ packages/scim/src/scim.test.ts | 83 ++++- packages/scim/src/types.ts | 53 +++ packages/sso/src/oidc/discovery.test.ts | 160 +++++++++ packages/sso/src/oidc/discovery.ts | 136 +++++++- packages/sso/src/providers.test.ts | 114 +++++++ packages/sso/src/routes/saml-pipeline.ts | 12 +- packages/sso/src/routes/sso.ts | 27 ++ packages/sso/src/saml.test.ts | 49 +-- 92 files changed, 4913 insertions(+), 316 deletions(-) create mode 100644 .changeset/admin-protected-field-permissions.md create mode 100644 .changeset/api-key-create-fresh-session.md create mode 100644 .changeset/api-key-verify-stale-write.md create mode 100644 .changeset/electron-require-s256-pkce.md create mode 100644 .changeset/facebook-access-token-app-binding.md create mode 100644 .changeset/generic-oauth-require-account-id.md create mode 100644 .changeset/google-enforce-hosted-domain.md create mode 100644 .changeset/jwks-cache-per-source.md create mode 100644 .changeset/oauth-continue-postlogin-revalidate.md create mode 100644 .changeset/oauth-introspect-require-azp.md create mode 100644 .changeset/oauth-provider-client-grant-types.md create mode 100644 .changeset/organization-invitation-team-scope.md create mode 100644 .changeset/paypal-verify-id-token.md create mode 100644 .changeset/reddit-require-real-email.md create mode 100644 .changeset/refresh-token-stale-account-cookie.md create mode 100644 .changeset/scim-existing-user-linking.md create mode 100644 .changeset/siwe-bind-message-to-nonce.md create mode 100644 .changeset/sso-oidc-endpoint-resolution.md create mode 100644 .changeset/sso-providerid-namespace.md create mode 100644 .changeset/verify-remote-introspection-audience.md create mode 100644 packages/better-auth/src/plugins/siwe/parse-message.ts create mode 100644 packages/core/src/env/env-impl.test.ts create mode 100644 packages/core/src/social-providers/facebook.test.ts create mode 100644 packages/core/src/social-providers/google.test.ts diff --git a/.changeset/admin-protected-field-permissions.md b/.changeset/admin-protected-field-permissions.md new file mode 100644 index 0000000000..291d20b3f0 --- /dev/null +++ b/.changeset/admin-protected-field-permissions.md @@ -0,0 +1,5 @@ +--- +"better-auth": patch +--- + +Guard protected user fields in the admin plugin behind their dedicated permissions. `/admin/create-user` now requires `user:set-role` when a `role` is supplied (top-level or via `data.role`), validates requested roles against the configured roles, requires `user:ban` for ban fields passed in `data`, and no longer lets `data` override `email`, `name`, or `role`. `/admin/update-user` now requires `user:ban` for `banned`/`banReason`/`banExpires` (revoking the user's sessions when banning and rejecting self-bans), requires the new `user:set-email` permission for `email`/`emailVerified` (with email validation, lowercasing, and uniqueness checks), and rejects `password` updates in favor of `/admin/set-user-password`. If you use a custom access control, add `set-email` to your statements and grant it (and `ban`) to roles that should be able to change those fields through `update-user`. diff --git a/.changeset/api-key-create-fresh-session.md b/.changeset/api-key-create-fresh-session.md new file mode 100644 index 0000000000..b775840c63 --- /dev/null +++ b/.changeset/api-key-create-fresh-session.md @@ -0,0 +1,5 @@ +--- +"@better-auth/api-key": patch +--- + +Resolve the session from the authoritative store when creating an API key, rather than the signed cookie-cache snapshot. `/api-key/create` now fetches the session with `disableCookieCache: true`, so a session that was revoked — including the session deletion performed when a user is banned — is no longer accepted within the cookie-cache window. Server-side calls that pass `userId` in the body are unaffected. diff --git a/.changeset/api-key-verify-stale-write.md b/.changeset/api-key-verify-stale-write.md new file mode 100644 index 0000000000..477438d3cc --- /dev/null +++ b/.changeset/api-key-verify-stale-write.md @@ -0,0 +1,5 @@ +--- +"@better-auth/api-key": patch +--- + +Fix API key verification overwriting concurrent changes with stale state. Verification now persists only the fields it mutates (rate-limit counters, remaining count, refill and request timestamps) instead of writing back the entire record it read, so a concurrent disable, permission change, or expiry update can no longer be reverted by an in-flight verification. In secondary-storage mode, the record is re-read and merged before writing, so a key deleted mid-verification is no longer recreated. diff --git a/.changeset/electron-require-s256-pkce.md b/.changeset/electron-require-s256-pkce.md new file mode 100644 index 0000000000..706de643c4 --- /dev/null +++ b/.changeset/electron-require-s256-pkce.md @@ -0,0 +1,5 @@ +--- +"@better-auth/electron": patch +--- + +Require S256 PKCE for Electron auth transfers. The transfer flow previously defaulted a missing `code_challenge_method` to `plain` and the token exchange fell back to a plain string comparison for any non-`s256` method. In `plain` mode the verifier equals the challenge, which already appears in the sign-in URL query, so the comparison did not add a meaningful check. Missing, unknown, and `plain` methods are now rejected at minting (`handleTransfer`, `/electron/transfer-user`, `/electron/init-oauth-proxy`) and at exchange (`/electron/token`). The official Electron client already uses S256, so existing flows are unaffected. diff --git a/.changeset/facebook-access-token-app-binding.md b/.changeset/facebook-access-token-app-binding.md new file mode 100644 index 0000000000..1c1f562b0e --- /dev/null +++ b/.changeset/facebook-access-token-app-binding.md @@ -0,0 +1,5 @@ +--- +"@better-auth/core": patch +--- + +Validate Facebook opaque access tokens against the configured app. Previously `verifyIdToken` returned `true` for any non-JWT token and `getUserInfo` called Graph `/me` with the caller-supplied token without checking which app issued it, so tokens issued for other Facebook apps were not distinguished on the direct sign-in path. Facebook tokens are now inspected via the `debug_token` endpoint, requiring `is_valid`, an `app_id` that matches one of the configured client ids, and a `user_id` that matches the returned profile, before the token is accepted. A client secret must be configured for access-token sign-in to work. diff --git a/.changeset/generic-oauth-require-account-id.md b/.changeset/generic-oauth-require-account-id.md new file mode 100644 index 0000000000..ac40a34195 --- /dev/null +++ b/.changeset/generic-oauth-require-account-id.md @@ -0,0 +1,5 @@ +--- +"better-auth": patch +--- + +Require a provider account id when signing in through generic OAuth. The default userinfo handler previously fell back to an empty string when the provider response had no `sub` (or `id`), and the callback never checked the resolved account id. With certain non-OIDC providers that omit `sub`, accounts could be stored under the same empty id and a later sign-in could resolve to an existing account. The generic OAuth callback now rejects sign-in when no account id can be resolved, the default userinfo handler returns no profile when neither `sub` nor `id` is present, and the built-in OAuth callback also rejects an empty account id. diff --git a/.changeset/google-enforce-hosted-domain.md b/.changeset/google-enforce-hosted-domain.md new file mode 100644 index 0000000000..aef98ee4d1 --- /dev/null +++ b/.changeset/google-enforce-hosted-domain.md @@ -0,0 +1,5 @@ +--- +"@better-auth/core": patch +--- + +Enforce the Google `hd` (hosted domain) option against the id token. Previously `hd` was only sent to Google as an authorization hint, which does not by itself restrict sign-in to the configured Workspace domain. When `hd` is set, the `hd` claim on the verified id token (`verifyIdToken`) and the decoded callback profile (`getUserInfo`) must be present and match, otherwise sign-in is rejected. diff --git a/.changeset/jwks-cache-per-source.md b/.changeset/jwks-cache-per-source.md new file mode 100644 index 0000000000..6f1fdbf2a7 --- /dev/null +++ b/.changeset/jwks-cache-per-source.md @@ -0,0 +1,5 @@ +--- +"@better-auth/core": patch +--- + +Scope the JWKS cache per source. Access-token verification previously kept a single global key set and reused it whenever it contained a key matching the token's `kid`, without considering which JWKS source the verification was for. When verifying tokens against more than one source, a token could end up matched against keys fetched for a different source if the two shared a `kid`. The cache is now keyed per JWKS source and honors a TTL, so each verification uses the keys for its own source and rotated or removed keys are no longer used after the TTL elapses. diff --git a/.changeset/oauth-continue-postlogin-revalidate.md b/.changeset/oauth-continue-postlogin-revalidate.md new file mode 100644 index 0000000000..00f535bea8 --- /dev/null +++ b/.changeset/oauth-continue-postlogin-revalidate.md @@ -0,0 +1,5 @@ +--- +"@better-auth/oauth-provider": patch +--- + +The `/oauth2/continue` post-login step no longer treats the client-submitted `postLogin` flag as proof that an interactive gate completed. Completion is now derived from the server-issued, session-bound marker on the signed `oauth_query` (matching the consent endpoint); when it is absent, `authorize` re-runs `postLogin.shouldRedirect` against the current session and redirects back to the gate if selection is still required. diff --git a/.changeset/oauth-introspect-require-azp.md b/.changeset/oauth-introspect-require-azp.md new file mode 100644 index 0000000000..a7034a16a4 --- /dev/null +++ b/.changeset/oauth-introspect-require-azp.md @@ -0,0 +1,5 @@ +--- +"@better-auth/oauth-provider": patch +--- + +Token introspection now requires JWT access tokens to carry an `azp` (client) claim and resolve to an enabled client before being reported as active. This ensures only tokens issued by the OAuth token endpoint are treated as access tokens, since the JWT plugin can mint session JWTs that share the same issuer, audience, and signing keys. diff --git a/.changeset/oauth-provider-client-grant-types.md b/.changeset/oauth-provider-client-grant-types.md new file mode 100644 index 0000000000..b58b490b11 --- /dev/null +++ b/.changeset/oauth-provider-client-grant-types.md @@ -0,0 +1,5 @@ +--- +"@better-auth/oauth-provider": patch +--- + +Enforce per-client grant types at the token endpoint. Previously only the provider-wide `grantTypes` allowlist was checked, so a client registered for `authorization_code` could still request `client_credentials` tokens, turning a user-delegated client into a machine-to-machine client. The `client_credentials` and `authorization_code` grants are now rejected with `unauthorized_client` unless the client declares them. Refresh tokens remain available to any client permitted the `authorization_code` grant (gated by `offline_access`), but are no longer issued to pure `client_credentials` clients. Clients with no recorded `grantTypes` fall back to `["authorization_code"]`, matching the registration default. diff --git a/.changeset/organization-invitation-team-scope.md b/.changeset/organization-invitation-team-scope.md new file mode 100644 index 0000000000..60552f44b7 --- /dev/null +++ b/.changeset/organization-invitation-team-scope.md @@ -0,0 +1,5 @@ +--- +"better-auth": patch +--- + +Scope organization invitation team IDs to the invited organization. `createInvitation` now validates that every requested `teamId` belongs to the invitation's organization regardless of whether `teams.maximumMembersPerTeam` is set, and `acceptInvitation` re-checks each stored team's organization before adding team membership. Previously, with the default unlimited team size, a team ID from another organization could be stored on an invitation and applied on acceptance. diff --git a/.changeset/paypal-verify-id-token.md b/.changeset/paypal-verify-id-token.md new file mode 100644 index 0000000000..9a98659d22 --- /dev/null +++ b/.changeset/paypal-verify-id-token.md @@ -0,0 +1,5 @@ +--- +"@better-auth/core": patch +--- + +Cryptographically verify PayPal ID tokens on direct sign-in. Previously `verifyIdToken` only decoded the JWT and checked that a `sub` claim was present, performing no signature, issuer, audience, or expiration checks, so any well-formed token paired with a valid access token would be accepted. The token is now verified against PayPal's issuer and published JWKS (RS256) or the client secret (HS256), with the `aud` pinned to the configured `clientId`, a `maxTokenAge` bound, and the `nonce` checked when supplied. diff --git a/.changeset/reddit-require-real-email.md b/.changeset/reddit-require-real-email.md new file mode 100644 index 0000000000..b307b998c6 --- /dev/null +++ b/.changeset/reddit-require-real-email.md @@ -0,0 +1,5 @@ +--- +"@better-auth/core": patch +--- + +Stop mapping the Reddit `oauth_client_id` to the user's email. Reddit's `identity` scope does not return an email address, and the provider previously stored `oauth_client_id` (which identifies the OAuth application and is the same for every user of the app) as `user.email` with `has_verified_email` as `emailVerified`. This collapsed all Reddit users of the same app onto a single "verified" email, which could enable implicit account linking/takeover. The Reddit provider now uses the email returned from `mapProfileToUser` when provided, otherwise falls back to a unique per-user synthetic address (`@reddit.com`), and no longer marks it as verified. Provide a real email via `mapProfileToUser` if you need the actual address. diff --git a/.changeset/refresh-token-stale-account-cookie.md b/.changeset/refresh-token-stale-account-cookie.md new file mode 100644 index 0000000000..e8f182e4ba --- /dev/null +++ b/.changeset/refresh-token-stale-account-cookie.md @@ -0,0 +1,5 @@ +--- +"better-auth": patch +--- + +Require `/refresh-token` to only trust the account cookie when its `userId`, `providerId` and (when supplied) `accountId` match the resolved session user. \ No newline at end of file diff --git a/.changeset/scim-existing-user-linking.md b/.changeset/scim-existing-user-linking.md new file mode 100644 index 0000000000..c28549a2a3 --- /dev/null +++ b/.changeset/scim-existing-user-linking.md @@ -0,0 +1,5 @@ +--- +"@better-auth/scim": patch +--- + +SCIM user provisioning no longer links to a pre-existing user by matching email alone. When a user with the same email already exists, `createSCIMUser` now returns `409` (uniqueness) unless the new `linkExistingUsers` option explicitly opts in (via `true`, `trustedDomains`, `requireExistingOrgMembership`, or a `shouldLinkUser` callback). Additionally, an organization-scoped SCIM `DELETE` now deprovisions the user — removing their organization membership and the SCIM account link — instead of deleting the global Better Auth user. A new `canGenerateToken` option lets applications authorize SCIM token creation, including restricting personal (non-org) tokens. diff --git a/.changeset/siwe-bind-message-to-nonce.md b/.changeset/siwe-bind-message-to-nonce.md new file mode 100644 index 0000000000..f5eac21be8 --- /dev/null +++ b/.changeset/siwe-bind-message-to-nonce.md @@ -0,0 +1,7 @@ +--- +"better-auth": patch +--- + +Bind the SIWE signed message to server state before creating a session. Previously `/siwe/verify` only checked that a nonce row existed for the wallet address and then delegated entirely to `verifyMessage`. Since the documented `verifyMessage` (viem) performs signature recovery only — without inspecting the message body — a signature the wallet produced for a different message (an earlier nonce, another domain, or arbitrary content) could also satisfy verification against a freshly minted nonce. + +The plugin now parses the ERC-4361 message itself and requires its nonce, domain, address, and chain ID to match the server-issued nonce and configured `domain`, and enforces the message's `Expiration Time` / `Not Before` bounds, before verifying the signature. `message` must now be a valid ERC-4361 message (which all standard SIWE clients produce); non-conforming or mismatched messages are rejected with a 401 (`UNAUTHORIZED_SIWE_MESSAGE_MISMATCH`, `UNAUTHORIZED_SIWE_MESSAGE_EXPIRED`, or `UNAUTHORIZED_SIWE_MESSAGE_NOT_YET_VALID`). `verifyMessage` implementations should continue to perform signature recovery only. diff --git a/.changeset/sso-oidc-endpoint-resolution.md b/.changeset/sso-oidc-endpoint-resolution.md new file mode 100644 index 0000000000..1e036d178c --- /dev/null +++ b/.changeset/sso-oidc-endpoint-resolution.md @@ -0,0 +1,5 @@ +--- +"@better-auth/sso": patch +--- + +Validate OIDC endpoints fetched server-side (token, userinfo, jwks) at request time by resolving the hostname and rejecting any host that resolves to a non-publicly-routable address. Discovery and userinfo requests no longer auto-follow redirects to unvalidated hosts. Operator-allowlisted origins (`trustedOrigins`) remain exempt for internal IdPs. diff --git a/.changeset/sso-providerid-namespace.md b/.changeset/sso-providerid-namespace.md new file mode 100644 index 0000000000..4af5a3736e --- /dev/null +++ b/.changeset/sso-providerid-namespace.md @@ -0,0 +1,8 @@ +--- +"@better-auth/sso": patch +"better-auth": patch +--- + +Separate SSO provider ids from the account-linking provider namespace used for social/OAuth providers. Previously an SSO provider registered with an id matching a configured `accountLinking.trustedProviders` entry (e.g. `google`) was treated as a trusted provider and could implicitly link to an existing verified account with the same email. + +SSO registration now rejects provider ids that collide with a configured social provider, a `trustedProviders` entry, or a reserved built-in id. In addition, the OIDC and SAML callbacks no longer derive trust from a `trustedProviders` name match — SSO trust comes solely from verified domain ownership (`domainVerified`). `handleOAuthUserInfo` gains a `trustProviderByName` option (default `true`, preserving social-provider behavior) that the SSO plugin sets to `false`. diff --git a/.changeset/verify-remote-introspection-audience.md b/.changeset/verify-remote-introspection-audience.md new file mode 100644 index 0000000000..deb9564728 --- /dev/null +++ b/.changeset/verify-remote-introspection-audience.md @@ -0,0 +1,5 @@ +--- +"@better-auth/core": patch +--- + +Fix `verifyAccessToken` silently dropping the configured audience check during remote introspection. Previously, when a required `audience` was set in `verifyOptions` but the introspection response omitted the `aud` claim, audience validation was skipped and any active token from the issuer was accepted — so a token issued for a different resource or client on the same issuer could also pass verification. Verification now requires the claim: a missing or mismatching `aud` is rejected. Authorization servers that legitimately omit `aud` from introspection responses (it is OPTIONAL per RFC 7662) can opt back into the old behavior with the new `remoteVerify.allowMissingAudience: true` flag, which still rejects mismatching audiences. diff --git a/.cspell/tech-terms.txt b/.cspell/tech-terms.txt index ac53ea3e49..d243d010e6 100644 --- a/.cspell/tech-terms.txt +++ b/.cspell/tech-terms.txt @@ -67,3 +67,4 @@ febf fdff fffe vbscript +unvalidated diff --git a/.github/workflows/auto-changeset.yml b/.github/workflows/auto-changeset.yml index 0c0ec4a05f..c8344f9104 100644 --- a/.github/workflows/auto-changeset.yml +++ b/.github/workflows/auto-changeset.yml @@ -256,7 +256,16 @@ jobs: IS_FORK=$(echo "$PR_JSON" | jq -r '.isCrossRepository') echo "head_ref=$HEAD_REF" >> "$GITHUB_OUTPUT" - if [[ "$HEAD_REF" == "main" || "$HEAD_REF" == "next" || "$HEAD_REF" == release/* ]]; then + # The ref is later interpolated into REST API paths + # (git/ref/heads/${HEAD_REF}). URI-reserved characters such as '#' or + # '%' can pass the protected-branch string checks below but resolve to + # a DIFFERENT ref once placed in a URL ('#' truncates as a fragment, + # '%2F' decodes to '/'). Reject anything outside a conservative + # branch-name allowlist so the guards operate on the literal ref. + if [[ ! "$HEAD_REF" =~ ^[A-Za-z0-9._/-]+$ ]]; then + echo "blocked=true" >> "$GITHUB_OUTPUT" + echo "blocked_reason=Refusing to commit to branch with unsupported characters (${HEAD_REF}) — only letters, digits, '.', '_', '/', and '-' are allowed." >> "$GITHUB_OUTPUT" + elif [[ "$HEAD_REF" == "main" || "$HEAD_REF" == "next" || "$HEAD_REF" == release/* ]]; then echo "blocked=true" >> "$GITHUB_OUTPUT" echo "blocked_reason=Cannot commit to protected branch (${HEAD_REF}) — this would trigger release automation." >> "$GITHUB_OUTPUT" elif [[ "$IS_FORK" == "true" ]]; then @@ -313,8 +322,18 @@ jobs: # This adds pr-N.md and deletes any superseded changeset files # in one commit, preventing duplicate changesets on the branch. - # Get branch head - COMMIT_SHA=$(gh api "repos/${GITHUB_REPOSITORY}/git/ref/heads/${HEAD_REF}" --jq '.object.sha') + # Get branch head. Verify the API resolved the EXACT ref we asked for: + # if URL semantics caused the path to resolve to a different branch + # (e.g. a fragment or decoded slash), `.ref` won't match and we abort + # before mutating anything. This is an additional check on top of the + # branch-name allowlist enforced in the "Get PR details" step. + REF_JSON=$(gh api "repos/${GITHUB_REPOSITORY}/git/ref/heads/${HEAD_REF}") + RESOLVED_REF=$(echo "$REF_JSON" | jq -r '.ref') + if [ "$RESOLVED_REF" != "refs/heads/${HEAD_REF}" ]; then + echo "::error::Ref mismatch: requested 'refs/heads/${HEAD_REF}' but API resolved '${RESOLVED_REF}'. Aborting." + exit 1 + fi + COMMIT_SHA=$(echo "$REF_JSON" | jq -r '.object.sha') TREE_SHA=$(gh api "repos/${GITHUB_REPOSITORY}/git/commits/${COMMIT_SHA}" --jq '.tree.sha') # Create blob for the new changeset diff --git a/.gitignore b/.gitignore index 15ff0d887f..cc478b4614 100644 --- a/.gitignore +++ b/.gitignore @@ -211,3 +211,5 @@ state.txt .cursor/ .claude/worktrees + +.deepsec \ No newline at end of file diff --git a/docs/content/docs/plugins/admin.mdx b/docs/content/docs/plugins/admin.mdx index 70ae810b5e..80a8bfd252 100644 --- a/docs/content/docs/plugins/admin.mdx +++ b/docs/content/docs/plugins/admin.mdx @@ -441,10 +441,10 @@ By default, there are two roles: ### Permissions -By default, there are two resources with up to six permissions. +By default, there are two resources with the following permissions. **user**: -`create` `list` `set-role` `ban` `impersonate` `impersonate-admins` `delete` `set-password` +`create` `list` `set-role` `ban` `impersonate` `impersonate-admins` `delete` `set-password` `set-email` `get` `update` **session**: `list` `revoke` `delete` diff --git a/docs/content/docs/plugins/siwe.mdx b/docs/content/docs/plugins/siwe.mdx index e6cb72c195..e545908f2e 100644 --- a/docs/content/docs/plugins/siwe.mdx +++ b/docs/content/docs/plugins/siwe.mdx @@ -122,6 +122,10 @@ if (data) { } ``` + + `message` must be a valid [ERC-4361](https://eips.ethereum.org/EIPS/eip-4361) message (which every standard SIWE client produces). Before accepting the signature, the plugin parses the message and requires its **nonce**, **domain**, **address**, and **Chain ID** to match the server-issued nonce and your configured `domain`, and it honors the message's `Expiration Time` / `Not Before` bounds. Signature recovery alone is **not** sufficient — this binding ensures a signature is only accepted together with the message it was produced for, bound to the current server-issued nonce. Verification fails with a 401 (`UNAUTHORIZED_SIWE_MESSAGE_MISMATCH`) if any field doesn't match. + + ### Chain-Specific Examples Here are examples for different blockchain networks: @@ -188,7 +192,7 @@ The SIWE plugin accepts the following configuration options: * **emailDomainName**: The email domain name for creating user accounts when not using anonymous mode. Defaults to the domain from your base URL * **anonymous**: Whether to allow anonymous sign-ins without requiring an email. Default is `true` * **getNonce**: Function to generate a unique nonce for each sign-in attempt. You must implement this function to return a cryptographically secure random string. Must return a `Promise` -* **verifyMessage**: Function to verify the signed SIWE message. Receives message details and should return `Promise` +* **verifyMessage**: Function to verify the signature over the SIWE message. It only needs to perform signature recovery for the supplied address (e.g. viem's `verifyMessage`) and return `Promise` — the plugin independently validates the message's nonce, domain, address, Chain ID, and time bounds before creating a session * **ensLookup**: Optional function to lookup ENS names and avatars for Ethereum addresses ### Client Options diff --git a/docs/content/docs/plugins/sso.mdx b/docs/content/docs/plugins/sso.mdx index a33c8b0f76..b8da12e9da 100644 --- a/docs/content/docs/plugins/sso.mdx +++ b/docs/content/docs/plugins/sso.mdx @@ -178,7 +178,13 @@ This allows most endpoint-related fields in `oidcConfig` to be **optional** — ```ts type registerSSOProvider = { /** - * Unique identifier for the provider + * Unique identifier for the provider. + * + * Must not collide with a configured social provider, an + * `accountLinking.trustedProviders` entry, or a reserved built-in id + * (e.g. `credential`). Registration is rejected (422) otherwise, since + * SSO provider ids share the account-linking provider namespace and a + * collision could otherwise inherit trust meant for that provider. */ providerId: string = "okta" /** diff --git a/knip.jsonc b/knip.jsonc index 683a6f5cee..b4b4d35cba 100644 --- a/knip.jsonc +++ b/knip.jsonc @@ -16,7 +16,8 @@ "ignoreIssues": { "packages/cli/src/commands/init/index.ts": ["exports"], "packages/cli/src/commands/init/utility/imports.ts": ["exports"], - "packages/sso/src/saml/assertions.ts": ["exports"] + "packages/sso/src/saml/assertions.ts": ["exports"], + "packages/sso/src/oidc/discovery.ts": ["exports"] }, "ignoreDependencies": ["@changesets/cli", "@vitest/coverage-istanbul"], "rules": { diff --git a/packages/api-key/src/api-key.test.ts b/packages/api-key/src/api-key.test.ts index b81e52d8e4..fc7394580c 100644 --- a/packages/api-key/src/api-key.test.ts +++ b/packages/api-key/src/api-key.test.ts @@ -4934,6 +4934,130 @@ describe("api-key", async () => { }); }); +describe("verify should not write back stale state", async () => { + let onValidate: (() => Promise) | null = null; + const customAPIKeyValidator = async () => { + const fn = onValidate; + onValidate = null; + if (fn) await fn(); + return true; + }; + + describe("database storage", async () => { + const { auth, signInWithTestUser } = await getTestInstance( + { + plugins: [apiKey({ customAPIKeyValidator })], + }, + { + clientOptions: { + plugins: [apiKeyClient()], + }, + }, + ); + + it("should not re-enable a key disabled during verification", async () => { + const { headers } = await signInWithTestUser(); + const created = await auth.api.createApiKey({ body: {}, headers }); + + onValidate = async () => { + await auth.api.updateApiKey({ + body: { keyId: created.id, enabled: false }, + headers, + }); + }; + + const result = await auth.api.verifyApiKey({ + body: { key: created.key }, + }); + // the verification read the key before the disable, so it passes, + // but its write-back must not revert the disable + expect(result.valid).toBe(true); + + const stored = await auth.api.getApiKey({ + query: { id: created.id }, + headers, + }); + expect(stored.enabled).toBe(false); + }); + }); + + describe("secondary storage", async () => { + const store = new Map(); + const secondaryStorage: SecondaryStorage = { + set(key, value) { + store.set(key, value); + }, + get(key) { + return store.get(key) || null; + }, + delete(key) { + store.delete(key); + }, + }; + + const { auth, signInWithTestUser } = await getTestInstance( + { + secondaryStorage, + plugins: [ + apiKey({ storage: "secondary-storage", customAPIKeyValidator }), + ], + }, + { + clientOptions: { + plugins: [apiKeyClient()], + }, + }, + ); + + beforeEach(() => { + store.clear(); + }); + + it("should not re-enable a key disabled during verification", async () => { + const { headers } = await signInWithTestUser(); + const created = await auth.api.createApiKey({ body: {}, headers }); + + onValidate = async () => { + await auth.api.updateApiKey({ + body: { keyId: created.id, enabled: false }, + headers, + }); + }; + + const result = await auth.api.verifyApiKey({ + body: { key: created.key }, + }); + expect(result.valid).toBe(true); + + const stored = store.get(`api-key:by-id:${created.id}`); + expect(stored).toBeDefined(); + expect(JSON.parse(stored!).enabled).toBe(false); + }); + + it("should not recreate a key deleted during verification", async () => { + const { headers } = await signInWithTestUser(); + const created = await auth.api.createApiKey({ body: {}, headers }); + + onValidate = async () => { + await auth.api.deleteApiKey({ + body: { keyId: created.id }, + headers, + }); + }; + + const result = await auth.api.verifyApiKey({ + body: { key: created.key }, + }); + expect(result.valid).toBe(false); + + const apiKeyEntries = [...store.keys()].filter((k) => + k.startsWith("api-key:"), + ); + expect(apiKeyEntries).toEqual([]); + }); + }); +}); + describe("listApiKeys with integer user.id (postgres + serial)", async () => { const testUserEmail = `api-key-serial-${crypto.randomUUID()}@test.com`; const { auth, signInWithTestUser } = await getTestInstance( @@ -4961,3 +5085,69 @@ describe("listApiKeys with integer user.id (postgres + serial)", async () => { expect(result.apiKeys.find((k) => k.id === created.id)).toBeDefined(); }); }); + +describe("api key creation uses a fresh session", async () => { + const { auth, client, testUser } = await getTestInstance( + { + session: { + cookieCache: { enabled: true, maxAge: 60 * 5 }, + }, + plugins: [apiKey()], + }, + { + clientOptions: { + plugins: [apiKeyClient()], + }, + }, + ); + + // Capture every cookie the response sets, including `session_data` (the + // signed cookie-cache snapshot), not just `session_token`. + function captureCookies(headers: Headers) { + return (context: { response: Response }) => { + const setCookies = context.response.headers.getSetCookie?.() ?? []; + for (const setCookie of setCookies) { + const pair = setCookie.split(";")[0]!; + const eq = pair.indexOf("="); + if (eq === -1) continue; + const name = pair.slice(0, eq).trim(); + const value = pair.slice(eq + 1).trim(); + if (!value) continue; + const current = headers.get("cookie"); + headers.set( + "cookie", + current ? `${current}; ${name}=${value}` : `${name}=${value}`, + ); + } + }; + } + + it("rejects creation when the session was revoked but the cookie cache is still valid", async () => { + const headers = new Headers(); + const signIn = await client.signIn.email({ + email: testUser.email, + password: testUser.password, + fetchOptions: { onSuccess: captureCookies(headers) }, + }); + const userId = signIn.data?.user.id; + expect(userId).toBeDefined(); + // The cookie cache must actually be present, otherwise this test would + // pass for the wrong reason. + expect(headers.get("cookie")).toContain("better-auth.session_data"); + + // With a live session, creation succeeds. + const first = await client.apiKey.create({}, { headers }); + expect(first.data?.key).toBeDefined(); + + // Simulate an admin ban / session revocation: the database sessions are + // deleted while the client still holds the valid signed cookie cache. + const ctx = await auth.$context; + await ctx.internalAdapter.deleteUserSessions(userId!); + + // Creation must re-check the authoritative store and reject, rather than + // minting a fresh key from the stale cookie-cache session. + const second = await client.apiKey.create({}, { headers }); + expect(second.data).toBeNull(); + expect(second.error?.status).toBe(401); + }); +}); diff --git a/packages/api-key/src/routes/create-api-key.ts b/packages/api-key/src/routes/create-api-key.ts index 1f4a45d73a..e8785fcbfb 100644 --- a/packages/api-key/src/routes/create-api-key.ts +++ b/packages/api-key/src/routes/create-api-key.ts @@ -288,7 +288,15 @@ export function createApiKey({ const opts = resolveConfiguration(ctx.context, configurations, configId); const keyGenerator = opts.customKeyGenerator || defaultKeyGenerator; - const session = await getSessionFromCtx(ctx); + // Creating an API key mints a long-lived credential, so resolve the + // session from the authoritative store rather than the signed + // cookie-cache snapshot. A revoked session — including the + // session deletion performed when a user is banned — would + // otherwise still be accepted within the cookie-cache window, + // letting it mint a fresh key that outlives the revoked session. + const session = await getSessionFromCtx(ctx, { + disableCookieCache: true, + }); const isClientRequest = ctx.request || ctx.headers; // if this endpoint was being called from the client, diff --git a/packages/api-key/src/routes/verify-api-key.ts b/packages/api-key/src/routes/verify-api-key.ts index 5e301e00a7..98ea10c17e 100644 --- a/packages/api-key/src/routes/verify-api-key.ts +++ b/packages/api-key/src/routes/verify-api-key.ts @@ -198,20 +198,24 @@ export async function validateApiKey({ }); } - const updated: ApiKey = { - ...apiKey, + const mutations: Partial = { ...update, remaining, lastRefillAt, updatedAt: new Date(), }; + const updated: ApiKey = { + ...apiKey, + ...mutations, + }; + const performUpdate = async (): Promise => { if (opts.storage === "database") { return ctx.context.adapter.update({ model: API_KEY_TABLE_NAME, where: [{ field: "id", value: apiKey.id }], - update: { ...updated, id: undefined }, + update: mutations, }); } else if ( opts.storage === "secondary-storage" && @@ -220,15 +224,20 @@ export async function validateApiKey({ const dbUpdated = await ctx.context.adapter.update({ model: API_KEY_TABLE_NAME, where: [{ field: "id", value: apiKey.id }], - update: { ...updated, id: undefined }, + update: mutations, }); if (dbUpdated) { await setApiKey(ctx, dbUpdated, opts); } return dbUpdated; } else { - await setApiKey(ctx, updated, opts); - return updated; + const fresh = await getApiKey(ctx, hashedKey, opts); + if (!fresh) { + return null; + } + const merged: ApiKey = { ...fresh, ...mutations }; + await setApiKey(ctx, merged, opts); + return merged; } }; diff --git a/packages/better-auth/src/api/routes/account.test.ts b/packages/better-auth/src/api/routes/account.test.ts index c7dee984d0..d55403a37f 100644 --- a/packages/better-auth/src/api/routes/account.test.ts +++ b/packages/better-auth/src/api/routes/account.test.ts @@ -1011,6 +1011,135 @@ describe("account", async () => { findAccountsSpy.mockRestore(); }); + it("should not refresh with a stale account cookie belonging to another user", async () => { + const { auth, client, cookieSetter } = await getTestInstance({ + socialProviders: { + google: { + clientId: "test", + clientSecret: "test", + enabled: true, + }, + }, + account: { + storeAccountCookie: true, + }, + }); + const testCtx = await auth.$context; + + let refreshedWith: string | null = null; + server.use( + http.post("https://oauth2.googleapis.com/token", async ({ request }) => { + const params = new URLSearchParams(await request.text()); + if (params.get("grant_type") === "refresh_token") { + refreshedWith = params.get("refresh_token"); + // the provider does not rotate the refresh token + return HttpResponse.json({ + access_token: "rotated-access-token", + }); + } + const data: GoogleProfile = { + email, + email_verified: true, + name: "First User", + picture: "https://lh3.googleusercontent.com/a-/AOh14GjQ4Z7Vw", + exp: 1234567890, + sub: "first-google-sub", + iat: 1234567890, + aud: "test", + azp: "test", + nbf: 1234567890, + iss: "test", + locale: "en", + jti: "test", + given_name: "First", + family_name: "User", + }; + const testIdToken = await signJWT(data, DEFAULT_SECRET); + return HttpResponse.json({ + access_token: "first-access-token", + refresh_token: "first-refresh-token", + id_token: testIdToken, + }); + }), + ); + + // The first user signs in with Google, storing their account_data cookie + const headers = new Headers(); + email = "stale-cookie-first@test.com"; + const signInRes = await client.signIn.social({ + provider: "google", + callbackURL: "/callback", + fetchOptions: { + onSuccess: cookieSetter(headers), + }, + }); + const state = + signInRes.data && "url" in signInRes.data && signInRes.data.url + ? new URL(signInRes.data.url).searchParams.get("state") || "" + : ""; + await client.$fetch("/callback/google", { + query: { + state, + code: "test", + }, + headers, + method: "GET", + onError(context) { + expect(context.response.status).toBe(302); + cookieSetter(headers)({ response: context.response }); + }, + }); + + await client.signUp.email( + { + email: "stale-cookie-second@test.com", + password: "password123456", + name: "Second User", + }, + { + headers, + onSuccess: cookieSetter(headers), + }, + ); + const session = await auth.api.getSession({ headers }); + assert(session, "second user should be signed in"); + expect(session.user.email).toBe("stale-cookie-second@test.com"); + + // The second user has their own google account + await testCtx.internalAdapter.createAccount({ + userId: session.user.id, + providerId: "google", + accountId: "second-google-sub", + accessToken: "second-access-token", + refreshToken: "second-refresh-token", + scope: "email", + }); + + const res = await client.$fetch<{ refreshToken?: string }>( + "/refresh-token", + { + body: { + providerId: "google", + }, + headers, + method: "POST", + }, + ); + + // The refresh must use the second user's own refresh token, not the + // the first user's token from the stale cookie + expect(refreshedWith).toBe("second-refresh-token"); + expect(res.data?.refreshToken).not.toBe("first-refresh-token"); + + // The second user's account row must not be overwritten with the + // the first user's tokens + const accounts = await testCtx.internalAdapter.findAccounts( + session.user.id, + ); + const googleAccount = accounts.find((a) => a.providerId === "google"); + expect(googleAccount?.refreshToken).toBe("second-refresh-token"); + }); + it("should persist refreshed idToken in database during getAccessToken auto-refresh", async () => { const { auth, client, cookieSetter } = await getTestInstance({ socialProviders: { diff --git a/packages/better-auth/src/api/routes/account.ts b/packages/better-auth/src/api/routes/account.ts index 21f4ed0ae0..90416ce1cb 100644 --- a/packages/better-auth/src/api/routes/account.ts +++ b/packages/better-auth/src/api/routes/account.ts @@ -760,11 +760,12 @@ export const refreshToken = createAuthEndpoint( // Try to read refresh token from cookie first let account: Account | undefined = undefined; const accountData = await getAccountCookie(ctx); - if ( - accountData && + const usedAccountCookie = + !!accountData && accountData.userId === resolvedUserId && - (!providerId || providerId === accountData?.providerId) - ) { + providerId === accountData.providerId && + (!accountId || accountData.accountId === accountId); + if (usedAccountCookie) { account = accountData; } else { const accounts = @@ -779,13 +780,7 @@ export const refreshToken = createAuthEndpoint( if (!account) { throw APIError.from("BAD_REQUEST", BASE_ERROR_CODES.ACCOUNT_NOT_FOUND); } - - let refreshToken: string | null | undefined = undefined; - if (accountData && providerId === accountData.providerId) { - refreshToken = accountData.refreshToken ?? undefined; - } else { - refreshToken = account.refreshToken ?? undefined; - } + const refreshToken = account.refreshToken ?? undefined; if (!refreshToken) { throw APIError.from("BAD_REQUEST", { @@ -823,8 +818,7 @@ export const refreshToken = createAuthEndpoint( } if ( - accountData && - providerId === accountData.providerId && + usedAccountCookie && ctx.context.options.account?.storeAccountCookie ) { const updateData = { diff --git a/packages/better-auth/src/api/routes/callback.ts b/packages/better-auth/src/api/routes/callback.ts index 6796ac3d7e..9b8bbf1bdf 100644 --- a/packages/better-auth/src/api/routes/callback.ts +++ b/packages/better-auth/src/api/routes/callback.ts @@ -151,7 +151,12 @@ export const callbackOAuth = createAuthEndpoint( }) .then((res) => res?.user); - if (!userInfo || userInfo.id === undefined || userInfo.id === null) { + if ( + !userInfo || + userInfo.id === undefined || + userInfo.id === null || + userInfo.id === "" + ) { c.context.logger.error("Unable to get user info"); redirectOnError(c, resolvedErrorURL, "unable_to_get_user_info"); } diff --git a/packages/better-auth/src/context/create-context.ts b/packages/better-auth/src/context/create-context.ts index dc6b98b730..01bb0f7a90 100644 --- a/packages/better-auth/src/context/create-context.ts +++ b/packages/better-auth/src/context/create-context.ts @@ -147,7 +147,7 @@ export async function createAuthContext( if (!baseURL && !isDynamicConfig) { logger.warn( - `[better-auth] Base URL could not be determined. Please set a valid base URL using the baseURL config option or the BETTER_AUTH_URL environment variable. Without this, callbacks and redirects may not work correctly.`, + `[better-auth] Base URL is not set. Set the baseURL option or BETTER_AUTH_URL env, or use a dynamic baseURL with allowedHosts for multi-host setups. Without it the origin is derived from the incoming request, and callbacks and redirects may not work correctly.`, ); } diff --git a/packages/better-auth/src/cookies/cookies.test.ts b/packages/better-auth/src/cookies/cookies.test.ts index eaacd2be29..e0027428e6 100644 --- a/packages/better-auth/src/cookies/cookies.test.ts +++ b/packages/better-auth/src/cookies/cookies.test.ts @@ -801,6 +801,61 @@ describe("getSessionCookie", async () => { }); }); +describe("sensitive session middleware cookie cache", () => { + /** + * Sensitive endpoints use `sensitiveSessionMiddleware`, which forces a + * database-backed session lookup via `disableCookieCache: true`. A request + * query parameter (e.g. `?disableCookieCache=`, which coerces to `false`) + * must NOT be able to override that internal flag and re-enable the cookie + * cache, otherwise a revoked session could still pass sensitive operations. + */ + it("should not let request query re-enable cookie cache on sensitive endpoints", async () => { + const { client, testUser, cookieSetter, db } = await getTestInstance({ + session: { + cookieCache: { + enabled: true, + }, + }, + }); + + const headers = new Headers(); + const signInRes = await client.signIn.email( + { + email: testUser.email, + password: testUser.password, + }, + { + onSuccess: cookieSetter(headers), + }, + ); + const userId = signInRes.data?.user.id!; + expect(userId).toBeDefined(); + + // Simulate the server-side session being revoked/deleted while the + // still-valid signed session_data cookie remains in the client. + await db.deleteMany({ + model: "session", + where: [{ field: "userId", value: userId }], + }); + + // Set the internal flag to a falsy value through the request query so it + // would skip the forced DB lookup if it took effect. The sensitive + // endpoint must still reject the request because the session no longer + // exists. + const res = await client.changePassword( + { + newPassword: "new-password", + currentPassword: testUser.password, + }, + { + headers, + query: { disableCookieCache: "" }, + }, + ); + expect(res.error?.status).toBe(401); + }); +}); + describe("Cookie Cache Field Filtering", () => { it("should exclude user fields with returned: false from cookie cache", async () => { const { client, testUser, cookieSetter } = await getTestInstance({ diff --git a/packages/better-auth/src/cookies/index.ts b/packages/better-auth/src/cookies/index.ts index d60a7a6f59..3feb2a8340 100644 --- a/packages/better-auth/src/cookies/index.ts +++ b/packages/better-auth/src/cookies/index.ts @@ -260,7 +260,17 @@ export async function setCookieCache( if (ctx.context.options.account?.storeAccountCookie) { const accountData = await getAccountCookie(ctx); if (accountData) { - await setAccountCookie(ctx, accountData); + if (accountData.userId === session.user.id) { + await setAccountCookie(ctx, accountData); + } else { + expireCookie(ctx, ctx.context.authCookies.accountData); + const accountStore = createAccountStore( + ctx.context.authCookies.accountData.name, + ctx.context.authCookies.accountData.attributes, + ctx, + ); + accountStore.setCookies(accountStore.clean()); + } } } } diff --git a/packages/better-auth/src/oauth2/link-account.ts b/packages/better-auth/src/oauth2/link-account.ts index 978c5c65b3..49b9796a4f 100644 --- a/packages/better-auth/src/oauth2/link-account.ts +++ b/packages/better-auth/src/oauth2/link-account.ts @@ -18,6 +18,19 @@ export async function handleOAuthUserInfo( disableSignUp?: boolean | undefined; overrideUserInfo?: boolean | undefined; isTrustedProvider?: boolean | undefined; + /** + * Whether `account.providerId` may be matched against the globally + * configured `accountLinking.trustedProviders` list to infer trust. + * + * Defaults to `true` for built-in social/OAuth providers, whose + * `providerId` namespace is controlled by the developer's config. Callers + * whose `providerId` is user-controlled (e.g. the SSO plugin, where any + * authenticated user can register a provider with an arbitrary id) must + * pass `false` so a provider named after a trusted social provider can't + * launder that trust. Such callers should supply their own + * `isTrustedProvider` signal instead. + */ + trustProviderByName?: boolean | undefined; }, ) { const { userInfo, account, callbackURL, disableSignUp, overrideUserInfo } = @@ -52,7 +65,8 @@ export async function handleOAuthUserInfo( const accountLinking = c.context.options.account?.accountLinking; const isTrustedProvider = opts.isTrustedProvider || - c.context.trustedProviders.includes(account.providerId); + (opts.trustProviderByName !== false && + c.context.trustedProviders.includes(account.providerId)); // FIXME(next-minor): drop `requireLocalEmailVerified` option and make // the gate unconditional. const requireLocalEmailVerified = diff --git a/packages/better-auth/src/plugins/admin/access/statement.ts b/packages/better-auth/src/plugins/admin/access/statement.ts index 680b44870e..5d43b4ac1a 100644 --- a/packages/better-auth/src/plugins/admin/access/statement.ts +++ b/packages/better-auth/src/plugins/admin/access/statement.ts @@ -10,6 +10,7 @@ export const defaultStatements = { "impersonate-admins", "delete", "set-password", + "set-email", "get", "update", ], @@ -27,6 +28,7 @@ export const adminAc = defaultAc.newRole({ "impersonate", "delete", "set-password", + "set-email", "get", "update", ], diff --git a/packages/better-auth/src/plugins/admin/admin.test.ts b/packages/better-auth/src/plugins/admin/admin.test.ts index 5bc8fd969d..901f0ef0f7 100644 --- a/packages/better-auth/src/plugins/admin/admin.test.ts +++ b/packages/better-auth/src/plugins/admin/admin.test.ts @@ -1402,6 +1402,115 @@ describe("Admin plugin", async () => { expect(res.error?.status).toBe(403); expect(res.error?.code).toBe("YOU_ARE_NOT_ALLOWED_TO_UPDATE_USERS"); }); + + it("should not allow updating password through update-user", async () => { + const res = await client.admin.updateUser( + { + userId: testNonAdminUser.id, + data: { + password: "new-password", + }, + }, + { + headers: adminHeaders, + }, + ); + expect(res.error?.status).toBe(400); + expect(res.error?.code).toBe("PASSWORD_CANNOT_BE_UPDATED_VIA_UPDATE_USER"); + }); + + it("should revoke sessions when banning a user via update-user", async () => { + const targetHeaders = new Headers(); + const { data: target } = await client.signUp.email( + { + email: "ban-via-update@test.com", + password: "password", + name: "Ban Via Update", + }, + { + onSuccess: cookieSetter(targetHeaders), + }, + ); + const targetId = target?.user.id || ""; + + const res = await client.admin.updateUser( + { + userId: targetId, + data: { + banned: true, + banReason: "Violation", + }, + }, + { + headers: adminHeaders, + }, + ); + expect(res.data?.banned).toBe(true); + + const sessions = await client.admin.listUserSessions( + { + userId: targetId, + }, + { + headers: adminHeaders, + }, + ); + expect(sessions.data?.sessions.length).toBe(0); + }); + + it("should not allow admins to ban themselves via update-user", async () => { + const { data: session } = await client.getSession({ + fetchOptions: { + headers: adminHeaders, + }, + }); + const res = await client.admin.updateUser( + { + userId: session?.user.id || "", + data: { + banned: true, + }, + }, + { + headers: adminHeaders, + }, + ); + expect(res.error?.status).toBe(400); + expect(res.error?.code).toBe("YOU_CANNOT_BAN_YOURSELF"); + }); + + it("should reject email updates that collide with another user", async () => { + const res = await client.admin.updateUser( + { + userId: testNonAdminUser.id, + data: { + email: "ban-via-update@test.com", + }, + }, + { + headers: adminHeaders, + }, + ); + expect(res.error?.status).toBe(400); + expect(res.error?.code).toBe("USER_ALREADY_EXISTS_USE_ANOTHER_EMAIL"); + }); + + it("should allow admin to update user email and normalize it", async () => { + const res = await client.admin.updateUser( + { + userId: testNonAdminUser.id, + data: { + email: "Updated-Email@Test.com", + emailVerified: false, + }, + }, + { + headers: adminHeaders, + }, + ); + expect(res.data?.email).toBe("updated-email@test.com"); + expect(res.data?.emailVerified).toBe(false); + }); }); describe("access control", async () => { @@ -1430,6 +1539,10 @@ describe("access control", async () => { user: ["update"], order: ["update"], }); + const creatorAc = ac.newRole({ + user: ["create"], + order: [], + }); const { signInWithTestUser, cookieSetter, auth, customFetchImpl } = await getTestInstance( @@ -1441,6 +1554,7 @@ describe("access control", async () => { admin: adminAc, user: userAc, support: supportAc, + creator: creatorAc, }, }), ], @@ -1464,6 +1578,14 @@ describe("access control", async () => { }, }; } + if (user.name === "Creator") { + return { + data: { + ...user, + role: "creator", + }, + }; + } }, }, }, @@ -1484,6 +1606,7 @@ describe("access control", async () => { admin: adminAc, user: userAc, support: supportAc, + creator: creatorAc, }, }), ], @@ -1593,6 +1716,159 @@ describe("access control", async () => { expect(res.data?.role).toBe("support"); }); + it("should require user:set-role to assign roles via create-user", async () => { + const creatorHeaders = new Headers(); + await client.signUp.email( + { + email: "creator@test.com", + password: "password", + name: "Creator", + }, + { + onSuccess: cookieSetter(creatorHeaders), + }, + ); + + // `user:create` alone is enough when no role is requested + const ok = await client.admin.createUser( + { + name: "Created User", + email: "created-by-creator@test.com", + password: "password", + }, + { + headers: creatorHeaders, + }, + ); + expect(ok.data?.user.role).toBe("user"); + + // but requesting a role requires `user:set-role` + const res = await client.admin.createUser( + { + name: "Role Requested User", + email: "role-requested@test.com", + password: "password", + role: "admin", + }, + { + headers: creatorHeaders, + }, + ); + expect(res.error?.status).toBe(403); + expect(res.error?.code).toBe("YOU_ARE_NOT_ALLOWED_TO_CHANGE_USERS_ROLE"); + + // `data.role` must go through the same check + const res2 = await client.admin.createUser( + { + name: "Role Requested User", + email: "role-requested2@test.com", + password: "password", + data: { + role: "admin", + }, + }, + { + headers: creatorHeaders, + }, + ); + expect(res2.error?.status).toBe(403); + expect(res2.error?.code).toBe("YOU_ARE_NOT_ALLOWED_TO_CHANGE_USERS_ROLE"); + }); + + it("should reject non-existent roles via create-user", async () => { + const res = await client.admin.createUser( + { + name: "Bad Role User", + email: "bad-role@test.com", + password: "password", + data: { + role: "non-existent-role", + }, + }, + { + headers, + }, + ); + expect(res.error?.status).toBe(400); + }); + + it("should apply a validated data.role for callers with user:set-role", async () => { + const res = await client.admin.createUser( + { + name: "Data Role User", + email: "data-role@test.com", + password: "password", + data: { + role: "support", + }, + }, + { + headers, + }, + ); + expect(res.data?.user.role).toBe("support"); + }); + + it("should require user:ban to update ban fields via update-user", async () => { + const supportHeaders = new Headers(); + const { data: supportSignUp } = await client.signUp.email( + { + email: "support-ban@test.com", + password: "password", + name: "Support", + }, + { + onSuccess: cookieSetter(supportHeaders), + }, + ); + const targetUserId = supportSignUp?.user.id || ""; + + const res = await client.admin.updateUser( + { + userId: targetUserId, + data: { + banned: true, + banReason: "self-served ban", + }, + }, + { + headers: supportHeaders, + }, + ); + expect(res.error?.status).toBe(403); + expect(res.error?.code).toBe("YOU_ARE_NOT_ALLOWED_TO_BAN_USERS"); + }); + + it("should require user:set-email to update email fields via update-user", async () => { + const supportHeaders = new Headers(); + const { data: supportSignUp } = await client.signUp.email( + { + email: "support-email@test.com", + password: "password", + name: "Support", + }, + { + onSuccess: cookieSetter(supportHeaders), + }, + ); + const targetUserId = supportSignUp?.user.id || ""; + + const res = await client.admin.updateUser( + { + userId: targetUserId, + data: { + email: "new-address@example.com", + emailVerified: true, + }, + }, + { + headers: supportHeaders, + }, + ); + expect(res.error?.status).toBe(403); + expect(res.error?.code).toBe("YOU_ARE_NOT_ALLOWED_TO_SET_USERS_EMAIL"); + }); + it("should validate on the client", async () => { const canCreateOrder = client.admin.checkRolePermission({ role: "admin", diff --git a/packages/better-auth/src/plugins/admin/error-codes.ts b/packages/better-auth/src/plugins/admin/error-codes.ts index 532360d02c..01f08a8362 100644 --- a/packages/better-auth/src/plugins/admin/error-codes.ts +++ b/packages/better-auth/src/plugins/admin/error-codes.ts @@ -29,4 +29,8 @@ export const ADMIN_ERROR_CODES = defineErrorCodes({ "You are not allowed to set a non-existent role value", YOU_CANNOT_IMPERSONATE_ADMINS: "You cannot impersonate admins", INVALID_ROLE_TYPE: "Invalid role type", + YOU_ARE_NOT_ALLOWED_TO_SET_USERS_EMAIL: + "You are not allowed to update users email", + PASSWORD_CANNOT_BE_UPDATED_VIA_UPDATE_USER: + "Password cannot be updated through update-user. Use the set-user-password endpoint instead", }); diff --git a/packages/better-auth/src/plugins/admin/routes.ts b/packages/better-auth/src/plugins/admin/routes.ts index e26b13b87d..e0713b2d74 100644 --- a/packages/better-auth/src/plugins/admin/routes.ts +++ b/packages/better-auth/src/plugins/admin/routes.ts @@ -354,6 +354,70 @@ export const createUser = (opts: O) => } } + // `data.role` must go through the same authorization and validation as + // the top-level `role` field, otherwise spreading it into the create + // payload would let `user:create` callers assign arbitrary roles. + const { role: dataRole, ...userData } = ctx.body.data ?? {}; + const requestedRole = ctx.body.role ?? dataRole; + + if (requestedRole !== undefined) { + if (session) { + const canSetRole = hasPermission({ + userId: session.user.id, + role: session.user.role, + options: opts, + permissions: { + user: ["set-role"], + }, + }); + if (!canSetRole) { + throw APIError.from( + "FORBIDDEN", + ADMIN_ERROR_CODES.YOU_ARE_NOT_ALLOWED_TO_CHANGE_USERS_ROLE, + ); + } + } + const inputRoles = Array.isArray(requestedRole) + ? requestedRole + : [requestedRole]; + for (const role of inputRoles) { + if (typeof role !== "string") { + throw APIError.from( + "BAD_REQUEST", + ADMIN_ERROR_CODES.INVALID_ROLE_TYPE, + ); + } + if (opts.roles && !opts.roles[role as keyof typeof opts.roles]) { + throw APIError.from( + "BAD_REQUEST", + ADMIN_ERROR_CODES.YOU_ARE_NOT_ALLOWED_TO_SET_NON_EXISTENT_VALUE, + ); + } + } + } + + if ( + session && + ["banned", "banReason", "banExpires"].some((key) => + Object.prototype.hasOwnProperty.call(userData, key), + ) + ) { + const canBanUser = hasPermission({ + userId: session.user.id, + role: session.user.role, + options: opts, + permissions: { + user: ["ban"], + }, + }); + if (!canBanUser) { + throw APIError.from( + "FORBIDDEN", + ADMIN_ERROR_CODES.YOU_ARE_NOT_ALLOWED_TO_BAN_USERS, + ); + } + } + const email = ctx.body.email.toLowerCase(); const isValidEmail = z.email().safeParse(email); if (!isValidEmail.success) { @@ -369,13 +433,13 @@ export const createUser = (opts: O) => ); } const user = await ctx.context.internalAdapter.createUser({ + ...userData, email: email, name: ctx.body.name, role: - (ctx.body.role && parseRoles(ctx.body.role)) ?? - opts?.defaultRole ?? - "user", - ...ctx.body.data, + requestedRole !== undefined + ? parseRoles(requestedRole as string | string[]) + : (opts?.defaultRole ?? "user"), }); if (!user) { @@ -478,6 +542,19 @@ export const adminUpdateUser = (opts: AdminOptions) => throw APIError.from("BAD_REQUEST", ADMIN_ERROR_CODES.NO_DATA_TO_UPDATE); } + const updateData = ctx.body.data as Record; + const hasDataKey = (key: string) => + Object.prototype.hasOwnProperty.call(updateData, key); + + // Passwords are hashed and stored on the credential account, never on + // the user model — writing one here would persist it in plaintext. + if (hasDataKey("password")) { + throw APIError.from( + "BAD_REQUEST", + ADMIN_ERROR_CODES.PASSWORD_CANNOT_BE_UPDATED_VIA_UPDATE_USER, + ); + } + // Role changes must be guarded by `user:set-role` and validated against the role allow-list. if (Object.prototype.hasOwnProperty.call(ctx.body.data, "role")) { const canSetRole = hasPermission({ @@ -516,6 +593,67 @@ export const adminUpdateUser = (opts: AdminOptions) => ); } + // Ban fields are guarded by `user:ban`, mirroring the ban/unban endpoints. + if (["banned", "banReason", "banExpires"].some(hasDataKey)) { + const canBanUser = hasPermission({ + userId: ctx.context.session.user.id, + role: ctx.context.session.user.role, + options: opts, + permissions: { + user: ["ban"], + }, + }); + if (!canBanUser) { + throw APIError.from( + "FORBIDDEN", + ADMIN_ERROR_CODES.YOU_ARE_NOT_ALLOWED_TO_BAN_USERS, + ); + } + if ( + updateData.banned === true && + ctx.body.userId === ctx.context.session.user.id + ) { + throw APIError.from( + "BAD_REQUEST", + ADMIN_ERROR_CODES.YOU_CANNOT_BAN_YOURSELF, + ); + } + } + + // Email and verification changes require the `user:set-email` permission. + if (hasDataKey("email") || hasDataKey("emailVerified")) { + const canSetEmail = hasPermission({ + userId: ctx.context.session.user.id, + role: ctx.context.session.user.role, + options: opts, + permissions: { + user: ["set-email"], + }, + }); + if (!canSetEmail) { + throw APIError.from( + "FORBIDDEN", + ADMIN_ERROR_CODES.YOU_ARE_NOT_ALLOWED_TO_SET_USERS_EMAIL, + ); + } + if (hasDataKey("email")) { + const email = String(updateData.email).toLowerCase(); + const isValidEmail = z.email().safeParse(email); + if (!isValidEmail.success) { + throw APIError.from("BAD_REQUEST", BASE_ERROR_CODES.INVALID_EMAIL); + } + const existUser = + await ctx.context.internalAdapter.findUserByEmail(email); + if (existUser && existUser.user.id !== ctx.body.userId) { + throw APIError.from( + "BAD_REQUEST", + ADMIN_ERROR_CODES.USER_ALREADY_EXISTS_USE_ANOTHER_EMAIL, + ); + } + updateData.email = email; + } + } + const isUserExist = await ctx.context.internalAdapter.findUserById( ctx.body.userId, ); @@ -528,6 +666,11 @@ export const adminUpdateUser = (opts: AdminOptions) => ctx.body.data, ); + // Match the ban-user endpoint: banning a user must revoke their sessions. + if (updateData.banned === true) { + await ctx.context.internalAdapter.deleteUserSessions(ctx.body.userId); + } + return ctx.json( parseUserOutput(ctx.context.options, updatedUser) as UserWithRole, ); diff --git a/packages/better-auth/src/plugins/email-otp/email-otp.test.ts b/packages/better-auth/src/plugins/email-otp/email-otp.test.ts index 77876ed670..42fa14d794 100644 --- a/packages/better-auth/src/plugins/email-otp/email-otp.test.ts +++ b/packages/better-auth/src/plugins/email-otp/email-otp.test.ts @@ -1,5 +1,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { createAuthClient } from "../../client"; +import { getCookieCache } from "../../cookies"; +import { parseSetCookieHeader } from "../../cookies/cookie-utils"; import { getTestInstance } from "../../test-utils/test-instance"; import { bearer } from "../bearer"; import { emailOTP } from "."; @@ -2202,3 +2204,121 @@ describe("email-otp-resendStrategy", async () => { expect(secondOtp).not.toBe(firstOtp); }); }); + +describe("email-otp verify-email cookie cache isolation", async () => { + const secret = "better-auth-secret-1234567890-cookie-cache"; + let otp = ""; + const { client, auth } = await getTestInstance( + { + secret, + plugins: [ + emailOTP({ + async sendVerificationOTP({ otp: _otp }) { + otp = _otp; + }, + }), + ], + session: { + cookieCache: { + enabled: true, + }, + }, + }, + { + clientOptions: { + plugins: [emailOTPClient()], + }, + }, + ); + + /** + * Verifying an OTP for one email must not stamp `emailVerified: true` onto the + * cookie cache of a *different* user that happens to be the current session. + * + * @see https://github.com/better-auth/better-auth/issues/9962 + */ + it("should not mark the current session's user as verified when a different user's email is verified", async () => { + // User B is signed into their own (unverified) account. + const currentUserEmail = "user-b@test.com"; + const currentUserPassword = "user-b-password"; + await auth.api.signUpEmail({ + body: { + email: currentUserEmail, + password: currentUserPassword, + name: "user-b", + }, + }); + + const currentUserHeaders = new Headers(); + await client.signIn.email( + { email: currentUserEmail, password: currentUserPassword }, + { + onSuccess(ctx) { + const cookies = parseSetCookieHeader( + ctx.response.headers.get("set-cookie") || "", + ); + const token = cookies.get("better-auth.session_token")?.value; + const data = cookies.get("better-auth.session_data")?.value; + currentUserHeaders.set( + "cookie", + `better-auth.session_token=${token}; better-auth.session_data=${data}`, + ); + }, + }, + ); + + // A separate account whose email the OTP is issued for. + const otherEmail = "other@test.com"; + await auth.api.signUpEmail({ + body: { + email: otherEmail, + password: "other-password", + name: "other", + }, + }); + await client.emailOtp.sendVerificationOtp({ + email: otherEmail, + type: "email-verification", + }); + + // Verify the OTP for the *other* email while still authenticated as + // user B. Capture whatever cookie cache verify-email writes. + let refreshedSessionData: string | undefined; + const verified = await client.emailOtp.verifyEmail( + { email: otherEmail, otp }, + { + headers: currentUserHeaders, + onSuccess(ctx) { + const cookies = parseSetCookieHeader( + ctx.response.headers.get("set-cookie") || "", + ); + refreshedSessionData = cookies.get("better-auth.session_data")?.value; + }, + }, + ); + expect(verified.data?.status).toBe(true); + + // Merge any refreshed cache back into user B's cookies, exactly as a + // browser would, then read what get-session would trust. + if (refreshedSessionData) { + const token = currentUserHeaders + .get("cookie") + ?.match(/better-auth\.session_token=([^;]+)/)?.[1]; + currentUserHeaders.set( + "cookie", + `better-auth.session_token=${token}; better-auth.session_data=${refreshedSessionData}`, + ); + } + + const cache = await getCookieCache(currentUserHeaders, { secret }); + expect(cache?.user.email).toBe(currentUserEmail); + expect(cache?.user.emailVerified).toBe(false); + + // And the live session must agree: user B is still unverified. + const session = await client.getSession({ + fetchOptions: { headers: currentUserHeaders }, + }); + expect(session.data?.user.email).toBe(currentUserEmail); + expect(session.data?.user.emailVerified).toBe(false); + }); +}); diff --git a/packages/better-auth/src/plugins/email-otp/routes.ts b/packages/better-auth/src/plugins/email-otp/routes.ts index 8522a39194..10bd911794 100644 --- a/packages/better-auth/src/plugins/email-otp/routes.ts +++ b/packages/better-auth/src/plugins/email-otp/routes.ts @@ -532,7 +532,11 @@ export const verifyEmailOTP = (opts: RequiredEmailOTPOptions) => }); } const currentSession = await getSessionFromCtx(ctx); - if (currentSession && updatedUser.emailVerified) { + if ( + currentSession && + updatedUser.emailVerified && + currentSession.user.id === updatedUser.id + ) { const dontRememberMeCookie = await ctx.getSignedCookie( ctx.context.authCookies.dontRememberToken.name, ctx.context.secret, diff --git a/packages/better-auth/src/plugins/generic-oauth/generic-oauth.test.ts b/packages/better-auth/src/plugins/generic-oauth/generic-oauth.test.ts index 592dc51bc0..fabc9f25c3 100644 --- a/packages/better-auth/src/plugins/generic-oauth/generic-oauth.test.ts +++ b/packages/better-auth/src/plugins/generic-oauth/generic-oauth.test.ts @@ -1192,6 +1192,170 @@ describe("oauth2", async () => { expect(accountsAfterSecond[0]!.accountId).toBe(String(numericAccountId)); }); + it("rejects sign-in when the provider omits an account id, preventing account collisions", async () => { + const { customFetchImpl, auth, cookieSetter } = await getTestInstance({ + databaseHooks: { + user: { + create: { + before: async (user) => ({ + data: { ...user, emailVerified: true }, + }), + }, + }, + }, + plugins: [ + genericOAuth({ + config: [ + { + providerId: "no-sub-test", + discoveryUrl: `http://localhost:${port}/.well-known/openid-configuration`, + clientId: clientId, + clientSecret: clientSecret, + pkce: true, + }, + ], + }), + ], + }); + const ctx = await auth.$context; + const authClient = createAuthClient({ + plugins: [genericOAuthClient()], + baseURL: "http://localhost:3000", + fetchOptions: { customFetchImpl }, + }); + + // A userinfo response that includes email/name but no `sub`/`id`. + const respondWithoutSub = (email: string, name: string) => { + server.service.once("beforeUserinfo", (userInfoResponse) => { + userInfoResponse.body = { + email, + name, + picture: "https://test.com/picture.png", + email_verified: true, + }; + userInfoResponse.statusCode = 200; + }); + }; + + // First user signs in — must be rejected, not stored under an empty id. + respondWithoutSub("first-no-sub@test.com", "First No Sub"); + const firstHeaders = new Headers(); + const firstSignIn = await authClient.signIn.oauth2({ + providerId: "no-sub-test", + callbackURL: "http://localhost:3000/dashboard", + newUserCallbackURL: "http://localhost:3000/new_user", + fetchOptions: { onSuccess: cookieSetter(firstHeaders) }, + }); + const firstFlow = await simulateOAuthFlow( + firstSignIn.data?.url || "", + firstHeaders, + customFetchImpl, + ); + // Rejected at the error page rather than landing on a success URL. + expect(firstFlow.callbackURL).toContain("/error?error="); + expect(firstFlow.callbackURL).not.toContain("/dashboard"); + expect(firstFlow.callbackURL).not.toContain("/new_user"); + + const firstSession = await authClient.getSession({ + fetchOptions: { headers: firstFlow.headers }, + }); + expect(firstSession.data).toBeNull(); + + // No account should have been created with an empty/undefined account id. + const emptyIdAccounts = await ctx.adapter.findMany<{ accountId: string }>({ + model: "account", + where: [{ field: "providerId", value: "no-sub-test" }], + }); + expect(emptyIdAccounts).toHaveLength(0); + + // A second, different user signing in through the same provider must not + // resolve to the first user's account. + respondWithoutSub("second-no-sub@test.com", "Second No Sub"); + const secondHeaders = new Headers(); + const secondSignIn = await authClient.signIn.oauth2({ + providerId: "no-sub-test", + callbackURL: "http://localhost:3000/dashboard", + fetchOptions: { onSuccess: cookieSetter(secondHeaders) }, + }); + const secondFlow = await simulateOAuthFlow( + secondSignIn.data?.url || "", + secondHeaders, + customFetchImpl, + ); + expect(secondFlow.callbackURL).toContain("/error?error="); + const secondSession = await authClient.getSession({ + fetchOptions: { headers: secondFlow.headers }, + }); + // Must NOT have resolved to the first user (the collision being fixed). + expect(secondSession.data).toBeNull(); + }); + + it("rejects sign-in when a custom getUserInfo returns an empty id", async () => { + const { customFetchImpl, auth, cookieSetter } = await getTestInstance({ + databaseHooks: { + user: { + create: { + before: async (user) => ({ + data: { ...user, emailVerified: true }, + }), + }, + }, + }, + plugins: [ + genericOAuth({ + config: [ + { + providerId: "empty-id-test", + authorizationUrl: `http://localhost:${port}/authorize`, + tokenUrl: `http://localhost:${port}/token`, + clientId: clientId, + clientSecret: clientSecret, + pkce: true, + // A misconfigured custom mapper that yields no account id. + getUserInfo: async () => ({ + id: "", + email: "empty-id@test.com", + name: "Empty Id", + emailVerified: true, + }), + }, + ], + }), + ], + }); + const ctx = await auth.$context; + const authClient = createAuthClient({ + plugins: [genericOAuthClient()], + baseURL: "http://localhost:3000", + fetchOptions: { customFetchImpl }, + }); + + const headers = new Headers(); + const signIn = await authClient.signIn.oauth2({ + providerId: "empty-id-test", + callbackURL: "http://localhost:3000/dashboard", + fetchOptions: { onSuccess: cookieSetter(headers) }, + }); + const flow = await simulateOAuthFlow( + signIn.data?.url || "", + headers, + customFetchImpl, + ); + // The callback guard rejects the empty resolved account id. + expect(flow.callbackURL).toContain("error=id_is_missing"); + + const session = await authClient.getSession({ + fetchOptions: { headers: flow.headers }, + }); + expect(session.data).toBeNull(); + + const accounts = await ctx.adapter.findMany({ + model: "account", + where: [{ field: "providerId", value: "empty-id-test" }], + }); + expect(accounts).toHaveLength(0); + }); + it("should handle custom getUserInfo returning numeric ID", async () => { const numericId = 987654321; diff --git a/packages/better-auth/src/plugins/generic-oauth/routes.ts b/packages/better-auth/src/plugins/generic-oauth/routes.ts index b97f3bc85d..c4495eeddd 100644 --- a/packages/better-auth/src/plugins/generic-oauth/routes.ts +++ b/packages/better-auth/src/plugins/generic-oauth/routes.ts @@ -445,7 +445,21 @@ export const oAuth2Callback = (options: GenericOAuthOptions) => ); redirectOnError(ctx, resolvedErrorURL, "email_is_missing"); } - const id = mapUser.id ? String(mapUser.id) : String(userInfo.id); + const rawId = + mapUser.id !== undefined && mapUser.id !== null && mapUser.id !== "" + ? mapUser.id + : userInfo.id; + const id = rawId !== undefined && rawId !== null ? String(rawId) : ""; + // A provider must return a stable account id (e.g. `sub`). + // Without one, every account would be stored under the same + // empty id, letting different users resolve to the same account. + if (!id) { + ctx.context.logger.error( + "Provider did not return an account id (e.g. `sub`). Unable to sign in.", + userInfo, + ); + redirectOnError(ctx, resolvedErrorURL, "id_is_missing"); + } const name = mapUser.name ? mapUser.name : userInfo.name; if (!name) { ctx.context.logger.error("Unable to get user info", userInfo); @@ -787,6 +801,7 @@ export async function getUserInfo( } const userInfo = await betterFetch<{ + id?: string | number | undefined; email: string; sub?: string | undefined; name: string; @@ -798,8 +813,14 @@ export async function getUserInfo( Authorization: `Bearer ${tokens.accessToken}`, }, }); + const subjectId = userInfo.data?.sub ?? userInfo.data?.id; + // Without a stable subject id, the account would be stored under an empty + // id and could be resolved by a different user on the same provider. + if (subjectId === undefined || subjectId === null || subjectId === "") { + return null; + } return { - id: userInfo.data?.sub ?? "", + id: subjectId, emailVerified: userInfo.data?.email_verified ?? false, email: userInfo.data?.email, image: userInfo.data?.picture, diff --git a/packages/better-auth/src/plugins/last-login-method/last-login-method.test.ts b/packages/better-auth/src/plugins/last-login-method/last-login-method.test.ts index f31d5a3b07..d6cd95752d 100644 --- a/packages/better-auth/src/plugins/last-login-method/last-login-method.test.ts +++ b/packages/better-auth/src/plugins/last-login-method/last-login-method.test.ts @@ -29,6 +29,23 @@ let handlers: ReturnType[]; const server = setupServer(); +const SIWE_WALLET = "0x000000000000000000000000000000000000dEaD"; +const SIWE_CHAIN_ID = 1; +const SIWE_DOMAIN = "example.com"; +const SIWE_NONCE = "A1b2C3d4E5f6G7h8J"; +// The siwe plugin now parses and validates the ERC-4361 message body (binding it +// to the server-issued nonce), so these tests must sign a real SIWE message +// rather than an arbitrary placeholder string. +const siweMessage = () => + `${SIWE_DOMAIN} wants you to sign in with your Ethereum account:\n` + + `${SIWE_WALLET}\n\n` + + `Sign in.\n\n` + + `URI: https://${SIWE_DOMAIN}\n` + + `Version: 1\n` + + `Chain ID: ${SIWE_CHAIN_ID}\n` + + `Nonce: ${SIWE_NONCE}\n` + + `Issued At: 2024-01-01T00:00:00.000Z`; + beforeAll(async () => { const data: GoogleProfile = { email: "github-issue-demo@example.com", @@ -81,12 +98,10 @@ describe("lastLoginMethod", async () => { siwe({ domain: "example.com", async getNonce() { - return "A1b2C3d4E5f6G7h8J"; + return SIWE_NONCE; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + return signature === "valid_signature"; }, }), ], @@ -117,12 +132,12 @@ describe("lastLoginMethod", async () => { it("should set the last login method cookie for siwe", async () => { const headers = new Headers(); - const walletAddress = "0x000000000000000000000000000000000000dEaD"; - const chainId = 1; + const walletAddress = SIWE_WALLET; + const chainId = SIWE_CHAIN_ID; await client.siwe.nonce({ walletAddress, chainId }); await client.siwe.verify( { - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress, chainId, @@ -260,21 +275,19 @@ describe("lastLoginMethod", async () => { }); it("should set the last login method for siwe in the database", async () => { - const walletAddress = "0x000000000000000000000000000000000000dEaD"; - const chainId = 1; + const walletAddress = SIWE_WALLET; + const chainId = SIWE_CHAIN_ID; const { client, auth } = await getTestInstance( { plugins: [ lastLoginMethod({ storeInDatabase: true }), siwe({ - domain: "example.com", + domain: SIWE_DOMAIN, async getNonce() { - return "A1b2C3d4E5f6G7h8J"; + return SIWE_NONCE; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + return signature === "valid_signature"; }, }), ], @@ -287,7 +300,7 @@ describe("lastLoginMethod", async () => { ); await client.siwe.nonce({ walletAddress, chainId }); const { data } = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress, chainId, diff --git a/packages/better-auth/src/plugins/organization/routes/crud-invites.test.ts b/packages/better-auth/src/plugins/organization/routes/crud-invites.test.ts index a308eb2268..be41a53f93 100644 --- a/packages/better-auth/src/plugins/organization/routes/crud-invites.test.ts +++ b/packages/better-auth/src/plugins/organization/routes/crud-invites.test.ts @@ -371,3 +371,232 @@ describe("organization invitation recipient ownership gates", async () => { expect(memberEmails).toContain(VICTIM_EMAIL); }); }); + +/** + * An invitation's teamId must be scoped to the invitation's organization at + * creation AND acceptance, and team read endpoints must verify organization + * membership rather than relying on a teamMember row alone. + */ +describe("invitation teamId must belong to the invitation's organization", async () => { + const OTHER_USER_EMAIL = "user-b@example.com"; + const INVITEE_EMAIL = "invitee@example.com"; + const PASSWORD = "test-password-123"; + + function setup() { + return getTestInstance( + { + databaseHooks: { + user: { + create: { + before: async (user) => ({ + data: { ...user, emailVerified: true }, + }), + }, + }, + }, + plugins: [ + organization({ + teams: { enabled: true }, + async sendInvitationEmail() {}, + }), + ], + }, + { + clientOptions: { + plugins: [organizationClient({ teams: { enabled: true } })], + }, + }, + ); + } + + it("rejects creating an invitation with a teamId from another organization", async () => { + const { client, signInWithTestUser, signInWithUser, cookieSetter } = + await setup(); + + // First org owner (default test user) creates an org and a team. + const { headers: ownerHeaders } = await signInWithTestUser(); + const firstOrg = await client.organization.create({ + name: "Org A", + slug: "org-a", + fetchOptions: { + headers: ownerHeaders, + onSuccess: cookieSetter(ownerHeaders), + }, + }); + const firstTeam = await client.organization.createTeam({ + name: "Team A", + organizationId: firstOrg.data!.id, + fetchOptions: { headers: ownerHeaders }, + }); + const firstTeamId = firstTeam.data!.id; + + // A second user creates their own organization. + await client.signUp.email({ + email: OTHER_USER_EMAIL, + password: PASSWORD, + name: "User B", + }); + const { headers: secondUserHeaders } = await signInWithUser( + OTHER_USER_EMAIL, + PASSWORD, + ); + const otherOrg = await client.organization.create({ + name: "Org B", + slug: "org-b", + fetchOptions: { + headers: secondUserHeaders, + onSuccess: cookieSetter(secondUserHeaders), + }, + }); + + // The second user invites into their own org with a teamId from the first org. + const invite = await client.organization.inviteMember({ + organizationId: otherOrg.data!.id, + email: INVITEE_EMAIL, + role: "member", + teamId: firstTeamId, + fetchOptions: { headers: secondUserHeaders }, + }); + + expect(invite.data).toBeNull(); + expect(invite.error?.code).toBe("TEAM_NOT_FOUND"); + }); + + it("rejects accepting an invitation whose teamId points at another org", async () => { + const { client, signInWithTestUser, signInWithUser, cookieSetter, db } = + await setup(); + + // First org + team. + const { headers: ownerHeaders } = await signInWithTestUser(); + const firstOrg = await client.organization.create({ + name: "Org A", + slug: "org-a", + fetchOptions: { + headers: ownerHeaders, + onSuccess: cookieSetter(ownerHeaders), + }, + }); + const firstTeam = await client.organization.createTeam({ + name: "Team A", + organizationId: firstOrg.data!.id, + fetchOptions: { headers: ownerHeaders }, + }); + const firstTeamId = firstTeam.data!.id; + + // Second org with its OWN team so the invitation passes the create-side check. + await client.signUp.email({ + email: OTHER_USER_EMAIL, + password: PASSWORD, + name: "User B", + }); + const { headers: secondUserHeaders } = await signInWithUser( + OTHER_USER_EMAIL, + PASSWORD, + ); + const otherOrg = await client.organization.create({ + name: "Org B", + slug: "org-b", + fetchOptions: { + headers: secondUserHeaders, + onSuccess: cookieSetter(secondUserHeaders), + }, + }); + const otherTeam = await client.organization.createTeam({ + name: "Team B", + organizationId: otherOrg.data!.id, + fetchOptions: { headers: secondUserHeaders }, + }); + + const invite = await client.organization.inviteMember({ + organizationId: otherOrg.data!.id, + email: INVITEE_EMAIL, + role: "member", + teamId: otherTeam.data!.id, + fetchOptions: { headers: secondUserHeaders }, + }); + const invitationId = String(invite.data!.id); + + // Update the persisted invitation directly in the database to point at + // the first org's team, standing in for a stale or moved team that the + // create-side check did not cover. + await db.update({ + model: "invitation", + where: [{ field: "id", value: invitationId }], + update: { teamId: firstTeamId }, + }); + + // The invited recipient accepts. + await client.signUp.email({ + email: INVITEE_EMAIL, + password: PASSWORD, + name: "Invitee", + }); + const { headers: inviteeHeaders } = await signInWithUser( + INVITEE_EMAIL, + PASSWORD, + ); + const accept = await client.organization.acceptInvitation({ + invitationId, + fetchOptions: { headers: inviteeHeaders }, + }); + + expect(accept.error?.code).toBe("TEAM_NOT_FOUND"); + + // No teamMember row may exist against the first org's team. + const firstTeamMembers = await db.findMany({ + model: "teamMember", + where: [{ field: "teamId", value: firstTeamId }], + }); + expect(firstTeamMembers.length).toBe(0); + }); + + it("does not list another organization's team members from a mismatched teamMember row", async () => { + const { client, signInWithTestUser, signInWithUser, cookieSetter, db } = + await setup(); + + // First org + team. + const { headers: ownerHeaders } = await signInWithTestUser(); + const firstOrg = await client.organization.create({ + name: "Org A", + slug: "org-a", + fetchOptions: { + headers: ownerHeaders, + onSuccess: cookieSetter(ownerHeaders), + }, + }); + const firstTeam = await client.organization.createTeam({ + name: "Team A", + organizationId: firstOrg.data!.id, + fetchOptions: { headers: ownerHeaders }, + }); + const firstTeamId = firstTeam.data!.id; + + // The second user is NOT a member of the first organization. + await client.signUp.email({ + email: OTHER_USER_EMAIL, + password: PASSWORD, + name: "User B", + }); + const { headers: secondUserHeaders, res: secondUserRes } = + await signInWithUser(OTHER_USER_EMAIL, PASSWORD); + + // Insert a teamMember row directly in the database tying the second user + // to the first org's team, standing in for a stale or mismatched row. + await db.create({ + model: "teamMember", + data: { + teamId: firstTeamId, + userId: secondUserRes.user.id, + createdAt: new Date(), + }, + }); + + const list = await client.organization.listTeamMembers({ + query: { teamId: firstTeamId }, + fetchOptions: { headers: secondUserHeaders }, + }); + + expect(list.data).toBeNull(); + expect(list.error?.code).toBe("USER_IS_NOT_A_MEMBER_OF_THE_TEAM"); + }); +}); diff --git a/packages/better-auth/src/plugins/organization/routes/crud-invites.ts b/packages/better-auth/src/plugins/organization/routes/crud-invites.ts index 8a1a491722..9ae9b869e1 100644 --- a/packages/better-auth/src/plugins/organization/routes/crud-invites.ts +++ b/packages/better-auth/src/plugins/organization/routes/crud-invites.ts @@ -466,6 +466,22 @@ export const createInvitation = (option: O) => { ORGANIZATION_ERROR_CODES.INVALID_TEAM_ID, ); } + // Every requested team must belong to the organization the + // invitation is for, so the stored team IDs stay consistent with + // the invitation's organization. This runs regardless of whether + // a team-size limit is configured. + for (const teamId of requestedTeamIds) { + const team = await adapter.findTeamById({ + teamId, + organizationId, + }); + if (!team) { + throw APIError.from( + "BAD_REQUEST", + ORGANIZATION_ERROR_CODES.TEAM_NOT_FOUND, + ); + } + } } if ( @@ -736,6 +752,22 @@ export const acceptInvitation = (options: O) => const onlyOne = teamIds.length === 1; for (const teamId of teamIds) { + // Confirm the team still belongs to the invitation's + // organization before adding the member. This keeps team + // membership consistent with the invitation's organization, + // including for older invitations and for teams that were + // moved or removed between invite and accept. + const team = await adapter.findTeamById({ + teamId, + organizationId: invitation.organizationId, + }); + if (!team) { + throw APIError.from( + "BAD_REQUEST", + ORGANIZATION_ERROR_CODES.TEAM_NOT_FOUND, + ); + } + await adapter.findOrCreateTeamMember({ teamId: teamId, userId: session.user.id, diff --git a/packages/better-auth/src/plugins/organization/routes/crud-team.ts b/packages/better-auth/src/plugins/organization/routes/crud-team.ts index 9cb96abaaf..4e2705d424 100644 --- a/packages/better-auth/src/plugins/organization/routes/crud-team.ts +++ b/packages/better-auth/src/plugins/organization/routes/crud-team.ts @@ -822,7 +822,26 @@ export const listUserTeams = (options: O) => userId: session.user.id, }); - return ctx.json(teams); + // Only return teams that belong to an organization the caller is + // actually a member of. A teamMember row on its own is not enough — + // a stray row could otherwise surface another organization's team + // metadata. + const orgIds = [...new Set(teams.map((team) => team.organizationId))]; + const memberships = await Promise.all( + orgIds.map((organizationId) => + adapter.checkMembership({ + userId: session.user.id, + organizationId, + }), + ), + ); + const memberOrgIds = new Set( + orgIds.filter((_, index) => memberships[index]), + ); + + return ctx.json( + teams.filter((team) => memberOrgIds.has(team.organizationId)), + ); }, ); @@ -898,6 +917,28 @@ export const listTeamMembers = (options: O) => ORGANIZATION_ERROR_CODES.YOU_DO_NOT_HAVE_AN_ACTIVE_TEAM, ); } + // Resolve the team to its organization and confirm the caller is a + // member of that organization. A teamMember row alone is not + // sufficient: a stray row pointing at another organization's team + // would otherwise return that team's member list. Organization + // membership is the requirement here. + const team = await adapter.findTeamById({ teamId }); + if (!team) { + throw APIError.from( + "BAD_REQUEST", + ORGANIZATION_ERROR_CODES.TEAM_NOT_FOUND, + ); + } + const isOrgMember = await adapter.checkMembership({ + userId: session.user.id, + organizationId: team.organizationId, + }); + if (!isOrgMember) { + throw APIError.from( + "BAD_REQUEST", + ORGANIZATION_ERROR_CODES.USER_IS_NOT_A_MEMBER_OF_THE_TEAM, + ); + } const member = await adapter.findTeamMember({ userId: session.user.id, teamId, diff --git a/packages/better-auth/src/plugins/organization/team.test.ts b/packages/better-auth/src/plugins/organization/team.test.ts index 76963192ec..e0e90ba316 100644 --- a/packages/better-auth/src/plugins/organization/team.test.ts +++ b/packages/better-auth/src/plugins/organization/team.test.ts @@ -1378,3 +1378,130 @@ describe("invitation with team id containing comma", async () => { }); }); }); + +describe("invitation team ids are scoped to the invited organization", async () => { + // Teams are enabled with the default (unlimited) team size, where team/org + // scoping previously did not run. + const { auth, client, signInWithTestUser, signInWithUser, cookieSetter } = + await getTestInstance( + { + plugins: [ + organization({ + async sendInvitationEmail() {}, + teams: { enabled: true }, + }), + ], + logger: { level: "error" }, + databaseHooks: { + user: { + create: { + before: async (user) => ({ + data: { ...user, emailVerified: true }, + }), + }, + }, + }, + }, + { testWith: "sqlite" }, + ); + + const ctx = await auth.$context; + + // Org A and its owner. + const orgAOwner = await signInWithTestUser(); + const orgA = await auth.api.createOrganization({ + headers: orgAOwner.headers, + body: { name: "Org A", slug: "org-a" }, + }); + if (!orgA) throw new Error("failed to create org A"); + + // A separate organization (Org B) with its own owner and team. + const inviteeEmail = "team-scope-invitee@email.com"; + const orgBOwner = { + email: "org-b-owner@email.com", + password: "password12345", + name: "Org B Owner", + }; + await client.signUp.email(orgBOwner); + const orgBHeaders = ( + await signInWithUser(orgBOwner.email, orgBOwner.password) + ).headers; + const orgB = await auth.api.createOrganization({ + headers: orgBHeaders, + body: { name: "Org B", slug: "org-b" }, + }); + if (!orgB) throw new Error("failed to create org B"); + const teamB = await auth.api.createTeam({ + headers: orgBHeaders, + body: { name: "Org B Team", organizationId: orgB.id }, + }); + const otherOrgTeamId = teamB.id; + + it("rejects createInvitation when the team belongs to another organization", async () => { + await expect( + auth.api.createInvitation({ + headers: orgAOwner.headers, + body: { + email: inviteeEmail, + role: "member", + organizationId: orgA.id, + teamId: otherOrgTeamId, + }, + }), + ).rejects.toMatchObject({ + status: "BAD_REQUEST", + body: { code: "TEAM_NOT_FOUND" }, + }); + }); + + it("does not add the member to another organization's team when accepting an invitation that stored one", async () => { + // An older invitation in Org A that references Org B's team directly, + // created before team/org scoping was enforced. + const invitationId = `team-scope-invitation-${Date.now()}`; + await ctx.adapter.create({ + model: "invitation", + forceAllowId: true, + data: { + id: invitationId, + email: inviteeEmail, + role: "member", + organizationId: orgA.id, + teamId: otherOrgTeamId, + status: "pending", + inviterId: orgAOwner.user.id, + expiresAt: new Date(Date.now() + 1000 * 60 * 60), + createdAt: new Date(), + }, + }); + + const inviteeHeaders = new Headers(); + const inviteeSignUp = await client.signUp.email( + { + email: inviteeEmail, + password: "password12345", + name: "Invitee", + }, + { onSuccess: cookieSetter(inviteeHeaders) }, + ); + const inviteeUserId = inviteeSignUp.data!.user.id; + + await expect( + auth.api.acceptInvitation({ + headers: inviteeHeaders, + body: { invitationId }, + }), + ).rejects.toMatchObject({ + status: "BAD_REQUEST", + body: { code: "TEAM_NOT_FOUND" }, + }); + + // The member should not be added to Org B's team. + const otherOrgTeamMembers = await ctx.adapter.findMany<{ userId: string }>({ + model: "teamMember", + where: [{ field: "teamId", value: otherOrgTeamId }], + }); + expect(otherOrgTeamMembers.some((m) => m.userId === inviteeUserId)).toBe( + false, + ); + }); +}); diff --git a/packages/better-auth/src/plugins/siwe/index.ts b/packages/better-auth/src/plugins/siwe/index.ts index 05900a3d5a..fd53eb2d1f 100644 --- a/packages/better-auth/src/plugins/siwe/index.ts +++ b/packages/better-auth/src/plugins/siwe/index.ts @@ -9,6 +9,7 @@ import { toChecksumAddress } from "../../utils/hashing"; import { isAPIError } from "../../utils/is-api-error"; import { getOrigin } from "../../utils/url"; import { PACKAGE_VERSION } from "../../version"; +import { normalizeSiweDomain, parseSiweMessage } from "./parse-message"; import type { WalletAddressSchema } from "./schema"; import { schema } from "./schema"; import type { @@ -148,6 +149,63 @@ export const siwe = (options: SIWEPluginOptions) => { // Verify SIWE message with enhanced parameters const { value: nonce } = verification; + + // Bind the *signed* message to server state before accepting the + // signature. Signature recovery alone (the documented `verifyMessage` + // using viem) does NOT inspect the message body, so a previously + // produced signature (stale, for another domain, or over an + // arbitrary string) could otherwise be presented alongside a freshly + // minted nonce. Parse the ERC-4361 message ourselves and require the + // nonce, address, chain id, and domain to match the server-issued + // values, plus honor the signed time bounds. + const parsedMessage = parseSiweMessage(message); + const nonceMatches = parsedMessage.nonce === nonce; + const addressMatches = + !!parsedMessage.address && + parsedMessage.address.toLowerCase() === + walletAddress.toLowerCase(); + const chainMatches = parsedMessage.chainId === chainId; + const domainMatches = + !!parsedMessage.domain && + normalizeSiweDomain(parsedMessage.domain) === + normalizeSiweDomain(options.domain); + + if ( + !nonceMatches || + !addressMatches || + !chainMatches || + !domainMatches + ) { + throw APIError.fromStatus("UNAUTHORIZED", { + message: + "Unauthorized: SIWE message does not match the expected nonce, domain, address, or chain ID", + status: 401, + code: "UNAUTHORIZED_SIWE_MESSAGE_MISMATCH", + }); + } + + const now = Date.now(); + if (parsedMessage.expirationTime) { + const expiresAt = Date.parse(parsedMessage.expirationTime); + if (!Number.isNaN(expiresAt) && now >= expiresAt) { + throw APIError.fromStatus("UNAUTHORIZED", { + message: "Unauthorized: SIWE message has expired", + status: 401, + code: "UNAUTHORIZED_SIWE_MESSAGE_EXPIRED", + }); + } + } + if (parsedMessage.notBefore) { + const notBefore = Date.parse(parsedMessage.notBefore); + if (!Number.isNaN(notBefore) && now < notBefore) { + throw APIError.fromStatus("UNAUTHORIZED", { + message: "Unauthorized: SIWE message is not yet valid", + status: 401, + code: "UNAUTHORIZED_SIWE_MESSAGE_NOT_YET_VALID", + }); + } + } + const verified = await options.verifyMessage({ message, signature, diff --git a/packages/better-auth/src/plugins/siwe/parse-message.ts b/packages/better-auth/src/plugins/siwe/parse-message.ts new file mode 100644 index 0000000000..d934a59b03 --- /dev/null +++ b/packages/better-auth/src/plugins/siwe/parse-message.ts @@ -0,0 +1,102 @@ +/** + * Minimal ERC-4361 (Sign-In with Ethereum) message parser. + * + * The plugin must independently extract the fields it validates (nonce, + * domain, address, chain id, time bounds) from the *signed* message — the + * caller-supplied `verifyMessage` cannot be relied on for this, since the + * documented `verifyMessage` (viem) only recovers the signature and never + * inspects the message body. + * + * Parsing is intentionally tolerant: it extracts the labeled fields it needs + * and leaves validation (presence + equality against server state) to the + * caller. It never throws. + * + * @see https://eips.ethereum.org/EIPS/eip-4361 + */ +export interface ParsedSiweMessage { + scheme?: string; + domain?: string; + address?: string; + uri?: string; + version?: string; + chainId?: number; + nonce?: string; + issuedAt?: string; + expirationTime?: string; + notBefore?: string; + requestId?: string; +} + +const HEADER_REGEX = + /^(?:([a-zA-Z][a-zA-Z0-9+.-]*):\/\/)?(\S+) wants you to sign in with your Ethereum account:$/; +const ADDRESS_REGEX = /^0x[a-fA-F0-9]{40}$/; +const FIELD_REGEX = /^([A-Za-z ]+): (.*)$/; + +export function parseSiweMessage(message: string): ParsedSiweMessage { + const result: ParsedSiweMessage = {}; + // Split tolerantly of CRLF; ERC-4361 uses LF but some wallets emit CRLF. + const lines = message.split(/\r?\n/); + + const headerMatch = lines[0]?.match(HEADER_REGEX); + if (headerMatch) { + if (headerMatch[1]) result.scheme = headerMatch[1]; + result.domain = headerMatch[2]; + } + + const addressLine = lines[1]?.trim(); + if (addressLine && ADDRESS_REGEX.test(addressLine)) { + result.address = addressLine; + } + + // Labeled fields appear in the suffix block. Parse them line-by-line so the + // optional statement (which may itself contain `: `) doesn't break parsing. + // The suffix fields always win because they come after the statement. + for (const line of lines) { + const match = line.match(FIELD_REGEX); + if (!match) continue; + const [, key, value] = match; + switch (key) { + case "URI": + result.uri = value; + break; + case "Version": + result.version = value; + break; + case "Chain ID": { + const parsed = Number(value); + if (Number.isInteger(parsed)) result.chainId = parsed; + break; + } + case "Nonce": + result.nonce = value; + break; + case "Issued At": + result.issuedAt = value; + break; + case "Expiration Time": + result.expirationTime = value; + break; + case "Not Before": + result.notBefore = value; + break; + case "Request ID": + result.requestId = value; + break; + } + } + + return result; +} + +/** + * Normalizes a SIWE `domain` (RFC 3986 authority) for comparison: strips any + * scheme and path, lowercases, leaving `host[:port]`. + */ +export function normalizeSiweDomain(domain: string): string { + const withoutScheme = domain + .trim() + .toLowerCase() + .replace(/^[a-z][a-z0-9+.-]*:\/\//, ""); + const pathStart = withoutScheme.indexOf("/"); + return pathStart === -1 ? withoutScheme : withoutScheme.slice(0, pathStart); +} diff --git a/packages/better-auth/src/plugins/siwe/siwe.test.ts b/packages/better-auth/src/plugins/siwe/siwe.test.ts index 06fcdb550f..5e49eec41d 100644 --- a/packages/better-auth/src/plugins/siwe/siwe.test.ts +++ b/packages/better-auth/src/plugins/siwe/siwe.test.ts @@ -7,6 +7,37 @@ describe("siwe", async () => { const walletAddress = "0x000000000000000000000000000000000000dEaD"; const domain = "example.com"; const chainId = 1; // Ethereum mainnet + const NONCE = "A1b2C3d4E5f6G7h8J"; + + // Builds a valid ERC-4361 message bound to the server-issued nonce. The + // plugin now parses and validates this message, so tests must sign a real + // SIWE message rather than an arbitrary string. + const siweMessage = (opts?: { + domain?: string; + address?: string; + chainId?: number; + nonce?: string; + expirationTime?: string; + notBefore?: string; + }) => { + const d = opts?.domain ?? domain; + const a = opts?.address ?? walletAddress; + const c = opts?.chainId ?? chainId; + const n = opts?.nonce ?? NONCE; + let msg = + `${d} wants you to sign in with your Ethereum account:\n` + + `${a}\n\n` + + `Sign in.\n\n` + + `URI: https://${d}\n` + + `Version: 1\n` + + `Chain ID: ${c}\n` + + `Nonce: ${n}\n` + + `Issued At: 2024-01-01T00:00:00.000Z`; + if (opts?.expirationTime) + msg += `\nExpiration Time: ${opts.expirationTime}`; + if (opts?.notBefore) msg += `\nNot Before: ${opts.notBefore}`; + return msg; + }; it("should generate a valid nonce for a valid public key", async () => { const { client } = await getTestInstance( @@ -17,10 +48,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -47,10 +77,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -79,10 +108,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -112,10 +140,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -127,7 +154,7 @@ describe("siwe", async () => { }, ); const { error } = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress, chainId, @@ -148,10 +175,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -179,10 +205,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -211,10 +236,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -241,10 +265,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -274,10 +297,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -290,7 +312,7 @@ describe("siwe", async () => { ); const { error } = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress, email: undefined, @@ -312,10 +334,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -330,7 +351,7 @@ describe("siwe", async () => { await client.siwe.nonce({ walletAddress, chainId }); const { data, error } = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress, chainId, @@ -350,10 +371,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -366,7 +386,7 @@ describe("siwe", async () => { ); const { error } = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress, email: "not-an-email", @@ -386,10 +406,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -403,7 +422,7 @@ describe("siwe", async () => { await client.siwe.nonce({ walletAddress, chainId }); const { data, error } = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress, chainId, @@ -421,10 +440,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -436,7 +454,7 @@ describe("siwe", async () => { await client.siwe.nonce({ walletAddress, chainId }); const first = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress, chainId, @@ -446,7 +464,7 @@ describe("siwe", async () => { // Try to verify again with the same nonce const second = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress, chainId, @@ -466,10 +484,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -481,7 +498,7 @@ describe("siwe", async () => { await client.siwe.nonce({ walletAddress, chainId }); const { error } = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress, chainId, @@ -503,10 +520,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -522,7 +538,7 @@ describe("siwe", async () => { chainId, }); const { data } = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress: walletAddress.toLowerCase(), chainId, @@ -545,7 +561,7 @@ describe("siwe", async () => { chainId, }); const { data: data2 } = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress: walletAddress.toUpperCase(), chainId, @@ -568,10 +584,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -588,7 +603,7 @@ describe("siwe", async () => { chainId: testChainId, }); const firstUser = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress: testAddress, chainId: testChainId, @@ -617,7 +632,7 @@ describe("siwe", async () => { chainId: testChainId, }); const secondUser = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress: testAddress, chainId: testChainId, @@ -660,10 +675,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, schema: { walletAddress: { @@ -692,7 +706,7 @@ describe("siwe", async () => { chainId: testChainId, }); const result = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress: testAddress, chainId: testChainId, @@ -725,10 +739,9 @@ describe("siwe", async () => { async getNonce() { return "A1b2C3d4E5f6G7h8J"; }, - async verifyMessage({ message, signature }) { - return ( - signature === "valid_signature" && message === "valid_message" - ); + async verifyMessage({ signature }) { + // Mirrors the documented viem pattern: signature recovery only. + return signature === "valid_signature"; }, }), ], @@ -743,7 +756,7 @@ describe("siwe", async () => { // First authentication on Ethereum await client.siwe.nonce({ walletAddress: testAddress, chainId: chainId1 }); const ethereumAuth = await client.siwe.verify({ - message: "valid_message", + message: siweMessage(), signature: "valid_signature", walletAddress: testAddress, chainId: chainId1, @@ -754,7 +767,7 @@ describe("siwe", async () => { // Second authentication on Polygon with same address await client.siwe.nonce({ walletAddress: testAddress, chainId: chainId2 }); const polygonAuth = await client.siwe.verify({ - message: "valid_message", + message: siweMessage({ chainId: chainId2 }), signature: "valid_signature", walletAddress: testAddress, chainId: chainId2, @@ -785,4 +798,137 @@ describe("siwe", async () => { expect(polygonRecord?.isPrimary).toBe(false); // Second address is not primary expect(ethereumRecord?.userId).toBe(polygonRecord?.userId); // Same user ID }); + + /** + * The plugin must bind the signed message to the server-issued nonce, + * configured domain, address, and chain id — signature recovery alone (the + * documented viem `verifyMessage`) is not sufficient. Otherwise a valid + * signature the wallet previously produced (stale, for another domain, or + * over an arbitrary string) could be reused with a freshly minted nonce to + * mint a session. + */ + describe("message binding", () => { + const setup = async () => { + // Verifier mirrors the documented viem pattern: signature recovery + // only, with no inspection of the message body. + const { client, auth } = await getTestInstance( + { + plugins: [ + siwe({ + domain, + async getNonce() { + return NONCE; + }, + async verifyMessage({ signature }) { + return signature === "valid_signature"; + }, + }), + ], + }, + { clientOptions: { plugins: [siweClient()] } }, + ); + return { client, auth }; + }; + + it("rejects a valid signature over a message with a non-matching nonce", async () => { + const { client } = await setup(); + await client.siwe.nonce({ walletAddress, chainId }); + const { error } = await client.siwe.verify({ + message: siweMessage({ nonce: "some-other-nonce" }), + signature: "valid_signature", + walletAddress, + chainId, + }); + expect(error?.status).toBe(401); + expect(error?.code).toBe("UNAUTHORIZED_SIWE_MESSAGE_MISMATCH"); + }); + + it("rejects a message bound to a different domain", async () => { + const { client } = await setup(); + await client.siwe.nonce({ walletAddress, chainId }); + const { error } = await client.siwe.verify({ + message: siweMessage({ domain: "other.example.com" }), + signature: "valid_signature", + walletAddress, + chainId, + }); + expect(error?.status).toBe(401); + expect(error?.code).toBe("UNAUTHORIZED_SIWE_MESSAGE_MISMATCH"); + }); + + it("rejects a message whose chain id does not match", async () => { + const { client } = await setup(); + await client.siwe.nonce({ walletAddress, chainId }); + const { error } = await client.siwe.verify({ + message: siweMessage({ chainId: 137 }), + signature: "valid_signature", + walletAddress, + chainId, + }); + expect(error?.status).toBe(401); + expect(error?.code).toBe("UNAUTHORIZED_SIWE_MESSAGE_MISMATCH"); + }); + + it("rejects an arbitrary (non-SIWE) message even with a valid signature", async () => { + const { client } = await setup(); + await client.siwe.nonce({ walletAddress, chainId }); + const { error } = await client.siwe.verify({ + message: "gm, please sign this to continue", + signature: "valid_signature", + walletAddress, + chainId, + }); + expect(error?.status).toBe(401); + expect(error?.code).toBe("UNAUTHORIZED_SIWE_MESSAGE_MISMATCH"); + }); + + it("rejects an expired SIWE message", async () => { + const { client } = await setup(); + await client.siwe.nonce({ walletAddress, chainId }); + const { error } = await client.siwe.verify({ + message: siweMessage({ + expirationTime: "2020-01-01T00:00:00.000Z", + }), + signature: "valid_signature", + walletAddress, + chainId, + }); + expect(error?.status).toBe(401); + expect(error?.code).toBe("UNAUTHORIZED_SIWE_MESSAGE_EXPIRED"); + }); + + it("does not mint a session for an existing wallet user when an unrelated signature is reused", async () => { + const { client, auth } = await setup(); + + // The wallet user signs in normally, creating the wallet user. + await client.siwe.nonce({ walletAddress, chainId }); + const legit = await client.siwe.verify({ + message: siweMessage(), + signature: "valid_signature", + walletAddress, + chainId, + }); + expect(legit.data?.success).toBe(true); + + const ctx = await auth.$context; + const sessionsBefore = await ctx.adapter.findMany({ + model: "session", + }); + + // A second request mints a fresh nonce and reuses a previously + // produced signature over an unrelated message for the same wallet. + await client.siwe.nonce({ walletAddress, chainId }); + const secondAttempt = await client.siwe.verify({ + message: "Approve transfer of 1 ETH", + signature: "valid_signature", + walletAddress, + chainId, + }); + expect(secondAttempt.error?.status).toBe(401); + expect(secondAttempt.data).toBeNull(); + + const sessionsAfter = await ctx.adapter.findMany({ model: "session" }); + expect(sessionsAfter.length).toBe(sessionsBefore.length); + }); + }); }); diff --git a/packages/better-auth/src/social.test.ts b/packages/better-auth/src/social.test.ts index 5db22f0699..f1645b4ee2 100644 --- a/packages/better-auth/src/social.test.ts +++ b/packages/better-auth/src/social.test.ts @@ -8,6 +8,7 @@ import type { RailwayProfile, VercelProfile, } from "@better-auth/core/social-providers"; +import { reddit } from "@better-auth/core/social-providers"; import { betterFetch } from "@better-fetch/fetch"; import { exportJWK, generateKeyPair, SignJWT } from "jose"; import { HttpResponse, http } from "msw"; @@ -2446,3 +2447,194 @@ describe("Railway Provider", async () => { expect(accounts[0]?.accountId).toBe("user_railway_123"); }); }); + +/** + * Regression tests for PayPal ID token verification. Previously + * `verifyIdToken` only decoded the JWT and checked that a `sub` claim was + * present, performing no signature/issuer/audience checks, so any well-formed + * token paired with a valid access token would be accepted. + */ +describe("PayPal Provider — id token verification", async () => { + const paypalKeyPair = await generateKeyPair("RS256"); + const paypalJwk = await exportJWK(paypalKeyPair.publicKey); + const paypalKid = "test-paypal-kid"; + paypalJwk.kid = paypalKid; + paypalJwk.alg = "RS256"; + paypalJwk.use = "sig"; + + // A key that is not in the provider's published JWKS, used to sign tokens + // that must be rejected. + const otherKeyPair = await generateKeyPair("RS256"); + + const clientId = "paypal-client-id"; + + const userInfo = { + user_id: "paypal-user-123", + name: "PayPal User", + email: "paypal-user@example.com", + email_verified: true, + picture: "https://example.com/avatar.png", + }; + + const buildToken = ( + signingKey: CryptoKey, + overrides: { + audience?: string; + issuer?: string; + kid?: string; + } = {}, + ) => + new SignJWT({ sub: "paypal-user-123" }) + .setProtectedHeader({ alg: "RS256", kid: overrides.kid ?? paypalKid }) + .setIssuedAt() + .setIssuer(overrides.issuer ?? "https://www.paypal.com") + .setAudience(overrides.audience ?? clientId) + .setExpirationTime("1h") + .sign(signingKey); + + beforeAll(() => { + mswServer.use( + http.get("https://api.paypal.com/v1/oauth2/certs", async () => + HttpResponse.json({ keys: [paypalJwk] }), + ), + http.get( + "https://api-m.paypal.com/v1/identity/oauth2/userinfo", + async () => HttpResponse.json(userInfo), + ), + ); + }); + + const getInstance = () => + getTestInstance( + { + socialProviders: { + paypal: { + clientId, + clientSecret: "paypal-client-secret", + environment: "live", + }, + }, + }, + { disableTestUser: true }, + ); + + it("accepts a properly signed id token", async () => { + const idToken = await buildToken(paypalKeyPair.privateKey); + const { client } = await getInstance(); + + const res = await client.signIn.social({ + provider: "paypal", + idToken: { token: idToken, accessToken: "paypal-access-token" }, + }); + + expect(res.error).toBeNull(); + expect(res.data?.redirect).toBe(false); + const data = res.data as { user: { email: string } }; + expect(data.user.email).toBe("paypal-user@example.com"); + }); + + it("rejects a token signed by a key not in the provider's JWKS", async () => { + const idToken = await buildToken(otherKeyPair.privateKey); + const { client } = await getInstance(); + + const res = await client.signIn.social({ + provider: "paypal", + idToken: { token: idToken, accessToken: "other-access-token" }, + }); + + expect(res.error?.status).toBe(401); + }); + + it("rejects a token whose audience is a different client", async () => { + const idToken = await buildToken(paypalKeyPair.privateKey, { + audience: "some-other-client", + }); + const { client } = await getInstance(); + + const res = await client.signIn.social({ + provider: "paypal", + idToken: { token: idToken, accessToken: "other-access-token" }, + }); + + expect(res.error?.status).toBe(401); + }); + + it("rejects a token with an unexpected issuer", async () => { + const idToken = await buildToken(paypalKeyPair.privateKey, { + issuer: "https://not-paypal.example.com", + }); + const { client } = await getInstance(); + + const res = await client.signIn.social({ + provider: "paypal", + idToken: { token: idToken, accessToken: "other-access-token" }, + }); + + expect(res.error?.status).toBe(401); + }); +}); + +/** + * Regression tests for the Reddit provider profile mapping. Reddit's `identity` + * scope does not return an email; previously the provider mapped the shared + * `oauth_client_id` (which identifies the OAuth app, not the user) to + * `user.email` and `has_verified_email` to `emailVerified`, collapsing every + * Reddit user of the same app onto one "verified" email and enabling implicit + * account linking/takeover. + */ +describe("Reddit Provider — profile email mapping", async () => { + const profile = { + id: "reddit-user-123", + name: "reddit_user", + icon_img: "https://example.com/avatar.png?x=1", + has_verified_email: true, + oauth_client_id: "shared-reddit-oauth-client-id", + verified: true, + }; + + beforeAll(() => { + mswServer.use( + http.get("https://oauth.reddit.com/api/v1/me", async () => + HttpResponse.json(profile), + ), + ); + }); + + it("never uses oauth_client_id as the email and falls back to a unique per-user email", async () => { + const provider = reddit({ + clientId: "reddit-client-id", + clientSecret: "reddit-client-secret", + }); + + const result = await provider.getUserInfo({ + accessToken: "reddit-access-token", + } as any); + + // Without a mapped email the provider derives a unique, per-user + // synthetic email from the Reddit user id rather than falling back to + // the shared oauth_client_id, so users can never collide on one address. + expect(result?.user.email).toBe("reddit-user-123@reddit.com"); + expect(result?.user.email).not.toBe(profile.oauth_client_id); + expect(result?.user.emailVerified).toBe(false); + }); + + it("uses the email supplied by mapProfileToUser and ignores oauth_client_id", async () => { + const provider = reddit({ + clientId: "reddit-client-id", + clientSecret: "reddit-client-secret", + mapProfileToUser: (p) => ({ + email: `${p.name}@example.com`, + }), + }); + + const result = await provider.getUserInfo({ + accessToken: "reddit-access-token", + } as any); + + expect(result?.user.email).toBe("reddit_user@example.com"); + expect(result?.user.email).not.toBe(profile.oauth_client_id); + // The synthetic "verified email" flag must not be applied to the mapped + // email unless the integrator opts in. + expect(result?.user.emailVerified).toBe(false); + }); +}); diff --git a/packages/cli/src/commands/info.ts b/packages/cli/src/commands/info.ts index d132688e99..c4a3d91065 100644 --- a/packages/cli/src/commands/info.ts +++ b/packages/cli/src/commands/info.ts @@ -147,6 +147,8 @@ function sanitizeBetterAuthConfig(config: any): any { // List of sensitive keys to redact const sensitiveKeys = [ "secret", + "secrets", + "secretKey", "clientSecret", "clientId", "authToken", @@ -240,6 +242,8 @@ function sanitizeBetterAuthConfig(config: any): any { ) { if (typeof value === "string" && value.length > 0) { result[key] = "[REDACTED]"; + } else if (Array.isArray(value)) { + result[key] = "[REDACTED]"; } else if (typeof value === "object" && value !== null) { // Still recurse into objects but mark them as potentially sensitive result[key] = redactSensitive(value, key); diff --git a/packages/cli/test/info.test.ts b/packages/cli/test/info.test.ts index baa628d023..faca7b9302 100644 --- a/packages/cli/test/info.test.ts +++ b/packages/cli/test/info.test.ts @@ -139,6 +139,46 @@ describe("info command", () => { expect(output.betterAuth.config.baseURL).toBe("https://example.com"); }); + it("should redact versioned secrets", async () => { + await fs.writeFile( + path.join(tmpDir, "package.json"), + JSON.stringify({ + name: "test-project", + version: "1.0.0", + dependencies: { + "better-auth": "^1.0.0", + }, + }), + ); + + await fs.writeFile( + path.join(tmpDir, "auth.ts"), + `import { betterAuth } from "better-auth"; + + export const auth = betterAuth({ + secrets: [ + { version: 1, value: "old-rotation-secret-123" }, + { version: 2, value: "new-rotation-secret-456" }, + ], + emailAndPassword: { + enabled: true, + } + })`, + ); + + const { stdout } = await execAsync(`node ${cliPath} info --json`, { + cwd: tmpDir, + }); + + const output = JSON.parse(stdout); + + expect(output.betterAuth.config).toBeDefined(); + expect(output.betterAuth.config.secrets).toBe("[REDACTED]"); + const configStr = JSON.stringify(output.betterAuth.config); + expect(configStr).not.toContain("old-rotation-secret-123"); + expect(configStr).not.toContain("new-rotation-secret-456"); + }); + it("should detect installed frameworks", async () => { // Create package.json with various frameworks await fs.writeFile( diff --git a/packages/core/src/env/env-impl.test.ts b/packages/core/src/env/env-impl.test.ts new file mode 100644 index 0000000000..916f603362 --- /dev/null +++ b/packages/core/src/env/env-impl.test.ts @@ -0,0 +1,61 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +/** + * `nodeENV` / `isProduction` are evaluated at module load time, so each scenario + * stubs the global env source, resets the module registry, and re-imports a fresh + * copy of the module. + */ +async function loadEnvModule() { + vi.resetModules(); + return import("./env-impl"); +} + +describe("cross-runtime production detection", () => { + afterEach(() => { + vi.unstubAllGlobals(); + vi.resetModules(); + }); + + it("detects production from process.env (Node/Bun)", async () => { + vi.stubGlobal("process", { env: { NODE_ENV: "production" } }); + const { nodeENV, isProduction } = await loadEnvModule(); + expect(nodeENV).toBe("production"); + expect(isProduction).toBe(true); + }); + + /** + * Regression: a non-Node runtime exposing NODE_ENV via Deno's env API (with no + * usable `process.env`) must still resolve `isProduction`, otherwise + * development-mode defaults (default secret acceptance, no rate limiting, + * non-Secure cookies) would carry over into production. + */ + it("detects production from Deno env when process.env is unavailable", async () => { + // process exists but without a usable env, mirroring a stripped runtime + vi.stubGlobal("process", { env: undefined }); + vi.stubGlobal("Deno", { + env: { + toObject: () => ({ NODE_ENV: "production" }), + get: (key: string) => (key === "NODE_ENV" ? "production" : undefined), + }, + }); + const { nodeENV, isProduction } = await loadEnvModule(); + expect(nodeENV).toBe("production"); + expect(isProduction).toBe(true); + }); + + it("detects production from a shimmed __env__ source", async () => { + vi.stubGlobal("process", { env: undefined }); + vi.stubGlobal("__env__", { NODE_ENV: "production" }); + const { nodeENV, isProduction } = await loadEnvModule(); + expect(nodeENV).toBe("production"); + expect(isProduction).toBe(true); + }); + + it("is not production when no env source reports production", async () => { + vi.stubGlobal("process", { env: {} }); + const { nodeENV, isProduction, isDevelopment } = await loadEnvModule(); + expect(nodeENV).toBe(""); + expect(isProduction).toBe(false); + expect(isDevelopment()).toBe(false); + }); +}); diff --git a/packages/core/src/env/env-impl.ts b/packages/core/src/env/env-impl.ts index 7e53816bd8..6ab4a7586e 100644 --- a/packages/core/src/env/env-impl.ts +++ b/packages/core/src/env/env-impl.ts @@ -46,8 +46,7 @@ function toBoolean(val: boolean | string | undefined) { return val ? val !== "false" : false; } -export const nodeENV = - (typeof process !== "undefined" && process.env && process.env.NODE_ENV) || ""; +export const nodeENV = env.NODE_ENV ?? ""; /** Detect if `NODE_ENV` environment variable is `production` */ export const isProduction = nodeENV === "production"; diff --git a/packages/core/src/oauth2/verify.test.ts b/packages/core/src/oauth2/verify.test.ts index 31a90b7255..15c4d54dae 100644 --- a/packages/core/src/oauth2/verify.test.ts +++ b/packages/core/src/oauth2/verify.test.ts @@ -79,6 +79,19 @@ describe("verifyAccessToken", () => { ); } + function jwksResponse(...publicJWKs: JWK[]) { + return new Response(JSON.stringify({ keys: publicJWKs }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + + function requestUrl(input: unknown): string { + if (typeof input === "string") return input; + if (input instanceof Request) return input.url; + return String((input as { url?: unknown } | null)?.url ?? input); + } + async function expectUnauthorized( promise: Promise, message = "invalid access token", @@ -199,6 +212,114 @@ describe("verifyAccessToken", () => { ); }); + it("should not verify a token against a JWKS cached for a different issuer with a colliding kid", async () => { + vi.resetModules(); + const { verifyAccessToken: verify } = await import("./verify"); + + const sharedKid = "shared-kid"; + const keyA = await createTestJWKS(sharedKid); + const keyB = await createTestJWKS(sharedKid); + + const issuerA = "https://issuer-a.example.com"; + const audienceA = "https://issuer-a.example.com/api"; + const jwksUrlA = `${issuerA}/jwks`; + const issuerB = "https://issuer-b.example.com"; + const jwksUrlB = `${issuerB}/jwks`; + + mockedFetch.mockImplementation((input: unknown) => { + const url = requestUrl(input); + return Promise.resolve( + url.includes("issuer-b") + ? jwksResponse(keyB.publicJWK) + : jwksResponse(keyA.publicJWK), + ); + }); + + // 1. Source B's JWKS gets cached by verifying a token signed with B's key + // against B's own source. + const tokenForB = await createSignedToken(keyB.privateKey, sharedKid, { + iss: issuerB, + aud: issuerB, + }); + await expect( + verify(tokenForB, { + jwksUrl: jwksUrlB, + verifyOptions: { issuer: issuerB, audience: issuerB }, + }), + ).resolves.toMatchObject({ iss: issuerB }); + + // 2. A token signed with B's key but carrying source A's issuer/audience + // and the same colliding kid. + const tokenWithAClaims = await createSignedToken( + keyB.privateKey, + sharedKid, + { iss: issuerA, aud: audienceA }, + ); + + // Verifying it against source A must fetch A's own key and reject it, + // rather than reusing the key set cached for source B. + await expectUnauthorized( + verify(tokenWithAClaims, { + jwksUrl: jwksUrlA, + verifyOptions: { issuer: issuerA, audience: audienceA }, + }), + ); + + mockedFetch.mockReset(); + vi.resetModules(); + }); + + it("should refetch a rotated key set once the cache TTL has elapsed", async () => { + vi.resetModules(); + const { verifyAccessToken: verify } = await import("./verify"); + + const rotatingKid = "rotating-kid"; + const oldKey = await createTestJWKS(rotatingKid); + const newKey = await createTestJWKS(rotatingKid); + const rotateIssuer = "https://rotate.example.com"; + const rotateJwksUrl = `${rotateIssuer}/jwks`; + + let currentKey = oldKey.publicJWK; + mockedFetch.mockImplementation(() => + Promise.resolve(jwksResponse(currentKey)), + ); + + const oldToken = await createSignedToken(oldKey.privateKey, rotatingKid, { + iss: rotateIssuer, + aud: rotateIssuer, + }); + await expect( + verify(oldToken, { + jwksUrl: rotateJwksUrl, + verifyOptions: { issuer: rotateIssuer, audience: rotateIssuer }, + }), + ).resolves.toMatchObject({ iss: rotateIssuer }); + + // The source rotates its key while keeping the same kid. A token signed + // with the rotated-out key must stop verifying once the cache expires. + currentKey = newKey.publicJWK; + const staleToken = await createSignedToken(oldKey.privateKey, rotatingKid, { + iss: rotateIssuer, + aud: rotateIssuer, + }); + + vi.useFakeTimers(); + try { + vi.setSystemTime(Date.now() + 6 * 60 * 1000); + await expectUnauthorized( + verify(staleToken, { + jwksUrl: rotateJwksUrl, + verifyOptions: { issuer: rotateIssuer, audience: rotateIssuer }, + }), + ); + } finally { + vi.useRealTimers(); + } + + mockedFetch.mockReset(); + vi.resetModules(); + }); + /** * @see https://github.com/better-auth/better-auth/issues/9654 */ @@ -222,4 +343,78 @@ describe("verifyAccessToken", () => { vi.resetModules(); } }); + + describe("remote introspection audience validation", () => { + const introspectUrl = `${issuer}/oauth2/introspect`; + const remoteVerify = { + introspectUrl, + clientId: "rs-client", + clientSecret: "rs-secret", + }; + + function mockIntrospection(body: Record) { + mockJSONResponse({ active: true, iss: issuer, sub: "user-123", ...body }); + } + + it("should reject an active token when audience is required but introspection omits aud", async () => { + mockIntrospection({ scope: "read" }); + + await expect( + verifyAccessToken("opaque-token-for-another-resource", { + verifyOptions: { issuer, audience }, + remoteVerify, + }), + ).rejects.toThrow(); + }); + + it("should reject an active token whose introspected aud is for a different resource", async () => { + mockIntrospection({ + aud: "https://api.example.com/other", + scope: "read", + }); + + await expect( + verifyAccessToken("token-minted-for-other-api", { + verifyOptions: { issuer, audience }, + remoteVerify, + }), + ).rejects.toThrow(); + }); + + it("should accept an active token whose introspected aud matches", async () => { + mockIntrospection({ aud: audience, scope: "read" }); + + await expect( + verifyAccessToken("valid-token", { + verifyOptions: { issuer, audience }, + remoteVerify, + }), + ).resolves.toMatchObject({ aud: audience, sub: "user-123" }); + }); + + it("should accept an aud-less introspection response only when allowMissingAudience is enabled", async () => { + mockIntrospection({ scope: "read" }); + + await expect( + verifyAccessToken("opaque-token", { + verifyOptions: { issuer, audience }, + remoteVerify: { ...remoteVerify, allowMissingAudience: true }, + }), + ).resolves.toMatchObject({ sub: "user-123" }); + }); + + it("should still enforce a mismatching aud even when allowMissingAudience is enabled", async () => { + mockIntrospection({ + aud: "https://api.example.com/other", + scope: "read", + }); + + await expect( + verifyAccessToken("token-minted-for-other-api", { + verifyOptions: { issuer, audience }, + remoteVerify: { ...remoteVerify, allowMissingAudience: true }, + }), + ).rejects.toThrow(); + }); + }); }); diff --git a/packages/core/src/oauth2/verify.ts b/packages/core/src/oauth2/verify.ts index 3267e5e4de..75fafc3086 100644 --- a/packages/core/src/oauth2/verify.ts +++ b/packages/core/src/oauth2/verify.ts @@ -25,8 +25,22 @@ function isJoseInfrastructureError(error: joseErrors.JOSEError) { return joseInfrastructureErrorCodes.has(error.code); } -/** Last fetched jwks used locally in getJwks @internal */ -let jwks: JSONWebKeySet | undefined; +interface JwksCacheEntry { + jwks: JSONWebKeySet; + fetchedAt: number; +} + +const jwksCache = new Map< + string | (() => Promise), + JwksCacheEntry +>(); + +/** + * How long a cached JWKS is trusted before it is refetched + * + * @internal + */ +const JWKS_CACHE_TTL_MS = 5 * 60 * 1000; export interface VerifyAccessTokenRemote { /** Full url of the introspect endpoint. Should end with `/oauth2/introspect` */ @@ -41,6 +55,20 @@ export interface VerifyAccessTokenRemote { * is also still active. */ force?: boolean; + /** + * Accept introspection responses that omit the `aud` claim even when a + * required `audience` is configured in `verifyOptions`. + * + * By default verification fails closed: if you configure an `audience` and + * the introspection response has no `aud` (or a mismatching one), the token + * is rejected. Some authorization servers legitimately omit `aud` from + * introspection responses (it is OPTIONAL per RFC 7662 §2.2); only enable + * this if you trust the issuer to bind the token to this resource through + * another mechanism, as it skips the audience check in that case. + * + * @default false + */ + allowMissingAudience?: boolean; } /** @@ -96,10 +124,21 @@ export async function getJwks( if (!jwtHeaders.kid) { throw new APIError("UNAUTHORIZED", { message: "invalid access token" }); } + const kid = jwtHeaders.kid; - // Fetch jwks if not set or has a different kid than the one stored - if (!jwks || !jwks.keys.find((jwk) => jwk.kid === jwtHeaders.kid)) { - jwks = + const cacheKey = opts.jwksFetch; + const cached = jwksCache.get(cacheKey); + const isFresh = cached + ? Date.now() - cached.fetchedAt < JWKS_CACHE_TTL_MS + : false; + const hasKid = cached?.jwks.keys.some((jwk) => jwk.kid === kid) ?? false; + + // Refetch when this source has no cached set, the cached set has expired, or + // it does not contain the token's kid (e.g. a newly rotated-in key). The + // cache is scoped to `cacheKey`, so a token is only ever matched against the + // key set published by its own source. + if (!cached || !isFresh || !hasKid) { + const jwks = typeof opts.jwksFetch === "string" ? await betterFetch(opts.jwksFetch, { headers: { @@ -114,9 +153,11 @@ export async function getJwks( }) : await opts.jwksFetch(); if (!jwks) throw new Error("No jwks found"); + jwksCache.set(cacheKey, { jwks, fetchedAt: Date.now() }); + return jwks; } - return jwks; + return cached.jwks; } /** @@ -201,13 +242,23 @@ export async function verifyAccessToken( throw new APIError("UNAUTHORIZED", { message: "token inactive", }); - // Verifies payload using verify options (token valid through introspect) + // Verifies payload using verify options (token valid through introspect). + // Audience is enforced by default: when `verifyOptions.audience` is set + // but the introspection response omits `aud` (or it mismatches), + // `UnsecuredJWT.decode` throws and the token is rejected. Otherwise a + // token issued for a different resource/client on the same issuer would + // also pass. Only drop the audience check when the caller has explicitly + // opted in via `remoteVerify.allowMissingAudience`. try { const unsecuredJwt = new UnsecuredJWT(introspect).encode(); - const { audience: _audience, ...verifyOptions } = opts.verifyOptions; - const verify = introspect.aud - ? UnsecuredJWT.decode(unsecuredJwt, opts.verifyOptions) - : UnsecuredJWT.decode(unsecuredJwt, verifyOptions); + const { audience: _audience, ...verifyOptionsNoAudience } = + opts.verifyOptions; + const skipAudience = + !introspect.aud && opts.remoteVerify.allowMissingAudience === true; + const verify = UnsecuredJWT.decode( + unsecuredJwt, + skipAudience ? verifyOptionsNoAudience : opts.verifyOptions, + ); payload = verify.payload; } catch (error) { throw new Error(error as unknown as string); diff --git a/packages/core/src/social-providers/facebook.test.ts b/packages/core/src/social-providers/facebook.test.ts new file mode 100644 index 0000000000..9028cf1261 --- /dev/null +++ b/packages/core/src/social-providers/facebook.test.ts @@ -0,0 +1,207 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@better-fetch/fetch", () => ({ + betterFetch: vi.fn(), +})); + +import { betterFetch } from "@better-fetch/fetch"; + +import { facebook } from "./facebook"; + +const mockedBetterFetch = vi.mocked(betterFetch); + +const options = { + clientId: "fb-app", + clientSecret: "fb-secret", +}; + +function debugTokenResponse(data: { + is_valid?: boolean; + app_id?: string; + user_id?: string; +}) { + return { data: { data }, error: null } as Awaited< + ReturnType + >; +} + +function profileResponse(profile: Record) { + return { data: profile, error: null } as Awaited< + ReturnType + >; +} + +function fbProfile(id: string, email = `${id}@example.com`) { + return { + id, + name: `User ${id}`, + email, + picture: { + data: { url: "https://x", height: 100, width: 100, is_silhouette: false }, + }, + }; +} + +/** + * Routes the two Graph calls (`debug_token` then `/me`) the access-token path + * makes, based on the request URL. + */ +function mockGraph(opts: { + debug?: ReturnType; + me?: ReturnType; +}) { + mockedBetterFetch.mockImplementation(((url: unknown) => { + const u = String(url); + if (u.includes("debug_token")) { + return Promise.resolve( + opts.debug ?? + ({ data: null, error: { message: "no debug mock" } } as any), + ); + } + return Promise.resolve( + opts.me ?? ({ data: null, error: { message: "no /me mock" } } as any), + ); + }) as unknown as typeof betterFetch); +} + +describe("facebook.verifyIdToken (opaque access token)", () => { + beforeEach(() => { + mockedBetterFetch.mockReset(); + }); + + it("accepts an opaque token bound to the configured app", async () => { + mockGraph({ + debug: debugTokenResponse({ + is_valid: true, + app_id: "fb-app", + user_id: "u1", + }), + }); + const provider = facebook(options); + await expect( + provider.verifyIdToken("opaque-access-token", undefined), + ).resolves.toBe(true); + }); + + it("accepts a token bound to any configured client id", async () => { + mockGraph({ + debug: debugTokenResponse({ + is_valid: true, + app_id: "fb-mobile", + user_id: "u1", + }), + }); + const provider = facebook({ + clientId: ["fb-app", "fb-mobile"], + clientSecret: "fb-secret", + }); + await expect( + provider.verifyIdToken("opaque-access-token", undefined), + ).resolves.toBe(true); + }); + + it("rejects an opaque token issued to a different app", async () => { + mockGraph({ + debug: debugTokenResponse({ + is_valid: true, + app_id: "someone-elses-app", + user_id: "u1", + }), + }); + const provider = facebook(options); + await expect( + provider.verifyIdToken("foreign-app-token", undefined), + ).resolves.toBe(false); + }); + + it("rejects an invalid opaque token", async () => { + mockGraph({ + debug: debugTokenResponse({ + is_valid: false, + app_id: "fb-app", + user_id: "u1", + }), + }); + const provider = facebook(options); + await expect( + provider.verifyIdToken("revoked-token", undefined), + ).resolves.toBe(false); + }); + + it("rejects when no client secret is configured", async () => { + mockGraph({ + debug: debugTokenResponse({ + is_valid: true, + app_id: "fb-app", + user_id: "u1", + }), + }); + const provider = facebook({ clientId: "fb-app", clientSecret: "" }); + await expect( + provider.verifyIdToken("opaque-access-token", undefined), + ).resolves.toBe(false); + }); +}); + +describe("facebook.getUserInfo (opaque access token)", () => { + beforeEach(() => { + mockedBetterFetch.mockReset(); + }); + + it("returns the profile for a token bound to the configured app", async () => { + mockGraph({ + debug: debugTokenResponse({ + is_valid: true, + app_id: "fb-app", + user_id: "u1", + }), + me: profileResponse(fbProfile("u1")), + }); + const provider = facebook(options); + const res = await provider.getUserInfo({ + accessToken: "opaque-access-token", + } as any); + expect(res?.user.id).toBe("u1"); + expect(res?.user.email).toBe("u1@example.com"); + }); + + it("rejects a token issued to a different app (token substitution)", async () => { + mockGraph({ + debug: debugTokenResponse({ + is_valid: true, + app_id: "someone-elses-app", + user_id: "other-user", + }), + me: profileResponse(fbProfile("other-user")), + }); + const provider = facebook(options); + const res = await provider.getUserInfo({ + accessToken: "foreign-app-token", + } as any); + expect(res).toBeNull(); + }); + + it("rejects when the profile id does not match the validated token", async () => { + mockGraph({ + debug: debugTokenResponse({ + is_valid: true, + app_id: "fb-app", + user_id: "u1", + }), + me: profileResponse(fbProfile("a-different-user")), + }); + const provider = facebook(options); + const res = await provider.getUserInfo({ + accessToken: "opaque-access-token", + } as any); + expect(res).toBeNull(); + }); + + it("rejects when no access token is supplied", async () => { + mockGraph({}); + const provider = facebook(options); + const res = await provider.getUserInfo({} as any); + expect(res).toBeNull(); + expect(mockedBetterFetch).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/social-providers/facebook.ts b/packages/core/src/social-providers/facebook.ts index 9f07163fbc..fd768dac6b 100644 --- a/packages/core/src/social-providers/facebook.ts +++ b/packages/core/src/social-providers/facebook.ts @@ -24,6 +24,58 @@ export interface FacebookProfile { }; } +interface FacebookDebugTokenData { + app_id?: string; + is_valid?: boolean; + user_id?: string; +} + +/** + * Validate an opaque Facebook access token against the configured app. + * + * Facebook access tokens are not audience-bound at the Graph `/me` endpoint: a + * token minted for any Facebook app returns that app's profile. Without this + * check, a token issued to an unrelated app could be presented to this + * app's direct sign-in path and accepted as proof of identity. We call the + * `debug_token` endpoint and require the token to be valid, bound to one of the + * configured client ids, and tied to a user. + * + * @see https://developers.facebook.com/docs/facebook-login/guides/access-tokens/debugging + * + * @returns the inspected token's `user_id` when the token is valid and bound to + * the configured app, otherwise `null`. + */ +async function verifyFacebookAccessToken( + accessToken: string, + options: FacebookOptions, +): Promise { + const primaryClientId = getPrimaryClientId(options.clientId); + if (!primaryClientId || !options.clientSecret) { + return null; + } + const clientIds = Array.isArray(options.clientId) + ? options.clientId + : [options.clientId]; + const appAccessToken = `${primaryClientId}|${options.clientSecret}`; + const { data, error } = await betterFetch<{ data?: FacebookDebugTokenData }>( + "https://graph.facebook.com/debug_token", + { + query: { + input_token: accessToken, + access_token: appAccessToken, + }, + }, + ); + if (error || !data?.data) { + return null; + } + const { is_valid, app_id, user_id } = data.data; + if (is_valid !== true || !app_id || !clientIds.includes(app_id) || !user_id) { + return null; + } + return user_id; +} + export interface FacebookOptions extends ProviderOptions { clientId: string | string[]; /** @@ -117,7 +169,10 @@ export const facebook = (options: FacebookOptions) => { } /* access_token */ - return true; + // An opaque access token carries no app binding of its own, so it + // must be validated against the configured app before it can be + // trusted as proof of identity. + return (await verifyFacebookAccessToken(token, options)) !== null; }, refreshAccessToken: options.refreshAccessToken ? options.refreshAccessToken @@ -178,6 +233,20 @@ export const facebook = (options: FacebookOptions) => { }; } + // The profile is fetched with `accessToken`, which is the credential + // that actually proves identity here — and a separate request field + // from the `idToken`/token validated by `verifyIdToken`. Since an + // opaque token is not app-bound at `/me`, validate this exact token + // against the configured app before trusting the profile it returns. + const accessToken = token.accessToken; + if (!accessToken) { + return null; + } + const tokenUserId = await verifyFacebookAccessToken(accessToken, options); + if (!tokenUserId) { + return null; + } + const fields = [ "id", "name", @@ -190,13 +259,17 @@ export const facebook = (options: FacebookOptions) => { { auth: { type: "Bearer", - token: token.accessToken, + token: accessToken, }, }, ); if (error) { return null; } + // Bind the validated token to the profile it returned. + if (profile.id !== tokenUserId) { + return null; + } const userMap = await options.mapProfileToUser?.(profile); return { user: { diff --git a/packages/core/src/social-providers/google.test.ts b/packages/core/src/social-providers/google.test.ts new file mode 100644 index 0000000000..429ccbbf17 --- /dev/null +++ b/packages/core/src/social-providers/google.test.ts @@ -0,0 +1,189 @@ +import type { JWK } from "jose"; +import { exportJWK, generateKeyPair, SignJWT } from "jose"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@better-fetch/fetch", () => ({ + betterFetch: vi.fn(), +})); + +import { betterFetch } from "@better-fetch/fetch"; + +import { google } from "./google"; + +const mockedBetterFetch = vi.mocked(betterFetch); + +const CLIENT_ID = "google-client-id"; +const CLIENT_SECRET = "google-client-secret"; + +async function createSignedGoogleToken(payload: Record) { + const { publicKey, privateKey } = await generateKeyPair("RS256", { + extractable: true, + }); + const publicJWK = await exportJWK(publicKey); + publicJWK.kid = "test-google-key"; + publicJWK.alg = "RS256"; + publicJWK.use = "sig"; + + const token = await new SignJWT({ + sub: "google-user-123", + email: "user@example.com", + email_verified: true, + name: "Workspace User", + picture: "https://example.com/avatar.png", + ...payload, + }) + .setProtectedHeader({ alg: "RS256", kid: "test-google-key" }) + .setIssuer("https://accounts.google.com") + .setAudience(CLIENT_ID) + .setIssuedAt() + .setExpirationTime("1h") + .sign(privateKey); + + return { publicJWK, token }; +} + +function mockGoogleJwks(publicJWK: JWK) { + mockedBetterFetch.mockResolvedValueOnce({ + data: { keys: [publicJWK] }, + error: null, + } as Awaited>); +} + +// decodeJwt (used by getUserInfo) does not verify the signature, so a plain +// encoded JWT is enough for the profile path. +async function encodeGoogleToken(payload: Record) { + const { token } = await createSignedGoogleToken(payload); + return token; +} + +describe("google hosted domain (hd) enforcement", () => { + beforeEach(() => { + mockedBetterFetch.mockReset(); + }); + + describe("verifyIdToken", () => { + it("accepts a token whose hd claim matches the configured hd", async () => { + const { publicJWK, token } = await createSignedGoogleToken({ + hd: "example.com", + }); + mockGoogleJwks(publicJWK); + + const provider = google({ + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + hd: "example.com", + }); + + await expect(provider.verifyIdToken(token, undefined)).resolves.toBe( + true, + ); + }); + + it("rejects a token whose hd claim does not match", async () => { + const { publicJWK, token } = await createSignedGoogleToken({ + hd: "other.com", + }); + mockGoogleJwks(publicJWK); + + const provider = google({ + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + hd: "example.com", + }); + + await expect(provider.verifyIdToken(token, undefined)).resolves.toBe( + false, + ); + }); + + it("rejects a token missing the hd claim when hd is configured", async () => { + const { publicJWK, token } = await createSignedGoogleToken({}); + mockGoogleJwks(publicJWK); + + const provider = google({ + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + hd: "example.com", + }); + + await expect(provider.verifyIdToken(token, undefined)).resolves.toBe( + false, + ); + }); + + it("does not require an hd claim when hd is not configured", async () => { + const { publicJWK, token } = await createSignedGoogleToken({}); + mockGoogleJwks(publicJWK); + + const provider = google({ + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + }); + + await expect(provider.verifyIdToken(token, undefined)).resolves.toBe( + true, + ); + }); + }); + + describe("getUserInfo", () => { + it("returns the user when the hd claim matches", async () => { + const idToken = await encodeGoogleToken({ hd: "example.com" }); + const provider = google({ + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + hd: "example.com", + }); + + const result = await provider.getUserInfo({ + idToken, + accessToken: "access", + }); + expect(result?.user.email).toBe("user@example.com"); + }); + + it("returns null when the hd claim does not match", async () => { + const idToken = await encodeGoogleToken({ hd: "other.com" }); + const provider = google({ + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + hd: "example.com", + }); + + const result = await provider.getUserInfo({ + idToken, + accessToken: "access", + }); + expect(result).toBeNull(); + }); + + it("returns null when the hd claim is missing and hd is configured", async () => { + const idToken = await encodeGoogleToken({}); + const provider = google({ + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + hd: "example.com", + }); + + const result = await provider.getUserInfo({ + idToken, + accessToken: "access", + }); + expect(result).toBeNull(); + }); + + it("returns the user regardless of hd when hd is not configured", async () => { + const idToken = await encodeGoogleToken({ hd: "anything.com" }); + const provider = google({ + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + }); + + const result = await provider.getUserInfo({ + idToken, + accessToken: "access", + }); + expect(result?.user.email).toBe("user@example.com"); + }); + }); +}); diff --git a/packages/core/src/social-providers/google.ts b/packages/core/src/social-providers/google.ts index b85a5b39ee..5f3cea14ef 100644 --- a/packages/core/src/social-providers/google.ts +++ b/packages/core/src/social-providers/google.ts @@ -48,7 +48,12 @@ export interface GoogleOptions extends ProviderOptions { */ display?: ("page" | "popup" | "touch" | "wap") | undefined; /** - * The hosted domain of the user + * The hosted domain (Google Workspace) the user must belong to. + * + * This is sent to Google as the `hd` authorization hint and, when set, is + * also enforced against the `hd` claim of the returned id token/profile. + * Sign-in is rejected when the claim is missing or does not match, so this + * can be used to restrict sign-in to a Workspace domain. */ hd?: string | undefined; } @@ -147,6 +152,15 @@ export const google = (options: GoogleOptions) => { return false; } + // Google's `hd` authorization parameter is only a UI hint and can + // be removed or changed by the user. When a hosted domain is + // configured, the `hd` claim in the verified id token is the + // authoritative value and must match, otherwise accounts outside + // the workspace domain would be accepted. + if (options.hd && jwtClaims.hd !== options.hd) { + return false; + } + return true; } catch { return false; @@ -160,6 +174,18 @@ export const google = (options: GoogleOptions) => { return null; } const user = decodeJwt(token.idToken) as GoogleProfile; + // Enforce the configured hosted domain on the callback profile path + // as well. The `hd` claim must be present and match, since the + // authorization-time `hd` hint does not restrict which account signs + // in. + if (options.hd && user.hd !== options.hd) { + logger.error( + `Google sign-in rejected: id token hosted domain (hd) "${ + user.hd ?? "" + }" does not match the configured "hd" option "${options.hd}".`, + ); + return null; + } const userMap = await options.mapProfileToUser?.(user); return { user: { diff --git a/packages/core/src/social-providers/paypal.ts b/packages/core/src/social-providers/paypal.ts index 9266b0c262..2e20ca3aef 100644 --- a/packages/core/src/social-providers/paypal.ts +++ b/packages/core/src/social-providers/paypal.ts @@ -1,11 +1,20 @@ import { base64 } from "@better-auth/utils/base64"; import { betterFetch } from "@better-fetch/fetch"; -import { decodeJwt } from "jose"; +import { decodeProtectedHeader, importJWK, jwtVerify } from "jose"; import { logger } from "../env"; -import { BetterAuthError } from "../error"; +import { APIError, BetterAuthError } from "../error"; import type { OAuthProvider, ProviderOptions } from "../oauth2"; import { createAuthorizationURL } from "../oauth2"; +/** + * ID token signing algorithms advertised by PayPal's OpenID configuration. + * Anything outside this allowlist is rejected so each token is only ever + * verified with the algorithm it was issued for. + * + * @see https://www.paypal.com/.well-known/openid-configuration + */ +const PAYPAL_ID_TOKEN_ALGORITHMS = ["RS256", "HS256"] as const; + export interface PayPalProfile { user_id: string; name: string; @@ -75,6 +84,19 @@ export const paypal = (options: PayPalOptions) => { ? "https://api-m.sandbox.paypal.com/v1/identity/oauth2/userinfo" : "https://api-m.paypal.com/v1/identity/oauth2/userinfo"; + /** + * Issuer and JWKS endpoints used to cryptographically verify ID tokens. + * + * @see https://www.paypal.com/.well-known/openid-configuration + */ + const issuer = isSandbox + ? "https://www.sandbox.paypal.com" + : "https://www.paypal.com"; + + const jwksEndpoint = isSandbox + ? "https://api.sandbox.paypal.com/v1/oauth2/certs" + : "https://api.paypal.com/v1/oauth2/certs"; + return { id: "paypal", name: "PayPal", @@ -201,9 +223,48 @@ export const paypal = (options: PayPalOptions) => { if (options.verifyIdToken) { return options.verifyIdToken(token, nonce); } + + // Cryptographically verify the ID token. Decoding alone is not enough: + // the signature, issuer, audience and expiration must all be checked + // before the token's claims can be relied on as proof of identity. + // See https://www.paypal.com/.well-known/openid-configuration + try { - const payload = decodeJwt(token); - return !!payload.sub; + const { kid, alg: jwtAlg } = decodeProtectedHeader(token); + if (!jwtAlg) return false; + if ( + !PAYPAL_ID_TOKEN_ALGORITHMS.includes( + jwtAlg as (typeof PAYPAL_ID_TOKEN_ALGORITHMS)[number], + ) + ) { + return false; + } + + // PayPal can sign ID tokens either asymmetrically (RS256, verified + // against the published JWKS) or symmetrically (HS256, verified with + // the client secret). Selecting the key by algorithm keeps the two + // paths separate so each algorithm is only verified with its + // corresponding key type. + const key = + jwtAlg === "HS256" + ? new TextEncoder().encode(options.clientSecret) + : kid + ? await getPayPalPublicKey(kid, jwksEndpoint) + : undefined; + if (!key) return false; + + const { payload: jwtClaims } = await jwtVerify(token, key, { + algorithms: [jwtAlg], + issuer, + audience: options.clientId, + maxTokenAge: "1h", + }); + + if (nonce && jwtClaims.nonce !== nonce) { + return false; + } + + return true; } catch (error) { logger.error("Failed to verify PayPal ID token:", error); return false; @@ -261,3 +322,29 @@ export const paypal = (options: PayPalOptions) => { options, } satisfies OAuthProvider; }; + +export const getPayPalPublicKey = async (kid: string, jwksUri: string) => { + const { data } = await betterFetch<{ + keys: Array<{ + kid: string; + alg: string; + kty: string; + use: string; + n: string; + e: string; + }>; + }>(jwksUri); + + if (!data?.keys) { + throw new APIError("BAD_REQUEST", { + message: "Keys not found", + }); + } + + const jwk = data.keys.find((key) => key.kid === kid); + if (!jwk) { + throw new Error(`JWK with kid ${kid} not found`); + } + + return await importJWK(jwk, jwk.alg); +}; diff --git a/packages/core/src/social-providers/reddit.ts b/packages/core/src/social-providers/reddit.ts index 9904ae6acb..1df2dced74 100644 --- a/packages/core/src/social-providers/reddit.ts +++ b/packages/core/src/social-providers/reddit.ts @@ -104,15 +104,15 @@ export const reddit = (options: RedditOptions) => { } const userMap = await options.mapProfileToUser?.(profile); - + const email = userMap?.email || `${profile.id}@reddit.com`; return { user: { id: profile.id, name: profile.name, - email: profile.oauth_client_id, - emailVerified: profile.has_verified_email, image: profile.icon_img?.split("?")[0]!, ...userMap, + email, + emailVerified: userMap?.emailVerified ?? false, }, data: profile, }; diff --git a/packages/electron/src/error-codes.ts b/packages/electron/src/error-codes.ts index 07d8d40be6..8207ff73d0 100644 --- a/packages/electron/src/error-codes.ts +++ b/packages/electron/src/error-codes.ts @@ -8,4 +8,5 @@ export const ELECTRON_ERROR_CODES = defineErrorCodes({ INVALID_CODE_VERIFIER: "Invalid code verifier", MISSING_STATE: "state is required", MISSING_PKCE: "pkce is required", + INVALID_PKCE_METHOD: "PKCE method must be S256", }); diff --git a/packages/electron/src/index.ts b/packages/electron/src/index.ts index 7f588ac781..d947dfdfb4 100644 --- a/packages/electron/src/index.ts +++ b/packages/electron/src/index.ts @@ -61,12 +61,7 @@ export const electron = (options?: ElectronOptions | undefined) => { code_challenge_method?: string | undefined; }, ) => { - const { - client_id, - state, - code_challenge, - code_challenge_method = "plain", - } = payload; + const { client_id, state, code_challenge, code_challenge_method } = payload; const userId = ctx.context.session?.user.id || ctx.context.newSession?.user.id; if (!userId || client_id !== opts.clientID) { @@ -78,6 +73,16 @@ export const electron = (options?: ElectronOptions | undefined) => { if (!code_challenge) { throw APIError.from("BAD_REQUEST", ELECTRON_ERROR_CODES.MISSING_PKCE); } + // Only S256 is accepted. With `plain` (or a missing/unknown method) the + // verifier equals the challenge that already travels in the sign-in URL + // query, so it must be rejected rather than silently downgraded to a + // plain verifier comparison at exchange time. + if (code_challenge_method?.toLowerCase() !== "s256") { + throw APIError.from( + "BAD_REQUEST", + ELECTRON_ERROR_CODES.INVALID_PKCE_METHOD, + ); + } const redirectCookieName = `${opts.cookiePrefix}.${opts.clientID}`; @@ -89,7 +94,7 @@ export const electron = (options?: ElectronOptions | undefined) => { value: JSON.stringify({ userId, codeChallenge: code_challenge, - codeChallengeMethod: code_challenge_method.toLowerCase(), + codeChallengeMethod: "s256", state, }), expiresAt, @@ -160,7 +165,7 @@ export const electron = (options?: ElectronOptions | undefined) => { const querySchema = z.object({ client_id: z.string(), code_challenge: z.string().nonempty(), - code_challenge_method: z.string().optional().default("plain"), + code_challenge_method: z.string().optional(), state: z.string().nonempty(), }); const cookie = ctx.context.createAuthCookie("transfer_token", { diff --git a/packages/electron/src/routes.ts b/packages/electron/src/routes.ts index e65be8d7f6..a5bdfefab1 100644 --- a/packages/electron/src/routes.ts +++ b/packages/electron/src/routes.ts @@ -83,30 +83,30 @@ export const electronToken = (_opts: ElectronOptions) => ELECTRON_ERROR_CODES.MISSING_CODE_CHALLENGE, ); } - if (tokenRecord.codeChallengeMethod === "s256") { - const codeChallenge = Buffer.from( - base64Url.decode(tokenRecord.codeChallenge), - ); - const codeVerifier = Buffer.from( - await createHash("SHA-256").digest(ctx.body.code_verifier), + // Only S256 is accepted. The legacy `plain` comparison is rejected: + // in plain mode the verifier equals the challenge, which travels in + // the sign-in URL, so the comparison adds nothing for this flow. + if (tokenRecord.codeChallengeMethod !== "s256") { + throw APIError.from( + "BAD_REQUEST", + ELECTRON_ERROR_CODES.INVALID_PKCE_METHOD, ); + } + const codeChallenge = Buffer.from( + base64Url.decode(tokenRecord.codeChallenge), + ); + const codeVerifier = Buffer.from( + await createHash("SHA-256").digest(ctx.body.code_verifier), + ); - if ( - codeChallenge.length !== codeVerifier.length || - !timingSafeEqual(codeChallenge, codeVerifier) - ) { - throw APIError.from( - "BAD_REQUEST", - ELECTRON_ERROR_CODES.INVALID_CODE_VERIFIER, - ); - } - } else { - if (tokenRecord.codeChallenge !== ctx.body.code_verifier) { - throw APIError.from( - "BAD_REQUEST", - ELECTRON_ERROR_CODES.INVALID_CODE_VERIFIER, - ); - } + if ( + codeChallenge.length !== codeVerifier.length || + !timingSafeEqual(codeChallenge, codeVerifier) + ) { + throw APIError.from( + "BAD_REQUEST", + ELECTRON_ERROR_CODES.INVALID_CODE_VERIFIER, + ); } await ctx.context.internalAdapter.deleteVerificationByIdentifier( `electron:${ctx.body.token}`, @@ -202,16 +202,25 @@ export const electronInitOAuthProxy = (opts: ElectronOptions) => throw APIError.from("BAD_REQUEST", BASE_ERROR_CODES.PROVIDER_NOT_FOUND); } + // Electron transfers require S256 PKCE; reject any other method + // rather than forwarding a downgraded `plain` challenge. + if ( + ctx.query.code_challenge_method && + ctx.query.code_challenge_method.toLowerCase() !== "s256" + ) { + throw APIError.from( + "BAD_REQUEST", + ELECTRON_ERROR_CODES.INVALID_PKCE_METHOD, + ); + } + const headers = new Headers(ctx.request?.headers); headers.set("origin", new URL(ctx.context.baseURL).origin); let setCookie: string | null = null; const searchParams = new URLSearchParams(); searchParams.set("client_id", opts.clientID || "electron"); searchParams.set("code_challenge", ctx.query.code_challenge); - searchParams.set( - "code_challenge_method", - ctx.query.code_challenge_method || "plain", - ); + searchParams.set("code_challenge_method", "S256"); searchParams.set("state", ctx.query.state); const res = await betterFetch<{ url: string | undefined; diff --git a/packages/electron/test/electron.test.ts b/packages/electron/test/electron.test.ts index abe615452c..a7dd5fda39 100644 --- a/packages/electron/test/electron.test.ts +++ b/packages/electron/test/electron.test.ts @@ -16,6 +16,13 @@ import { electron } from "../src/index"; import { fetchUserImage, normalizeUserOutput } from "../src/user"; import { encodeRedirectToken, it, testUtils } from "./utils"; +// Electron transfers require S256 PKCE. These provide a consistent +// verifier/challenge pair for token-exchange tests. +const TEST_PKCE_VERIFIER = "test-challenge"; +const TEST_PKCE_CHALLENGE = base64Url.encode( + await createHash("SHA-256").digest(TEST_PKCE_VERIFIER), +); + const mockElectron = vi.hoisted(() => { const BrowserWindow = { constructor: vi.fn(), @@ -123,7 +130,7 @@ describe("Electron", () => { query: { client_id: "electron", code_challenge: "test-challenge", - code_challenge_method: "plain", + code_challenge_method: "S256", state: "abc", }, onResponse: async (ctx) => { @@ -160,7 +167,7 @@ describe("Electron", () => { query: { client_id: "electron", code_challenge: "test-challenge", - code_challenge_method: "plain", + code_challenge_method: "S256", state: "abc", }, }, @@ -236,8 +243,13 @@ describe("Electron", () => { }, }); + const codeVerifier = base64Url.encode(randomBytes(32)); + const codeChallenge = base64Url.encode( + await createHash("SHA-256").digest(codeVerifier), + ); + (globalThis as any)[kElectron] = new Map([ - ["abc", "test-challenge"], + ["abc", codeVerifier], ]); const identifier = generateRandomString(16, "A-Z", "a-z", "0-9"); @@ -247,8 +259,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "test-challenge", - codeChallengeMethod: "plain", + codeChallenge, + codeChallengeMethod: "s256", state: "abc", }), expiresAt: new Date(Date.now() + 300 * 1000), @@ -319,8 +331,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "test-challenge", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", state: "abc", }), expiresAt: new Date(Date.now() + 999), @@ -397,8 +409,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: "non-existent-user", - codeChallenge: "x", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", state: "abc", }), expiresAt: new Date(Date.now() + 300_000), @@ -409,7 +421,11 @@ describe("Electron", () => { client .$fetch("/electron/token", { method: "POST", - body: { token: identifier, code_verifier: "x", state: "abc" }, + body: { + token: identifier, + code_verifier: TEST_PKCE_VERIFIER, + state: "abc", + }, throw: true, customFetchImpl: (url, init) => { const req = new Request(url.toString(), init); @@ -446,8 +462,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "x", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", state: "abc", }), expiresAt: new Date(Date.now() + 300_000), @@ -461,7 +477,11 @@ describe("Electron", () => { await expect( client.$fetch("/electron/token", { method: "POST", - body: { token: identifier, code_verifier: "x", state: "abc" }, + body: { + token: identifier, + code_verifier: TEST_PKCE_VERIFIER, + state: "abc", + }, throw: true, customFetchImpl: (url, init) => { const req = new Request(url.toString(), init); @@ -534,8 +554,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "test-challenge", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", state: "abc", }), expiresAt: new Date(Date.now() + 300 * 1000), @@ -588,8 +608,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "test-challenge", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", state: "abc", }), expiresAt: new Date(Date.now() + 300 * 1000), @@ -658,7 +678,7 @@ describe("Electron", () => { describe("transferUser", () => { const transferQuery = - "client_id=electron&state=xyz&code_challenge=challenge"; + "client_id=electron&state=xyz&code_challenge=challenge&code_challenge_method=S256"; const post = (cookie: string, body?: object) => auth.handler( new Request( @@ -761,6 +781,40 @@ describe("Electron", () => { const cookies = parseSetCookieHeader(setCookie); expect(cookies.has("better-auth.electron")).toBe(true); }); + + it("should reject a transfer with a non-S256 PKCE method", async () => { + const cookie = await getSessionCookie(); + const res = await auth.handler( + new Request( + "http://localhost:3000/api/auth/electron/transfer-user?client_id=electron&state=xyz&code_challenge=plain-text-challenge&code_challenge_method=plain", + { + method: "POST", + headers: { cookie, "content-type": "application/json" }, + body: JSON.stringify({}), + }, + ), + ); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.code).toBe(ELECTRON_ERROR_CODES.INVALID_PKCE_METHOD.code); + }); + + it("should reject a transfer with a missing PKCE method", async () => { + const cookie = await getSessionCookie(); + const res = await auth.handler( + new Request( + "http://localhost:3000/api/auth/electron/transfer-user?client_id=electron&state=xyz&code_challenge=plain-text-challenge", + { + method: "POST", + headers: { cookie, "content-type": "application/json" }, + body: JSON.stringify({}), + }, + ), + ); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.code).toBe(ELECTRON_ERROR_CODES.INVALID_PKCE_METHOD.code); + }); }); it("should register protocol", async ({ setProcessType }) => { @@ -876,8 +930,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "test-challenge", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", }), expiresAt: new Date(Date.now() + 300 * 1000), }, @@ -918,8 +972,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "test-challenge", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", state: "def", }), expiresAt: new Date(Date.now() + 300 * 1000), @@ -1022,6 +1076,52 @@ describe("Electron", () => { }), ).rejects.toThrowError("BAD_REQUEST"); }); + + // A `plain` PKCE method adds nothing: the verifier equals the challenge, + // which travels in the sign-in URL, so whoever chose the challenge + // already knows the verifier. The exchange must reject it. + it("should reject token exchange when the stored PKCE method is not S256", async ({ + setProcessType, + }) => { + setProcessType("browser"); + + const { user } = await auth.api.signInEmail({ + body: { email: "test@test.com", password: "password" }, + }); + + const plainChallenge = "client-known-challenge"; + const identifier = generateRandomString(16, "A-Z", "a-z", "0-9"); + await (await auth.$context).adapter.create({ + model: "verification", + data: { + identifier: `electron:${identifier}`, + value: JSON.stringify({ + userId: user.id, + codeChallenge: plainChallenge, + codeChallengeMethod: "plain", + state: "abc", + }), + expiresAt: new Date(Date.now() + 300_000), + }, + }); + + await expect( + client.$fetch("/electron/token", { + method: "POST", + // In plain mode this verifier would have matched the challenge. + body: { + token: identifier, + code_verifier: plainChallenge, + state: "abc", + }, + throw: true, + customFetchImpl: (url, init) => { + const req = new Request(url.toString(), init); + return auth.handler(req); + }, + }), + ).rejects.toThrowError("BAD_REQUEST"); + }); }); describe("cookies", () => { @@ -1039,8 +1139,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "test-challenge", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", state: "abc", }), expiresAt: new Date(Date.now() + 300 * 1000), @@ -1647,8 +1747,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "test-challenge", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", state: "abc", }), expiresAt: new Date(Date.now() + 300 * 1000), @@ -1705,8 +1805,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "test-challenge", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", state: "abc", }), expiresAt: new Date(Date.now() + 300 * 1000), @@ -1889,8 +1989,8 @@ describe("Electron", () => { identifier: `electron:${identifier}`, value: JSON.stringify({ userId: user.id, - codeChallenge: "test-challenge", - codeChallengeMethod: "plain", + codeChallenge: TEST_PKCE_CHALLENGE, + codeChallengeMethod: "s256", state: "abc", }), expiresAt: new Date(Date.now() + 300 * 1000), diff --git a/packages/oauth-provider/src/authorize.ts b/packages/oauth-provider/src/authorize.ts index ae9b9e0e89..87c78dab52 100644 --- a/packages/oauth-provider/src/authorize.ts +++ b/packages/oauth-provider/src/authorize.ts @@ -15,6 +15,7 @@ import type { } from "./types"; import { + clientAllowsGrant, getClient, getJwtPlugin, isPKCERequired, @@ -262,6 +263,18 @@ export async function authorizeEndpoint( getErrorURL(ctx, "client_disabled", "client is disabled"), ); } + // The authorize endpoint only serves the authorization_code grant; reject + // clients that are not registered for it. + if (!clientAllowsGrant(client, "authorization_code")) { + return handleRedirect( + ctx, + getErrorURL( + ctx, + "unauthorized_client", + "client is not authorized to use the authorization_code grant", + ), + ); + } const redirectUri = client.redirectUris?.find((url) => { if (url === query.redirect_uri) return true; diff --git a/packages/oauth-provider/src/client-resource.ts b/packages/oauth-provider/src/client-resource.ts index f0e6de40c1..8220de2346 100644 --- a/packages/oauth-provider/src/client-resource.ts +++ b/packages/oauth-provider/src/client-resource.ts @@ -230,6 +230,20 @@ export interface VerifyAccessTokenRemote { * is also still active. */ force?: boolean; + /** + * Accept introspection responses that omit the `aud` claim even when a + * required `audience` is configured in `verifyOptions`. + * + * By default verification fails closed: if you configure an `audience` and + * the introspection response has no `aud` (or a mismatching one), the token + * is rejected. Some authorization servers legitimately omit `aud` from + * introspection responses (it is OPTIONAL per RFC 7662 §2.2); only enable + * this if you trust the issuer to bind the token to this resource through + * another mechanism, as it skips the audience check in that case. + * + * @default false + */ + allowMissingAudience?: boolean; } type VerifyAccessTokenOutput = T extends undefined diff --git a/packages/oauth-provider/src/continue.ts b/packages/oauth-provider/src/continue.ts index 46ba0afad7..b4851da5d2 100644 --- a/packages/oauth-provider/src/continue.ts +++ b/packages/oauth-provider/src/continue.ts @@ -1,5 +1,5 @@ import type { GenericEndpointContext } from "@better-auth/core"; -import { APIError } from "better-auth/api"; +import { APIError, getSessionFromCtx } from "better-auth/api"; import type { AuthorizeEndpointSettings } from "./authorize"; import { oAuthState } from "./oauth"; import { removePromptFromQuery, searchParamsToQuery } from "./utils"; @@ -73,7 +73,8 @@ async function postLogin( ctx: GenericEndpointContext, authorize: AuthorizeEndpointCaller, ) { - const _query = (await oAuthState.get())?.query as string | undefined; + const state = await oAuthState.get(); + const _query = state?.query as string | undefined; if (!_query) { throw new APIError("BAD_REQUEST", { error_description: "missing oauth query", @@ -83,7 +84,18 @@ async function postLogin( const query = new URLSearchParams(_query); ctx.headers?.set("accept", "application/json"); ctx.query = searchParamsToQuery(query); + // The client-submitted `postLogin: true` only selects this continuation + // branch — it is NOT proof that the post-login gate (e.g. org/team + // selection) actually completed. Trust only a server-issued, session-bound + // marker carried by the signed oauth_query (mirrors the consent endpoint). + // When that marker is absent (it is only minted at the consent redirect), + // `authorize` re-runs `opts.postLogin.shouldRedirect` against the live + // session and redirects back to the gate if selection is still required. + const session = await getSessionFromCtx(ctx); + const postLoginCleared = + state?.postLoginClearedForSession !== undefined && + state.postLoginClearedForSession === session?.session.id; return await authorize(ctx, { - postLogin: true, + postLogin: postLoginCleared, }); } diff --git a/packages/oauth-provider/src/introspect.test.ts b/packages/oauth-provider/src/introspect.test.ts index 325c712826..af80cd42d3 100644 --- a/packages/oauth-provider/src/introspect.test.ts +++ b/packages/oauth-provider/src/introspect.test.ts @@ -551,6 +551,11 @@ describe("oauth introspect - config", async () => { const registeredClient = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -715,3 +720,141 @@ describe("oauth introspect - config", async () => { }); }); }); + +describe("oauth introspect - rejects non-OAuth same-issuer JWTs", async () => { + const authServerBaseUrl = "http://localhost:3000"; + const rpBaseUrl = "http://localhost:5000"; + const providerId = "test"; + const redirectUri = `${rpBaseUrl}/api/auth/oauth2/callback/${providerId}`; + + // The JWT plugin shares issuer, audience, and signing keys with the OAuth + // provider. We deliberately make the auth-server origin a valid OAuth + // audience so a plain session JWT satisfies the signature/issuer/audience + // checks — isolating the `azp` gate as the only thing that should reject it. + const { auth, signInWithTestUser, customFetchImpl } = await getTestInstance({ + baseURL: authServerBaseUrl, + plugins: [ + jwt({ + jwt: { + issuer: authServerBaseUrl, + audience: authServerBaseUrl, + }, + }), + oauthProvider({ + loginPage: "/login", + consentPage: "/consent", + validAudiences: [authServerBaseUrl], + silenceWarnings: { + oauthAuthServerConfig: true, + openidConfig: true, + }, + }), + ], + }); + + const { headers } = await signInWithTestUser(); + const client = createAuthClient({ + plugins: [oauthProviderClient()], + baseURL: authServerBaseUrl, + fetchOptions: { + customFetchImpl, + headers, + }, + }); + + let oauthClient: OAuthClient | null = null; + + beforeAll(async () => { + oauthClient = await auth.api.adminCreateOAuthClient({ + headers, + body: { + redirect_uris: [redirectUri], + scope: "openid profile email offline_access", + skip_consent: true, + }, + }); + expect(oauthClient?.client_id).toBeDefined(); + }); + + async function getOAuthJwtAccessToken() { + const codeVerifier = generateRandomString(32); + const authUrl = await createAuthorizationURL({ + id: providerId, + options: { + clientId: oauthClient!.client_id!, + clientSecret: oauthClient!.client_secret!, + redirectURI: redirectUri, + }, + redirectURI: "", + authorizationEndpoint: `${authServerBaseUrl}/api/auth/oauth2/authorize`, + state: "123", + scopes: ["openid", "profile", "email", "offline_access"], + codeVerifier, + }); + let callbackRedirectUrl = ""; + await client.$fetch(authUrl.toString(), { + headers, + onError(context) { + callbackRedirectUrl = context.response.headers.get("Location") || ""; + }, + }); + const code = new URL(callbackRedirectUrl).searchParams.get("code")!; + const { body, headers: tokenHeaders } = createAuthorizationCodeRequest({ + code, + codeVerifier, + redirectURI: redirectUri, + // resource makes this a JWT access token with aud = authServerBaseUrl + resource: authServerBaseUrl, + options: { + clientId: oauthClient!.client_id!, + clientSecret: oauthClient!.client_secret!, + redirectURI: redirectUri, + }, + }); + const tokens = await client.$fetch<{ access_token?: string }>( + "/oauth2/token", + { method: "POST", body, headers: tokenHeaders }, + ); + return tokens.data?.access_token!; + } + + function introspect(token: string) { + return client.oauth2.introspect( + { + client_id: oauthClient?.client_id, + client_secret: oauthClient?.client_secret, + token, + token_type_hint: "access_token", + }, + { + headers: { + accept: "application/json", + "content-type": "application/x-www-form-urlencoded", + }, + }, + ); + } + + // Positive control: a real OAuth JWT access token (with azp + matching aud) + // is active in this exact config, proving the iss/aud checks pass here — so + // the session-token rejection below can only be due to the missing azp. + it("treats a real OAuth JWT access token as active", async () => { + const accessToken = await getOAuthJwtAccessToken(); + const introspection = await introspect(accessToken); + expect(introspection.data?.active).toBe(true); + expect(introspection.data?.client_id).toBe(oauthClient?.client_id); + }); + + /** + * A JWT-plugin session token shares the issuer, audience, and signing keys + * but has no `azp`/client binding and was never issued through the OAuth + * token endpoint. It must not introspect as an active access token. + */ + it("rejects a JWT plugin session token presented as an access token", async () => { + const { token: sessionJwt } = await auth.api.getToken({ headers }); + // Sanity: it is a real, signed JWS (three segments). + expect(sessionJwt.split(".")).toHaveLength(3); + const introspection = await introspect(sessionJwt); + expect(introspection.data?.active).toBe(false); + }); +}); diff --git a/packages/oauth-provider/src/introspect.ts b/packages/oauth-provider/src/introspect.ts index abde79a13a..27b1126906 100644 --- a/packages/oauth-provider/src/introspect.ts +++ b/packages/oauth-provider/src/introspect.ts @@ -87,19 +87,29 @@ async function validateJwtAccessToken( throw new Error(error as unknown as string); } - let client: SchemaClient | null | undefined; - if (jwtPayload.azp) { - client = await getClient(ctx, opts, jwtPayload.azp); - if (!client || client?.disabled) { - return { - active: false, - }; - } - if (clientId && jwtPayload.azp !== clientId) { - return { - active: false, - }; - } + // An OAuth access token issued by this provider always carries an `azp` + // (authorized party = client) claim, stamped by `createJwtAccessToken`. The + // JWT plugin shares the same issuer, audience, and signing keys, so a plain + // session JWT (e.g. from its `/token` endpoint) can otherwise satisfy the + // signature/issuer/audience checks above. Such a token was never issued + // through the OAuth token endpoint and has no client or consent binding, so + // it must not be reported as an active access token. Require `azp` and a + // matching, enabled client before considering the token active. + if (!jwtPayload.azp) { + return { + active: false, + }; + } + const client = await getClient(ctx, opts, jwtPayload.azp); + if (!client || client?.disabled) { + return { + active: false, + }; + } + if (clientId && jwtPayload.azp !== clientId) { + return { + active: false, + }; } // Validate JWT against its session if it exists @@ -121,9 +131,7 @@ async function validateJwtAccessToken( // Return the JWT payload in introspection format // https://datatracker.ietf.org/doc/html/rfc7662#section-2.2 - if (jwtPayload.azp) { - jwtPayload.client_id = jwtPayload.azp; - } + jwtPayload.client_id = jwtPayload.azp; jwtPayload.active = true; return jwtPayload; } diff --git a/packages/oauth-provider/src/oauth.test.ts b/packages/oauth-provider/src/oauth.test.ts index 902419e5bc..653358e307 100644 --- a/packages/oauth-provider/src/oauth.test.ts +++ b/packages/oauth-provider/src/oauth.test.ts @@ -281,6 +281,11 @@ describe("oauth", async () => { const response = await authorizationServer.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -728,6 +733,11 @@ describe("oauth", async () => { const tempClient = await authorizationServer.api.adminCreateOAuthClient({ headers: adminHeaders, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -819,6 +829,11 @@ describe("oauth", async () => { const tempClient = await authorizationServer.api.adminCreateOAuthClient({ headers: adminHeaders, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -1133,6 +1148,11 @@ describe("oauth - prompt", async () => { const response = await authorizationServer.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], }, }); @@ -1367,6 +1387,11 @@ describe("oauth - prompt", async () => { const tempClient = await authorizationServer.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], }, }); @@ -2370,6 +2395,82 @@ describe("oauth - prompt", async () => { enablePostLogin = false; }); + it("shall not let a client self-attest post login completion via continue", async ({ + onTestFinished, + }) => { + if (!oauthClient?.client_id || !oauthClient?.client_secret) { + throw Error("beforeAll not run properly"); + } + enablePostLogin = true; + // Return a default reference instead of throwing, so the only thing that + // can stop token issuance is the post-login gate itself — isolating the + // self-attestation behavior under test. + bypassReferenceIdCheck = true; + const { customFetchImpl: customFetchImplRP, cookieSetter } = + await createTestInstance(); + const client = createAuthClient({ + plugins: [genericOAuthClient(), organization()], + baseURL: rpBaseUrl, + fetchOptions: { + customFetchImpl: customFetchImplRP, + }, + }); + + const oauthHeaders = new Headers(); + const data = await client.signIn.oauth2( + { + providerId, + callbackURL: "/success", + }, + { + headers, + throw: true, + onSuccess: cookieSetter(oauthHeaders), + }, + ); + + // Authorize redirects to the post-login gate (no org selected yet). + let selectOrgRedirectUri = ""; + await serverClient.$fetch(data.url, { + method: "GET", + headers, + onError(context) { + selectOrgRedirectUri = context.response.headers.get("Location") || ""; + cookieSetter(headers)(context); + }, + }); + expect(selectOrgRedirectUri).toContain(`/select-organization`); + vi.stubGlobal("window", { + location: { + search: new URL(selectOrgRedirectUri, authServerBaseUrl).search, + }, + }); + onTestFinished(() => { + vi.unstubAllGlobals(); + bypassReferenceIdCheck = false; + selectedPostLogin = false; + enablePostLogin = false; + }); + + // Skip the actual selection: resubmit the signed oauth_query and claim + // the gate completed by posting `postLogin: true`. The server must re-run + // the gate against the live (still-unselected) session and redirect back + // rather than mint an authorization code. + selectedPostLogin = false; + const continueRes = await serverClient.oauth2.continue( + { + postLogin: true, + }, + { + headers, + throw: true, + onResponse: cookieSetter(headers), + }, + ); + expect(continueRes.url).toContain(`/select-organization`); + expect(continueRes.url).not.toContain(`code=`); + }); + it("shall allow user to select an organization/team post login and consent", async ({ onTestFinished, }) => { @@ -2861,6 +2962,11 @@ describe("oauth - config", () => { const createdClient = await authorizationServer.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -2940,6 +3046,11 @@ describe("oauth - config", () => { const createdClient = await authorizationServer.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -3042,6 +3153,11 @@ describe("oauth - config", () => { const createdClient = await authorizationServer.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -3153,6 +3269,11 @@ describe("oauth - config", () => { const createdClient = await authorizationServer.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], token_endpoint_auth_method: publicClient ? "none" : undefined, skip_consent: true, @@ -3258,6 +3379,10 @@ describe("oauth - config", () => { introspectUrl: `${authServerUrl}/oauth2/introspect`, clientId: createdClient?.client_id!, clientSecret: createdClient?.client_secret!, + // Tokens minted without a resource (or with the JWT plugin + // disabled) carry no `aud`, which the configured `audience` + // would otherwise reject by default. + allowMissingAudience: !(resource && !disableJwtPlugin), }, }, ); @@ -3478,6 +3603,11 @@ describe("oauth - rate limiting", () => { const client = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: ["http://localhost:5000/callback"], }, }); @@ -3538,6 +3668,11 @@ describe("oauth - rate limiting", () => { const client = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: ["http://localhost:5000/callback"], }, }); diff --git a/packages/oauth-provider/src/revoke.test.ts b/packages/oauth-provider/src/revoke.test.ts index e619c709c9..629892ab31 100644 --- a/packages/oauth-provider/src/revoke.test.ts +++ b/packages/oauth-provider/src/revoke.test.ts @@ -386,6 +386,11 @@ describe("oauth revoke - config", async () => { const registeredClient = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, diff --git a/packages/oauth-provider/src/token.test.ts b/packages/oauth-provider/src/token.test.ts index 16f729cb69..c41b1357a0 100644 --- a/packages/oauth-provider/src/token.test.ts +++ b/packages/oauth-provider/src/token.test.ts @@ -67,6 +67,11 @@ describe("oauth token - authorization_code", async () => { const response = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -488,6 +493,11 @@ describe("oauth token - refresh_token", async () => { const response = await authorizationServer.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -1326,6 +1336,11 @@ describe("oauth token - client_credentials", async () => { const response = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, scope: clientScopes.join(" "), @@ -1512,6 +1527,11 @@ describe("oauth token - customIdTokenClaims precedence", async () => { const response = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -1665,6 +1685,11 @@ describe("oauth token - config", async () => { const registeredClient = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -2045,6 +2070,11 @@ describe("oauth token - client secret validation", async () => { const oauthClient = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -2188,6 +2218,11 @@ describe("id token claim override security", async () => { const response = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -2614,6 +2649,11 @@ describe("customTokenResponseFields", async () => { const response = await auth.api.adminCreateOAuthClient({ headers, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -2712,6 +2752,11 @@ describe("customTokenResponseFields", async () => { const response = await auth2.api.adminCreateOAuthClient({ headers: headers2, body: { + grant_types: [ + "authorization_code", + "client_credentials", + "refresh_token", + ], redirect_uris: [redirectUri], skip_consent: true, }, @@ -2881,3 +2926,174 @@ describe("verificationValueSchema", () => { } }); }); + +describe("oauth token - per-client grant_type enforcement", async () => { + const authServerBaseUrl = "http://localhost:3000"; + const rpBaseUrl = "http://localhost:5000"; + const { auth, signInWithTestUser, customFetchImpl } = await getTestInstance({ + baseURL: authServerBaseUrl, + plugins: [ + jwt({ jwt: { issuer: authServerBaseUrl } }), + oauthProvider({ + loginPage: "/login", + consentPage: "/consent", + silenceWarnings: { + oauthAuthServerConfig: true, + openidConfig: true, + }, + }), + ], + }); + const { headers } = await signInWithTestUser(); + const client = createAuthClient({ + plugins: [oauthProviderClient(), jwtClient()], + baseURL: authServerBaseUrl, + fetchOptions: { customFetchImpl, headers }, + }); + + const providerId = "test"; + const redirectUri = `${rpBaseUrl}/api/auth/oauth2/callback/${providerId}`; + + // A confidential client registered for the authorization_code grant only. + let authCodeOnlyClient: OAuthClient | null = null; + beforeAll(async () => { + authCodeOnlyClient = await auth.api.adminCreateOAuthClient({ + headers, + body: { + grant_types: ["authorization_code"], + redirect_uris: [redirectUri], + skip_consent: true, + }, + }); + expect(authCodeOnlyClient?.client_secret).toBeDefined(); + }); + + // The core fix: a user-delegated (authorization_code) client must not be + // able to mint machine-to-machine tokens. + it("rejects client_credentials for an authorization_code-only client", async () => { + const { body, headers: reqHeaders } = createClientCredentialsTokenRequest({ + options: { + clientId: authCodeOnlyClient!.client_id!, + clientSecret: authCodeOnlyClient!.client_secret!, + redirectURI: redirectUri, + }, + }); + const res = await client.$fetch<{ access_token?: string }>( + "/oauth2/token", + { method: "POST", body, headers: reqHeaders }, + ); + expect(res.data?.access_token).toBeUndefined(); + expect(res.error?.status).toBe(400); + expect((res.error as { error?: string } | null)?.error).toBe( + "unauthorized_client", + ); + }); + + // Targeted policy: refresh tokens stay tied to the authorization_code flow + + // offline_access, so an authorization_code client keeps getting them without + // having to also register the refresh_token grant explicitly. + it("still issues refresh tokens to an authorization_code client with offline_access", async () => { + const codeVerifier = generateRandomString(32); + const authUrl = await createAuthorizationURL({ + id: providerId, + options: { + clientId: authCodeOnlyClient!.client_id!, + clientSecret: authCodeOnlyClient!.client_secret!, + redirectURI: redirectUri, + }, + redirectURI: "", + authorizationEndpoint: `${authServerBaseUrl}/api/auth/oauth2/authorize`, + state: "grant-enforcement", + scopes: ["openid", "offline_access"], + codeVerifier, + }); + + let callbackRedirectUrl = ""; + await client.$fetch(authUrl.toString(), { + onError(context) { + callbackRedirectUrl = context.response.headers.get("Location") || ""; + }, + }); + const code = new URL(callbackRedirectUrl).searchParams.get("code")!; + + const { body, headers: reqHeaders } = createAuthorizationCodeRequest({ + code, + codeVerifier, + redirectURI: redirectUri, + options: { + clientId: authCodeOnlyClient!.client_id!, + clientSecret: authCodeOnlyClient!.client_secret!, + redirectURI: redirectUri, + }, + }); + const tokens = await client.$fetch<{ + access_token?: string; + refresh_token?: string; + }>("/oauth2/token", { method: "POST", body, headers: reqHeaders }); + + expect(tokens.error).toBeNull(); + expect(tokens.data?.access_token).toBeDefined(); + expect(tokens.data?.refresh_token).toBeDefined(); + }); + + // Guard against over-blocking: a client explicitly registered for + // client_credentials must still be able to use it. + it("allows client_credentials for a client registered for it", async () => { + const ccClient = await auth.api.adminCreateOAuthClient({ + headers, + body: { + grant_types: ["client_credentials"], + redirect_uris: [redirectUri], + skip_consent: true, + }, + }); + const { body, headers: reqHeaders } = createClientCredentialsTokenRequest({ + options: { + clientId: ccClient!.client_id!, + clientSecret: ccClient!.client_secret!, + redirectURI: redirectUri, + }, + }); + const res = await client.$fetch<{ access_token?: string }>( + "/oauth2/token", + { method: "POST", body, headers: reqHeaders }, + ); + expect(res.error).toBeNull(); + expect(res.data?.access_token).toBeDefined(); + }); + + // The /authorize endpoint only serves authorization_code; a machine-to-machine + // (client_credentials-only) client must be rejected there too. + it("rejects /authorize for a client not registered for authorization_code", async () => { + const ccClient = await auth.api.adminCreateOAuthClient({ + headers, + body: { + grant_types: ["client_credentials"], + redirect_uris: [redirectUri], + skip_consent: true, + }, + }); + const codeVerifier = generateRandomString(32); + const authUrl = await createAuthorizationURL({ + id: providerId, + options: { + clientId: ccClient!.client_id!, + clientSecret: ccClient!.client_secret!, + redirectURI: redirectUri, + }, + redirectURI: "", + authorizationEndpoint: `${authServerBaseUrl}/api/auth/oauth2/authorize`, + state: "authorize-grant-enforcement", + scopes: ["openid"], + codeVerifier, + }); + + let location = ""; + await client.$fetch(authUrl.toString(), { + onError(context) { + location = context.response.headers.get("Location") || ""; + }, + }); + expect(location).toContain("error=unauthorized_client"); + }); +}); diff --git a/packages/oauth-provider/src/token.ts b/packages/oauth-provider/src/token.ts index 2862d49fc7..4390af97df 100644 --- a/packages/oauth-provider/src/token.ts +++ b/packages/oauth-provider/src/token.ts @@ -18,6 +18,7 @@ import { verificationValueSchema } from "./types/zod"; import { userNormalClaims } from "./userinfo"; import { basicToClientCredentials, + clientAllowsGrant, decryptStoredClientSecret, getJwtPlugin, getStoredToken, @@ -506,8 +507,12 @@ async function createUserTokens( // Check requested audience if sent as the resource parameter const audience = await checkResource(ctx, opts, scopes); + // Only mint a refresh token when the client may use refresh tokens. + // Otherwise an `offline_access` scope alone would hand a refresh token to a + // pure machine-to-machine client that was never authorized for one. const isRefreshToken = user && + clientAllowsGrant(client, "refresh_token") && (existingRefreshToken?.scopes?.includes("offline_access") || scopes.includes("offline_access")); const isJwtAccessToken = audience && !opts.disableJwtPlugin; @@ -770,6 +775,7 @@ async function handleAuthorizationCodeGrant( client_id, client_secret, scopes, + "authorization_code", ); // Parse scopes from the authorization request @@ -937,6 +943,8 @@ async function handleClientCredentialsGrant( opts, client_id, client_secret, + undefined, + "client_credentials", ); // OIDC scopes should not be requestable (code authorization grant should be used) @@ -1087,6 +1095,7 @@ async function handleRefreshTokenGrant( client_id, client_secret, // Optional for refresh_grant but required on confidential clients requestedScopes ?? scopes, + "refresh_token", ); const user = await ctx.context.internalAdapter.findUserById( diff --git a/packages/oauth-provider/src/utils/index.ts b/packages/oauth-provider/src/utils/index.ts index 7d779c5cae..20c6c8d347 100644 --- a/packages/oauth-provider/src/utils/index.ts +++ b/packages/oauth-provider/src/utils/index.ts @@ -12,6 +12,7 @@ import type { jwt } from "better-auth/plugins"; import { APIError } from "better-call"; import type { oauthProvider } from "../oauth"; import type { + GrantType, OAuthOptions, Prompt, SchemaClient, @@ -401,6 +402,33 @@ export function basicToClientCredentials(authorization: string) { } } +/** + * Whether a client is allowed to use a given grant type. + * + * A client's registered `grantTypes` defaults to the documented default + * `["authorization_code"]` when unset (see client registration). Refresh tokens + * are only ever issued through the authorization_code flow, so a client allowed + * to use `authorization_code` is implicitly allowed to use `refresh_token`. + * + * @internal + */ +export function clientAllowsGrant( + client: Pick, "grantTypes">, + grantType: GrantType, +) { + const allowedGrants = + client.grantTypes && client.grantTypes.length > 0 + ? client.grantTypes + : (["authorization_code"] as GrantType[]); + if ( + grantType === "refresh_token" && + allowedGrants.includes("authorization_code") + ) { + return true; + } + return allowedGrants.includes(grantType); +} + /** * Validates client credentials failing on mismatches * and incorrectly provided information @@ -413,6 +441,7 @@ export async function validateClientCredentials( clientId: string, clientSecret?: string, // optional because required if client is confidential or this value is defined scopes?: string[], // checks requested scopes against allowed scopes + grantType?: GrantType, // if set, enforces the client is registered for this grant type ) { const client = await getClient(ctx, options, clientId); if (!client) { @@ -473,6 +502,14 @@ export async function validateClientCredentials( } } + // Enforce the client is registered for the requested grant type + if (grantType && !clientAllowsGrant(client, grantType)) { + throw new APIError("BAD_REQUEST", { + error_description: `client is not authorized to use grant type ${grantType}`, + error: "unauthorized_client", + }); + } + return client; } diff --git a/packages/scim/src/index.ts b/packages/scim/src/index.ts index 2996f4130f..9e1862bf16 100644 --- a/packages/scim/src/index.ts +++ b/packages/scim/src/index.ts @@ -50,7 +50,7 @@ export const scim = (options?: SCIMOptions) => { getSCIMProviderConnection: getSCIMProviderConnection(opts), deleteSCIMProviderConnection: deleteSCIMProviderConnection(opts), getSCIMUser: getSCIMUser(authMiddleware), - createSCIMUser: createSCIMUser(authMiddleware), + createSCIMUser: createSCIMUser(authMiddleware, opts), patchSCIMUser: patchSCIMUser(authMiddleware), deleteSCIMUser: deleteSCIMUser(authMiddleware), updateSCIMUser: updateSCIMUser(authMiddleware), diff --git a/packages/scim/src/routes.ts b/packages/scim/src/routes.ts index 0d458f56f9..acb3ab5fca 100644 --- a/packages/scim/src/routes.ts +++ b/packages/scim/src/routes.ts @@ -136,6 +136,62 @@ async function findOrganizationMember( }); } +/** + * Decides whether SCIM provisioning may attach to a pre-existing user that + * matched by email. Linking by email alone would give the SCIM token full + * read/write/delete access to a user it never provisioned, so this returns + * `false` unless `opts.linkExistingUsers` explicitly opts in and every + * configured constraint passes. + */ +async function canLinkExistingUser( + ctx: GenericEndpointContext, + opts: SCIMOptions, + existingUser: User, + email: string, +): Promise { + const policy = opts.linkExistingUsers; + if (!policy) return false; + if (policy === true) return true; + + const { organizationId, providerId } = ctx.context.scimProvider; + + // An empty policy object must not silently allow linking — require at least + // one positive constraint to be configured. + const hasConstraint = + (policy.trustedDomains?.length ?? 0) > 0 || + policy.requireExistingOrgMembership === true || + typeof policy.shouldLinkUser === "function"; + if (!hasConstraint) return false; + + if (policy.requireExistingOrgMembership) { + if (!organizationId) return false; + const member = await findOrganizationMember( + ctx, + existingUser.id, + organizationId, + ); + if (!member) return false; + } + + if (policy.trustedDomains?.length) { + const domain = email.split("@")[1]?.toLowerCase(); + const allowed = + !!domain && policy.trustedDomains.some((d) => d.toLowerCase() === domain); + if (!allowed) return false; + } + + if (policy.shouldLinkUser) { + const ok = await policy.shouldLinkUser({ + user: existingUser, + email, + provider: { providerId, organizationId }, + }); + if (!ok) return false; + } + + return true; +} + async function assertSCIMProviderAccess( ctx: GenericEndpointContext, userId: string, @@ -294,6 +350,23 @@ export const generateSCIMToken = (opts: SCIMOptions) => } } + // Optional app-level gate. Personal (non-org) tokens otherwise have no + // authorization beyond a valid session, so this is the hook to + // restrict who can mint them. + if (opts.canGenerateToken) { + const allowed = await opts.canGenerateToken({ + user, + providerId, + organizationId, + member, + }); + if (!allowed) { + throw new APIError("FORBIDDEN", { + message: "You are not allowed to generate a SCIM token", + }); + } + } + const scimProvider = await ctx.context.adapter.findOne({ model: "scimProvider", where: [ @@ -542,7 +615,10 @@ export const deleteSCIMProviderConnection = (opts: SCIMOptions) => }, ); -export const createSCIMUser = (authMiddleware: AuthMiddleware) => +export const createSCIMUser = ( + authMiddleware: AuthMiddleware, + opts: SCIMOptions, +) => createAuthEndpoint( "/scim/v2/Users", { @@ -643,6 +719,21 @@ export const createSCIMUser = (authMiddleware: AuthMiddleware) => let account: Account; if (existingUser) { + // Do not auto-link a pre-existing user by email alone — that would + // grant this SCIM token access to an account it never provisioned. + // Require an explicit, configured policy to allow it. + const allowLink = await canLinkExistingUser( + ctx, + opts, + existingUser, + email, + ); + if (!allowLink) { + throw new SCIMAPIError("CONFLICT", { + detail: "User already exists", + scimType: "uniqueness", + }); + } user = existingUser; account = await ctx.context.adapter.transaction(async () => { const account = await createAccount(user.id); @@ -1038,7 +1129,7 @@ export const deleteSCIMUser = (authMiddleware: AuthMiddleware) => const providerId = ctx.context.scimProvider.providerId; const organizationId = ctx.context.scimProvider.organizationId; - const { user } = await findUserById(ctx.context.adapter, { + const { user, account } = await findUserById(ctx.context.adapter, { userId, providerId, organizationId, @@ -1050,6 +1141,32 @@ export const deleteSCIMUser = (authMiddleware: AuthMiddleware) => }); } + // Organization-scoped SCIM must not delete the *global* Better Auth + // user — that would remove the person's access to every other + // organization and identity, well outside this token's boundary. + // Deprovision instead: drop their membership in this organization and + // the SCIM account link for this provider, leaving the user intact. + if (organizationId) { + await ctx.context.adapter.transaction(async () => { + await ctx.context.adapter.deleteMany({ + model: "member", + where: [ + { field: "organizationId", value: organizationId }, + { field: "userId", value: userId }, + ], + }); + if (account) { + await ctx.context.adapter.delete({ + model: "account", + where: [{ field: "id", value: account.id }], + }); + } + }); + + ctx.setStatus(204); + return; + } + await ctx.context.internalAdapter.deleteUserSessions(userId); await ctx.context.internalAdapter.deleteUser(userId); diff --git a/packages/scim/src/scim-users.test.ts b/packages/scim/src/scim-users.test.ts index 01f2cf2fb9..0ef38f3357 100644 --- a/packages/scim/src/scim-users.test.ts +++ b/packages/scim/src/scim-users.test.ts @@ -688,7 +688,9 @@ describe("SCIM", () => { database: memory, baseURL: "http://localhost:3000", emailAndPassword: { enabled: true }, - plugins: [scim(), organization()], + // This regression test links a pre-existing user by email, which + // is now opt-in via `linkExistingUsers`. + plugins: [scim({ linkExistingUsers: true }), organization()], secondaryStorage: { set(key, value) { store.set(key, value); @@ -750,6 +752,68 @@ describe("SCIM", () => { expect(store.has(victimToken)).toBe(false); }); + + it("should deprovision (not delete the global user) for an org-scoped DELETE", async () => { + const { auth, getSCIMToken, registerOrganization } = createTestInstance(); + const organization = await registerOrganization("org:deprovision"); + const scimToken = await getSCIMToken( + "provider-deprovision", + organization?.id, + ); + + const created = await auth.api.createSCIMUser({ + body: { + userName: "scim-user", + emails: [{ value: "scim-user@email.com" }], + }, + headers: { authorization: `Bearer ${scimToken}` }, + }); + + const ctx = await auth.$context; + + // SCIM provisioning created an org membership for the new user. + const memberBefore = await ctx.adapter.findOne({ + model: "member", + where: [ + { field: "organizationId", value: organization!.id }, + { field: "userId", value: created.id }, + ], + }); + expect(memberBefore).not.toBeNull(); + + await auth.api.deleteSCIMUser({ + params: { userId: created.id }, + headers: { authorization: `Bearer ${scimToken}` }, + }); + + // The global Better Auth user must NOT be deleted by an org-scoped token. + const userAfter = await ctx.adapter.findOne({ + model: "user", + where: [{ field: "id", value: created.id }], + }); + expect(userAfter).not.toBeNull(); + + // The org membership is removed (deprovisioned). + const memberAfter = await ctx.adapter.findOne({ + model: "member", + where: [ + { field: "organizationId", value: organization!.id }, + { field: "userId", value: created.id }, + ], + }); + expect(memberAfter).toBeNull(); + + // The SCIM account link is removed, so the user is no longer + // reachable through this provider. + await expect( + auth.api.getSCIMUser({ + params: { userId: created.id }, + headers: { authorization: `Bearer ${scimToken}` }, + }), + ).rejects.toThrowError( + expect.objectContaining({ message: "User not found" }), + ); + }); }); describe("Default SCIM provider", () => { diff --git a/packages/scim/src/scim.management.test.ts b/packages/scim/src/scim.management.test.ts index d3f31fea4e..23b807c40b 100644 --- a/packages/scim/src/scim.management.test.ts +++ b/packages/scim/src/scim.management.test.ts @@ -133,6 +133,47 @@ describe("SCIM provider management", () => { ); }); + it("should deny personal token creation when canGenerateToken returns false", async () => { + const { auth, getAuthCookieHeaders } = createTestInstance({ + canGenerateToken: ({ organizationId }) => !!organizationId, + }); + const headers = await getAuthCookieHeaders(); + + const generateSCIMToken = () => + auth.api.generateSCIMToken({ + body: { providerId: "personal-provider" }, + headers, + }); + + await expect(generateSCIMToken()).rejects.toThrowError( + expect.objectContaining({ + message: "You are not allowed to generate a SCIM token", + }), + ); + }); + + it("should allow token creation when canGenerateToken returns true (member is null for personal)", async () => { + let received: { providerId: string; member: unknown } | null = null; + const { auth, getAuthCookieHeaders } = createTestInstance({ + canGenerateToken: ({ providerId, member }) => { + received = { providerId, member }; + return true; + }, + }); + const headers = await getAuthCookieHeaders(); + + const { scimToken } = await auth.api.generateSCIMToken({ + body: { providerId: "personal-provider" }, + headers, + }); + + expect(scimToken).toBeTruthy(); + expect(received).toEqual({ + providerId: "personal-provider", + member: null, + }); + }); + it("should fail if the authenticated user does not belong to the given org", async () => { const { auth, getAuthCookieHeaders } = createTestInstance(); const headers = await getAuthCookieHeaders(); diff --git a/packages/scim/src/scim.test.ts b/packages/scim/src/scim.test.ts index dda553a7f7..c4abedd7f7 100644 --- a/packages/scim/src/scim.test.ts +++ b/packages/scim/src/scim.test.ts @@ -644,7 +644,11 @@ describe("SCIM", () => { }); it("should create a new account linked to an existing user", async () => { - const { auth, authClient, getSCIMToken } = createTestInstance(); + // Linking a pre-existing user by email is opt-in via + // `linkExistingUsers`; enable the legacy behavior for this test. + const { auth, authClient, getSCIMToken } = createTestInstance({ + linkExistingUsers: true, + }); const scimToken = await getSCIMToken(); await authClient.signUp.email({ @@ -697,6 +701,83 @@ describe("SCIM", () => { }); }); + it("should NOT link a pre-existing user by email when linkExistingUsers is disabled (default)", async () => { + const { auth, authClient, getSCIMToken } = createTestInstance(); + const scimToken = await getSCIMToken(); + + await authClient.signUp.email({ + email: "existing-user@email.com", + password: "the password", + name: "existing-user", + }); + + const createUser = () => + auth.api.createSCIMUser({ + body: { + userName: "existing-user", + emails: [{ value: "existing-user@email.com" }], + }, + headers: { + authorization: `Bearer ${scimToken}`, + }, + }); + + // Must not silently link to the existing account. + await expect(createUser()).rejects.toThrowError( + expect.objectContaining({ + message: "User already exists", + body: expect.objectContaining({ + scimType: "uniqueness", + status: "409", + }), + }), + ); + }); + + it("should only link a pre-existing user whose email domain is trusted", async () => { + const { auth, authClient, getSCIMToken } = createTestInstance({ + linkExistingUsers: { trustedDomains: ["trusted.com"] }, + }); + const scimToken = await getSCIMToken(); + + await authClient.signUp.email({ + email: "user@other.com", + password: "the password", + name: "other", + }); + await authClient.signUp.email({ + email: "user@trusted.com", + password: "the password", + name: "trusted", + }); + + // Domain not in trustedDomains: rejected. + await expect( + auth.api.createSCIMUser({ + body: { + userName: "other", + emails: [{ value: "user@other.com" }], + }, + headers: { authorization: `Bearer ${scimToken}` }, + }), + ).rejects.toThrowError( + expect.objectContaining({ message: "User already exists" }), + ); + + // Trusted domain: linked. + const linked = await auth.api.createSCIMUser({ + body: { + userName: "trusted", + emails: [{ value: "user@trusted.com" }], + }, + headers: { authorization: `Bearer ${scimToken}` }, + }); + expect(linked.id).toBeTruthy(); + expect(linked.emails).toEqual([ + { primary: true, value: "user@trusted.com" }, + ]); + }); + it("should create a new user with external id", async () => { const { auth, getSCIMToken } = createTestInstance(); const scimToken = await getSCIMToken(); diff --git a/packages/scim/src/types.ts b/packages/scim/src/types.ts index 0e32aa1092..f7db117ef6 100644 --- a/packages/scim/src/types.ts +++ b/packages/scim/src/types.ts @@ -37,6 +37,43 @@ export type SCIMOptions = { * These will take precedence over the database when present. */ defaultSCIM?: Omit[]; + /** + * Controls whether SCIM provisioning may link to a *pre-existing* Better + * Auth user whose email matches the incoming SCIM resource. + * + * Disabled by default: when a user with the same email already exists, + * `createSCIMUser` returns `409` (uniqueness) instead of silently creating a + * SCIM account link for that user. Linking by email alone would give a SCIM + * token access to an account it never provisioned. + * + * - `true` restores the legacy behavior of linking any existing user that + * matches by email. Only use this with a fully trusted token-issuance flow. + * - An object enables linking only when *every* provided constraint passes. + */ + linkExistingUsers?: + | boolean + | { + /** + * Only link when the email's domain is in this allow-list + * (case-insensitive). An empty/absent list is not a match. + */ + trustedDomains?: string[]; + /** + * For organization-scoped tokens, only link a user who is already + * a member of the token's organization (never auto-add them). Has + * no effect for non-org (personal) tokens, which then never match + * on this constraint. + */ + requireExistingOrgMembership?: boolean; + /** + * Full control: return `true` to allow linking the matched user. + */ + shouldLinkUser?: (payload: { + user: User; + email: string; + provider: { providerId: string; organizationId?: string }; + }) => boolean | Promise; + }; /** * A callback that runs before a new SCIM token is generated. * Runs after the built-in role check, so it can add additional @@ -56,6 +93,22 @@ export type SCIMOptions = { scimToken: string; scimProvider: SCIMProvider; }) => Promise; + /** + * Authorize who may generate a SCIM token. Runs after the built-in checks + * (org-scoped tokens still require org membership + the required role), so it + * can add restrictions but cannot loosen them. + * + * Use this to lock down *personal* (non-org-scoped) token creation, which is + * otherwise available to any authenticated user. SCIM tokens can provision + * and manage users, so return `false` to deny. `member` is `null` for + * personal tokens. + */ + canGenerateToken?: (payload: { + user: User; + providerId: string; + organizationId?: string; + member: Member | null; + }) => boolean | Promise; /** * How to store the SCIM token in the database. * diff --git a/packages/sso/src/oidc/discovery.test.ts b/packages/sso/src/oidc/discovery.test.ts index a40690e269..d54653209a 100644 --- a/packages/sso/src/oidc/discovery.test.ts +++ b/packages/sso/src/oidc/discovery.test.ts @@ -1,5 +1,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { + assertEndpointResolvesPublic, + assertOIDCEndpointsResolvePublic, computeDiscoveryUrl, discoverOIDCConfig, ensureRuntimeDiscovery, @@ -18,6 +20,9 @@ vi.mock("@better-fetch/fetch", () => ({ betterFetch: vi.fn(), })); +const { lookupMock } = vi.hoisted(() => ({ lookupMock: vi.fn() })); +vi.mock("node:dns/promises", () => ({ lookup: lookupMock })); + import { betterFetch } from "@better-fetch/fetch"; /** @@ -1241,4 +1246,159 @@ describe("ensureRuntimeDiscovery", () => { expect.objectContaining({ code: "discovery_untrusted_origin" }), ); }); + + it("rejects a stored endpoint whose DNS resolves to an internal address", async () => { + const completeConfig = { + ...baseConfig, + authorizationEndpoint: "https://idp.internal.example/oauth2/authorize", + tokenEndpoint: "https://idp.internal.example/oauth2/token", + jwksEndpoint: "https://idp.internal.example/.well-known/jwks.json", + }; + lookupMock.mockResolvedValue([{ address: "169.254.169.254", family: 4 }]); + await expect( + ensureRuntimeDiscovery(completeConfig, issuer, () => false), + ).rejects.toThrow( + expect.objectContaining({ code: "discovery_private_host" }), + ); + expect(betterFetch).not.toHaveBeenCalled(); + }); +}); + +describe("resolved-address guard", () => { + beforeEach(() => { + lookupMock.mockReset(); + }); + + const notTrusted = () => false; + + describe("assertEndpointResolvesPublic", () => { + it.each([ + ["loopback", "127.0.0.1"], + ["link-local / cloud metadata", "169.254.169.254"], + ["private RFC 1918", "10.0.0.5"], + ["IPv6 loopback", "::1"], + ])("rejects an FQDN that resolves to %s (%s)", async (_label, address) => { + lookupMock.mockResolvedValue([{ address, family: 4 }]); + await expect( + assertEndpointResolvesPublic( + "tokenEndpoint", + "https://idp.internal.example/token", + notTrusted, + ), + ).rejects.toThrow( + expect.objectContaining({ code: "discovery_private_host" }), + ); + }); + + it("allows an FQDN that resolves to a public address", async () => { + lookupMock.mockResolvedValue([{ address: "93.184.216.34", family: 4 }]); + await expect( + assertEndpointResolvesPublic( + "tokenEndpoint", + "https://idp.example.com/token", + notTrusted, + ), + ).resolves.toBeUndefined(); + }); + + it("rejects if ANY resolved address is internal (mixed result)", async () => { + lookupMock.mockResolvedValue([ + { address: "93.184.216.34", family: 4 }, + { address: "127.0.0.1", family: 4 }, + ]); + await expect( + assertEndpointResolvesPublic( + "jwksEndpoint", + "https://idp.internal.example/jwks", + notTrusted, + ), + ).rejects.toThrow( + expect.objectContaining({ code: "discovery_private_host" }), + ); + }); + + it("skips DNS resolution for operator-allowlisted origins (internal IdP escape hatch)", async () => { + await assertEndpointResolvesPublic( + "tokenEndpoint", + "https://internal-idp.corp/token", + () => true, + ); + expect(lookupMock).not.toHaveBeenCalled(); + }); + + it("does not resolve IP-literal hosts (already covered by the sync check)", async () => { + await assertEndpointResolvesPublic( + "tokenEndpoint", + "https://93.184.216.34/token", + notTrusted, + ); + expect(lookupMock).not.toHaveBeenCalled(); + }); + + it("does not throw when resolution itself fails (lets the fetch surface it)", async () => { + lookupMock.mockRejectedValue(new Error("ENOTFOUND")); + await expect( + assertEndpointResolvesPublic( + "tokenEndpoint", + "https://idp.example.com/token", + notTrusted, + ), + ).resolves.toBeUndefined(); + }); + }); + + describe("assertOIDCEndpointsResolvePublic", () => { + it("checks token, userinfo, and jwks endpoints", async () => { + lookupMock.mockResolvedValue([{ address: "93.184.216.34", family: 4 }]); + await assertOIDCEndpointsResolvePublic( + { + tokenEndpoint: "https://idp.example.com/token", + userInfoEndpoint: "https://idp.example.com/userinfo", + jwksEndpoint: "https://idp.example.com/jwks", + }, + notTrusted, + ); + expect(lookupMock).toHaveBeenCalledTimes(3); + }); + + it("rejects when the userinfo endpoint resolves internally", async () => { + lookupMock.mockResolvedValue([{ address: "10.1.2.3", family: 4 }]); + await expect( + assertOIDCEndpointsResolvePublic( + { userInfoEndpoint: "https://idp.internal.example/userinfo" }, + notTrusted, + ), + ).rejects.toThrow( + expect.objectContaining({ code: "discovery_private_host" }), + ); + }); + + it("rejects a syntactically-private endpoint before any DNS lookup", async () => { + await expect( + assertOIDCEndpointsResolvePublic( + { tokenEndpoint: "https://169.254.169.254/token" }, + notTrusted, + ), + ).rejects.toThrow( + expect.objectContaining({ code: "discovery_private_host" }), + ); + expect(lookupMock).not.toHaveBeenCalled(); + }); + }); +}); + +describe("fetchDiscoveryDocument redirect handling", () => { + it("disables automatic redirect following", async () => { + vi.mocked(betterFetch).mockResolvedValueOnce({ + data: createMockDiscoveryDocument(), + error: null, + }); + await fetchDiscoveryDocument( + "https://idp.example.com/.well-known/openid-configuration", + ); + expect(betterFetch).toHaveBeenCalledWith( + "https://idp.example.com/.well-known/openid-configuration", + expect.objectContaining({ redirect: "error" }), + ); + }); }); diff --git a/packages/sso/src/oidc/discovery.ts b/packages/sso/src/oidc/discovery.ts index db33ecbf75..8433d62093 100644 --- a/packages/sso/src/oidc/discovery.ts +++ b/packages/sso/src/oidc/discovery.ts @@ -8,7 +8,10 @@ * @see https://openid.net/specs/openid-connect-discovery-1_0.html */ -import { isPublicRoutableHost } from "@better-auth/core/utils/host"; +import { + classifyHost, + isPublicRoutableHost, +} from "@better-auth/core/utils/host"; import { betterFetch } from "@better-fetch/fetch"; import type { OIDCConfig } from "../types"; import type { @@ -195,6 +198,100 @@ export function validateSkipDiscoveryEndpoints( } } +/** + * Re-validate an endpoint by resolving its hostname and rejecting any resolved + * address that is not publicly routable. + * + * {@link validateSkipDiscoveryEndpoint} only classifies the literal hostname, so + * a host like `idp.example` whose DNS record points at `127.0.0.1`, + * `169.254.169.254`, or an RFC 1918 address passes that check unchanged. This + * function closes that gap by performing the same RFC 6890 classification on the + * addresses the host actually resolves to, right before the server-side fetch. + * + * Best-effort by design: + * - Operator-allowlisted origins (trustedOrigins) are skipped — this is the + * documented escape hatch for internal IdPs. + * - IP-literal hosts are already fully covered by the synchronous check. + * - On runtimes without `node:dns` (e.g. Cloudflare Workers / edge), DNS + * resolution is unavailable; we fall back to the synchronous host check and + * the platform's own egress controls. + * + * Note: this resolves once and validates the result; it does not pin the address + * for the subsequent connection, so a change in the resolved address between + * this lookup and the fetch remains theoretically possible. It nonetheless + * rejects the common case of a DNS record that statically points at an internal + * address. + * + * @throws DiscoveryError(discovery_private_host) if any resolved address is not public + */ +export async function assertEndpointResolvesPublic( + name: string, + endpoint: string, + isTrustedOrigin: (url: string) => boolean, +): Promise { + const parsed = parseURL(name, endpoint); + + // Operator opt-in for internal IdPs (mirrors validateSkipDiscoveryEndpoint). + if (isTrustedOrigin(parsed.toString())) return; + + const host = parsed.hostname; + // IP literals are already classified synchronously; only FQDNs need resolving. + if (classifyHost(host).literal !== "fqdn") return; + + let dns: typeof import("node:dns/promises"); + try { + dns = await import("node:dns/promises"); + } catch { + // Runtime without node:dns — rely on the synchronous host check. + return; + } + + let resolved: Array<{ address: string }>; + try { + resolved = await dns.lookup(host, { all: true }); + } catch { + // Resolution failure: let the actual fetch surface the network error. + return; + } + + for (const { address } of resolved) { + if (!isPublicRoutableHost(address)) { + throw new DiscoveryError( + "discovery_private_host", + `The ${name} host "${host}" resolves to a non-publicly-routable address (${address}). If this is an internal IdP, add its origin to trustedOrigins.`, + { endpoint: name, url: endpoint, hostname: host, resolved: address }, + ); + } + } +} + +/** + * Re-validate, at fetch time, every OIDC endpoint that is fetched server-side + * (token, userinfo, jwks). Runs the synchronous host classification plus the + * best-effort DNS resolution check. `authorizationEndpoint` is intentionally + * excluded — it is a browser redirect target, not a server-side fetch, so these + * checks don't apply to it. + */ +export async function assertOIDCEndpointsResolvePublic( + config: { + tokenEndpoint?: string | null; + userInfoEndpoint?: string | null; + jwksEndpoint?: string | null; + }, + isTrustedOrigin: (url: string) => boolean, +): Promise { + const fields: Array<[string, string | null | undefined]> = [ + ["tokenEndpoint", config.tokenEndpoint], + ["userInfoEndpoint", config.userInfoEndpoint], + ["jwksEndpoint", config.jwksEndpoint], + ]; + for (const [name, url] of fields) { + if (!url) continue; + validateSkipDiscoveryEndpoint(name, url, isTrustedOrigin); + await assertEndpointResolvesPublic(name, url, isTrustedOrigin); + } +} + /** * Fetch the OIDC discovery document from the IdP. * @@ -211,6 +308,11 @@ export async function fetchDiscoveryDocument( const response = await betterFetch(url, { method: "GET", timeout, + // Never auto-follow redirects on this server-side fetch. A redirect + // Location is not re-validated against the private-host checks, so a + // redirect could send the fetch to an internal/loopback/metadata + // address. Reject redirects outright. + redirect: "error", }); if (response.error) { @@ -578,20 +680,22 @@ export async function ensureRuntimeDiscovery( issuer: string, isTrustedOrigin: (url: string) => boolean, ): Promise { - if (!needsRuntimeDiscovery(config)) { - return config; + let resolved = config; + if (needsRuntimeDiscovery(config)) { + const hydrated = await discoverOIDCConfig({ + issuer, + existingConfig: config, + isTrustedOrigin, + }); + resolved = { + ...config, + authorizationEndpoint: hydrated.authorizationEndpoint, + tokenEndpoint: hydrated.tokenEndpoint, + tokenEndpointAuthentication: hydrated.tokenEndpointAuthentication, + userInfoEndpoint: hydrated.userInfoEndpoint, + jwksEndpoint: hydrated.jwksEndpoint, + }; } - const hydrated = await discoverOIDCConfig({ - issuer, - existingConfig: config, - isTrustedOrigin, - }); - return { - ...config, - authorizationEndpoint: hydrated.authorizationEndpoint, - tokenEndpoint: hydrated.tokenEndpoint, - tokenEndpointAuthentication: hydrated.tokenEndpointAuthentication, - userInfoEndpoint: hydrated.userInfoEndpoint, - jwksEndpoint: hydrated.jwksEndpoint, - }; + await assertOIDCEndpointsResolvePublic(resolved, isTrustedOrigin); + return resolved; } diff --git a/packages/sso/src/providers.test.ts b/packages/sso/src/providers.test.ts index eb83695c94..d4848da36c 100644 --- a/packages/sso/src/providers.test.ts +++ b/packages/sso/src/providers.test.ts @@ -1460,3 +1460,117 @@ describe("SSO provider read endpoints", () => { }); }); }); + +/** + * SSO provider ids share the account-linking provider namespace with + * social/OAuth providers. A user-registered SSO provider named after a + * configured social/trusted provider could otherwise inherit trust meant for + * the real provider and implicitly link to a verified local account. + * Registration must reject such colliding ids. + */ +describe("SSO providerId namespace collisions", () => { + const setup = () => { + const data: Record = { + user: [], + session: [], + verification: [], + account: [], + ssoProvider: [], + }; + const auth = betterAuth({ + database: memoryAdapter(data), + baseURL: "http://localhost:3000", + emailAndPassword: { enabled: true }, + socialProviders: { + google: { clientId: "test", clientSecret: "test" }, + }, + account: { + accountLinking: { + enabled: true, + trustedProviders: ["github"], + }, + }, + plugins: [sso()], + }); + const authClient = createAuthClient({ + baseURL: "http://localhost:3000", + plugins: [ssoClient()], + fetchOptions: { + customFetchImpl: async (url, init) => + auth.handler(new Request(url, init)), + }, + }); + const signIn = async () => { + const headers = new Headers(); + await authClient.signUp.email({ + email: "user@example.com", + password: "password123", + name: "User", + }); + await authClient.signIn.email( + { email: "user@example.com", password: "password123" }, + { throw: true, onSuccess: setCookieToHeader(headers) }, + ); + return headers; + }; + return { auth, signIn }; + }; + + const registerBody = (providerId: string) => ({ + providerId, + issuer: "https://idp.example.com", + domain: "example.com", + samlConfig: { + entryPoint: "https://idp.example.com/sso", + cert: TEST_CERT, + callbackUrl: "http://localhost:3000/api/sso/callback", + audience: "my-audience", + wantAssertionsSigned: true, + spMetadata: {}, + }, + }); + + it("rejects a providerId that collides with a configured social provider", async () => { + const { auth, signIn } = setup(); + const headers = await signIn(); + const response = await auth.api.registerSSOProvider({ + body: registerBody("google"), + headers, + asResponse: true, + }); + expect(response.status).toBe(422); + }); + + it("rejects a providerId that collides with a trustedProviders entry", async () => { + const { auth, signIn } = setup(); + const headers = await signIn(); + const response = await auth.api.registerSSOProvider({ + body: registerBody("github"), + headers, + asResponse: true, + }); + expect(response.status).toBe(422); + }); + + it("rejects the reserved 'credential' providerId", async () => { + const { auth, signIn } = setup(); + const headers = await signIn(); + const response = await auth.api.registerSSOProvider({ + body: registerBody("credential"), + headers, + asResponse: true, + }); + expect(response.status).toBe(422); + }); + + it("allows a non-colliding providerId", async () => { + const { auth, signIn } = setup(); + const headers = await signIn(); + const response = await auth.api.registerSSOProvider({ + body: registerBody("okta-corp"), + headers, + asResponse: true, + }); + expect(response.status).toBe(200); + }); +}); diff --git a/packages/sso/src/routes/saml-pipeline.ts b/packages/sso/src/routes/saml-pipeline.ts index 3185e49eb5..d641178f86 100644 --- a/packages/sso/src/routes/saml-pipeline.ts +++ b/packages/sso/src/routes/saml-pipeline.ts @@ -424,11 +424,14 @@ export async function processSAMLResponse( } // 15. Session creation + // SSO provider ids are user-controlled and share the social-provider account + // namespace, so trust must come solely from verified domain ownership — + // never from a name match against the global `trustedProviders` list + // (enforced via `trustProviderByName: false` below). const isTrustedProvider: boolean = - ctx.context.trustedProviders.includes(providerId) || - ("domainVerified" in provider && - !!(provider as { domainVerified?: boolean }).domainVerified && - validateEmailDomain(userInfo.email as string, provider.domain)); + "domainVerified" in provider && + !!(provider as { domainVerified?: boolean }).domainVerified && + validateEmailDomain(userInfo.email as string, provider.domain); // TODO: split callbackUrl into separate ACS URL and post-auth redirect // fields. Currently callbackUrl serves both purposes, which means @@ -458,6 +461,7 @@ export async function processSAMLResponse( callbackURL: callbackUrl, disableSignUp: options?.disableImplicitSignUp, isTrustedProvider, + trustProviderByName: false, }); } catch (e) { if (isAPIError(e) && e.body?.code) { diff --git a/packages/sso/src/routes/sso.ts b/packages/sso/src/routes/sso.ts index 259c5e4e29..5b3a09453a 100644 --- a/packages/sso/src/routes/sso.ts +++ b/packages/sso/src/routes/sso.ts @@ -658,6 +658,27 @@ export const registerSSOProvider = (options: O) => { } } + // SSO provider ids share the account-linking provider namespace with + // social/OAuth providers. Reject ids that collide with a configured + // social provider, a trusted provider, or a reserved built-in id so a + // user-registered SSO provider can't be confused with one of them. + // Trust for SSO providers is established separately via + // verified domain ownership, never by this shared namespace. + const reservedProviderIds = new Set([ + "credential", + ...ctx.context.socialProviders.map((p) => p.id), + ...ctx.context.trustedProviders, + ]); + if (reservedProviderIds.has(body.providerId)) { + ctx.context.logger.warn( + `SSO provider registration rejected for reserved providerId: ${body.providerId}`, + ); + throw new APIError("UNPROCESSABLE_ENTITY", { + message: + "This providerId is reserved and cannot be used for an SSO provider", + }); + } + const existingProvider = await ctx.context.adapter.findOne({ model: "ssoProvider", where: [ @@ -1566,6 +1587,7 @@ async function handleOIDCCallback( headers: { Authorization: `Bearer ${tokenResponse.accessToken}`, }, + redirect: "error", }, ); if (userInfoResponse.error) { @@ -1688,6 +1710,11 @@ async function handleOIDCCallback( disableSignUp: options?.disableImplicitSignUp && !requestSignUp, overrideUserInfo: config.overrideUserInfo, isTrustedProvider, + // SSO provider ids are user-controlled and live in the same namespace + // as social providers. Never inherit trust from the global + // `trustedProviders` list by name — rely solely on the SSO-specific + // `isTrustedProvider` (verified domain ownership) computed above. + trustProviderByName: false, }); } catch (e) { if (isAPIError(e) && e.body?.code) { diff --git a/packages/sso/src/saml.test.ts b/packages/sso/src/saml.test.ts index da1ae57dd0..a5f8f8e8cf 100644 --- a/packages/sso/src/saml.test.ts +++ b/packages/sso/src/saml.test.ts @@ -1971,7 +1971,11 @@ describe("SAML SSO", async () => { expect(redirectLocation).toContain("error=account_not_linked"); }); - it("should allow account linking when provider is in trustedProviders", async () => { + // SSO trust must come from verified domain ownership, never from a name + // match against the global `trustedProviders` list — otherwise a + // user-registered SSO provider named after a trusted provider could inherit + // that trust. Registering such a colliding id is now rejected outright. + it("should reject registering an SSO provider whose id collides with a trustedProviders entry", async () => { const { auth: authWithTrusted, signInWithTestUser } = await getTestInstance( { account: { @@ -1986,7 +1990,7 @@ describe("SAML SSO", async () => { const { headers } = await signInWithTestUser(); - await authWithTrusted.api.registerSSOProvider({ + const response = await authWithTrusted.api.registerSSOProvider({ body: { providerId: "trusted-saml-provider", issuer: "http://localhost:8081", @@ -2009,47 +2013,10 @@ describe("SAML SSO", async () => { }, }, headers, + asResponse: true, }); - const ctx = await authWithTrusted.$context; - await ctx.adapter.create({ - model: "user", - data: { - id: "existing-user-id-2", - email: "test@email.com", - name: "Existing User", - emailVerified: true, - createdAt: new Date(), - updatedAt: new Date(), - }, - }); - - let samlResponse: any; - await betterFetch("http://localhost:8081/api/sso/saml2/idp/post", { - onSuccess: async (context) => { - samlResponse = await context.data; - }, - }); - - const response = await authWithTrusted.handler( - new Request( - "http://localhost:3000/api/auth/sso/saml2/callback/trusted-saml-provider", - { - method: "POST", - headers: { - "Content-Type": "application/x-www-form-urlencoded", - }, - body: new URLSearchParams({ - SAMLResponse: samlResponse.samlResponse, - }), - }, - ), - ); - - expect(response.status).toBe(302); - const redirectLocation = response.headers.get("location") || ""; - expect(redirectLocation).not.toContain("error"); - expect(redirectLocation).toContain("dashboard"); + expect(response.status).toBe(422); }); it("should reject unsolicited SAML response when allowIdpInitiated is false", async () => {