fix(authserver): scope upstream-token callback cleanup to the chain's providers - #6476
fix(authserver): scope upstream-token callback cleanup to the chain's providers#6476tgrunnagle wants to merge 4 commits into
Conversation
… providers The /oauth/callback error paths cleaned up "the tokens this authorization collected" by calling the session-wide DeleteUpstreamTokens(ctx, sessionID), which removes every provider row under that session. That is correct for the native (session, provider) storage scheme, where each authorization mints its own session. It is wrong under a storage decorator that re-keys upstream tokens by (gateway, user) so a session-independent caller (e.g. a user-facing UI) can find a user's tokens. Under that scheme every one of a user's connector tokens shares the session slot, so a session-wide delete on any failing login leg deletes the user's entire connector set — a transient Directory/storage blip on the most-traversed leg (chain resolution) wipes a user's whole historical token set, and correlated failures widen that to every user logging in during the window. It fails safe (never grants access) and is largely self-healing, but the availability hit is real. The callback is the only production caller of the session-wide delete, so: - Route all nine callback cleanup sites through a new cleanupUpstreamTokens helper that deletes per provider via DeleteUpstreamTokensForProvider (already on the storage interface, implemented by memory and redis), sourcing the provider list from the chain config — never a storage read, which a broadening decorator cannot scope. Sites with the resolved chain pass it; pre-resolve sites pass pending.ChainUpstreams plus the current leg. - A user-keyed decorator can now scope each delete to exactly this chain's providers, so a connector outside the chain is never touched. Behavior note: a stale subsequent-leg pending with no ChainUpstreams can only name the leg just processed; an earlier leg it cannot name is left to expire by TTL rather than risk a session-wide delete widening to the whole user. This matches the existing abandoned-flow orphan behavior. Adds a regression test asserting an out-of-chain provider's token survives a chain cleanup, and extends the handler mock with a DeleteUpstreamTokensForProvider capture. Signed-off-by: Trey <tgrunnagle@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6476 +/- ##
==========================================
+ Coverage 78.05% 78.13% +0.08%
==========================================
Files 767 767
Lines 74343 74405 +62
==========================================
+ Hits 58025 58133 +108
+ Misses 16313 16267 -46
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tgrunnagle
left a comment
tgrunnagle
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: correctness, security, test-coverage, general-quality
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | Three cleanup call sites bypass validateChain, trusting an unvalidated stored chain |
9/10 | MEDIUM | Fix |
| 2 | Stale comment still names the removed DeleteUpstreamTokens cleanup call |
9/10 | MEDIUM | Fix |
| 3 | StoreUpstreamTokens-failure cleanup path has zero test coverage |
8/10 | MEDIUM | Fix |
| 4 | 3 of 9 cleanupUpstreamTokens call sites are never exercised by a test |
8/10 | MEDIUM | Fix |
| 5 | Context-propagation test never asserts the token was actually deleted | 8/10 | MEDIUM | Fix |
| 6 | (gateway, user) rationale is duplicated at call sites instead of centralized on the doc comment | 7/10 | LOW | Fix |
| 7 | New WARN log lacks the //nolint:gosec // G706 annotation used elsewhere in this file |
7/10 | LOW | Fix |
| 8 | The per-provider WARN-and-continue error path is never tested | 7/10 | LOW | Fix |
| 9 | Mock doesn't replicate the real store's empty-input rejection | 7/10 | LOW | Fix (optional) |
| 10 | Per-provider cleanup adds up to N sequential storage round-trips on error paths | 7/10 | LOW | Fix (optional) |
| 11 | providers param doc describes current callers rather than the parameter's contract |
7/10 | LOW | Fix (optional) |
Overall
This is a well-targeted fix: it replaces 9 call sites of a session-wide upstream-token delete with a per-provider helper, so a storage layer that re-keys tokens more broadly than "one session, one login" can never have a single failing login leg wipe more than that leg's own chain. I traced every modified call site against how ChainUpstreams is populated and validated elsewhere in this package (resolveChain/computeChain/validateChain), and the provider lists constructed at each site are correct for normal operation. The new regression test is genuinely load-bearing — it fails against the pre-fix code path, not just against a mock that happens to agree with the new implementation.
The findings below are refinement, not pushback on the approach. The most substantive one is that three of the nine migrated call sites source their provider list straight from the stored pending.ChainUpstreams/pending.UpstreamProviderName without running it through the same validateChain check every other call site benefits from — low likelihood of triggering without a corrupted pending row, but worth closing for symmetry with the rest of the fix. The test-coverage gaps are the next tier: three call sites, plus the dedup logic (which actually fires in production on every non-first-leg failure), have no test today. The rest is small comment/consistency polish.
Documentation
The PR description's "Does this introduce a user-facing change?" section states "No behavior change for the native single-session storage scheme." That's very slightly overbroad: in the narrow edge case already flagged in "Special notes for reviewers" (a stale pending missing ChainUpstreams), native storage now also leaves an earlier leg's token alive until TTL instead of deleting it immediately — not only under a re-keying storage layer. Worth a one-line tightening of that claim; no code change implied.
Generated with Claude Code
Addresses #6476 review comments: - MEDIUM callback.go (3897108470): skip provider names that are not a configured upstream inside cleanupUpstreamTokens, so the three pre-resolveChain sites that pass an unvalidated stored chain can no longer name an arbitrary delete target; a no-op for the post-resolveChain sites, which already pass a validateChain-checked chain. - MEDIUM callback.go (3897108480): the WithPlatformUser comment now names DeleteUpstreamTokensForProvider instead of the removed session-wide delete. - LOW callback.go (3897108524): trim the (gateway, user) rationale duplicated at the two call sites down to their call-site nuance; the shared rationale lives on the cleanupUpstreamTokens doc comment. - LOW callback.go (3897108537): annotate the cleanup WARN with //nolint:gosec // G706, matching the other server-storage-derived logs in this file. - LOW callback.go (3897108558): note the per-provider round-trip cost (bounded to the chain length, error-path only) on the doc comment. - LOW callback.go (3897108562): state the providers parameter's contract rather than what today's callers happen to pass. Signed-off-by: Trey <tgrunnagle@gmail.com>
Addresses #6476 review comments: - MEDIUM callback.go (3897108490): add withStoreUpstreamTokensError and a subsequent-leg store-failure test, exercising the one cleanup site where the dedup in cleanupUpstreamTokens is load-bearing (ChainUpstreams already contains the current provider). - MEDIUM callback.go (3897108499): add tests for the nextMissingUpstream-failure site (GetAllUpstreamTokens error) and the writeAuthorizationResponse-failure site on a fully satisfied chain (GetClient failing only on its second call). The third branch it names — "next upstream provider not found" — is unreachable because resolveChain runs validateChain over the chain before it is walked, so every nextProvider is a configured upstream; covered by reply, not a test. - MEDIUM callback_test.go (3897108506): the OnUpstreamErrorCleanup test now asserts the earlier-leg row was actually deleted, not merely that the ctx was captured. - LOW callback.go (3897108545): add withDeleteUpstreamTokensForProviderError and a test proving cleanup continues past a per-provider delete failure to remove sibling providers. - LOW helpers_test.go (3897108551): the DeleteUpstreamTokensForProvider mock now mirrors the real backends' rejection of an empty session or provider name. Extracts multiUpstreamProviders and adds multiUpstreamTestSetupWithStorage so the two-upstream setups can thread storage error injection without duplicating the mock providers. Signed-off-by: Trey <tgrunnagle@gmail.com>
github-actions
Bot
added
size/M
…hole chain A failed authorization leg now cleans up only the leg being processed and leaves the user's earlier legs intact, rather than deleting every provider in the chain. Under a storage decorator that re-keys upstream tokens by (gateway, user), each earlier leg is the user's own live credential for another connector, so a transient failure on one login leg must not take the others down with it — most importantly at the chain-resolution site, the issue's most-reachable trigger (a Directory/filter blip on leg 1). The one exception is the identity-mismatch fail-closed (verifyChainIdentity): a detected mismatch means the session's stored state is no longer trustworthy, so that site still tears down the whole chain. - 8 of the 9 cleanup sites now pass just the current leg's provider; the verifyChainIdentity site keeps the whole-chain delete. - Drop the now-dead dedup in cleanupUpstreamTokens: every caller passes a single provider or a validateChain-checked, duplicate-free chain, and a repeated name is a harmless idempotent no-op delete. - Update the affected tests to assert earlier legs survive an ordinary error, and add a direct cleanupUpstreamTokens test covering the skip-unconfigured / skip-empty guard. Signed-off-by: Trey <tgrunnagle@gmail.com>
github-actions
Bot
added
size/M
| 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}) |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
| // 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}) |
There was a problem hiding this comment.
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.
| // 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}) |
There was a problem hiding this comment.
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.
| // A token for a provider that is NOT part of this login's chain, stored under the | ||
| // same session slot — this is what a (gateway, user)-keyed decorator looks like from | ||
| // the backend's side: every connector the user holds shares one session key. | ||
| outOfChainKey := sessionID + ":provider-out-of-chain" |
There was a problem hiding this comment.
suggestion: This test still catches regression to session-wide deletion, but its survivor is not a configured upstream, so it is also protected by cleanupUpstreamTokens's independent skip-unconfigured guard. It would not catch cleanup accidentally widening from the effective chain to every configured provider. Could we configure a third provider, exclude it through the upstream filter, and assert that its token survives? That would directly cover the PR's effective-chain scoping guarantee.
3 participants
Summary
The
/oauth/callbackerror paths clean up "the tokens this authorization collected" by calling the session-wideDeleteUpstreamTokens(ctx, sessionID), which removes every provider row under that session. That is correct for the native(session, provider)storage scheme, where each authorization mints its own session.It is wrong under a storage decorator that re-keys upstream tokens by
(gateway, user)so a session-independent caller (e.g. a user-facing UI) can find a user's tokens. Under that scheme every one of a user's connector tokens shares the session slot, so a session-wide delete on any failing login leg deletes the user's entire connector set. A transient identity-provider/storage blip on the most-traversed leg (chain resolution) wipes a user's whole historical token set, and correlated failures widen that to every user logging in during the window. It fails safe (never grants access) and is largely self-healing, but the availability hit is real.The callback is the only production caller of the session-wide delete. This PR:
cleanupUpstreamTokenshelper that deletes per provider viaDeleteUpstreamTokensForProvider(already on thestorage.Storageinterface, implemented by both memory and redis), sourcing the provider list from the chain config — never a storage read, which a broadening decorator cannot scope. Sites that hold the resolvedchainpass it; pre-resolve sites passpending.ChainUpstreamsplus the leg currently being processed.Behavior note: a stale subsequent-leg pending with no
ChainUpstreamscan only name the leg just processed; an earlier leg it cannot name is left to expire by TTL rather than risk a session-wide delete widening to the whole user. This matches the existing abandoned-flow orphan behavior already noted in the chain-continuation TODO.Type of change
Test plan
task test) — fullpkg/authserver/...suite passestask lint-fix) —golangci-lintclean on the changed packageAdds
TestCallbackHandler_Cleanup_LeavesOutOfChainProviderTokens, which stores a token for a provider outside the login's chain under the same session slot and asserts it survives a chain cleanup (while the failing chain's own provider token is removed). Extends the handler storage mock with aDeleteUpstreamTokensForProvidercapture, and updates the two existing tests whose assertions depend on the cleanup scope.API Compatibility
v1beta1API. No operator API surface is touched;DeleteUpstreamTokensForProvideralready exists on the storage interface.Does this introduce a user-facing change?
No behavior change for the native single-session storage scheme. For deployments layering a
(gateway, user)-keyed token store, a failed login leg no longer deletes the user's other connectors' upstream tokens.Special notes for reviewers
TestCallbackHandler_SubsequentLeg_MissingChain_FailsClosed): it now cleans only the just-stored leg and leaves an unnameable earlier leg to TTL, rather than wiping the whole session. This is the inherent trade of provider-scoped cleanup vs. stamping each row with an authorization id; happy to switch to the stamping approach if reviewers prefer the earlier leg also be cleaned.