Skip to content

Conversation

amirejaz
Copy link
Contributor

@amirejaz amirejaz commented Sep 10, 2025

When using dynamic client registration with some MCP servers, the system was failing with "issuer mismatch" errors because:

  • The issuer was derived from the MCP server URL.
  • The OIDC discovery document returned a different issuer from the mcp server URL.
  • The system performed strict issuer validation even for derived issuers.

@amirejaz amirejaz requested review from JAORMX and yrobla September 10, 2025 15:47
Copy link

codecov bot commented Sep 10, 2025

Codecov Report

❌ Patch coverage is 36.84211% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.65%. Comparing base (e454f65) to head (aa41018).

Files with missing lines Patch % Lines
pkg/runner/remote_auth.go 0.00% 11 Missing ⚠️
pkg/auth/discovery/discovery.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1825      +/-   ##
==========================================
+ Coverage   42.59%   42.65%   +0.05%     
==========================================
  Files         184      184              
  Lines       21775    21777       +2     
==========================================
+ Hits         9276     9289      +13     
+ Misses      11777    11770       -7     
+ Partials      722      718       -4     

☔ View full report in Codecov by Sentry.
📢 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.

@coveralls
Copy link
Collaborator

coveralls commented Sep 10, 2025

Coverage Status

coverage: 39.415% (+0.02%) from 39.391%
when pulling aa41018 on issuer-mistmatch
into e454f65 on main.

@jhrozek
Copy link
Contributor

jhrozek commented Sep 11, 2025

After talking to @amirejaz about the issue in more detail, it seems like the difference is typically in the issuer subdomain e.g. for mcp.example.com the discovered issuer might be auth.example.com. In that case we agreed to only relax the check insofar as to allow subdomains of the MCP server domain, not skip the check completely.

@JAORMX JAORMX requested a review from Copilot September 11, 2025 10:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements flexible issuer validation for OIDC discovery to resolve "issuer mismatch" errors that occur when MCP servers derive issuers from URLs but return different issuer values in their OIDC discovery documents.

  • Adds validateIssuerMatch parameter to OIDC discovery functions to control issuer validation strictness
  • Tracks whether issuer was explicitly provided vs. derived from URL via new IssuerProvided field
  • Updates validation logic to skip issuer matching when issuer is derived (flexible validation)

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/runner/remote_auth.go Adds tracking of whether issuer was explicitly provided and passes it to OAuth flow
pkg/auth/oauth/oidc.go Implements flexible issuer validation by adding validateIssuerMatch parameter to discovery functions
pkg/auth/oauth/oidc_test.go Updates tests to include validateIssuerMatch parameter and adds new test cases for flexible validation
pkg/auth/oauth/dynamic_registration_test.go Updates test call to use flexible validation (validateIssuerMatch=false)
pkg/auth/discovery/discovery.go Adds IssuerProvided field to OAuthFlowConfig and uses it to determine validation mode
cmd/thv/app/proxy.go Sets IssuerProvided field based on whether issuer was explicitly provided

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@@ -37,12 +37,18 @@ type httpClient interface {
}

// DiscoverOIDCEndpoints discovers OAuth endpoints from an OIDC issuer
func DiscoverOIDCEndpoints(ctx context.Context, issuer string) (*OIDCDiscoveryDocument, error) {
return discoverOIDCEndpointsWithClient(ctx, issuer, nil)
// Uses flexible issuer validation to support cases where issuer is derived from URL
Copy link
Preview

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment should end with a period for consistency with other function documentation.

Suggested change
// Uses flexible issuer validation to support cases where issuer is derived from URL
// Uses flexible issuer validation to support cases where issuer is derived from URL.

Copilot uses AI. Check for mistakes.

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.

3 participants