Filter upstream auth chain via callback hook#5725
Conversation
The embedded OAuth authorization server walks every configured upstream on every authorization, prompting for and storing tokens that a given authorization may not need. This widens the stored-token surface and forces needless upstream prompts. Add an optional UpstreamFilter hook (WithUpstreamFilter), consulted once in the callback handler after the first upstream resolves, that narrows the remaining chain to a subset of the configured upstreams: - The first upstream is always required and is never passed to nor removable by the filter. - The kept set is computed once and carried in PendingAuthorization (ChainUpstreams) so the filter is not re-run per leg. - nextMissingUpstream and the cross-leg identity check now operate on the effective chain rather than the raw config. - A filter error fails the authorization with a server error; it never silently falls back to walking every upstream. With no filter configured, behaviour is unchanged: the handler walks all configured upstreams as before. SingleLeg flows are unaffected and the upstream-token storage key contracts are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5725 +/- ##
==========================================
+ Coverage 70.65% 70.70% +0.05%
==========================================
Files 682 682
Lines 68854 68890 +36
==========================================
+ Hits 48651 48712 +61
+ Misses 16657 16611 -46
- Partials 3546 3567 +21 ☔ 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: security, architecture-go, storage-persistence, test-coverage, general-quality
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | Legacy-pending recompute can use the wrong leg's request context for the filter | 9/10 | MEDIUM | Fix |
| 2 | Stored ChainUpstreams is trusted without validation against configured upstreams |
7/10 | MEDIUM | Fix |
| 3 | Identity-mismatch log loses structured fields after the verifyChainIdentity extraction |
7/10 | MEDIUM | Fix |
| 4 | Missing backward-compatibility test for legacy Redis pending records lacking chain_upstreams |
7/10 | MEDIUM | Fix |
Overall
This adds an optional UpstreamFilter hook, injected via WithUpstreamFilter, that narrows a multi-upstream OAuth authorization chain to a subset after the first upstream resolves. The implementation matches the linked issue's proposed shape closely, computes the effective chain once and carries it forward in PendingAuthorization.ChainUpstreams rather than re-invoking the filter per leg, and fails closed (server error, no fallback to a full walk) on filter error. computeChain's narrowing logic is small, pure, and has thorough table-driven coverage of every scenario called out in the linked issue. For existing no-filter deployments, behavior is unchanged end to end.
The findings below are all MEDIUM and none block merging. The most notable one (#1) is a narrow edge case in the "empty ChainUpstreams" recompute branch: it can't distinguish a genuine first leg from a non-first leg whose pending predates this field (a realistic rolling-deploy scenario for the Redis backend), so a context-sensitive filter could see the wrong leg's request context during that window. The other three are smaller and self-contained: a defense-in-depth gap where a chain loaded from storage isn't cross-checked against the configured upstream list, a logging regression where the identity-mismatch check lost its structured slog fields during extraction into verifyChainIdentity, and a test-coverage gap for the legacy-Redis-record backward-compatibility case (the repo already has a precedent for exactly this kind of test at redis_test.go:1109).
Documentation
UpstreamFilter's doc comment (pkg/authserver/server/handlers/handler.go) states it receives "the request context of the first leg's callback" — worth adding a note there about the rolling-upgrade edge case in finding #1, and mentioning that the context it receives already carries auth.WithPlatformUser, since that's the most concrete signal a real filter implementation would key off of.
Generated with Claude Code
| // subsequent leg reuses the chain carried in the pending authorization so the | ||
| // filter is not re-run per leg. A legacy pending predating ChainUpstreams also | ||
| // lands here and is safely recomputed. | ||
| chain := pending.ChainUpstreams |
There was a problem hiding this comment.
[MEDIUM] Legacy-pending recompute can use the wrong leg's request context for the filter (Consensus: 9/10)
len(chain) == 0 is meant to catch a true first leg or a legacy pending predating ChainUpstreams, but it also fires for a genuine non-first leg whose pending was written before this field existed (a realistic rolling-deploy window for the Redis backend). In that case computeChain(ctx) — and the filter — runs with the later leg's context instead of the first leg's, contradicting UpstreamFilter's "consulted once, first-leg context" doc contract.
Consider disambiguating with pending.ResolvedUserID == "" (true first leg) in addition to len(chain) == 0, and treating a non-first leg missing its chain as a hard error requiring re-authentication rather than silently recomputing with a mismatched context.
Raised by: security, architecture-go, storage-persistence, general-quality
| chain = computed | ||
| } | ||
|
|
||
| nextProvider, err := h.nextMissingUpstream(ctx, sessionID, chain) |
There was a problem hiding this comment.
[MEDIUM] Stored ChainUpstreams is trusted without validation against configured upstreams (Consensus: 7/10)
chain is used as-is here once non-empty — nothing asserts chain[0] == h.upstreams[0].Name or that every other entry resolves via h.upstreamByName. Combined with verifyChainIdentity's len(chain) <= 1 no-op gate, a party able to write a single PendingAuthorization row could shrink an in-flight chain to one element, skipping legs and disabling the cross-leg identity check. This requires storage write access to the specific row, which tempers real-world severity, but the handler already validates the analogous pending.UpstreamProviderName field elsewhere in this file — worth applying the same discipline here.
Raised by: security
| return | ||
| } | ||
| if err := h.verifyChainIdentity(ctx, sessionID, chain, subject); err != nil { | ||
| slog.Error("chain identity verification failed", "error", err) |
There was a problem hiding this comment.
[MEDIUM] Identity-mismatch log loses structured fields after the verifyChainIdentity extraction (Consensus: 7/10)
The pre-PR inline check logged expected, got, and provider as separate structured slog keys. Here, only "error", err is logged, with those three values now folded into one formatted string inside verifyChainIdentity. A log pipeline that filtered/alerted on those keys for this defense-in-depth check loses that capability.
Consider having verifyChainIdentity return a typed error (or having the caller re-derive the fields) so this call site can re-emit "expected", subject, "got", firstTokens.UserID, "provider", firstProvider alongside the error.
Raised by: general-quality
| Scopes: []string{"openid", "profile"}, InternalState: state, | ||
| UpstreamPKCEVerifier: "verifier", UpstreamNonce: "nonce", | ||
| SingleLeg: true, CreatedAt: time.Now(), | ||
| SingleLeg: true, ChainUpstreams: []string{"provider-1", "provider-2"}, |
There was a problem hiding this comment.
[MEDIUM] Missing backward-compatibility test for legacy Redis records lacking chain_upstreams (Consensus: 7/10)
makePending() always sets a non-empty ChainUpstreams, so no test exercises loading a pre-upgrade JSON blob that lacks the chain_upstreams key — the exact scenario the PR's backward-compatibility claim rests on. The repo already has a precedent for this pattern at redis_test.go:1109 ("legacy JSON without session_expires_at decodes with zero SessionExpiresAt") that could be mirrored here: seed a raw JSON string missing chain_upstreams via s.client.Set, then assert LoadPendingAuthorization returns a nil/empty ChainUpstreams.
Raised by: storage-persistence
1 participant
Summary
The embedded OAuth authorization server walks every configured upstream provider
on every authorization. There is no way for a consumer to say that only a subset
of the configured upstreams is relevant for a given authorization, so the flow
prompts for upstreams the authorization does not need and stores upstream tokens
that are never used — widening the stored-token surface for no benefit.
This PR adds an optional filter hook, consulted once in the callback handler after
the first upstream resolves, that narrows the remaining chain to a subset of the
configured upstreams. The change is fully backward compatible: with no filter
configured, the handler walks all configured upstreams exactly as before.
avoiding needless prompts and reducing the stored-token surface.
UpstreamFilterinterface andWithUpstreamFilterconstructionoption; the effective chain is computed once and carried in
PendingAuthorizationso the filter is not re-run per leg; the missing-leg walkand the cross-leg identity check now operate on the effective chain rather than
the raw config.
Closes #5724
Type of change
Test plan
task test)task test-e2e)task lint-fix)New unit tests cover the callback flow and the chain computation:
issues the code without prompting for the dropped leg.
pending authorization.
to a full walk).
computeChaintable cases: no-filter full walk in order, single upstream skipsthe filter, subset kept with the first upstream always leading, empty keep set
leaves only the first upstream, filter return order ignored in favor of
configured order, and unknown / first-upstream names in the keep set ignored.
nextMissingUpstreamrespects the chain subset rather than the raw config.Changes
pkg/authserver/server/handlers/handler.goUpstreamFilterinterface,WithUpstreamFilteroption, andcomputeChain;nextMissingUpstreamnow walks the effective chainpkg/authserver/server/handlers/callback.goverifyChainIdentityto gate on the effective chainpkg/authserver/storage/types.goChainUpstreamsfield toPendingAuthorizationpkg/authserver/storage/memory.goChainUpstreamsin the in-memory storepkg/authserver/storage/redis.goChainUpstreamsin the Redis storepkg/authserver/server/handlers/*_test.go,pkg/authserver/storage/*_test.goDoes this introduce a user-facing change?
No end-user-facing change to the
thvCLI. For consumers embedding the authserver, this adds a new optional construction option (
WithUpstreamFilter). It isopt-in and behavior is unchanged when unset.
Special notes for reviewers
upstreams[0]; it is not passed to the filter and cannot be removed by it.computeChainiterates theconfigured order and ignores any returned name that is not a non-first configured
upstream, so a filter cannot reorder the chain or inject unknown providers.
server error rather than reverting to walking every upstream.
PendingAuthorizationpredating thenew
ChainUpstreamsfield (empty on load) is safely recomputed in the callback,which yields all configured upstreams in order for the no-filter case.
SingleLegflows are unaffected and theupstream-token storage key contracts are unchanged.
Generated with Claude Code