Skip to content

Conversation

@saucow
Copy link
Collaborator

@saucow saucow commented Oct 28, 2025

What I did

Made the OAuth redirect URI configurable in the DCR (Dynamic Client Registration) helper library while maintaining backwards
compatibility.

Changes:

  • Added redirectURI parameter to PerformDCR() function
  • Implemented automatic fallback to default https://mcp.docker.com/oauth/callback when empty string is passed
  • Added DefaultRedirectURI constant for consumers to reference
  • Maintains backwards compatibility: old consumers can pass "" to use default behavior

Why:

  • Enables CE (Community Edition) mode to use localhost redirect URIs for OAuth flows
  • Allows different deployment modes (Desktop vs CE) to use appropriate callback URLs
  • Supports flexible OAuth configurations without hardcoding redirect destinations

API Change:
// Before (hypothetical old signature):
PerformDCR(ctx, discovery, serverName)

// After (backwards compatible):
PerformDCR(ctx, discovery, serverName, "") // Uses default
PerformDCR(ctx, discovery, serverName, "http://localhost:5000/callback") // Custom

Migration Guide for Consumers:
Existing code needs minimal update - just add "" as 4th parameter to use default redirect URI:

  • creds, err := oauth.PerformDCR(ctx, discovery, "github")
  • creds, err := oauth.PerformDCR(ctx, discovery, "github", "")

Related issue
Enables implementation of mcp-gateway CE mode OAuth flow with localhost callbacks.

@saucow saucow marked this pull request as ready for review October 28, 2025 18:35
@saucow saucow requested a review from a team as a code owner October 28, 2025 18:35
@saucow saucow requested a review from kgprs October 28, 2025 18:35
kgprs
kgprs previously approved these changes Oct 28, 2025
Copy link

@kgprs kgprs left a comment

Choose a reason for hiding this comment

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

Looks good - added a small comment about a safety check. Thanks!

// Use provided redirectURI, fallback to default if empty
if redirectURI == "" {
redirectURI = DefaultRedirectURI
}
Copy link

Choose a reason for hiding this comment

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

When the redirect url is specified, should we add a check that it's either 127.0.0.1 or mcp.docker.com? Might avoid a situation where an attacker passes in their own url

Copy link

Choose a reason for hiding this comment

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

I think we should drop the localhost since an attacker can override it

@saucow saucow merged commit dc700b1 into docker:main Oct 31, 2025
2 checks passed
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