Skip to content

fix(auth): unify refresh checks and error handling - #1236

Open
DaleSeo wants to merge 2 commits into
mainfrom
fix/auth-refresh-guard
Open

fix(auth): unify refresh checks and error handling#1236
DaleSeo wants to merge 2 commits into
mainfrom
fix/auth-refresh-guard

Conversation

@DaleSeo

@DaleSeo DaleSeo commented Sep 1, 2026

Copy link
Copy Markdown
Member

Motivation and Context

PR #1232 added CredentialStore::acquire_refresh_guard. Three refresh behaviors ended up depending on whether a store returns a guard.

  • The stored client ID is checked only for guarded stores.
  • The stored granted scopes are treated as authoritative only for guarded stores.
  • call_reacting_to_challenges only propagates CredentialStoreError and converts every other refresh error into AuthRequired.

The presence of a guard tells us whether a store can coordinate. It does not tell us whether its contents are trustworthy. For example, a store might return Ok(None) in single-process mode because it does not need a lock. In that case, we would silently skip the client ID check. The trait docs also describe this hook as coordination only, so implementors have no reason to know that validation depends on it.

This PR makes the following changes:

  • It applies the client ID check to every store, so mismatches are caught before the refresh token leaves the process.
  • It routes refresh scopes through the existing resolve_granted_scopes helper. This follows the same SEP-2350 and RFC 6749 section 5.1 rule already used by the authorization-code path. The current_scopes fallback is used only when no scopes were recorded.
  • It maps only AuthorizationRequired to the challenge in call_reacting_to_challenges and propagates every other error. This removes the special handling for CredentialStoreError.

How Has This Been Tested?

Added regression tests

Breaking Changes

None

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@DaleSeo
DaleSeo requested a review from a team as a code owner September 1, 2026 14:26
@DaleSeo DaleSeo self-assigned this Sep 1, 2026
@github-actions github-actions Bot added T-core Core library changes T-transport Transport layer changes labels Sep 1, 2026
@DaleSeo
DaleSeo force-pushed the fix/auth-refresh-guard branch from dbaa814 to 4e1b16a Compare September 2, 2026 03:44
@DaleSeo
DaleSeo force-pushed the fix/auth-refresh-guard branch from 4e1b16a to 4cf38f7 Compare September 2, 2026 04:07
@github-actions github-actions Bot added the T-test Testing related changes label Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-core Core library changes T-test Testing related changes T-transport Transport layer changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant