Skip to content

fix(dashboard-auth): don't abort the refresh chain when a provider declines the token - #40847

Open
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/dashboard-auth-refresh-chain
Open

Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/dashboard-auth-refresh-chain

Conversation

@Dusk1e

@Dusk1e Dusk1e commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

What

_attempt_refresh now tries every registered auth provider instead of
giving up on the first one's failure. It continues past both a foreign-RT
rejection (RefreshExpiredError) and an unreachable provider
(ProviderError), forcing re-login only when no provider can refresh the
refresh token.

Why

The session cookie carries no provider identity, so the middleware cannot
know which provider owns the refresh token — it must try each in turn,
exactly like the verify loop. Before this change, a stacked multi-provider
dashboard bounced the user to /login on every access-token expiry whenever
a non-owning or unreachable provider was tried before the provider that
actually owns the RT.

Mirrors the verify-loop fix in 616c0a3
(fix(dashboard-auth): don't abort verify chain on one provider's
ProviderError
) and closes the same gap on the refresh path.

Tests

Added 3 regression tests to
tests/hermes_cli/test_dashboard_auth_middleware.py:

  • unreachable provider first → a later provider still refreshes
  • foreign-RT-rejecting provider first → the owning provider still refreshes
  • every provider declines → clean re-login (not 200, not 500)

All 3 fail on the pre-fix code and pass with the fix.

Result — dashboard-auth gate + provider suite:

255 passed

…clines the token

The session cookie carries no provider identity, so _attempt_refresh must offer the refresh token to every registered provider in turn -- like the verify loop already does. It previously returned on the first provider's failure, so in a stacked multi-provider dashboard a session whose RT belonged to a later provider was bounced to /login on every access-token expiry whenever a non-owning provider (foreign RT -> token-endpoint 400 -> RefreshExpiredError) or an unreachable provider (ProviderError) was tried first.

Continue past both failure modes; only force re-login when no provider can refresh. Mirrors the verify-loop stacked-provider fix. Adds regression tests for unreachable-first, foreign-RT-first, and the all-decline re-login guard.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools labels Jun 6, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Verification: refresh-chain parity gap fix looks correct.

The _attempt_refresh loop had the same stacked-provider bug that the verify loop above it already fixed: when providers are stacked (e.g., [nous, self-hosted]), a self-hosted session's RT handed to the nous provider first yields a RefreshExpiredError (token-endpoint 400 — foreign RT is indistinguishable from a dead one). The old code did return None on the first failure, bouncing the user to /login on every access-token expiry even though the owning provider was later in the list.

What changed: Two return Nonecontinue in the exception handlers (RefreshExpiredError and ProviderError), mirroring the verify-loop's stacked-provider handling. The chain now exhausts all providers before falling through to re-login.

Test coverage is thorough:

  1. test_unreachable_first_provider_does_not_block_refresh — ProviderError from first provider, working second provider succeeds
  2. test_foreign_rt_rejecting_first_provider_does_not_block_refresh — RefreshExpiredError from first provider, working second provider succeeds
  3. test_refresh_bounces_when_no_provider_can_refresh — every provider declines → still forces 401 re-login (guard against over-reach)

The all-providers-fail guard ensures we don't silently accept a dead session when no provider can rotate the RT. Clean fix, well-tested. LGTM.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for identifying the real refresh-chain gap: current main still returns from _attempt_refresh on the first provider failure (hermes_cli/dashboard_auth/middleware.py:441-462).

Problems

  • The proposed continue at hermes_cli/dashboard_auth/middleware.py:369 / :385 sends a bearer refresh token to every later provider. This is not equivalent to access-token verification: the Nous provider transmits the supplied token to its remote endpoint in both request data and x-nous-refresh-token (plugins/dashboard_auth/nous/__init__.py:282-290). A self-hosted provider's RT could therefore be disclosed to Nous when Nous is registered first.
  • The branch also predates the current list_session_providers() filtering (hermes_cli/dashboard_auth/middleware.py:438), so it should not restore the older list_providers() loop.

Suggested changes

  • Bind the issuing provider to the session with an integrity-protected, server-verifiable mechanism, then refresh only that provider. Add a regression proving non-owning providers never receive the RT.

Automated hermes-sweeper review.

return None
for provider in list_providers():
try:
new_session = provider.refresh_session(refresh_token=refresh_token)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Blocking: this fan-outs a bearer refresh token to each registered provider after a rejection. That is credential disclosure, not just a verification retry: the Nous provider transmits its refresh_token argument to the remote Portal token endpoint (plugins/dashboard_auth/nous/__init__.py:282-290). Persist and validate the issuing provider, then call only that provider's refresh path.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants