[tgrunnagle]

Summary

A refreshed access token keeps its token session (tsid), so it keeps that session's upstream credential too. When that credential is dead, the bearer validator answers every request with 401. A client that handles the 401 by redeeming its refresh token gets a 200 with new tokens for the same dead session, and keeps looping without ever signing in again. This PR is item 3 of #6901. Items 1 and 2 landed in #6902.

• Adds a built-in upstream refresh guard, a refresh-grant validator that is on by default whenever at least one upstream is configured. When the session's upstream credential is definitively dead, it refuses the refresh_token grant with fosite.ErrInvalidGrant (HTTP 400 invalid_grant), which sends the client to a full authorization and a new tsid.

• It revokes the refresh token (and the grant's access tokens) only when the stored rows prove the credential can never be used again. Otherwise it refuses and keeps the token, so a wrong refusal costs one re-authorization, not the grant. Here is every cause; the arch doc ("Upstream refresh guard") holds the canonical copy:

  Cause	Revoked
  Rows exist but none holds an access token	Yes
  A row is expired and has no refresh token	Yes
  The session has no readable upstream rows	No (Redis's bulk read skips rows it cannot parse)
  A row is expired and its provider is not configured on this replica	No (this replica's config can lag a rolling deploy)
  The row was deleted while the guard refreshed it	No
  The IdP rejects the row's refresh with invalid_grant (upstream.ErrGrantRejected)	No (a lost replica race looks the same)

• Before deciding, the guard refreshes an expired row through the server's shared refresher, the same singleflight instance the other refresh paths use, one provider at a time. Upstream refresh is otherwise lazy, so an idle client often has an expired stored token whose upstream refresh token still works. Deciding from stored state alone would force every idle client to sign in again.

• Some outcomes are uncertain, and those grant the refresh:

  • network errors, 5xx and 429 from the IdP;
  • storage errors;
  • a cancelled request (no further IdP call is made);
  • the check running past its 10-second budget.

  Sessions without a tsid are not checked.

• The guard runs ahead of the embedder's Config.RefreshGrantHandlers, so those handlers only see grants the guard accepted.

• Adds Config.DisableUpstreamRefreshGuard as an opt-out, for Go embedders only. A RunConfig/CRD switch is tracked in Expose the upstream refresh guard opt-out in RunConfig and CRDs #6931.

• Updates docs/arch/11-auth-server-storage.md, docs/middleware.md and the seam's comments to describe the new behavior.

How this addresses the follow-up notes in #6901 (comment):

• (a) Lost replica race: with a rotating IdP, a replica that loses a cross-replica refresh race also receives invalid_grant. The guard waits 500ms and reads the row again. If the row now holds any different refresh token, the session counts as live and the grant goes through. If the recheck cannot complete (the request was cancelled or the read failed), the result is uncertain and the grant goes through. A confirmed rejection refuses but does not revoke, so a winner whose write lands after the recheck costs the client one re-authorization, not its grant.
• (b) Error type: the guard returns fosite.ErrInvalidGrant (400), not a plain error, which fosite would turn into a 500.

Closes #6901

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e): not passing locally. See below.
• Linting (task lint-fix), 0 issues
• Manual testing (describe below)

task test-e2e could not complete on the author's machine, for reasons unrelated to this change:

• Run 1: ToolHive Desktop is installed, so every thv call failed with "CLI conflict detected".
• Run 2: re-run with TOOLHIVE_SKIP_DESKTOP_CHECK=1. It hit the 20-minute suite timeout after 65 of 544 specs: 47 passed and 18 failed.

None of the failures exercise the embedded auth server:

• thvignore;
• telemetry containers not starting;
• CIMD remote-auth client;
• network isolation;
• the desktop-validation spec, which the skip flag defeats.

Relying on CI for e2e.

New tests (the guard tests also pass under -race):

• TestUpstreamRefreshGuard: table-driven tests covering every refuse and grant outcome and whether each revokes. Also covers:

  • a winner row that lands during the recheck delay;
  • a request cancelled during the recheck, and one cancelled before the IdP call;
  • a refresh that outlives the check budget;
  • a row deleted during the refresh.

• TestUpstreamRefreshGuard_Gates: the constructor rejects missing storage or refresher and wires the recheck delay and check budget; the guard runs only on the refresh_token grant.

• TestRefreshGrantValidatorChain: the guard runs before embedder handlers; disabled and no-upstream configs leave only the embedder handlers; the embedder's slice is not written.

• TestUpstreamRefreshGuard_StorageFailures: storage read failures grant the refresh; a failed revocation still refuses; revocation runs on a live, bounded context even when the request is cancelled.

• TestUpstreamRefreshGuard_RealIdPRejection: an invalid_grant returned by a real IdP is classified as ErrGrantRejected and refused, and a 5xx is granted.

• TestIntegration_UpstreamRefreshGuard: end-to-end through the token endpoint with mockoidc, on memory and on Redis (miniredis) storage. Cases:

  • a live session;
  • an idle session that can still be refreshed;
  • absent rows;
  • a corrupt Redis row;
  • a rejected upstream refresh;
  • an expired row without a refresh token;
  • the guard disabled.

  After a refusal it restores a live row and checks that the same refresh token works again (kept) or fails (revoked).

Changes

File	Change
pkg/authserver/refresh_grant_guard.go	New upstreamRefreshGuard refresh-grant validator
pkg/authserver/server_impl.go	Builds the shared refresher before the provider and registers the guard ahead of embedder handlers, with providers from the resolved upstreams
pkg/authserver/config.go	Adds DisableUpstreamRefreshGuard and updates the RefreshGrantHandlers docs
pkg/authserver/refresh_grant_handlers.go	Doc comments: the wrapper now also runs the guard
pkg/authserver/refresh_grant_guard_test.go	Unit and integration tests for the guard
pkg/authserver/integration_test.go	withoutUpstreamRefreshGuard test option
docs/arch/11-auth-server-storage.md	Canonical refusal list with revoke status, race handling, upgrade impact and opt-out
docs/middleware.md	Notes that a client refreshing after the 401 gets 400 invalid_grant

Does this introduce a user-facing change?

Yes. The guard is on by default whenever an upstream is configured.

• Before: a refresh grant for a session whose upstream credential was dead returned 200 with new tokens. The tokens kept the same tsid, so the client looped on 401.
• After: that refresh grant returns 400 invalid_grant and the client has to go through a full authorization. The refresh token and the grant's access tokens are revoked only when the session has rows but no access token, or an expired row has no refresh token. For the other causes the refresh token is kept and refused again on each retry.
• Refresh-token lifetime: a refresh token now stays useful only while the upstream credential does, even in deployments whose resource server never reads upstream tokens.
• Latency: a refresh grant that finds an expired upstream row makes an upstream refresh call before tokens are issued. The whole check is capped at 10 seconds; past that the grant goes through. When the upstream rejects the refresh, the recheck adds 500ms.
• Opt-out: Go embedders can set Config.DisableUpstreamRefreshGuard. There is no RunConfig, CLI or CRD setting yet (Expose the upstream refresh guard opt-out in RunConfig and CRDs #6931).

Special notes for reviewers

Accepted trade-offs:

1. Broader than the acceptance criterion. The guard also refuses on a missing refresh token, an unconfigured provider, and rows without an access token, which the issue's acceptance criterion does not list. The bearer validator already answers each of these with 401, so granting the refresh would keep the client looping.
2. Narrower revocation than the acceptance criterion. The issue asks that an absent row and an IdP rejection revoke the refresh token. Both now refuse without revoking, as do an unconfigured provider and a row deleted mid-refresh. Each of these can be wrong: Redis skipping an unparsable row, a lost replica race, a replica's config lagging a rolling deploy. A false revoke cannot be undone, while a kept token is refused again on every later presentation anyway.
3. Retrying a kept token repeats the check. For an IdP rejection, each retry makes another upstream refresh call, waits out the 500ms recheck and logs another WARN, where a revoked token would be refused once. This is bounded: the caller must authenticate as the token's client, and well-behaved clients treat invalid_grant as terminal.
4. "Row missing on re-read" stays uncertain. The refresher reports a row missing on its own re-read as a plain error, not storage.ErrNotFound, so the guard grants. The next refresh grant finds no row and refuses. A refresher error that wraps storage.ErrNotFound refuses.
5. The guard's rules duplicate the bearer validator's 401 rules. They agree today. A shared classifier was deferred because it would touch pkg/auth and upstreamtoken.

Revocation runs on a context detached from the request, with a 5s timeout. This way a client that disconnects right after a revoking refusal does not leave the refresh token live.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 96.62162% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.16%. Comparing base (7970d58) to head (7fec04a).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/authserver/refresh_grant_guard.go	98.40%	2 Missing ⚠️
pkg/authserver/server_impl.go	84.61%	2 Missing ⚠️
pkg/authserver/refresher.go	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6930      +/-   ##
==========================================
+ Coverage   80.01%   80.16%   +0.15%     
==========================================
  Files         837      840       +3     
  Lines       85215    85474     +259     
==========================================
+ Hits        68182    68521     +339     
+ Misses      17028    16948      -80     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tgrunnagle]

Multi-Agent Consensus Review

Agents consulted: security, concurrency-resilience, test-coverage, architecture-docs, general-quality

Consensus Summary

#	Finding	Consensus	Severity	Action
1	Irreversible revocation hinges on a fixed 500ms recheck	10/10	MEDIUM	Discuss
2	Redis skips unreadable rows; the guard reads "no rows" and revokes	10/10	MEDIUM	Fix
3	No overall deadline; sequential, detached upstream refreshes on the token endpoint	10/10	MEDIUM	Fix
4	Default-on with a Go-only opt-out; upgrade impact not stated	10/10	MEDIUM	Discuss
5	Recheck delay and the race window it exists for are untested	8/10	MEDIUM	Fix
6	No Redis-backed coverage of the guard	8/10	MEDIUM	Fix
7	"Dead" rules duplicate the bearer validator's with no shared check	7/10	MEDIUM	Discuss
8	Detached revocation context is untested	7/10	MEDIUM	Fix
9	refreshGrantValidatorChain wiring has no unit test	7/10	MEDIUM	Fix
10	deadUpstream/checkRow control flow is hard to follow	7/10	MEDIUM	Discuss
11	Row-gone refresher errors are granted as transient	10/10	LOW	Fix
12	rejectionStands ErrExpired comment and refresh-token comparison	8/10	LOW	Fix
13	Constructor shape and unreachable interface methods	8/10	LOW	Discuss
14	Provider names built separately from the refresher's	8/10	LOW	Fix
15	Doc and comment drift around the refresh-grant seam	8/10	LOW	Fix

Overall

This is a bug fix that adds a default-on refresh-grant validator to the embedded auth server, so a client that answers the bearer validator's 401 by refreshing is sent to a full authorization instead of looping on a dead tsid. The approach is sound. The guard runs after the built-in refresh handler has authenticated the client and loaded the grant. It uses the same storage and the same shared refresher as the other refresh paths. It revokes only the requester's own grant family. It returns invalid_grant (400) rather than a plain error. Fail-open on transient errors and the pre-decision refresh of idle rows are sensible choices. The classification of "dead" agrees with the bearer validator today.

None of the findings blocks merge. The ones worth discussing share one theme: the guard's refusal also revokes, and revocation cannot be undone. A fixed 500ms recheck decides a lost-replica race heuristically (#1), and a Redis read that silently skips unreadable rows is indistinguishable from an absent session (#2). A session-level deadline is also missing, so a slow IdP can hold the token endpoint (#3). The remaining findings are test gaps (the real recheck delay, the detached revocation context, Redis, the wiring), a few places where a row that is gone is granted instead of refused, and readability and doc drift.

Documentation

• docs/arch/11-auth-server-storage.md (line 439): the paragraph above the new text still presents Config.RefreshGrantHandlers as the way to refuse a dead-credential refresh, and refresh_grant_handlers.go still describes its wrapper as running only embedder handlers. Both now run the built-in guard first.
• The refusal list is written out in four places (type doc, Config, the arch doc, docs/middleware.md) and the transient-failure lists already differ (the arch doc includes 429, Config does not).
• State in the docs that the guard also bounds a refresh token's life by the upstream refresh token's liveness, and that the opt-out is available to Go embedders only.

---

Generated with Claude Code

[tgrunnagle]

All 15 earlier threads are resolved. The fixes match the code, tests and docs at 78c040014: task test passes and task lint reports 0 issues. The one suggestion that was declined (a shared classifier for the guard and the bearer validator) is reasonable to defer; a test that runs the same rows through both paths would catch drift.

Two things are still worth changing before this leaves draft.

1. The PR body still describes the old behavior.
The body is what the user-facing section of the PR is read from, and it now disagrees with the code and the arch doc:

• "After" says the refusal revokes the refresh token and access tokens. Now only three causes revoke (an expired row whose provider is unconfigured, an expired row with no refresh token, rows with no access token). No readable rows, an IdP invalid_grant, and a row deleted during the refresh refuse without revoking.
• "Definitively dead" lists the IdP rejection as a cause without saying it keeps the token, and the body does not mention the 10-second check budget, so "Latency" is incomplete.
• The test list omits TestRefreshGrantValidatorChain and the Redis and corrupt-row integration cases, and TestUpstreamRefreshGuard_Gates no longer covers "disabled".
• The reviewer notes should say that the guard now ties a refresh token's life to the upstream credential, and point at Expose the upstream refresh guard opt-out in RunConfig and CRDs #6931 for the RunConfig/CRD switch.

2. Revoking on an unconfigured provider rests on this replica's config, not on the stored rows.
The arch doc says revocation "applies only when the stored rows prove the credential can never be used again", and the other causes keep the token because they could be wrong. An expired row whose provider is unconfigured is decided by one replica's config, and trade-off 2 in the PR body already accepts that during a rolling deploy that adds an upstream, an old replica can revoke those sessions. The same reasoning that stopped revoking on an IdP rejection applies here: a false revoke cannot be undone, and a true one gains little, because the guard refuses the token on every later presentation anyway. Consider refusing without revoking for this cause too and moving it to the "No" rows of the table. If it stays, state in the doc that this row depends on config and carries the rolling-deploy risk.

For awareness, no change requested: a refusal that keeps the refresh token means a client that retries the same token repeats the check each time. For a dead upstream rejection, each retry makes another IdP refresh call, holds the request for the 500ms recheck, and logs another WARN, whereas a revoked token was refused once. Well-behaved clients treat invalid_grant as terminal, and the caller must be authenticated, so this is bounded. It may still be worth a note in the doc.

Generated with Claude Code

[tgrunnagle]

Thanks. Both points are addressed:

1. PR body: rewritten to match the code at e904668. It now has the cause table with revoke status, the 10-second budget under Latency, the refresh-token lifetime bound, Expose the upstream refresh guard opt-out in RunConfig and CRDs #6931 for the RunConfig/CRD switch, and the current test list (including TestRefreshGrantValidatorChain and the Redis and corrupt-row cases). It also records the narrower revocation as a deliberate departure from authserver: let embedders validate refresh-token grants, and type upstream refresh rejections #6901's acceptance criteria.
2. Unconfigured provider: addressed in e904668. An expired row whose provider isn't configured now refuses without revoking, and moved to the "No" rows of the arch doc table. The guard comment explains that this replica's config decides it.

On the retry note: also in e904668. The arch doc now describes the cost of keeping a refused token (another IdP call, the 500ms recheck and another WARN on each retry) and why it is bounded. The PR body repeats it as trade-off 3.

[jhrozek]

Nice work. The fosite ordering, the keep-vs-revoke split and the cross-replica recheck all check out against fosite 0.49 and both storage backends. Approving. I left a few inline comments; feel free to address them before merging or in a follow-up, your call. The one I'd look at first is about a kept refresh token re-running the full IdP check on every replay, and there's a cheap way to bound that.