Skip to content

Conversation

mishamo
Copy link
Contributor

@mishamo mishamo commented Aug 27, 2025

Problem

PR #145 fixed the "Missing required parameter: scope" error with OAuth providers like Google, but it introduced a regression that broke existing users who relied on --static-oauth-client-metadata '{"scope": "custom scopes"}' for custom scope configuration.

Root Cause: The original implementation always forced scopes to be hardcoded defaults ('openid email profile'), overriding any custom scope provided in staticOAuthClientMetadata.

Solution

This PR fixes the regression while preserving the original OAuth scope parameter fix. It implements a proper scope priority system:

Scope Priority (Backward Compatible)

  1. Custom scope from staticOAuthClientMetadata.scope (user's custom scope) - HIGHEST PRIORITY
  2. Extracted scopes from registration response
  3. Default scopes ('openid email profile') as fallback only

Core Changes

  • Preserve custom scopes: staticOAuthClientMetadata.scope always takes precedence
  • Add scope parameter to authorization URLs: Fixes "Missing required parameter: scope" errors
  • Extract scopes from registration responses: Handles multiple formats (scope, default_scope, scopes[], default_scopes[])
  • Smart scope extraction: Only extracts when no custom scope is provided
  • Proper cleanup: Stored scopes are cleaned up during credential invalidation

Implementation Details

  • getEffectiveScope(): Determines which scope to use based on priority
  • extractScopesFromRegistration(): Handles different scope formats from OAuth providers
  • Conditional scope extraction: Respects existing staticOAuthClientMetadata.scope
  • Persistent scope storage: Extracted scopes stored in scopes.json and loaded on startup
  • Authorization URL enhancement: Adds effective scope parameter automatically

Testing

Added 19 comprehensive test cases covering:

  • Custom scopes preservation: Verifies staticOAuthClientMetadata.scope always takes priority
  • Scope extraction: Tests all registration response formats (scope, default_scope, scopes[], default_scopes[])
  • Priority behavior: Ensures custom scopes are never overridden
  • Backward compatibility: Existing usage patterns work exactly as before
  • Credential invalidation: Proper cleanup of stored scopes
  • Authorization URLs: Correct scope parameter inclusion

Compatibility

✅ Backward Compatible

  • Existing custom scope users: --static-oauth-client-metadata '{"scope": "custom scopes"}' works exactly as before
  • No breaking changes: All existing APIs remain unchanged
  • Graceful fallbacks: Missing scope data handled appropriately

✅ Fixes Original Issue

  • OAuth providers like Google: Now receive required scope parameter in authorization URLs
  • Dynamic client registration: Scopes extracted from registration responses
  • Default behavior: Users without custom scopes get sensible defaults

Testing Done

  • All 19 OAuth tests pass ✅
  • No changes to utils.test.ts (upstream compatibility) ✅
  • Manual testing with OAuth providers requiring scope parameter ✅
  • Verified existing custom scope workflows still work ✅

Example Usage

Before (broken in v0.1.20):

# This stopped working in v0.1.20 - custom scope was ignored
mcp-remote --static-oauth-client-metadata '{"scope": "user:email repo"}' --server-url https://api.example.com

After (fixed in this PR):

# Works perfectly - custom scope is preserved and used
mcp-remote --static-oauth-client-metadata '{"scope": "user:email repo"}' --server-url https://api.example.com
# Authorization URL includes: &scope=user:email+repo

# Also works - uses extracted scopes from registration
mcp-remote --server-url https://google-oauth-provider.com  
# Authorization URL includes: &scope=openid+email+profile (or extracted scopes)

This fix ensures both existing users with custom scopes and new users with providers like Google have a seamless OAuth experience.

mishamo and others added 2 commits August 27, 2025 17:40
This fixes the OAuth "Missing required parameter: scope" issue while maintaining
backward compatibility with existing --static-oauth-client-metadata usage.

Core Changes:
- Add scope parameter to authorization URLs to satisfy OAuth provider requirements
- Extract and store scopes from registration responses in multiple formats
- Preserve custom scopes provided via staticOAuthClientMetadata.scope
- Add comprehensive scope priority handling

Scope Priority (fixed the original regression):
1. Custom scope from staticOAuthClientMetadata.scope (user's custom scope)
2. Extracted scopes from registration response
3. Default scopes ('openid email profile') as fallback

Implementation Details:
- getEffectiveScope() determines which scope to use based on priority
- extractScopesFromRegistration() handles scope, default_scope, scopes[], default_scopes[]
- Only extract/store scopes when no custom scope is provided
- Clean up stored scopes during credential invalidation
- Added 19 comprehensive tests covering all scenarios

Backward Compatibility:
- Existing --static-oauth-client-metadata usage works exactly as before
- Users with custom scopes won't be affected by scope extraction
- All existing APIs remain unchanged

Fixes the regression in v0.1.20 that broke custom scopes while maintaining
the OAuth scope parameter fix for providers like Google.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Copy link

pkg-pr-new bot commented Aug 27, 2025

Open in StackBlitz

npx https://pkg.pr.new/mcp-remote@147

commit: 2e18f20

@mishamo
Copy link
Contributor Author

mishamo commented Aug 30, 2025

@geelen, any chance you could take a look? I'm keen to use Google oauth via mcp-remote ASAP.

Comment on lines +212 to +214
const effectiveScope = this.getEffectiveScope()
authorizationUrl.searchParams.set('scope', effectiveScope)
debugLog('Added scope parameter to authorization URL', { scopes: effectiveScope })
Copy link

Choose a reason for hiding this comment

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

Hello! I'm running into this issue myself and really appreciate this PR. Unfortunately when using your branch, I'm still not seeing the correct scopes attached. This piece of code successfully attaches the scope for me, but I'm not sure how to integrate it into your getEffectiveScope method.

Suggested change
const effectiveScope = this.getEffectiveScope()
authorizationUrl.searchParams.set('scope', effectiveScope)
debugLog('Added scope parameter to authorization URL', { scopes: effectiveScope })
const effectiveScope = this.getEffectiveScope()
authorizationUrl.searchParams.set('scope', effectiveScope)
debugLog('Added scope parameter to authorization URL', { scopes: effectiveScope })
// The correct custom scopes are found in the authorization URL resource + using the discoverOAuthProtectedResourceMetadata
const resourceURL = authorizationUrl.searchParams.get('resource')
if (resourceURL) {
const metadata = await discoverOAuthProtectedResourceMetadata(resourceURL)
if (metadata.scopes_supported !== undefined) {
authorizationUrl.searchParams.set('scope', metadata.scopes_supported.join(' '))
}
}

Any guidance on how to integrate this into the PR would be very much appreciated! Hoping to get this fixed to address this reported issue.

@mishamo
Copy link
Contributor Author

mishamo commented Sep 25, 2025

Thanks for the feedback and the code suggestion! I really appreciate you testing this branch and providing a concrete solution.

Your approach using discoverOAuthProtectedResourceMetadata to get scopes_supported from the resource server is exactly the right way to handle this according to RFC 9728. This would definitely solve the issue you're encountering and address #162.

However, I think this enhancement would be better suited for a separate PR since:

  1. This current PR is focused specifically on fixing the regression where custom scopes were being ignored
  2. Your suggestion adds new functionality (resource metadata discovery) which deserves its own focused review and testing
  3. It keeps the git history clean - regression fix separate from feature enhancement

Would you be interested in creating a follow-up PR with your discoverOAuthProtectedResourceMetadata approach? I'd be happy to help review it. It's a solid solution that would complement this regression fix nicely.

Thanks again for the detailed analysis and code - it's exactly what's needed to properly implement RFC 9728 support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants