Skip to content

Comments

chore: gh-host as oauth auth server#2046

Merged
omgitsads merged 12 commits intogithub:mainfrom
mercedes-benz:main
Feb 24, 2026
Merged

chore: gh-host as oauth auth server#2046
omgitsads merged 12 commits intogithub:mainfrom
mercedes-benz:main

Conversation

@atharva1051
Copy link
Contributor

@atharva1051 atharva1051 commented Feb 19, 2026

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

The program was tested solely for our own use cases, which might differ from yours.

Atharva Patil <atharva.a.patil@mercedes-benz.com> on behalf of Mercedes-Benz Research And Development India, Provider Information

atharva1051 and others added 4 commits February 19, 2026 12:15
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
@atharva1051 atharva1051 marked this pull request as ready for review February 19, 2026 12:35
@atharva1051 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
Copy link
Contributor

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

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.

Copy link
Contributor

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

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

Copy link

@arhiv1973b arhiv1973b left a comment

Choose a reason for hiding this comment

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

LGTM! ✅

@atharva1051
Copy link
Contributor Author

Hello @SamMorrowDrums or @omgitsads ,

Could you please review this once?

Thank you

@omgitsads
Copy link
Member

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
Copy link
Contributor Author

atharva1051 commented Feb 23, 2026

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.

Copy link
Contributor

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

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

@atharva1051
Copy link
Contributor Author

@omgitsads made the changes you made in #2070
Thanks again

@omgitsads omgitsads merged commit 48a2a05 into github:main Feb 24, 2026
11 checks passed
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.

6 participants