Skip to content

fix: always emit email and name claims in the planner id_token - #83

Open
mroderick wants to merge 3 commits into
mainfrom
fix/id-token-email-claim
Open

mroderick wants to merge 3 commits into
mainfrom
fix/id-token-email-claim

Conversation

@mroderick

@mroderick mroderick commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

Users signing in through the auth app could be dropped into the planner's new-member flow, losing their subscriptions and roles, because the planner received an id_token without an email claim and fell back to sub (the better-auth user id). This fix makes every id_token carry the user's email and name, and bumps better-auth to 1.7.7.

  • customIdTokenClaims now always emits email and name from the user record, for every session type (previously it only returned github_id)
  • better-auth and @better-auth/oauth-provider bumped 1.7.5 → 1.7.7
  • Test suite aligned with production config and now covers the regression
Background and mechanism

The planner's codebar OmniAuth strategy resolves the member by the id_token's email claim, falling back to sub when the claim is absent:

email = payload['email'] || payload['sub']

better-auth 1.7 stopped including user-record claims (email, name) in the id_token for sessions not created through a social provider. A user who signed in by magic link therefore got a planner member whose email and name were the better-auth user id — an empty account with no subscriptions or roles. Two such duplicate members were created in production on 2026-10-02; the incident was mitigated by rolling back the auth deploy.

The missing user-record claims affect any session without provider data; the explicit claims in customIdTokenClaims are spread after the library's default claims, so they are always present.

A separate integration test reproduces the scenario: magic-link session → authorize → token exchange → id_token claims. Under 1.7.5/1.7.7 without this change, the email and name claims are absent; with it, they match the user record.

Review notes

Focus first on the claims semantics: every id_token now carries the better-auth user-record email. For users whose GitHub primary email differs from their stored planner email, the planner now matches (or keys a new member) on the user-record email, with the github_id claim still available to resolve returning members. Check that interplay for accounts where the two emails diverge.

Also worth a look: the lockfile diff is now minimal. Only the better-auth family moves 1.7.5 to 1.7.7 (plus one @types/node patch float from peer materialisation), and better-auth is pinned exactly to 1.7.7 to match the @better-auth/oauth-provider pin. Review fixes landed on top: the claims builder is shared between src/auth.js and the test helper, the linked-GitHub test asserts email and name, and the oauth-flow test registers teardown.

