Upstream refresh tokens orphaned on re-authorization

[lorr1]

Summary

Upstream refresh tokens are orphaned when a user re-authorizes. Two independent mechanisms cause this: (1) storage is keyed by an ephemeral session ID that is freshly generated on every /authorize call, and (2) the callback stores the new IDP response verbatim, overwriting a previously-stored refresh token with the empty string when the provider does not re-issue one.

The second mechanism is most visible with providers that only return refresh_token on first consent or when prompt=consent is explicitly passed (Google OAuth 2.0 is a prominent example). After any re-authorization, the stored refresh token is effectively lost, and subsequent refresh attempts fail with ErrNoRefreshToken.

Root cause A — session-keyed storage

pkg/authserver/server/handlers/authorize.go:91 generates a fresh session ID on every /authorize:

pending := &storage.PendingAuthorization{
    ...
    SessionID: rand.Text(),   // ← fresh random on every /authorize
    CreatedAt: time.Now(),
}

pkg/authserver/server/handlers/callback.go:142 stores upstream tokens keyed by that session. The session ID is then embedded as the tsid claim in the issued JWT (pkg/authserver/server/session/session.go:120-123), and downstream read paths look up by tsid/session ID (pkg/auth/token.go:1189-1196, pkg/authserver/storage/types.go:222-240).

UserID (the internal ToolHive user subject) is stored as a field on UpstreamTokens (callback.go:138) but no read path uses it as a lookup key. It is only used for cleanup/metadata.

Consequence: any re-authorization mints a new session; the old session's tokens become unreachable through the normal read path.

Root cause B — callback clobbers prior refresh token

pkg/authserver/server/handlers/callback.go:131-142 stores idpTokens.RefreshToken verbatim, with no preservation of a previously-stored refresh token:

storageTokens := &storage.UpstreamTokens{
    ProviderID:   providerID,
    AccessToken:  idpTokens.AccessToken,
    RefreshToken: idpTokens.RefreshToken,   // empty if provider didn't re-issue
    ...
}
if err := h.storage.StoreUpstreamTokens(ctx, sessionID, providerID, storageTokens); err != nil {

pkg/authserver/refresher.go:73-75 already has the correct pattern for the refresh path (preserve prior refresh token when provider omits a new one). The callback path does not mirror this.

Even if root cause A is fixed (e.g., switching to UserID-keyed storage), root cause B will still overwrite a good refresh token with the empty string on every re-auth where the provider does not re-issue a refresh token.

pkg/auth/upstreamtoken/service.go:135-136 then returns ErrNoRefreshToken:

if expired.RefreshToken == "" {
    return nil, ErrNoRefreshToken
}

Failure mode

1. Initial consent completes. Upstream tokens, including a refresh_token, are stored under session S1 with key (S1, providerID).
2. Some time later, a new /authorize call is made (any reason — including the compound effect of Non-expiring upstream tokens incorrectly marked as 1h-expiring #5046 causing spurious refresh/reauth cycles).
3. Authorize handler mints a new session S2. The new JWT's tsid claim points at S2.
4. Callback completes. If the provider does not re-issue refresh_token (e.g., Google without prompt=consent — pkg/authserver/upstream/oidc.go:367-369 shows prompt=consent is optional, not default), the callback stores {AccessToken: new, RefreshToken: ""} at (S2, providerID).
5. When the new access token expires, the refresher reads from (S2, providerID), finds RefreshToken == "", and returns ErrNoRefreshToken. The provider is then omitted from downstream responses with the log line omitting provider with unrefreshable expired token (pkg/auth/upstreamtoken/service.go:114).
6. The user is forced to re-authorize. The cycle repeats.

Proposed fix

Two independent changes — neither alone is sufficient.

1. Key upstream tokens by UserID, not session ID.

   • Add a (user_id, provider_id) → tokens index in both memory and Redis storage backends.
   • Update the downstream read path (pkg/auth/token.go:1189-1196 and friends) to resolve tsid → user_id via the session store, then read upstream tokens by user_id.
   • This is net-new index work; UserID is currently only metadata.

2. Preserve prior refresh token in the callback. Before calling StoreUpstreamTokens, load any existing record for (user_id, provider_id) and, if idpTokens.RefreshToken == "", carry forward the existing RefreshToken. Mirror the pattern already used at pkg/authserver/refresher.go:73-75.

Related

Compounds with #5046. The fake 1h expiry described there triggers the re-authorization cascade that exposes this bug.

Reproduction

Assumes a Google OAuth client (OIDC) is configured as a ToolHive upstream with the default config (no prompt=consent set).

1. Start ToolHive with Google configured as an upstream OIDC provider.

2. As a user, complete the initial /authorize flow. Accept Google's consent screen.

3. Verify the stored upstream tokens include a non-empty RefreshToken. For the in-memory backend this can be inspected via the authserver's storage; for Redis, read the upstream_tokens:<session_id>:google key.

4. Trigger a second /authorize for the same Google account, without the initial session being invalidated. This can be done by:

   • Calling the OAuth endpoint again from a fresh client registration, or
   • Signing out of the ToolHive client and signing back in, or
   • Any path that causes the client to restart the authorization flow.

5. Complete the flow. Google returns a new access token but no refresh_token (this is Google's documented behavior without prompt=consent).

6. Inspect the stored tokens under the new session's key: RefreshToken is now "". The previous session's tokens (with the good refresh token) still exist under the old session key but are unreachable via the new JWT's tsid.

7. Wait for the new access token to expire (~1 hour with Google's default).

8. Trigger a downstream request that causes InProcessService.GetAccessTokens to run. Observe the log line:

   omitting provider with unrefreshable expired token provider=google
   

9. The Google upstream is now omitted from downstream responses until the user re-authorizes — which will reproduce the same failure.

Caveats

The claim "Google does not re-issue refresh tokens without prompt=consent" reflects Google's documented OAuth 2.0 behavior. The repo only shows prompt=consent is optional (pkg/authserver/upstream/oidc.go:367-369).

[tgrunnagle]

Plan — Root Cause A: User-Keyed Upstream Token Storage

Context

Upstream OAuth tokens are stored at the composite key (sessionID, providerID), where sessionID is freshly generated on every /authorize (pkg/authserver/server/handlers/authorize.go:91). The same sessionID is embedded as the tsid JWT claim (pkg/authserver/server/session/session.go:120-123), and the downstream read path looks tokens up by tsid (pkg/auth/token.go:1189-1196). Re-authorization mints a new session, so previous tokens become unreachable. UserID is already on UpstreamTokens (callback.go:138) but no read path uses it as a key.

This plan rekeys upstream-token storage by (userID, providerID). Root Cause B is not addressed here — it lands in a separate PR.

Design Decisions

1. Read path uses the JWT sub claim directly (not a session-store lookup). sub is already populated as the ToolHive userID by session.New (line 143). The JWT signature authenticates the claim — no separate storage round-trip needed.
2. Per-(user, provider) delete is added for partial-failure rollback. A bulk DeleteUpstreamTokens(userID) would wipe a user's good tokens on a re-auth that fails midway through a multi-upstream chain.
3. One-time Redis migration in redis_migrate.go rewrites legacy upstream:{sessionID}:{providerID} keys under upstream:{userID}:{providerID} using the embedded UserID field.

Files to Modify

Storage layer

• pkg/authserver/storage/types.go:217-255 — change UpstreamTokenStorage and UpstreamTokenRefresher parameter names from sessionID to userID. Add DeleteUpstreamTokensByProvider(ctx, userID, providerID) error. Update interface doc comment.
• pkg/authserver/storage/memory.go:36-39, 693-833 — rename the upstreamKey field from sessionID to userID; rename method parameters; add DeleteUpstreamTokensByProvider.
• pkg/authserver/storage/redis_keys.go — change primary key format to upstream:{userID}:{providerID}. The existing user:upstream:{userID} SET is repurposed as the per-user provider index. The upstream:idx:{sessionID} SET is dropped.
• pkg/authserver/storage/redis.go:818-1068 — update the storeUpstreamTokensScript Lua script to maintain user:upstream:{userID} directly. Update Get/GetAll/Delete and add DeleteUpstreamTokensByProvider. GetAll uses SMEMBERS on user:upstream:{userID} then MGET.
• pkg/authserver/storage/redis_migrate.go — add migrateUpstreamTokensToUserKey(ctx, client, prefix) following the existing migrateSingleUpstreamToken pattern (lines 136-230). SCAN for upstream:*:* keys (excluding idx: and user:upstream:*), parse stored token, re-SET under upstream:{UserID}:{ProviderID}, SADD to user:upstream:{UserID}, DEL legacy key + legacy upstream:idx:{sessionID} set. Idempotent. Wire into the migration entrypoint at storage init.

Callback handler

• pkg/authserver/server/handlers/callback.go:142 — pass subject (resolved userID) instead of sessionID to StoreUpstreamTokens.
• pkg/authserver/server/handlers/callback.go:147, 286, 322, 335, 346, 355, 390, 404, 412 — replace each bulk DeleteUpstreamTokens(ctx, sessionID) cleanup with DeleteUpstreamTokensByProvider(ctx, subject, providerID) for the providers actually stored in this in-flight chain. Where rolling back a chain, iterate the chain's stored providers (track locally as it progresses) and delete each.
• pkg/authserver/server/handlers/callback.go:332-340 — the defense-in-depth identity-consistency check via GetAllUpstreamTokens(sessionID) becomes a tautology under user-keyed storage. Drop the UserID comparison; keep the bulk read only if needed for chain accounting.

Refresher

• pkg/authserver/refresher.go:31-94 — RefreshAndStore no longer needs the sessionID parameter; use expired.UserID for both logging and StoreUpstreamTokens. Update interface signature in pkg/authserver/storage/types.go:250-255.

Service layer

• pkg/auth/upstreamtoken/service.go:51-174 — rename the sessionID parameter to userID throughout (GetValidTokens, GetAllValidTokens, refreshOrFail). Update the singleflight key (line 147) to use userID. Update log fields (session_id → user_id). Drop the now-unused sessionID parameter on RefreshAndStore (line 153).
• pkg/auth/upstreamtoken/types.go — update the Service and TokenReader interface signatures.

Read path (token validator)

• pkg/auth/token.go:1184-1200 — loadUpstreamTokens extracts sub instead of tsid:

  sub, ok := claims["sub"].(string)
  if !ok || sub == "" {
      return nil, nil
  }
  tokens, err := v.upstreamTokenReader.GetAllValidTokens(ctx, sub)

  Update doc comment and the error-wrap message. The tsid claim itself stays in the JWT (no JWT-structure change) — it's just no longer the upstream-token lookup key.

Tests

Mechanical updates — change sessionID literals/parameter values to userID in:

• pkg/authserver/storage/memory_test.go — TestMemoryStorage_UpstreamTokens (lines 429-580).
• pkg/authserver/storage/redis_test.go — TestRedisStorage_UpstreamTokens (lines 572-687).
• pkg/authserver/storage/redis_integration_test.go — TestIntegration_UpstreamTokens, TestIntegration_MigrateLegacyUpstreamData. Add a new integration test exercising migrateUpstreamTokensToUserKey against legacy data.
• pkg/auth/upstreamtoken/service_test.go — TestGetValidTokens, TestGetAllValidTokens.
• pkg/auth/token_test.go — TestTokenValidator_loadUpstreamTokens, TestTokenValidator_Middleware. Update fixtures to put userID in sub and assert lookup by sub.
• pkg/authserver/refresher_test.go — drop sessionID parameter from RefreshAndStore calls.
• pkg/authserver/integration_test.go — TestAuthorizeWithUpstreamTokenRefresh, TestUpstreamTokenRefreshForwardedToProxyMiddleware, TestSessionIDPreservedAcrossAuthorizationCodeRefresh, TestMultiUpstreamAuthorizationChain.
• pkg/vmcp/auth/factory/incoming_upstream_test.go — TestNewIncomingAuthMiddleware_OIDC_UpstreamTokens.
• pkg/authserver/server/handlers/callback_test.go — assertions that read by sessionID.
• Mocks — regenerate via task gen after interface signatures change.

New regression test: in integration_test.go, add TestReAuthorizationPreservesAccessToUpstreamTokens that:

1. Completes initial /authorize for user U with provider P; asserts tokens accessible.
2. Triggers a second /authorize for U (mints a new sessionID/JWT).
3. Asserts the new JWT's read path returns the same upstream tokens (not orphaned).

This guards Root Cause A and should fail on main.

What This Plan Does NOT Change

• JWT structure: tsid claim stays for now (used elsewhere for session correlation/logs); just no longer the lookup key. Removing tsid would be a separate cleanup PR.
• Root Cause B: callback still overwrites RefreshToken with whatever the IdP returned. After this fix, re-auth where the IdP omits refresh_token will overwrite a good refresh token in the user-keyed slot — same orphaning end-state, different mechanism. Must be followed by a Root Cause B PR.
• Session-level revocation: a tsid-based revocation mechanism, if/when added, is a separate concern from upstream-token keying.

Verification

1. `task lint-fix && task test` — unit tests pass, including the new `TestReAuthorizationPreservesAccessToUpstreamTokens` regression.
2. Storage integration tests — confirms `migrateUpstreamTokensToUserKey` is idempotent and handles legacy data.
3. Manual stub-upstream check:
   • Start authserver with the in-memory backend.
   • Complete `/authorize` for user U → upstream tokens stored with non-empty refresh.
   • Trigger a second `/authorize` for U.
   • Make a downstream request with the new JWT; assert `Identity.UpstreamTokens` is non-empty (fails on `main`).
4. Redis path: pre-populate Redis with legacy-format keys, run authserver init, confirm migration rewrites them and reads via the new code path succeed.

Risks / Open Items

• Mock regeneration: `task gen` must run after interface changes. Mocks live under `pkg/authserver/storage/mocks` and `pkg/auth/upstreamtoken/mocks` (verify locations during implementation).
• Migration ordering: Redis migration must run before new code starts serving reads. Confirm the migration entrypoint is invoked at storage init and is gated correctly under multiple replicas (one wins, others skip — see existing migration's locking pattern).
• Logging fields: many `slog` calls reference `session_id`. Switching to `user_id` is a breaking change for any log-based dashboards. Worth a note in the PR description.

[tgrunnagle]

Plan — Root cause B: Callback clobbers prior refresh token

Context

Per issue #5047, upstream refresh tokens are orphaned on re-authorization. Two independent root causes are described; this plan addresses root cause B only: the OAuth callback at pkg/authserver/server/handlers/callback.go:131-142 stores idpTokens.RefreshToken verbatim. When the upstream IDP omits refresh_token on a re-authorization (e.g., Google without prompt=consent), the new record is written with RefreshToken: "", overwriting the previously-stored good refresh token. Subsequent refresh attempts then fail with ErrNoRefreshToken and the user is forced to re-authorize again.

The refresher already has the correct preservation pattern at pkg/authserver/refresher.go:73-75 — the callback must mirror it. The catch: the callback's sessionID is freshly generated on every /authorize (root cause A), so a same-key GetUpstreamTokens(sessionID, providerID) lookup will not find the prior record. The fix needs a (userID, providerID)-keyed lookup.

This is a partial-but-valuable fix on its own: even with root cause A unresolved, carrying the refresh token forward to the new sessionID lets the refresher succeed when the new access token expires, ending the re-auth cycle.

Approach

1. Add a small, targeted storage method that returns the most recent prior upstream tokens for (userID, providerID) across any session.
2. In the callback, when idpTokens.RefreshToken == "", load the prior record and copy its RefreshToken into the new storage.UpstreamTokens before storing.
3. Mirror the defensive pattern from refresher.go — only fall back when the new value is empty.

The new method also lays usable groundwork for a future root cause A fix (user-keyed read path) without committing the design.

Files to change

1. pkg/authserver/storage/types.go

Add a new method on UpstreamTokenStorage:

// GetLatestUpstreamTokensForUser returns the most recently stored upstream
// tokens for (userID, providerID) across any session.
//
// Used by the OAuth callback to preserve a previously-issued refresh token
// when the IDP omits refresh_token on re-authorization (e.g., Google without
// prompt=consent). Mirrors the preservation pattern in
// upstreamTokenRefresher.RefreshAndStore.
//
// Returns ErrNotFound if no record exists for the (userID, providerID) pair.
GetLatestUpstreamTokensForUser(ctx context.Context, userID, providerID string) (*UpstreamTokens, error)

2. pkg/authserver/storage/memory.go

Implement via linear scan of s.upstreamTokens filtered by UserID == userID && ProviderID == providerID, returning the entry with the latest createdAt. This mirrors the existing scan pattern at memory.go:1012-1015. Return ErrNotFound when no match. Use s.mu.RLock().

3. pkg/authserver/storage/redis.go

Leverage the existing KeyTypeUserUpstream:{userID} set (already populated by StoreUpstreamTokens at redis.go:925):

• SMEMBERS on redisSetKey(s.keyPrefix, KeyTypeUserUpstream, userID) to get all provider keys for the user.
• Filter keys by suffix :{providerID} (key format is {prefix}upstream:{sessionID}:{providerName}).
• MGET the matching keys; unmarshal via existing unmarshalUpstreamTokens (tolerate ErrExpired — we still want the refresh token).
• Pick the entry with the latest ExpiresAt as a stable proxy for "most recent" (no createdAt is persisted in Redis).
• Return ErrNotFound when no match.

Tolerate stale entries in the index per the warnOnCleanupErr contract at redis.go:35-50.

4. pkg/authserver/storage/mocks/mock_storage.go

Regenerate via task gen after the interface change.

5. pkg/authserver/server/handlers/callback.go

Insert preservation logic between callback.go:128 and callback.go:131 (after subject is resolved, before storageTokens is built):

refreshToken := idpTokens.RefreshToken
if refreshToken == "" {
    if prior, err := h.storage.GetLatestUpstreamTokensForUser(ctx, subject, providerID); err == nil &&
        prior != nil && prior.UpstreamSubject == providerSubject {
        refreshToken = prior.RefreshToken
    }
}

Then use refreshToken in the storage.UpstreamTokens literal. The defensive prior.UpstreamSubject == providerSubject check guards against carrying a refresh token across distinct IDP identities — a no-op today (userResolver.ResolveUser makes subject already imply upstreamSubject) but it costs nothing and matches the binding-validation pattern in types.go:66-76.

A slog.Debug line on successful carry-forward helps diagnose this in the field. No slog.Warn on the lookup error: a missing prior record is the common case (first-ever auth), not a problem.

6. pkg/authserver/server/handlers/helpers_test.go

Wire up the new method in the handler test storage mock (add a DoAndReturn clause that scans storState.upstreamTokens by UserID/ProviderID).

7. Tests

• pkg/authserver/storage/memory_test.go — table-driven test for GetLatestUpstreamTokensForUser: empty, one match, multiple matches across sessions (assert latest), userID/providerID mismatch returns ErrNotFound.
• pkg/authserver/storage/redis_integration_test.go — equivalent integration test using the existing miniredis fixture.
• pkg/authserver/server/handlers/callback_test.go — new test TestCallbackHandler_PreservesPriorRefreshToken:
  • Pre-populate storState.upstreamTokens with (oldSession, providerID) carrying RefreshToken: "rt-prior" and matching UserID/UpstreamSubject.
  • Configure mockUpstream.exchangeResult.Tokens.RefreshToken = "".
  • Use a different pending.SessionID for the callback.
  • Run callback, assert the new (newSession, providerID) record has RefreshToken == "rt-prior".
• A negative-case test where the prior record's UpstreamSubject differs — assert we do NOT carry forward.

Out of scope

• Root cause A (session-keyed storage): not addressed here. After this fix, the prior session's tokens are still orphaned by the JWT tsid lookup path — but the new session's record now has a usable refresh token, so the refresh path succeeds.
• Adding prompt=consent to Google upstream config — separate decision.
• Issue Non-expiring upstream tokens incorrectly marked as 1h-expiring #5046 (spurious refresh/reauth cycles that compound this bug).

Verification

• task lint-fix
• task gen (regenerate mocks after interface change)
• task test — full unit suite, in particular:
  • go test ./pkg/authserver/storage/... (memory + redis)
  • go test ./pkg/authserver/server/handlers/... (callback)
• Manual reproduction following the issue's repro steps (Google upstream, default config), confirming that after step 6, the new session's stored RefreshToken is the carried-forward value rather than "", and that step 8's omitting provider with unrefreshable expired token log line no longer appears.

[jhrozek]

There must have been a bad linking, this is not completely fixed.