chore: gh-host as oauth auth server by atharva1051 · Pull Request #2046 · github/github-mcp-server

atharva1051 · 2026-02-19T12:16:14Z

Summary

Conditionally set AuthorizationServer only when --gh-host is explicitly provided, ensuring github.com users receive the correct default.

Why

To ensure that OAuth protected resource metadata correctly exposes the authorization server endpoint for all clients, including those using github.com where cfg.Host might be empty. This conditional setting prevents an invalid AuthorizationServer value and ensures proper discovery of authentication details.

What changed

pkg/http/server.go: AuthorizationServer is now only overridden when cfg.Host != "". Otherwise, it falls back to DefaultAuthorizationServer (https://github.com/login/oauth).

MCP impact

No tool or API changes

Prompts tested (tool changes only)

N/A

Security / limits

Auth / permissions considered — OAuth authorization server URL now correctly resolves for both github.com and GHES

Tool renaming

I am not renaming tools as part of this PR

Lint & tests

Linted locally with ./script/lint
Tested locally with ./script/test

Docs

Not needed

When gh-host is not set (github.com users), the AuthorizationServer field was being set to just "login/oauth" (empty host + path), breaking OAuth metadata. Now it only overrides the default when gh-host is provided (GHES users), allowing github.com users to get the correct default of "https://github.com/login/oauth". Co-authored-by: atharva1051 <53966412+atharva1051@users.noreply.github.com>

…rver-condition Fix: Only set custom OAuth AuthorizationServer when gh-host is configured

Copilot

Pull request overview

Adjusts HTTP-mode OAuth protected resource metadata so the authorization server URL is only overridden when --gh-host is explicitly provided, allowing github.com users to fall back to the built-in default authorization server.

Changes:

Conditionally set oauthCfg.AuthorizationServer only when cfg.Host != "" (otherwise rely on oauth.NewAuthHandler’s default).
Avoid generating an invalid authorization server value when cfg.Host is empty.

pkg/http/server.go

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

arhiv1973b

LGTM! ✅

atharva1051 · 2026-02-23T12:10:30Z

Hello @SamMorrowDrums or @omgitsads ,

Could you please review this once?

Thank you

pkg/http/server.go

omgitsads · 2026-02-23T16:51:41Z

Hi @atharva1051, thanks for raising this issue. I think theres some additional complexity around handling GHES for both http/https, so not quite as simple as just putting the cfg.Host in.

Like I said in the review, I think a better place to make this decision is in during the url resolution in the APIHostResolver implementation, then passing the resolver into the OAuth Handler. I put together a quick PR of that, but happy to make those changes here so that your contribution is not just dismissed.

atharva1051 · 2026-02-23T17:13:45Z

Oh that would be really helpful, although i did end up making few changes the moment I saw your comment.
But I got the idea. Thanks :) Do also re-review my changes for learning purposes.

Copilot

Pull request overview

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

Copilot · 2026-02-23T19:35:21Z

pkg/utils/api.go

 	BaseRESTURL(ctx context.Context) (*url.URL, error)
 	GraphqlURL(ctx context.Context) (*url.URL, error)
 	UploadURL(ctx context.Context) (*url.URL, error)
 	RawURL(ctx context.Context) (*url.URL, error)
+	OAuthURL(ctx context.Context) (*url.URL, error)


Adding OAuthURL to the exported utils.APIHostResolver interface is a source-breaking change for any downstream code that implements this interface. To avoid breaking external consumers, consider keeping APIHostResolver focused on API endpoints and introducing a separate, smaller interface for OAuth resolution (e.g., an interface with just OAuthURL), then have utils.APIHost satisfy both.

Copilot · 2026-02-23T19:35:21Z

pkg/http/oauth/oauth.go

+		oauthURL, err := cfg.APIHost.OAuthURL(context.Background())
+		if err != nil {
+			return nil, fmt.Errorf("failed to get OAuth URL from API host: %w", err)
+		}


NewAuthHandler assumes APIHost.OAuthURL returns a non-nil *url.URL; if an implementation returns (nil, nil), oauthURL.String() will panic. Please add a nil check (and return a descriptive error) before dereferencing, and consider validating the URL has scheme/host to avoid emitting invalid metadata.

Suggested change

}

}

if oauthURL == nil {

return nil, fmt.Errorf("API host returned nil OAuth URL")

}

if oauthURL.Scheme == "" || oauthURL.Host == "" {

return nil, fmt.Errorf("API host returned invalid OAuth URL: missing scheme or host")

}

Copilot · 2026-02-23T19:35:22Z

pkg/utils/api_test.go

+			name:          "GHES with HTTP and custom port (port stripped - not supported yet)",
+			host:          "http://ghes.local:8080",
+			expectedOAuth: "http://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment
+		},
+		{
+			name:          "GHES with HTTPS and custom port (port stripped - not supported yet)",
+			host:          "https://ghes.local:8443",
+			expectedOAuth: "https://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment


This test case codifies the current behavior of silently stripping ports from the configured host. Since parseAPIHost explicitly notes ports are not handled, accepting a host with a port but then discarding it can lead to requests being sent to the wrong endpoint. Prefer returning an error when a port is present (until ports are supported), and update this test to expect an error instead of a rewritten URL.

Suggested change

name: "GHES with HTTP and custom port (port stripped - not supported yet)",

host: "http://ghes.local:8080",

expectedOAuth: "http://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment

},

{

name: "GHES with HTTPS and custom port (port stripped - not supported yet)",

host: "https://ghes.local:8443",

expectedOAuth: "https://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment

name: "GHES with HTTP and custom port (should error - ports not supported)",

host: "http://ghes.local:8080",

expectError: true,

},

{

name: "GHES with HTTPS and custom port (should error - ports not supported)",

host: "https://ghes.local:8443",

expectError: true,

Copilot · 2026-02-23T19:35:22Z

pkg/utils/api_test.go

+			expectedOAuth: "http://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment
+		},
+		{
+			name:          "GHES with HTTPS and custom port (port stripped - not supported yet)",
+			host:          "https://ghes.local:8443",
+			expectedOAuth: "https://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment


Referencing a specific line number in another file ("ref: ln222") is brittle and will quickly go stale as the file changes. Prefer referencing the function name (parseAPIHost) or quoting the relevant comment text instead.

Suggested change

expectedOAuth: "http://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment

},

{

name: "GHES with HTTPS and custom port (port stripped - not supported yet)",

host: "https://ghes.local:8443",

expectedOAuth: "https://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment

expectedOAuth: "http://ghes.local/login/oauth", // Port is stripped; ports are not yet supported by parseAPIHost

},

{

name: "GHES with HTTPS and custom port (port stripped - not supported yet)",

host: "https://ghes.local:8443",

expectedOAuth: "https://ghes.local/login/oauth", // Port is stripped; ports are not yet supported by parseAPIHost

Copilot · 2026-02-23T19:35:22Z

pkg/utils/api_test.go

+			expectedOAuth: "http://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment
+		},
+		{
+			name:          "GHES with HTTPS and custom port (port stripped - not supported yet)",
+			host:          "https://ghes.local:8443",
+			expectedOAuth: "https://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment


Same issue here: the "ref: ln222" line-number reference is brittle. Consider updating the comment to reference the parseAPIHost port-handling note without tying it to a specific line number.

Suggested change

expectedOAuth: "http://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment

},

{

name: "GHES with HTTPS and custom port (port stripped - not supported yet)",

host: "https://ghes.local:8443",

expectedOAuth: "https://ghes.local/login/oauth", // Port is stripped ref: ln222 api.go comment

expectedOAuth: "http://ghes.local/login/oauth", // Port is stripped; see parseAPIHost port-handling note in api.go

},

{

name: "GHES with HTTPS and custom port (port stripped - not supported yet)",

host: "https://ghes.local:8443",

expectedOAuth: "https://ghes.local/login/oauth", // Port is stripped; see parseAPIHost port-handling note in api.go

Copilot · 2026-02-23T19:35:22Z

pkg/http/oauth/oauth.go

+	// APIHost is the GitHub API host resolver that provides OAuth URL.
+	// If set, this takes precedence over AuthorizationServer.
+	APIHost utils.APIHostResolver
+
 	// AuthorizationServer is the OAuth authorization server URL.
 	// Defaults to GitHub's OAuth server if not specified.
+	// This field is ignored if APIHost is set.
 	AuthorizationServer string


oauth.Config depends on utils.APIHostResolver, which requires several unrelated URL methods that oauth doesn’t use. This increases coupling and is what forced a breaking interface change in utils. Consider changing Config.APIHost to a minimal local interface (e.g., OAuthURL(ctx) (*url.URL, error)) so callers can supply only what oauth needs and utils.APIHost can be adapted without expanding exported interfaces.

atharva1051 and others added 4 commits February 19, 2026 12:15

chore: gh-host as oauth auth server

0ef5a75

Initial plan

ee63c51

Merge pull request #7 from mercedes-benz/copilot/fix-authorization-se…

c7931aa

…rver-condition Fix: Only set custom OAuth AuthorizationServer when gh-host is configured

atharva1051 marked this pull request as ready for review February 19, 2026 12:35

atharva1051 requested a review from a team as a code owner February 19, 2026 12:35

Copilot AI review requested due to automatic review settings February 19, 2026 12:35

Copilot started reviewing on behalf of atharva1051 February 19, 2026 12:35 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

pkg/http/server.go Outdated Show resolved Hide resolved

pkg/http/server.go Show resolved Hide resolved

SamMorrowDrums reviewed Feb 19, 2026

View reviewed changes

pkg/http/server.go Outdated Show resolved Hide resolved

fix: joining url using net/url

f5b8ecc

atharva1051 requested a review from Copilot February 19, 2026 13:56

Copilot started reviewing on behalf of atharva1051 February 19, 2026 13:57 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

Merge branch 'github:main' into main

f202f14

atharva1051 requested a review from SamMorrowDrums February 19, 2026 18:15

arhiv1973b approved these changes Feb 20, 2026

View reviewed changes

atharva1051 added 2 commits February 22, 2026 19:01

Merge branch 'main' into main

841dc1b

Merge branch 'main' into main

43e207d

omgitsads reviewed Feb 23, 2026

View reviewed changes

pkg/http/server.go Outdated Show resolved Hide resolved

omgitsads mentioned this pull request Feb 23, 2026

Generate authorization server urls during API resolution. #2070

Open

13 tasks

fix: extended api host to have a oauth url

412c6c9

atharva1051 added 2 commits February 23, 2026 17:19

chore: oauth accepts APIHostResolver as argument

fbc5ba1

Merge branch 'main' into main

82eaaaa

atharva1051 requested a review from Copilot February 23, 2026 19:31

Copilot started reviewing on behalf of atharva1051 February 23, 2026 19:32 View session

Copilot AI reviewed Feb 23, 2026

View reviewed changes

-		}
+		}
+		if oauthURL == nil {
+			return nil, fmt.Errorf("API host returned nil OAuth URL")
+		}
+		if oauthURL.Scheme == "" || oauthURL.Host == "" {
+			return nil, fmt.Errorf("API host returned invalid OAuth URL: missing scheme or host")
+		}

Comments

Conversation

atharva1051 commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

What changed

MCP impact

Prompts tested (tool changes only)

Security / limits

Tool renaming

Lint & tests

Docs

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

arhiv1973b left a comment

Choose a reason for hiding this comment

Uh oh!

atharva1051 commented Feb 23, 2026

Uh oh!

Uh oh!

omgitsads commented Feb 23, 2026

Uh oh!

atharva1051 commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

atharva1051 commented Feb 19, 2026 •

edited

Loading

atharva1051 commented Feb 23, 2026 •

edited

Loading