Skip to content

fix(authserver): scope upstream-token callback cleanup to the chain's providers #6476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tgrunnagle wants to merge 4 commits into
base: main
Choose a base branch
from
+520 −44
Open

fix(authserver): scope upstream-token callback cleanup to the chain's providers #6476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a479f1
fix(authserver): scope upstream-token callback cleanup to the chain's…
tgrunnagle Aug 31, 2026
a5d4e6c
fix(authserver): guard per-provider cleanup and polish its docs
tgrunnagle Aug 31, 2026
9c4e1a7
test(authserver): cover the per-provider cleanup paths
tgrunnagle Aug 31, 2026
ac6b820
fix(authserver): scope callback cleanup to the current leg, not the w…
tgrunnagle Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 84 additions & 20 deletions pkg/authserver/server/handlers/callback.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,8 @@ func (h *Handler) CallbackHandler(w http.ResponseWriter, req *http.Request) {

// Place the resolved canonical user in the request context so callback storage
// calls that carry no tokens argument — GetAllUpstreamTokens during chain
// consistency, DeleteUpstreamTokens on cleanup — can resolve the user from
// context. We use WithPlatformUser, not WithIdentity: no ToolHive bearer has
// consistency, DeleteUpstreamTokensForProvider on cleanup — can resolve the user
// from context. We use WithPlatformUser, not WithIdentity: no ToolHive bearer has
// been issued at the callback, so there is no authenticated identity to assert —
// only the canonical user for storage keying. (StoreUpstreamTokens does not need
// this; it keys off tokens.UserID below.)
Expand Down Expand Up @@ -182,8 +182,10 @@ func (h *Handler) CallbackHandler(w http.ResponseWriter, req *http.Request) {
slog.Error("failed to store upstream tokens",
Comment thread
tgrunnagle marked this conversation as resolved.
"error", err,
)
// Clean up any tokens stored by earlier legs of a multi-upstream chain.
_ = h.storage.DeleteUpstreamTokens(ctx, sessionID)
// Clean up only this leg's token (see cleanupUpstreamTokens): its store just
// failed, so this removes any partial or carried-forward row and is otherwise a
// no-op. Earlier legs are the user's own valid tokens and are left intact.
h.cleanupUpstreamTokens(ctx, sessionID, []string{providerID})

@jhrozek jhrozek Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

blocker: A failed StoreUpstreamTokens call does not prove that this attempt created a row, so the subsequent delete can erase the previous valid credential under gateway/user keying. The carried-forward token exists only in the local value until the store succeeds, and the built-in Redis write is atomic. Please remove cleanup from this store-error path and add a regression test that seeds an existing current-provider token, injects a store failure, and verifies the existing token remains unchanged.

h.provider.WriteAuthorizeError(ctx, w, ar, fosite.ErrServerError.WithHint("failed to store session"))
return
}
Expand Down Expand Up @@ -398,19 +400,20 @@ func (h *Handler) handleUpstreamError(
pending, err := h.storage.LoadPendingAuthorization(ctx, internalState)
if err == nil {
_ = h.storage.DeletePendingAuthorization(ctx, internalState)
// Clean up any upstream tokens stored by earlier legs of a multi-upstream chain.
// On a subsequent leg the resolved user is carried in pending.ResolvedUserID;
// place it in ctx via WithPlatformUser so a context-keyed storage decorator can
// resolve the canonical user for this delete (DeleteUpstreamTokens takes no tokens
// argument). WithPlatformUser, not WithIdentity: there is no authenticated identity
// at the callback, only the storage-scoped user. A first-leg error has no resolved
// user and no earlier-leg tokens to clean up, so the bare ctx is correct there.
// Clean up only this leg's token (see cleanupUpstreamTokens); earlier legs are
// the user's own valid tokens and are left intact. This leg failed at the
// upstream IdP before a token was stored, so the delete is normally a no-op,
// but it also removes a row left by a prior attempt on the same leg. On a
// subsequent leg the resolved user is carried in pending.ResolvedUserID; place
// it in ctx via WithPlatformUser so a context-keyed storage decorator resolves
// the canonical user for the cleanup. WithPlatformUser, not WithIdentity: there
// is no authenticated identity at the callback, only the storage-scoped user.
if pending.SessionID != "" {
cleanupCtx := ctx
if pending.ResolvedUserID != "" {
cleanupCtx = auth.WithPlatformUser(ctx, pending.ResolvedUserID)
}
_ = h.storage.DeleteUpstreamTokens(cleanupCtx, pending.SessionID)
h.cleanupUpstreamTokens(cleanupCtx, pending.SessionID, []string{pending.UpstreamProviderName})
Comment on lines 411 to +416

@jhrozek jhrozek Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

blocker: This callback branches here before exchanging or storing a token for the current attempt. Under the gateway/user-keyed storage model introduced by this PR, deleting the pending upstream provider can therefore remove an existing credential from an earlier successful authorization—for example, canceling a Slack reconnection would disconnect the already-connected Slack account. Please limit cleanup here to the single-use pending authorization and add a regression test that seeds an existing current-provider token and verifies it survives error=access_denied.

}
ar := h.buildAuthorizeRequesterFromPending(ctx, pending)
if ar != nil {
Expand All @@ -428,6 +431,53 @@ func (h *Handler) handleUpstreamError(
http.Error(w, "upstream authentication failed", http.StatusBadGateway)
}

// cleanupUpstreamTokens best-effort removes upstream tokens on a failed authorization
// leg, one provider at a time. Callers pass the leg currently being processed, so an
// ordinary error — a transient store/read failure, or a chain that cannot be resolved —
// takes down only that leg's token and leaves the user's earlier legs intact. That
// matters under a storage decorator that re-keys upstream tokens by (gateway, user),
// where each earlier leg is the user's own live credential for another connector rather
// than throwaway session state, so a failed connector login must not delete the others.
// The one exception is the identity-mismatch fail-closed, which passes the whole chain
// to tear down a session whose stored identity is no longer trustworthy.
//
// It deletes per provider rather than calling the session-wide DeleteUpstreamTokens
// because that method carries no provider name: a (gateway, user)-keyed decorator cannot
// narrow a session-wide delete back to one leg and would delete every connector token
// the user holds on the gateway. Naming the provider lets such a decorator scope the
// delete to exactly the named leg(s).
//
// Each name is checked against the configured upstream set before it is deleted, so a
// name that is not a configured upstream — e.g. from a corrupted or tampered pending
// row reaching a pre-resolveChain cleanup site, which unlike the post-resolve sites has
// not been through validateChain — is skipped rather than turned into a delete.
//
// providers is the set of provider names whose upstream-token rows should be removed;
// empty entries and names outside the configured upstream set are skipped, and deleting
// an already-absent row is a no-op (the storage contract makes a missing-row delete
// non-fatal), so a repeated name is harmless. Errors are logged and swallowed — cleanup
// is best-effort and must not mask the original failure.
func (h *Handler) cleanupUpstreamTokens(ctx context.Context, sessionID string, providers []string) {
for _, provider := range providers {
if provider == "" {
continue
}
// Only a configured upstream can legitimately have a stored token, so skip
// anything else: it keeps a tampered pending row reaching a pre-resolveChain
// cleanup site from naming an arbitrary delete target. The post-resolveChain
// sites pass a validateChain-checked chain, so this is a no-op for them.
if _, ok := h.upstreamByName(provider); !ok {
continue
}
if err := h.storage.DeleteUpstreamTokensForProvider(ctx, sessionID, provider); err != nil {
Comment thread
tgrunnagle marked this conversation as resolved.
slog.Warn("failed to clean up upstream token for provider", //nolint:gosec // G706 - provider name from server-side chain state
"provider", provider,
"error", err,
)
}
}
}

// continueChainOrComplete checks whether all upstream providers in the authorization
// chain have been satisfied. If so, it issues the authorization code and redirects
// to the client. If not, it redirects to the next upstream provider to continue
Expand Down Expand Up @@ -476,32 +526,43 @@ func (h *Handler) continueChainOrComplete(
chain, err := h.resolveChain(ctx, pending, principal)
if err != nil {
slog.Error("failed to resolve upstream chain", "error", err)
_ = h.storage.DeleteUpstreamTokens(ctx, sessionID)
// Clean up only this leg's token; earlier legs are the user's own valid tokens
// and are left intact. This is the issue's most-reachable trigger — a transient
// filter/Directory failure on leg 1 — so scoping it to the one leg is what keeps
// a blip from taking the user's other connectors down with it.
h.cleanupUpstreamTokens(ctx, sessionID, []string{pending.UpstreamProviderName})
h.provider.WriteAuthorizeError(ctx, w, ar, fosite.ErrServerError.WithHint("failed to determine authorization chain"))
return
}

nextProvider, err := h.nextMissingUpstream(ctx, sessionID, chain)
if err != nil {
slog.Error("failed to determine next upstream", "error", err)
_ = h.storage.DeleteUpstreamTokens(ctx, sessionID)
// Clean up only this leg's token; earlier legs are left intact.
h.cleanupUpstreamTokens(ctx, sessionID, []string{pending.UpstreamProviderName})
h.provider.WriteAuthorizeError(ctx, w, ar, fosite.ErrServerError.WithHint("failed to check authorization chain state"))
return
}

if nextProvider == "" {
if err := h.verifyChainIdentity(ctx, sessionID, chain, subject); err != nil {
// verifyChainIdentity already logged the specific cause (with structured
// fields for a mismatch); here we just clean up and fail closed.
_ = h.storage.DeleteUpstreamTokens(ctx, sessionID)
// fields for a mismatch). This is the one cleanup site that tears down the
// WHOLE chain rather than just this leg: a detected identity mismatch means
// the session's stored state is no longer trustworthy, so we fail closed and
// remove every leg's token rather than leave a possibly cross-keyed row.
h.cleanupUpstreamTokens(ctx, sessionID, chain)
Comment on lines 548 to +554

@jhrozek jhrozek Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

blocker: verifyChainIdentity returns an error for both a confirmed identity mismatch and a transient GetAllUpstreamTokens failure, but this caller deletes the whole chain for either result. The callback reads chain state once in nextMissingUpstream and again here, so the first read can succeed while the second fails temporarily, causing all valid connector credentials to be deleted. Please distinguish the mismatch error and reserve whole-chain cleanup for that case; storage failures should still fail authorization but preserve the tokens. Add a test where the first bulk read succeeds and the second fails.

h.provider.WriteAuthorizeError(ctx, w, ar, fosite.ErrServerError.WithHint("identity verification failed"))
return
}

// All upstreams satisfied — issue authorization code
if err := h.writeAuthorizationResponse(ctx, w, pending, sessionID, subject, name, email); err != nil {
slog.Error("failed to create authorization response", "error", err)
_ = h.storage.DeleteUpstreamTokens(ctx, sessionID)
// The chain was fully satisfied and identity-verified; only the response
// write failed (a retryable server error). Clean up just this final leg and
// leave the earlier legs, so a retry re-collects only this one.
h.cleanupUpstreamTokens(ctx, sessionID, []string{pending.UpstreamProviderName})

@jhrozek jhrozek Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

blocker: At this point the final upstream credential was exchanged and stored successfully, the chain is complete, and identity verification passed. A failure in writeAuthorizationResponse concerns ToolHive's downstream client lookup/redirect parsing/code generation and does not invalidate that credential. The single-leg branch explicitly preserves tokens for the same failure class, while this branch deletes the final provider. Please make the multi-leg behavior consistent by preserving all upstream tokens and update the test to assert both providers remain.

h.provider.WriteAuthorizeError(ctx, w, ar, fosite.ErrServerError.WithHint("failed to create authorization code"))
}
return
Expand Down Expand Up @@ -539,7 +600,8 @@ func (h *Handler) continueChainOrComplete(

if err := h.storage.StorePendingAuthorization(ctx, secrets.State, nextPending); err != nil {
slog.Error("failed to store next chain leg", "error", err)
_ = h.storage.DeleteUpstreamTokens(ctx, sessionID)
// Clean up only this leg's token; earlier legs are left intact.
h.cleanupUpstreamTokens(ctx, sessionID, []string{pending.UpstreamProviderName})
h.provider.WriteAuthorizeError(ctx, w, ar, fosite.ErrServerError.WithHint("failed to continue authorization chain"))
return
}
Expand All @@ -553,15 +615,17 @@ func (h *Handler) continueChainOrComplete(
if !ok {
slog.Error("next upstream provider not found", "provider", nextProvider)
_ = h.storage.DeletePendingAuthorization(ctx, secrets.State)
_ = h.storage.DeleteUpstreamTokens(ctx, sessionID)
// Clean up only this leg's token; earlier legs are left intact.
h.cleanupUpstreamTokens(ctx, sessionID, []string{pending.UpstreamProviderName})
h.provider.WriteAuthorizeError(ctx, w, ar, fosite.ErrServerError.WithHint("upstream provider configuration error"))
return
}
nextURL, err := nextUpstream.AuthorizationURL(secrets.State, secrets.PKCEChallenge, authOpts...)
if err != nil {
slog.Error("failed to build next upstream authorization URL", "error", err)
_ = h.storage.DeletePendingAuthorization(ctx, secrets.State)
_ = h.storage.DeleteUpstreamTokens(ctx, sessionID)
// Clean up only this leg's token; earlier legs are left intact.
h.cleanupUpstreamTokens(ctx, sessionID, []string{pending.UpstreamProviderName})
h.provider.WriteAuthorizeError(ctx, w, ar, fosite.ErrServerError.WithHint("failed to build authorization URL"))
return
}
Expand Down
Loading
Loading