refactor(oauth): address review — omit empty bearer header, guard tok…

…en/oauth - BearerAuthTransport omits the Authorization header entirely when the token is empty (pre-authorization) rather than sending an empty "Bearer " value. - RunStdioServer rejects the ambiguous combination of a static Token and an OAuthManager up front, enforcing the documented mutual exclusivity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SamMorrowDrums · Jun 26, 2026 · Jun 16, 2026 · Jun 16, 2026 · Jun 18, 2026 · Jun 25, 2026
commit 622d429004373b3f508ed1ccbef164a1c5973e84
diff --git a/internal/ghmcp/oauth_test.go b/internal/ghmcp/oauth_test.go
@@ -286,6 +286,22 @@ func TestCreateOAuthMiddleware(t *testing.T) {
 	})
 }
 
+// TestRunStdioServerRejectsTokenAndOAuth verifies the mutually-exclusive guard:
+// supplying both a static token and an OAuth manager is rejected before the
+// server starts, rather than silently preferring one for auth and the other for
+// scope filtering.
+func TestRunStdioServerRejectsTokenAndOAuth(t *testing.T) {
+	t.Parallel()
+
+	mgr := oauth.NewManager(oauth.NewGitHubConfig("client-id", "", nil, "", 0), discardLogger())
+	err := RunStdioServer(StdioServerConfig{
+		Token:        "ghp_static",
+		OAuthManager: mgr,
+	})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "mutually exclusive")
+}
+
 // TestCreateGitHubClientsTokenProvider proves the OAuth wiring: when a
 // TokenProvider is configured the REST client authenticates with the provider's
 // current token on every request (and never pins a stale one), which is what the
@@ -317,7 +333,7 @@ func TestCreateGitHubClientsTokenProvider(t *testing.T) {
 	}
 
 	do()
-	assert.Equal(t, "Bearer", gotAuth, "no token before authorization")
+	assert.Equal(t, "", gotAuth, "no auth header before authorization")
 
 	current = "oauth-token"
 	do()

diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go
@@ -261,6 +261,13 @@ type StdioServerConfig struct {
 
 // RunStdioServer is not concurrent safe.
 func RunStdioServer(cfg StdioServerConfig) error {
+	// OAuth login and a static token are mutually exclusive: they would
+	// disagree on how the token is sourced (lazy provider vs. static) and on
+	// scope filtering, so reject the ambiguous combination up front.
+	if cfg.OAuthManager != nil && cfg.Token != "" {
+		return fmt.Errorf("OAuthManager and a static Token are mutually exclusive: provide one or the other")
+	}
+
 	// Create app context
 	ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
 	defer stop()

diff --git a/pkg/http/transport/bearer.go b/pkg/http/transport/bearer.go
@@ -25,7 +25,11 @@ func (t *BearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro
 	if t.TokenProvider != nil {
 		token = t.TokenProvider()
 	}
-	req.Header.Set(headers.AuthorizationHeader, "Bearer "+token)
+	// Before OAuth authorization completes the token is empty; send an
+	// unauthenticated request rather than an empty "Bearer " header.
+	if token != "" {
+		req.Header.Set(headers.AuthorizationHeader, "Bearer "+token)
+	}
 
 	// Check for GraphQL-Features in context and add header if present
 	if features := ghcontext.GetGraphQLFeatures(req.Context()); len(features) > 0 {

diff --git a/pkg/http/transport/bearer_test.go b/pkg/http/transport/bearer_test.go
@@ -41,7 +41,7 @@ func TestBearerAuthTransport(t *testing.T) {
 		{
 			name:          "token provider may return empty before authorization",
 			tokenProvider: func() string { return "" },
-			wantAuth:      "Bearer",
+			wantAuth:      "",
 		},
 	}
 
@@ -103,7 +103,7 @@ func TestBearerAuthTransport_TokenProviderResolvedPerRequest(t *testing.T) {
 	}
 
 	do()
-	assert.Equal(t, "Bearer", gotAuth, "no token yet before authorization")
+	assert.Equal(t, "", gotAuth, "no auth header before authorization")
 
 	current = "first-token"
 	do()