Skip to content

Commit 9d031c0

Browse files
authored
fix(oauth): abort with oauth_client_id_required instead of empty-client_id authorize URL (#976)
* fix(oauth): abort with oauth_client_id_required instead of emitting empty-client_id authorize URL (#975) When a provider lacks a Dynamic Client Registration endpoint (GitHub's https://github.com/login/oauth advertises none), mcpproxy swallowed the DCR failure and proceeded as a 'public client without client_id', producing https://github.com/login/oauth/authorize?client_id=&... which GitHub 404s. OAuth public clients (RFC 8252 + PKCE) still require a client_id — PKCE replaces the client secret, not the id. - New Client.emptyClientIDFlowError guard: parses the final authorization URL (after oauth.extra_params injection) and returns a structured oauth_client_id_required / OAUTH_NO_CLIENT_ID contracts.OAuthFlowError with an actionable suggestion when client_id is empty or absent. - Wired into getAuthorizationURLQuick (REST POST /servers/{name}/login and 'mcpproxy auth login' daemon path) which previously had no guard — the CLI used to print 'OAuth flow initiated successfully' and exit 0 on a broken URL; it now exits 1 with the structured error. - Replaces the connection path's 403-only inline check (Figma case), which misreported every DCR failure as 'returned 403'; the guard is now unconditional on the final URL in both paths. Verified live against api.githubcopilot.com on an isolated instance: REST login returns the structured error (no auth_url), CLI exits 1 with suggestion, and a configured oauth.client_id still produces a working authorize URL. * fix(oauth): guard third URL-emission path + preserve real DCR outcome in error details Codex cross-review round 1 findings, both verified genuine: - handleOAuthAuthorizationWithResult (ForceOAuthFlowWithResult / manual CLI flows) still built, stored, and opened an empty-client_id authorize URL when DCR failed with anything other than a 403. Same guard now applied after extra-param injection, before the URL is stored or the browser opened. - The unified error previously discarded the actual DCR result (the old connection-path shape hardcoded 403/Forbidden for every failure, and the replacement dropped it entirely). emptyClientIDFlowError now takes the DCR error: details.dcr_status carries the real error text, with status_code=403 preserved when the failure was an actual 403 — a Figma-style rejection stays distinguishable from a GitHub-style missing registration endpoint. - All three 403-only early returns removed; every path now produces one consistent oauth_client_id_required shape via the final-URL guard. 3 new unit tests (403 details, unsupported details, no-DCR-attempt); verified live: REST login now returns dcr_status.error='server does not support dynamic client registration', zero authorize URLs in logs. * fix(oauth): diagnose extra_params client_id escape hatch + widen best-effort 403 detection Codex cross-review round 2, both verified: - A client_id injected only via oauth.extra_params passes the URL guard but is never sent by the handler at token exchange (mcp-go has no SetClientID), so it only works against lenient providers. Keep the escape hatch (pre-existing behavior) but WARN with a pointer to oauth.client_id so the post-callback failure is diagnosable. Guard now takes the handler via a clientIDProvider interface. - DCRStatus.StatusCode: also map 'Forbidden' to 403; documented as best-effort since mcp-go registration errors are untyped (a 403 with a valid OAuth JSON body surfaces without any status text) — the verbatim error string remains the authoritative detail. 2 new unit tests (10 total). * fix(oauth): drop incorrect extra_params token-exchange warning Codex round 3 (verified): OAuthTransportWrapper injects every oauth.extra_params entry — client_id included — into token request bodies, so an extra_params client_id DOES reach token exchange and the round-2 warning would have misdiagnosed working configurations. Warning and the clientIDProvider plumbing removed; the guard comment now documents why extra_params client_id pass-through is fully functional.
1 parent 2b90a10 commit 9d031c0

2 files changed

Lines changed: 232 additions & 72 deletions

File tree

‎internal/upstream/core/connection_oauth.go‎

Lines changed: 94 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,6 +1010,7 @@ func (c *Client) handleOAuthAuthorization(ctx context.Context, authErr error, oa
10101010

10111011
// Determine OAuth mode and attempt registration if needed
10121012
var oauthMode string
1013+
var dcrErr error
10131014
if hasStaticCredentials {
10141015
// Skip DCR when static credentials are provided in config
10151016
oauthMode = "static credentials"
@@ -1040,7 +1041,9 @@ func (c *Client) handleOAuthAuthorization(ctx context.Context, authErr error, oa
10401041
}()
10411042

10421043
if regErr != nil {
1043-
// DCR failed - proceed with public client OAuth (PKCE without client_id)
1044+
// DCR failed - proceed and let the empty-client_id guard below
1045+
// surface a structured error if no client_id materializes
1046+
dcrErr = regErr
10441047
oauthMode = "public client (PKCE)"
10451048
c.logger.Warn("⚠️ Dynamic Client Registration not supported - using public client OAuth with PKCE",
10461049
zap.String("server", c.config.Name),
@@ -1080,9 +1083,11 @@ func (c *Client) handleOAuthAuthorization(ctx context.Context, authErr error, oa
10801083
}
10811084
}
10821085

1083-
// Continue with OAuth flow regardless of DCR result
1084-
// Public client OAuth (RFC 8252) with PKCE doesn't require client_id
1085-
// If server doesn't support this, it will reject the authorization request
1086+
// Continue with the OAuth flow when a client_id is available (static,
1087+
// persisted, or obtained via DCR). OAuth public clients (RFC 8252 + PKCE)
1088+
// still require a client_id — PKCE replaces the client secret, not the
1089+
// id — so the empty-client_id guard below aborts before opening a
1090+
// guaranteed-broken authorization URL (issue #975).
10861091

10871092
c.logger.Info("🌟 Starting OAuth authentication flow",
10881093
zap.String("server", c.config.Name),
@@ -1159,36 +1164,12 @@ func (c *Client) handleOAuthAuthorization(ctx context.Context, authErr error, oa
11591164
}
11601165
}
11611166

1162-
// Check if we're attempting public client OAuth with empty client_id
1163-
// Some servers (like Figma) advertise DCR but return 403, then reject empty client_id
1164-
parsedAuthURL, parseErr := url.Parse(authURL)
1165-
if parseErr == nil {
1166-
clientIDParam := parsedAuthURL.Query().Get("client_id")
1167-
if clientIDParam == "" && oauthMode == "public client (PKCE)" {
1168-
c.logger.Error("❌ OAuth server requires client_id but DCR failed",
1169-
zap.String("server", c.config.Name),
1170-
zap.String("url", c.config.URL),
1171-
zap.String("help", "Configure oauth.client_id in server config or contact the OAuth provider"))
1172-
// Return structured error for Spec 020
1173-
return &contracts.OAuthFlowError{
1174-
Success: false,
1175-
ErrorType: contracts.OAuthErrorClientIDRequired,
1176-
ErrorCode: contracts.OAuthCodeNoClientID,
1177-
ServerName: c.config.Name,
1178-
Message: fmt.Sprintf("Server '%s' requires client_id but Dynamic Client Registration returned 403", c.config.Name),
1179-
Details: &contracts.OAuthErrorDetails{
1180-
ServerURL: c.config.URL,
1181-
DCRStatus: &contracts.DCRStatus{
1182-
Attempted: true,
1183-
Success: false,
1184-
StatusCode: 403,
1185-
Error: "Forbidden",
1186-
},
1187-
},
1188-
Suggestion: "Register an OAuth app with the provider and configure oauth.client_id in server config.",
1189-
DebugHint: fmt.Sprintf("For logs: mcpproxy upstream logs %s", c.config.Name),
1190-
}
1191-
}
1167+
// Never proceed with an authorization URL that lacks a client_id — the
1168+
// provider will reject it (e.g. Figma after a DCR 403, GitHub which has no
1169+
// DCR endpoint at all; issue #975). Checked on the final URL so a
1170+
// client_id supplied via oauth.extra_params still passes.
1171+
if flowErr := c.emptyClientIDFlowError(authURL, "", dcrErr); flowErr != nil {
1172+
return flowErr
11921173
}
11931174

11941175
// Always log the computed authorization URL so users can copy/paste if auto-launch fails.
@@ -1393,6 +1374,7 @@ func (c *Client) handleOAuthAuthorizationWithResult(ctx context.Context, authErr
13931374
hasStaticCredentials := c.config.OAuth != nil && c.config.OAuth.ClientID != ""
13941375
hasPersistedCredentials := oauthConfig.ClientID != ""
13951376

1377+
var dcrErr error
13961378
if !hasStaticCredentials && !hasPersistedCredentials {
13971379
c.logger.Info("📋 Attempting Dynamic Client Registration (optional)",
13981380
zap.String("server", c.config.Name))
@@ -1412,31 +1394,12 @@ func (c *Client) handleOAuthAuthorizationWithResult(ctx context.Context, authErr
14121394
}()
14131395

14141396
if regErr != nil {
1397+
// DCR failed - the empty-client_id guard below surfaces a
1398+
// structured error if no client_id materializes (issue #975)
1399+
dcrErr = regErr
14151400
c.logger.Info("ℹ️ DCR not available, continuing with public client OAuth",
14161401
zap.String("server", c.config.Name),
14171402
zap.Error(regErr))
1418-
1419-
if strings.Contains(regErr.Error(), "403") {
1420-
return result, &contracts.OAuthFlowError{
1421-
Success: false,
1422-
ErrorType: contracts.OAuthErrorClientIDRequired,
1423-
ErrorCode: contracts.OAuthCodeNoClientID,
1424-
ServerName: c.config.Name,
1425-
CorrelationID: result.CorrelationID,
1426-
Message: fmt.Sprintf("Server '%s' requires client_id but Dynamic Client Registration returned 403", c.config.Name),
1427-
Details: &contracts.OAuthErrorDetails{
1428-
ServerURL: c.config.URL,
1429-
DCRStatus: &contracts.DCRStatus{
1430-
Attempted: true,
1431-
Success: false,
1432-
StatusCode: 403,
1433-
Error: "Forbidden",
1434-
},
1435-
},
1436-
Suggestion: "Register an OAuth app with the provider and configure oauth.client_id in server config.",
1437-
DebugHint: fmt.Sprintf("For logs: mcpproxy upstream logs %s", c.config.Name),
1438-
}
1439-
}
14401403
} else {
14411404
clientID := oauthHandler.GetClientID()
14421405
clientSecret := oauthHandler.GetClientSecret()
@@ -1519,6 +1482,12 @@ func (c *Client) handleOAuthAuthorizationWithResult(ctx context.Context, authErr
15191482
}
15201483
}
15211484

1485+
// Never store or open an authorization URL that lacks a client_id — the
1486+
// provider is guaranteed to reject it (issue #975).
1487+
if flowErr := c.emptyClientIDFlowError(authURL, result.CorrelationID, dcrErr); flowErr != nil {
1488+
return result, flowErr
1489+
}
1490+
15221491
// Store the auth URL in the result
15231492
result.AuthURL = authURL
15241493
c.logger.Info("🌐 Authorization URL obtained",
@@ -1914,6 +1883,7 @@ func (c *Client) getAuthorizationURLQuick(ctx context.Context, oauthConfig *clie
19141883
hasStaticCredentials := c.config.OAuth != nil && c.config.OAuth.ClientID != ""
19151884
hasPersistedCredentials := oauthConfig.ClientID != ""
19161885

1886+
var dcrErr error
19171887
if !hasStaticCredentials && !hasPersistedCredentials {
19181888
c.logger.Info("📋 Attempting Dynamic Client Registration (DCR)",
19191889
zap.String("server", c.config.Name),
@@ -1930,26 +1900,13 @@ func (c *Client) getAuthorizationURLQuick(ctx context.Context, oauthConfig *clie
19301900
}()
19311901

19321902
if regErr != nil {
1903+
// DCR failed - the empty-client_id guard below surfaces a
1904+
// structured error if no client_id materializes (issue #975)
1905+
dcrErr = regErr
19331906
c.logger.Warn("⚠️ DCR failed",
19341907
zap.String("server", c.config.Name),
19351908
zap.String("correlation_id", correlationID),
19361909
zap.Error(regErr))
1937-
1938-
if strings.Contains(regErr.Error(), "403") {
1939-
c.logger.Error("❌ DCR returned 403 - client_id required",
1940-
zap.String("server", c.config.Name),
1941-
zap.String("correlation_id", correlationID),
1942-
zap.String("suggestion", "Register an OAuth app with the provider"))
1943-
return "", nil, "", "", &contracts.OAuthFlowError{
1944-
Success: false,
1945-
ErrorType: contracts.OAuthErrorClientIDRequired,
1946-
ErrorCode: contracts.OAuthCodeNoClientID,
1947-
ServerName: c.config.Name,
1948-
CorrelationID: correlationID,
1949-
Message: fmt.Sprintf("Server '%s' requires client_id but DCR returned 403", c.config.Name),
1950-
Suggestion: "Register an OAuth app with the provider and configure oauth.client_id in server config.",
1951-
}
1952-
}
19531910
} else {
19541911
c.logger.Info("✅ DCR succeeded",
19551912
zap.String("server", c.config.Name),
@@ -2017,9 +1974,74 @@ func (c *Client) getAuthorizationURLQuick(ctx context.Context, oauthConfig *clie
20171974
}
20181975
}
20191976

1977+
// Issue #975: never hand back an authorization URL without a client_id —
1978+
// the provider is guaranteed to reject it (GitHub 404s on client_id=).
1979+
if flowErr := c.emptyClientIDFlowError(authURL, correlationID, dcrErr); flowErr != nil {
1980+
return "", nil, "", "", flowErr
1981+
}
1982+
20201983
return authURL, oauthHandler, codeVerifier, state, nil
20211984
}
20221985

1986+
// emptyClientIDFlowError returns a structured oauth_client_id_required error
1987+
// when the final authorization URL carries no client_id. OAuth public clients
1988+
// (RFC 8252 + PKCE) still require a client_id — PKCE replaces the client
1989+
// secret, not the id — so a provider is guaranteed to reject such a URL
1990+
// (GitHub responds 404; issue #975). Returns nil when the URL has a client_id
1991+
// or cannot be parsed. dcrErr, when non-nil, is the DCR failure that left the
1992+
// client without an id; its real outcome is preserved in the error details so
1993+
// a 403 rejection (Figma) stays distinguishable from a provider with no
1994+
// registration endpoint at all (GitHub).
1995+
func (c *Client) emptyClientIDFlowError(authURL, correlationID string, dcrErr error) *contracts.OAuthFlowError {
1996+
parsed, err := url.Parse(authURL)
1997+
if err != nil {
1998+
return nil
1999+
}
2000+
if parsed.Query().Get("client_id") != "" {
2001+
// A client_id present in the URL but absent from the OAuth handler
2002+
// came from oauth.extra_params. That configuration is fully
2003+
// functional: OAuthTransportWrapper (internal/oauth/config.go) injects
2004+
// every extra param — client_id included — into token requests as
2005+
// well, so the exchange after callback carries it too.
2006+
return nil
2007+
}
2008+
c.logger.Error("❌ OAuth provider requires a client_id but none is available (DCR unsupported or failed)",
2009+
zap.String("server", c.config.Name),
2010+
zap.String("url", c.config.URL),
2011+
zap.NamedError("dcr_error", dcrErr),
2012+
zap.String("help", "Register an OAuth app with the provider and set oauth.client_id in the server config"))
2013+
details := &contracts.OAuthErrorDetails{
2014+
ServerURL: c.config.URL,
2015+
}
2016+
if dcrErr != nil {
2017+
dcrStatus := &contracts.DCRStatus{
2018+
Attempted: true,
2019+
Success: false,
2020+
Error: dcrErr.Error(),
2021+
}
2022+
// Best-effort: mcp-go returns untyped registration errors (a 403 whose
2023+
// body is valid OAuth JSON surfaces as e.g. "OAuth error:
2024+
// unauthorized_client" with no status), so the code is only set when
2025+
// the text makes it unambiguous; the verbatim error above is the
2026+
// authoritative detail.
2027+
if strings.Contains(dcrErr.Error(), "403") || strings.Contains(dcrErr.Error(), "Forbidden") {
2028+
dcrStatus.StatusCode = 403
2029+
}
2030+
details.DCRStatus = dcrStatus
2031+
}
2032+
return &contracts.OAuthFlowError{
2033+
Success: false,
2034+
ErrorType: contracts.OAuthErrorClientIDRequired,
2035+
ErrorCode: contracts.OAuthCodeNoClientID,
2036+
ServerName: c.config.Name,
2037+
CorrelationID: correlationID,
2038+
Message: fmt.Sprintf("Server '%s' requires a client_id: the OAuth provider does not support Dynamic Client Registration (or registration failed) and no oauth.client_id is configured", c.config.Name),
2039+
Details: details,
2040+
Suggestion: "Register an OAuth app with the provider and set oauth.client_id in the server config.",
2041+
DebugHint: fmt.Sprintf("For logs: mcpproxy upstream logs %s", c.config.Name),
2042+
}
2043+
}
2044+
20232045
// waitForOAuthCallbackAsync waits for OAuth callback and handles token exchange in background.
20242046
func (c *Client) waitForOAuthCallbackAsync(ctx context.Context, oauthHandler *uptransport.OAuthHandler, codeVerifier, state, correlationID string) {
20252047
c.markOAuthInProgress()
Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
package core
2+
3+
import (
4+
"errors"
5+
"testing"
6+
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
"go.uber.org/zap"
10+
11+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
12+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts"
13+
)
14+
15+
func newClientIDGuardTestClient(t *testing.T) *Client {
16+
t.Helper()
17+
return &Client{
18+
config: &config.ServerConfig{
19+
Name: "github",
20+
URL: "https://api.githubcopilot.com/mcp/",
21+
},
22+
logger: zap.NewNop(),
23+
}
24+
}
25+
26+
// Issue #975: providers without Dynamic Client Registration (e.g. GitHub) must
27+
// produce a structured oauth_client_id_required error instead of an
28+
// authorization URL with an empty client_id, which the provider 404s.
29+
func TestEmptyClientIDFlowError_EmptyClientID(t *testing.T) {
30+
c := newClientIDGuardTestClient(t)
31+
32+
authURL := "https://github.com/login/oauth/authorize?client_id=&code_challenge=abc&code_challenge_method=S256&redirect_uri=http%3A%2F%2F127.0.0.1%3A62876%2Foauth%2Fcallback"
33+
flowErr := c.emptyClientIDFlowError(authURL, "corr-123", errors.New("server does not support dynamic client registration"))
34+
35+
require.NotNil(t, flowErr, "empty client_id in authorization URL must be rejected")
36+
assert.Equal(t, contracts.OAuthErrorClientIDRequired, flowErr.ErrorType)
37+
assert.Equal(t, contracts.OAuthCodeNoClientID, flowErr.ErrorCode)
38+
assert.Equal(t, "github", flowErr.ServerName)
39+
assert.Equal(t, "corr-123", flowErr.CorrelationID)
40+
assert.Contains(t, flowErr.Suggestion, "oauth.client_id")
41+
assert.False(t, flowErr.Success)
42+
}
43+
44+
func TestEmptyClientIDFlowError_MissingClientIDParam(t *testing.T) {
45+
c := newClientIDGuardTestClient(t)
46+
47+
authURL := "https://github.com/login/oauth/authorize?code_challenge=abc&code_challenge_method=S256"
48+
flowErr := c.emptyClientIDFlowError(authURL, "", nil)
49+
50+
require.NotNil(t, flowErr, "authorization URL without client_id param must be rejected")
51+
assert.Equal(t, contracts.OAuthErrorClientIDRequired, flowErr.ErrorType)
52+
}
53+
54+
func TestEmptyClientIDFlowError_PresentClientID(t *testing.T) {
55+
c := newClientIDGuardTestClient(t)
56+
57+
authURL := "https://github.com/login/oauth/authorize?client_id=Iv1.abc123&code_challenge=abc"
58+
flowErr := c.emptyClientIDFlowError(authURL, "corr-123", nil)
59+
60+
assert.Nil(t, flowErr, "a URL carrying a client_id must pass the guard")
61+
}
62+
63+
func TestEmptyClientIDFlowError_ClientIDViaExtraParams(t *testing.T) {
64+
c := newClientIDGuardTestClient(t)
65+
66+
// A client_id injected through oauth.extra_params must also satisfy the
67+
// guard — the check is on the final URL, not on the handler state.
68+
authURL := "https://example.com/authorize?client_id=from-extra-params&resource=https%3A%2F%2Fexample.com"
69+
flowErr := c.emptyClientIDFlowError(authURL, "", nil)
70+
71+
assert.Nil(t, flowErr)
72+
}
73+
74+
func TestEmptyClientIDFlowError_UnparseableURL(t *testing.T) {
75+
c := newClientIDGuardTestClient(t)
76+
77+
// An unparseable URL is not this guard's concern — never block on it.
78+
flowErr := c.emptyClientIDFlowError("://not-a-url", "", nil)
79+
80+
assert.Nil(t, flowErr)
81+
}
82+
83+
// The structured error must preserve the real DCR outcome: a 403 rejection
84+
// (e.g. Figma) keeps its status code, a provider without a registration
85+
// endpoint (e.g. GitHub) keeps the "not supported" message — they are
86+
// distinguishable in the error details.
87+
func TestEmptyClientIDFlowError_DCR403Details(t *testing.T) {
88+
c := newClientIDGuardTestClient(t)
89+
90+
authURL := "https://example.com/authorize?client_id="
91+
flowErr := c.emptyClientIDFlowError(authURL, "", errors.New("registration failed: HTTP 403 Forbidden"))
92+
93+
require.NotNil(t, flowErr)
94+
require.NotNil(t, flowErr.Details)
95+
require.NotNil(t, flowErr.Details.DCRStatus)
96+
assert.True(t, flowErr.Details.DCRStatus.Attempted)
97+
assert.False(t, flowErr.Details.DCRStatus.Success)
98+
assert.Equal(t, 403, flowErr.Details.DCRStatus.StatusCode)
99+
assert.Contains(t, flowErr.Details.DCRStatus.Error, "403")
100+
}
101+
102+
func TestEmptyClientIDFlowError_DCRUnsupportedDetails(t *testing.T) {
103+
c := newClientIDGuardTestClient(t)
104+
105+
authURL := "https://github.com/login/oauth/authorize?client_id="
106+
flowErr := c.emptyClientIDFlowError(authURL, "", errors.New("server does not support dynamic client registration"))
107+
108+
require.NotNil(t, flowErr)
109+
require.NotNil(t, flowErr.Details)
110+
require.NotNil(t, flowErr.Details.DCRStatus)
111+
assert.True(t, flowErr.Details.DCRStatus.Attempted)
112+
assert.False(t, flowErr.Details.DCRStatus.Success)
113+
assert.Zero(t, flowErr.Details.DCRStatus.StatusCode)
114+
assert.Contains(t, flowErr.Details.DCRStatus.Error, "does not support dynamic client registration")
115+
}
116+
117+
func TestEmptyClientIDFlowError_NoDCRAttempt(t *testing.T) {
118+
c := newClientIDGuardTestClient(t)
119+
120+
// nil dcrErr (DCR not attempted or succeeded): no DCRStatus fabricated.
121+
flowErr := c.emptyClientIDFlowError("https://example.com/authorize?client_id=", "", nil)
122+
123+
require.NotNil(t, flowErr)
124+
require.NotNil(t, flowErr.Details)
125+
assert.Nil(t, flowErr.Details.DCRStatus)
126+
}
127+
128+
// Codex r2 finding 2: "Forbidden" without a literal "403" still maps to
129+
// status_code 403 (best-effort; mcp-go errors are untyped).
130+
func TestEmptyClientIDFlowError_ForbiddenMapsTo403(t *testing.T) {
131+
c := newClientIDGuardTestClient(t)
132+
133+
flowErr := c.emptyClientIDFlowError("https://example.com/authorize?client_id=", "", errors.New("registration request failed: Forbidden"))
134+
135+
require.NotNil(t, flowErr)
136+
require.NotNil(t, flowErr.Details.DCRStatus)
137+
assert.Equal(t, 403, flowErr.Details.DCRStatus.StatusCode)
138+
}

0 commit comments

Comments
 (0)