Deliberately not done here: hardening the planner's || payload['sub'] fallback (separate planner change), and setting the client's scopes column in the seed (authorize falls back to the plugin's allowed scopes).

@mroderick
mroderick force-pushed the fix/id-token-email-claim branch 2 times, most recently from 7e06a8b to e0d0228 Compare October 3, 2026 07:59
@mroderick
mroderick changed the base branch from main to chore/fallow-3-31-dead-code October 3, 2026 07:59
@mroderick
mroderick added this pull request to stack #85 October 3, 2026 08:05
@mroderick
mroderick marked this pull request as ready for review October 3, 2026 08:05
@mroderick
mroderick marked this pull request as draft October 3, 2026 08:06
@mroderick
mroderick force-pushed the fix/id-token-email-claim branch from e0d0228 to ce12016 Compare October 3, 2026 08:08
Base automatically changed from chore/fallow-3-31-dead-code to main October 3, 2026 10:25
@mroderick
mroderick force-pushed the fix/id-token-email-claim branch 3 times, most recently from d4036bd to c7f6afb Compare October 3, 2026 13:40
@mroderick

Copy link
Copy Markdown
Collaborator Author

Tracked in #87.

@mroderick
mroderick marked this pull request as ready for review October 3, 2026 14:02
@mroderick
mroderick marked this pull request as draft October 3, 2026 14:33
@mroderick
mroderick force-pushed the fix/id-token-email-claim branch from c7f6afb to 232dcd1 Compare October 3, 2026 14:48
Both packages are pinned exactly so patch-level claim changes cannot float in
noticed. The lockfile diff is limited to the better-auth family (1.7.5 -> 1.7.7)
plus one `@types/node` patch float from peer materialisation; no unrelated
runtime deps move.
Users signing in through the auth app could be dropped into the planner's new-member flow, losing 
their subscriptions and roles: the planner resolves members by the id_token `email` claim and falls 
back to `sub` (the better-auth user id) when the claim is absent, and better-auth 1.7 stopped 
including user-record claims for sessions without provider data. Two duplicate members were created 
in production on 2026-10-02.

`customIdTokenClaims` now always emits `email` and `name` from the user record, keeping `github_id` 
for returning-member resolution. The claims builder lives in `src/auth/id-token-claims.js` and is 
shared with the test helper, so the regression tests exercise the code production runs; the test 
instance is aligned with the production scopes list.

Tests cover the magic-link path (email/name asserted against the user record) and the linked-GitHub 
path (all three claims in one payload), and the oauth-flow test now registers tap teardown so it 
stops leaking a schema per run.
The marker is a working note, not documentation. Keep the explanation of why `skipStateCookieCheck` 
is set.

@till till left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would this be fixed if the planner requested email explicitly?

@mroderick

mroderick commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator Author

Would this be fixed if the planner requested email explicitly?

No. Me and my synthetic research assistant checked the 1.7.7 source to be sure

@mroderick

mroderick commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator Author

TL;DR: The planner already requests email (openid profile email in the strategy's authorization request) — that's not the gap. Better-auth 1.7 deliberately stopped putting standard claims like email in the id_token and serves them only from /oauth2/userinfo; that's consistent with OIDC Core §5.4, which places scope-requested claims in the UserInfo endpoint for flows that issue an access token. So no planner-side scope change can fix this — restoring the claims via customIdTokenClaims (this PR) is the library's intended path. There is one alternative that keeps the auth app untouched: a second HTTP call from the planner to the UserInfo endpoint. See the open question below — Till, which direction do you prefer?

Why requesting email in the planner doesn't fix this

The planner already requests email. The codebar strategy asks for openid profile email in the authorization request (codebar.rb L36), and the auth app's provider allows exactly those scopes (auth.js L82). The initializer (omniauth.rb L16-L19) carries no scope because the strategy owns that — unlike the old GitHub provider, which declared scope: 'user:email' at config level.

The request isn't the problem. What better-auth does with a granted email scope is the problem, and that's decided entirely provider-side.

What better-auth 1.7.x does with the id_token

Since 1.7, @better-auth/oauth-provider deliberately zeroes every standard OIDC claim name in the id_token payload before anything else runs — token.ts L73-L81, spread into the payload at L375. Scope-gated resolution of email/name from the user record (standard-claims.ts L42-L55) happens only in userNormalClaims (userinfo.ts L31-L41), which feeds the UserInfo endpoint, not the id_token. There is no code path in 1.7.7 where a granted email scope adds email to the id_token; the only sanctioned source is customIdTokenClaims or a claims extension, and we have none configured apart from this PR's.

This is a deliberate redesign, motivated by issue #7864

#7864: in earlier versions, auto-derived standard claims were spread after customIdTokenClaims, overwriting operator values. 1.7 fixed that by reserving standard claim names entirely via the guards above. Side effect: the id_token became sparse unless the operator supplies the claims — which is what this PR restores. Notably, the v1.7.0 release notes list no breaking change for @better-auth/oauth-provider, so nothing flagged this to anyone upgrading.

Is 1.7's behaviour a spec violation? No.

OIDC Core §5.4 specifies where scope-requested claims land:

The Claims requested by the profile, email, address, and phone scope values are returned from the UserInfo Endpoint [...] when a response_type value is used that results in an Access Token being issued. However, when no Access Token is issued (which is the case for the response_type value id_token), the resulting Claims are returned in the ID Token.

We use the authorization code flow, so an access token is issued — and §5.4 puts email/email_verified in the UserInfo endpoint for that flow. §5.1 leaves the location to the OP ("either in the UserInfo Response [...] or in the ID Token"). Better-auth 1.7 implements the spec's baseline; 1.6's id_token embedding was a superset. Support for the OIDC claims parameter is optional (§5.5), and better-auth's implementation only accepts acr for id_token claims requests, so the planner can't request the claim that way either.

Why the fix belongs in the auth app

customIdTokenClaims is the library's intended extension point for operator-controlled identity claims — its own source calls it "the deliberate first-party override path for the operator's own customIdTokenClaims, which is trusted to override identity claims". This PR uses exactly that (id-token-claims.js L30-L32), so it isn't a workaround around the library — it's the library's design.

Open question: the second-call approach

There is one route that keeps the auth app untouched. The spec-consistent way for the planner to get email in the code flow is to call the UserInfo endpoint with the access token it already receives in the token response — that response is scope-gated by userNormalClaims (userinfo.ts L31-L41), so with the email scope granted it returns email and email_verified (standard-claims.ts L52-L55).

Concretely, callback_phase in the strategy would make a second GET {auth_url}/api/auth/oauth2/userinfo with Authorization: Bearer <access_token> after the code exchange, and resolve identity from that response instead of (or in addition to) the id_token claims.

Trade-offs to weigh:

  • One extra HTTP round trip per sign-in on the planner's critical path, inside the strategy's 5-second open/read timeouts — a slow or failing auth app would now block sign-in even when the id_token exchange succeeded.
  • The userinfo endpoint has its own recent rough edges upstream — better-auth#11193 (500s when user.name is null) sits on the same claim-resolution path we'd be depending on.
  • It doesn't remove the need for this PR unless we also drop the id_token as the identity source entirely; github_id and sub would still need somewhere to come from, and the planner's || payload['sub'] fallback would still need hardening separately.

@till — would you prefer this direction? If the second call is acceptable, we could revert the auth change and do the UserInfo lookup in the strategy instead. If not, this PR stands as the fix and the scope-visibility improvement in the initializer is a small follow-up.

Follow-ups worth considering

  • Add a scope option to the codebar strategy and set it in the initializer, so the config reads like the old GitHub provider did. Cosmetic; the strategy already sends the scopes.
  • File an upstream issue asking better-auth to document the id_token change and/or offer an opt-in for scope-gated standard claims in the id_token.
  • Harden the planner's payload['email'] || payload['sub'] fallback — separate planner change, already noted as deliberately out of scope here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants