Fix remote MCP server authentication with issuer mismatch (#1980)

This fixes authentication failures when remote MCP servers return a
different issuer URL than their public-facing URL in the OIDC discovery
response.

The fix adds a new priority level to the issuer discovery algorithm,
placing well-known endpoint discovery before URL derivation. This allows
accepting the authoritative issuer from the .well-known/openid-configuration
endpoint even when it differs from the server URL, complying with RFC 8414.

New discovery priority chain:
1. Configured issuer (if provided)
2. Realm-derived issuer (RFC 8414)
3. Resource metadata discovery (RFC 9728)
4. Well-known endpoint discovery (NEW - accepts actual issuer)
5. URL-derived issuer (fallback)

Changes:
- Add tryDiscoverFromWellKnown to attempt discovery before URL derivation
- Accept the actual issuer from ValidateAndDiscoverAuthServer
- Preserve HTTP scheme for localhost URLs in DeriveIssuerFromURL
- Add path normalization to DeriveIssuerFromRealm for security
- Add comprehensive test coverage for discovery priority chain
- Fix linting issues with constants and parallel test execution

The fix maintains backward compatibility while supporting real-world
OAuth deployments where the issuer differs from the server endpoint.

Fixes #1957

Signed-off-by: Juan Antonio Osorio <[email protected]>

Author: JAORMX

pkg/auth/discovery/discovery.go

@@ -15,6 +15,7 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"path"
 	"strings"
 	"time"
 
@@ -252,9 +253,16 @@ func DeriveIssuerFromURL(remoteURL string) string {
 		host = fmt.Sprintf("%s:%s", host, port)
 	}
 
+	// For localhost, preserve the original scheme (HTTP or HTTPS)
+	// This supports local development and testing scenarios
+	scheme := networking.HttpsScheme
+	if networking.IsLocalhost(host) && parsedURL.Scheme != "" {
+		scheme = parsedURL.Scheme
+	}
+
 	// General pattern: use the domain as the issuer
 	// This works for most OAuth providers that use their domain as the issuer
-	issuer := fmt.Sprintf("%s://%s", networking.HttpsScheme, host)
+	issuer := fmt.Sprintf("%s://%s", scheme, host)
 
 	logger.Debugf("Derived issuer from URL - remoteURL: %s, issuer: %s", remoteURL, issuer)
 	return issuer
@@ -327,11 +335,22 @@ func DeriveIssuerFromRealm(realm string) string {
 
 	// RFC 8414: The issuer identifier MUST be a URL using the "https" scheme
 	// with no query or fragment components
-	if parsedURL.Scheme != "https" {
+	if parsedURL.Scheme != "https" && !networking.IsLocalhost(parsedURL.Host) {
 		logger.Debugf("Realm is not using HTTPS scheme: %s", realm)
 		return ""
 	}
 
+	// Normalize the path to prevent path traversal attacks
+	if parsedURL.Path != "" {
+		// Clean the path to resolve . and .. elements
+		cleanPath := path.Clean(parsedURL.Path)
+		// Ensure the path doesn't escape the root
+		if !strings.HasPrefix(cleanPath, "/") {
+			cleanPath = "/" + cleanPath
+		}
+		parsedURL.Path = cleanPath
+	}
+
 	if parsedURL.RawQuery != "" || parsedURL.Fragment != "" {
 		logger.Debugf("Realm contains query or fragment components: %s", realm)
 		// Remove query and fragment to make it a valid issuer

pkg/runner/config_test.go

@@ -21,6 +21,10 @@ import (
 	"github.com/stacklok/toolhive/pkg/transport/types"
 )
 
+const (
+	localhostStr = "localhost"
+)
+
 func TestNewRunConfig(t *testing.T) {
 	t.Parallel()
 	config := NewRunConfig()
@@ -531,13 +535,13 @@ func TestRunConfigBuilder(t *testing.T) {
 		TargetPort: 9090,
 		Args:       []string{"--metadata-arg"},
 	}
-	host := "localhost"
+	host := localhostStr
 	debug := true
 	volumes := []string{"/host:/container"}
 	secretsList := []string{"secret1,target=ENV_VAR1"}
 	authzConfigPath := "" // Empty to skip loading the authorization configuration
 	permissionProfile := permissions.ProfileNone
-	targetHost := "localhost"
+	targetHost := localhostStr
 	mcpTransport := "sse"
 	proxyPort := 60000
 	targetPort := 9000
@@ -803,8 +807,8 @@ func TestRunConfigBuilder_MetadataOverrides(t *testing.T) {
 				WithCmdArgs(nil),
 				WithName("test-server"),
 				WithImage("test-image"),
-				WithHost("localhost"),
-				WithTargetHost("localhost"),
+				WithHost(localhostStr),
+				WithTargetHost(localhostStr),
 				WithDebug(false),
 				WithVolumes(nil),
 				WithSecrets(nil),
@@ -850,8 +854,8 @@ func TestRunConfigBuilder_EnvironmentVariableTransportDependency(t *testing.T) {
 		WithCmdArgs(nil),
 		WithName("test-server"),
 		WithImage("test-image"),
-		WithHost("localhost"),
-		WithTargetHost("localhost"),
+		WithHost(localhostStr),
+		WithTargetHost(localhostStr),
 		WithDebug(false),
 		WithVolumes(nil),
 		WithSecrets(nil),
@@ -902,8 +906,8 @@ func TestRunConfigBuilder_CmdArgsMetadataOverride(t *testing.T) {
 		WithCmdArgs(userArgs),
 		WithName("test-server"),
 		WithImage("test-image"),
-		WithHost("localhost"),
-		WithTargetHost("localhost"),
+		WithHost(localhostStr),
+		WithTargetHost(localhostStr),
 		WithDebug(false),
 		WithVolumes(nil),
 		WithSecrets(nil),
@@ -957,8 +961,8 @@ func TestRunConfigBuilder_CmdArgsMetadataDefaults(t *testing.T) {
 		WithCmdArgs(userArgs),
 		WithName("test-server"),
 		WithImage("test-image"),
-		WithHost("localhost"),
-		WithTargetHost("localhost"),
+		WithHost(localhostStr),
+		WithTargetHost(localhostStr),
 		WithDebug(false),
 		WithVolumes(nil),
 		WithSecrets(nil),
@@ -1008,8 +1012,8 @@ func TestRunConfigBuilder_VolumeProcessing(t *testing.T) {
 		WithCmdArgs(nil),
 		WithName("test-server"),
 		WithImage("test-image"),
-		WithHost("localhost"),
-		WithTargetHost("localhost"),
+		WithHost(localhostStr),
+		WithTargetHost(localhostStr),
 		WithDebug(false),
 		WithVolumes(volumes),
 		WithSecrets(nil),
@@ -1081,8 +1085,8 @@ func TestRunConfigBuilder_FilesystemMCPScenario(t *testing.T) {
 		WithCmdArgs(userArgs),
 		WithName("filesystem"),
 		WithImage("mcp/filesystem:latest"),
-		WithHost("localhost"),
-		WithTargetHost("localhost"),
+		WithHost(localhostStr),
+		WithTargetHost(localhostStr),
 		WithDebug(false),
 		WithVolumes(nil),
 		WithSecrets(nil),

pkg/runner/remote_auth.go

@@ -114,9 +114,19 @@ func (h *RemoteAuthHandler) discoverIssuerAndScopes(
 		return h.tryDiscoverFromResourceMetadata(ctx, authInfo.ResourceMetadata)
 	}
 
-	issuer := discovery.DeriveIssuerFromURL(remoteURL)
-	if issuer != "" {
-		return issuer, h.config.Scopes, nil, nil
+	// Priority 4: Try to discover actual issuer from the server's well-known endpoint
+	// This handles cases where the issuer differs from the server URL (e.g., Atlassian)
+	issuer, scopes, authServerInfo, err := h.tryDiscoverFromWellKnown(ctx, remoteURL)
+	if err == nil {
+		return issuer, scopes, authServerInfo, nil
+	}
+	logger.Debugf("Could not discover from well-known endpoint: %v", err)
+
+	// Priority 5: Last resort - derive issuer from URL without discovery
+	derivedIssuer := discovery.DeriveIssuerFromURL(remoteURL)
+	if derivedIssuer != "" {
+		logger.Infof("Using derived issuer from URL: %s", derivedIssuer)
+		return derivedIssuer, h.config.Scopes, nil, nil
 	}
 
 	// No issuer could be determined
@@ -183,3 +193,40 @@ func (*RemoteAuthHandler) findValidAuthServer(
 
 	return nil, ""
 }
+
+// tryDiscoverFromWellKnown attempts to discover the actual OAuth issuer
+// by probing the server's well-known endpoints without validating issuer match
+// This is useful when the issuer differs from the server URL (e.g., Atlassian case)
+func (h *RemoteAuthHandler) tryDiscoverFromWellKnown(
+	ctx context.Context,
+	remoteURL string,
+) (string, []string, *discovery.AuthServerInfo, error) {
+	// First try to derive a base URL from the remote URL
+	derivedURL := discovery.DeriveIssuerFromURL(remoteURL)
+	if derivedURL == "" {
+		return "", nil, nil, fmt.Errorf("could not derive base URL from %s", remoteURL)
+	}
+
+	// Try to discover the actual issuer without validation
+	// This uses DiscoverActualIssuer which doesn't validate issuer match
+	authServerInfo, err := discovery.ValidateAndDiscoverAuthServer(ctx, derivedURL)
+	if err != nil {
+		return "", nil, nil, fmt.Errorf("well-known discovery failed: %w", err)
+	}
+
+	// Successfully discovered the actual issuer
+	if authServerInfo.Issuer != derivedURL {
+		logger.Infof("Discovered actual issuer: %s (differs from server URL: %s)",
+			authServerInfo.Issuer, derivedURL)
+	}
+
+	// Determine scopes - use configured or fall back to defaults
+	scopes := h.config.Scopes
+	if len(scopes) == 0 {
+		// Use some reasonable defaults if no scopes configured
+		scopes = []string{"openid", "profile"}
+		logger.Debugf("No scopes configured, using defaults: %v", scopes)
+	}
+
+	return authServerInfo.Issuer, scopes, authServerInfo, nil
+}