From c754ab1ff534e118db512e71769ab9630ef566d2 Mon Sep 17 00:00:00 2001 From: vedant381 Date: Tue, 1 Sep 2026 23:40:27 +0530 Subject: [PATCH] Support GitHub App auth across multiple installations A GitHub App can be installed on several accounts, but each installation has its own ID and mints its own access token. Today the server takes a single installation ID, so an enterprise whose repositories are spread across organizations needs one server process per organization. Make --app-installation-id optional. Without it, the server lists the app's installations, caches the account-to-installation map, and mints a token per installation on demand, routing each API request to the installation that owns the resource it addresses: REST requests by the owner in the path, GraphQL requests by the owner or login variable. Routing needs the request, which the existing func() string token provider cannot see, so BearerAuthTransport gains an optional RequestTokenProvider that takes precedence over it. A request that names no owner, or names an account the app is not installed on, is sent unauthenticated rather than falling back to another installation's token, so a misrouted call fails visibly instead of running against the wrong organization. Passing --app-installation-id keeps the existing single-installation behavior unchanged. --- cmd/github-mcp-server/main.go | 74 ++++++-- docs/github-app-auth.md | 33 +++- internal/ghmcp/server.go | 27 ++- internal/githubapp/multi.go | 278 ++++++++++++++++++++++++++++++ internal/githubapp/multi_test.go | 214 +++++++++++++++++++++++ internal/githubapp/owner.go | 98 +++++++++++ internal/githubapp/owner_test.go | 81 +++++++++ pkg/github/server.go | 8 + pkg/http/transport/bearer.go | 12 +- pkg/http/transport/bearer_test.go | 67 +++++++ 10 files changed, 869 insertions(+), 23 deletions(-) create mode 100644 internal/githubapp/multi.go create mode 100644 internal/githubapp/multi_test.go create mode 100644 internal/githubapp/owner.go create mode 100644 internal/githubapp/owner_test.go diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index c0cadbbc63..1ddb876dc0 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net/http" "os" "strings" "time" @@ -149,12 +150,25 @@ var ( stdioServerConfig.OAuthScopes = scopes } + // With an installation ID, the server authenticates as that single + // installation. Without one, it discovers every installation of the + // app and picks the one that owns the resource each request + // addresses, so repositories spread across organizations all work + // from one app ID and private key. if appAuthRequested { - tokenProvider, err := newGitHubAppTokenProvider(appID, appInstallationID, appPrivateKeyPath, appPrivateKeyInline, viper.GetString("host")) - if err != nil { - return err + if appInstallationID != "" { + tokenProvider, err := newGitHubAppTokenProvider(appID, appInstallationID, appPrivateKeyPath, appPrivateKeyInline, viper.GetString("host")) + if err != nil { + return err + } + stdioServerConfig.TokenProvider = tokenProvider + } else { + requestTokenProvider, err := newGitHubAppRequestTokenProvider(appID, appPrivateKeyPath, appPrivateKeyInline, viper.GetString("host")) + if err != nil { + return err + } + stdioServerConfig.RequestTokenProvider = requestTokenProvider } - stdioServerConfig.TokenProvider = tokenProvider } return ghmcp.RunStdioServer(stdioServerConfig) @@ -257,7 +271,7 @@ func init() { // The private key has no flag because passing it in argv would expose it. stdioCmd.Flags().String("app-id", "", "GitHub App ID or client ID, enabling non-interactive server-to-server authentication") - stdioCmd.Flags().String("app-installation-id", "", "GitHub App installation ID to mint installation access tokens for") + stdioCmd.Flags().String("app-installation-id", "", "GitHub App installation ID to mint installation access tokens for. Omit to use every installation of the app, selecting the one that owns each requested resource") stdioCmd.Flags().String("app-private-key-path", "", "Path to the GitHub App private key (PEM). Preferred over GITHUB_APP_PRIVATE_KEY: keeps the key off the command line and out of the environment") // HTTP-specific flags @@ -322,20 +336,16 @@ func newGitHubAppTokenProvider(appID, installationID, keyPath, keyInline, host s return nil, err } - apiHost, err := utils.NewAPIHost(host) - if err != nil { - return nil, fmt.Errorf("failed to parse host for GitHub App authentication: %w", err) - } - restURL, err := apiHost.BaseRESTURL(context.Background()) + restURL, err := appRESTBaseURL(host) if err != nil { - return nil, fmt.Errorf("failed to resolve REST URL for GitHub App authentication: %w", err) + return nil, err } provider, err := githubapp.NewProvider(githubapp.Config{ AppID: appID, InstallationID: installationID, PrivateKeyPEM: keyBytes, - BaseRESTURL: restURL.String(), + BaseRESTURL: restURL, }, nil) if err != nil { return nil, fmt.Errorf("failed to configure GitHub App authentication: %w", err) @@ -343,6 +353,46 @@ func newGitHubAppTokenProvider(appID, installationID, keyPath, keyInline, host s return provider.AccessToken, nil } +// newGitHubAppRequestTokenProvider builds a token provider for a GitHub App +// installed on more than one account. It mints a token per installation on +// demand, routing each request to the installation that owns the resource it +// addresses. +func newGitHubAppRequestTokenProvider(appID, keyPath, keyInline, host string) (func(*http.Request) string, error) { + keyBytes, err := loadAppPrivateKey(keyPath, keyInline) + if err != nil { + return nil, err + } + + restURL, err := appRESTBaseURL(host) + if err != nil { + return nil, err + } + + provider, err := githubapp.NewMultiProvider(githubapp.MultiConfig{ + AppID: appID, + PrivateKeyPEM: keyBytes, + BaseRESTURL: restURL, + }, nil) + if err != nil { + return nil, fmt.Errorf("failed to configure GitHub App authentication: %w", err) + } + return provider.TokenForRequest, nil +} + +// appRESTBaseURL resolves the REST API base used to mint installation tokens +// for the configured host. +func appRESTBaseURL(host string) (string, error) { + apiHost, err := utils.NewAPIHost(host) + if err != nil { + return "", fmt.Errorf("failed to parse host for GitHub App authentication: %w", err) + } + restURL, err := apiHost.BaseRESTURL(context.Background()) + if err != nil { + return "", fmt.Errorf("failed to resolve REST URL for GitHub App authentication: %w", err) + } + return restURL.String(), nil +} + func loadAppPrivateKey(path, inline string) ([]byte, error) { switch { case path != "": diff --git a/docs/github-app-auth.md b/docs/github-app-auth.md index f1da08c7bc..c6679e96ca 100644 --- a/docs/github-app-auth.md +++ b/docs/github-app-auth.md @@ -21,7 +21,7 @@ authentication. | Flag | Environment variable | Description | |------|----------------------|-------------| | `--app-id` | `GITHUB_APP_ID` | App ID or client ID used as the JWT issuer | -| `--app-installation-id` | `GITHUB_APP_INSTALLATION_ID` | Installation whose access token is used | +| `--app-installation-id` | `GITHUB_APP_INSTALLATION_ID` | Installation whose access token is used. Omit to use every installation of the app (see [Multiple organizations](#multiple-organizations)) | | `--app-private-key-path` | `GITHUB_APP_PRIVATE_KEY_PATH` | Path to the private key PEM | | _(none)_ | `GITHUB_APP_PRIVATE_KEY` | PEM contents, optionally with literal `\n` escapes | @@ -57,6 +57,33 @@ docker run -i --rm \ ghcr.io/github/github-mcp-server ``` +## Multiple organizations + +A GitHub App can be installed on several accounts, and each installation has its +own ID and its own access token. Omit `--app-installation-id` to work across all +of them from a single app ID and private key: + +```bash +github-mcp-server stdio \ + --app-id 123456 \ + --app-private-key-path /secrets/github-app.pem +``` + +The server then lists the app's installations, caches the map of account to +installation, and mints a token per installation on demand. Each API request is +routed to the installation that owns the resource it addresses: REST requests by +the owner in the path (`/repos/{owner}/...`, `/orgs/{org}/...`, +`/users/{user}/...`), and GraphQL requests by the `owner` or `login` variable in +the query. The installation directory is refreshed at most every 10 minutes, +when a lookup misses, so installing the app on a new organization is picked up +without a restart. + +Requests that name no owner are sent unauthenticated, and so are requests for an +account the app is not installed on — the server does not fall back to another +installation's token. Endpoints that are not owner-scoped (`/user`, +`/rate_limit`, `/repositories/{id}`) therefore do not work in this mode; set +`--app-installation-id` to authenticate as one specific installation instead. + For GitHub Enterprise Server or `ghe.com`, also set `--gh-host` or `GITHUB_HOST`. The server derives the installation-token endpoint from that host. @@ -71,3 +98,7 @@ host. private key, target host, and system clock. - **404 from the installation-token endpoint**: verify the installation ID and that the app is installed on the target host. +- **401 or 404 for one organization only** (multi-installation mode): the app is + not installed on that account, or the tool call named an owner that does not + match the account login. The server logs `GitHub App is not installed on this + account` once per owner. diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index dadc05744b..260c67b6e3 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -104,9 +104,10 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv Transport: &transport.GraphQLFeaturesTransport{ Transport: http.DefaultTransport, }, - Token: cfg.Token, - TokenProvider: cfg.TokenProvider, - AllowedHosts: allowedHosts, + Token: cfg.Token, + TokenProvider: cfg.TokenProvider, + RequestTokenProvider: cfg.RequestTokenProvider, + AllowedHosts: allowedHosts, }, } @@ -157,10 +158,11 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv func newRESTClient(cfg github.MCPServerConfig, uaTransport *transport.UserAgentTransport, restURL, uploadURL string, allowedHosts []string) (*gogithub.Client, error) { return gogithub.NewClient( gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{ - Transport: uaTransport, - Token: cfg.Token, - TokenProvider: cfg.TokenProvider, - AllowedHosts: allowedHosts, + Transport: uaTransport, + Token: cfg.Token, + TokenProvider: cfg.TokenProvider, + RequestTokenProvider: cfg.RequestTokenProvider, + AllowedHosts: allowedHosts, }}), gogithub.WithEnterpriseURLs(restURL, uploadURL), ) @@ -303,18 +305,24 @@ type StdioServerConfig struct { // TokenProvider supplies a token for each GitHub API request. TokenProvider func() string + + // RequestTokenProvider supplies a token for each GitHub API request based on + // the request itself. GitHub App authentication that spans several + // installations uses it to pick the installation that owns the resource + // being addressed. + RequestTokenProvider func(*http.Request) string } // RunStdioServer is not concurrent safe. func RunStdioServer(cfg StdioServerConfig) error { authModes := 0 - for _, on := range []bool{cfg.Token != "", cfg.OAuthManager != nil, cfg.TokenProvider != nil} { + for _, on := range []bool{cfg.Token != "", cfg.OAuthManager != nil, cfg.TokenProvider != nil, cfg.RequestTokenProvider != nil} { if on { authModes++ } } if authModes > 1 { - return fmt.Errorf("choose exactly one authentication mode: a static Token, OAuthManager, or TokenProvider") + return fmt.Errorf("choose exactly one authentication mode: a static Token, OAuthManager, TokenProvider, or RequestTokenProvider") } // Create app context @@ -384,6 +392,7 @@ func RunStdioServer(cfg StdioServerConfig) error { RepoAccessTTL: cfg.RepoAccessCacheTTL, TokenScopes: tokenScopes, TokenProvider: tokenProvider, + RequestTokenProvider: cfg.RequestTokenProvider, ToolHandlerMiddleware: toolHandlerMiddleware, }) if err != nil { diff --git a/internal/githubapp/multi.go b/internal/githubapp/multi.go new file mode 100644 index 0000000000..dd5fb50709 --- /dev/null +++ b/internal/githubapp/multi.go @@ -0,0 +1,278 @@ +package githubapp + +import ( + "context" + "crypto/rsa" + "encoding/json" + "errors" + "fmt" + "io" + "log/slog" + "net/http" + "net/url" + "strconv" + "strings" + "sync" + "time" +) + +// installationsRefreshInterval bounds how often the installation directory is +// re-listed when a lookup misses. Installing the app on a new organization is +// rare, so a miss is far more likely to be a repository the app cannot see than +// a stale directory. +const installationsRefreshInterval = 10 * time.Minute + +// maxInstallationPages caps directory pagination so a misbehaving or very large +// deployment cannot spin here indefinitely. +const maxInstallationPages = 100 + +// MultiConfig describes a GitHub App that is installed on more than one account. +// Unlike Config it carries no installation ID: installations are discovered from +// the App itself and a token is minted per installation on demand. +type MultiConfig struct { + // AppID is used as the JWT issuer. GitHub accepts an app ID or client ID. + AppID string + + // PrivateKeyPEM is the RSA key used to sign app JWTs. + PrivateKeyPEM []byte + + // BaseRESTURL is the REST API base, e.g. https://api.github.com/ for + // github.com or https://HOST/api/v3/ for GitHub Enterprise Server. + BaseRESTURL string +} + +func (c MultiConfig) validate() error { + switch { + case c.AppID == "": + return errors.New("GitHub App ID or client ID is required (GITHUB_APP_ID)") + case len(c.PrivateKeyPEM) == 0: + return errors.New("GitHub App private key is required (GITHUB_APP_PRIVATE_KEY_PATH or GITHUB_APP_PRIVATE_KEY)") + case c.BaseRESTURL == "": + return errors.New("GitHub App REST base URL is required") + } + return nil +} + +// MultiProvider mints installation access tokens for every account a GitHub App +// is installed on, routing each request to the installation that owns the +// resource it addresses. +// +// It keeps a directory of account login to installation ID, refreshed from +// GET /app/installations, and one cached token per installation. Both the +// directory and the tokens are shared across goroutines. +type MultiProvider struct { + cfg MultiConfig + privateKey *rsa.PrivateKey + httpClient *http.Client + logger *slog.Logger + + mu sync.Mutex + // byAccount maps a lowercased account login to its installation ID. + byAccount map[string]string + // listedAt is when byAccount was last refreshed; zero means never. + listedAt time.Time + // providers caches a token provider per installation ID. + providers map[string]*Provider + // warnedOwners records owners we have already logged a miss for, so a + // repeatedly failing tool call does not flood the log. + warnedOwners map[string]bool +} + +// NewMultiProvider validates cfg and returns a provider. Installations are +// discovered lazily on the first token request, so construction does not +// require network access. +func NewMultiProvider(cfg MultiConfig, logger *slog.Logger) (*MultiProvider, error) { + if err := cfg.validate(); err != nil { + return nil, err + } + privateKey, err := parsePrivateKey(cfg.PrivateKeyPEM) + if err != nil { + return nil, fmt.Errorf("invalid GitHub App private key: %w", err) + } + if logger == nil { + logger = slog.Default() + } + return &MultiProvider{ + cfg: cfg, + privateKey: privateKey, + httpClient: &http.Client{Timeout: httpTimeout}, + logger: logger, + byAccount: map[string]string{}, + providers: map[string]*Provider{}, + warnedOwners: map[string]bool{}, + }, nil +} + +// TokenForOwner returns an installation access token for the account that owns +// the resource being addressed, or "" when the app is not installed there. An +// empty token leaves the request unauthenticated, which surfaces as a 401 or +// 404 from the API rather than as a silent call against the wrong installation. +func (p *MultiProvider) TokenForOwner(owner string) string { + if owner == "" { + return "" + } + installationID, ok := p.installationFor(owner) + if !ok { + p.warnOnce(owner) + return "" + } + provider, err := p.providerFor(installationID) + if err != nil { + p.logger.Error("failed to configure GitHub App installation", "owner", owner, "installationID", installationID, "error", err) + return "" + } + return provider.AccessToken() +} + +// TokenForRequest routes an outbound GitHub API request to the installation +// that owns the resource it addresses. See OwnerFromRequest for how the owner +// is determined. +func (p *MultiProvider) TokenForRequest(req *http.Request) string { + return p.TokenForOwner(OwnerFromRequest(req)) +} + +// installationFor resolves an account login to an installation ID, refreshing +// the directory when the login is unknown and the cached copy is stale. +func (p *MultiProvider) installationFor(owner string) (string, bool) { + key := strings.ToLower(owner) + + p.mu.Lock() + id, ok := p.byAccount[key] + stale := time.Since(p.listedAt) >= installationsRefreshInterval + p.mu.Unlock() + + if ok { + return id, true + } + if !stale { + return "", false + } + + installations, err := p.listInstallations() + if err != nil { + p.logger.Error("failed to list GitHub App installations", "error", err) + return "", false + } + + p.mu.Lock() + p.byAccount = installations + p.listedAt = time.Now() + id, ok = p.byAccount[key] + if ok { + delete(p.warnedOwners, key) + } + p.mu.Unlock() + + return id, ok +} + +// providerFor returns the cached single-installation provider for id, creating +// it on first use. +func (p *MultiProvider) providerFor(installationID string) (*Provider, error) { + p.mu.Lock() + defer p.mu.Unlock() + + if provider, ok := p.providers[installationID]; ok { + return provider, nil + } + provider, err := NewProvider(Config{ + AppID: p.cfg.AppID, + InstallationID: installationID, + PrivateKeyPEM: p.cfg.PrivateKeyPEM, + BaseRESTURL: p.cfg.BaseRESTURL, + }, p.logger) + if err != nil { + return nil, err + } + p.providers[installationID] = provider + return provider, nil +} + +func (p *MultiProvider) warnOnce(owner string) { + key := strings.ToLower(owner) + p.mu.Lock() + defer p.mu.Unlock() + if p.warnedOwners[key] { + return + } + p.warnedOwners[key] = true + p.logger.Warn("GitHub App is not installed on this account; the request will be unauthenticated", "owner", owner) +} + +// listInstallations pages through GET /app/installations and returns a map of +// lowercased account login to installation ID. +func (p *MultiProvider) listInstallations() (map[string]string, error) { + jwt, err := mintJWT(p.cfg.AppID, p.privateKey, time.Now()) + if err != nil { + return nil, err + } + + endpoint, err := url.JoinPath(p.cfg.BaseRESTURL, "app", "installations") + if err != nil { + return nil, fmt.Errorf("building installations URL: %w", err) + } + + result := map[string]string{} + const perPage = 100 + for page := 1; page <= maxInstallationPages; page++ { + installations, err := p.listInstallationsPage(jwt, endpoint, page, perPage) + if err != nil { + return nil, err + } + for _, installation := range installations { + if installation.Account.Login == "" { + continue + } + result[strings.ToLower(installation.Account.Login)] = strconv.FormatInt(installation.ID, 10) + } + if len(installations) < perPage { + return result, nil + } + } + p.logger.Warn("stopped listing GitHub App installations at the page limit", "pages", maxInstallationPages) + return result, nil +} + +type appInstallation struct { + ID int64 `json:"id"` + Account struct { + Login string `json:"login"` + } `json:"account"` +} + +func (p *MultiProvider) listInstallationsPage(jwt, endpoint string, page, perPage int) ([]appInstallation, error) { + ctx, cancel := context.WithTimeout(context.Background(), httpTimeout) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) + if err != nil { + return nil, fmt.Errorf("creating installations request: %w", err) + } + query := req.URL.Query() + query.Set("per_page", strconv.Itoa(perPage)) + query.Set("page", strconv.Itoa(page)) + req.URL.RawQuery = query.Encode() + req.Header.Set("Authorization", "Bearer "+jwt) + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + + resp, err := p.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("requesting installations: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + snippet, readErr := io.ReadAll(io.LimitReader(resp.Body, 512)) + if readErr != nil { + return nil, fmt.Errorf("installations request failed: %s (reading response: %w)", resp.Status, readErr) + } + return nil, fmt.Errorf("installations request failed: %s: %s", resp.Status, strings.TrimSpace(string(snippet))) + } + + var installations []appInstallation + if err := json.NewDecoder(resp.Body).Decode(&installations); err != nil { + return nil, fmt.Errorf("decoding installations response: %w", err) + } + return installations, nil +} diff --git a/internal/githubapp/multi_test.go b/internal/githubapp/multi_test.go new file mode 100644 index 0000000000..f28ff81d43 --- /dev/null +++ b/internal/githubapp/multi_test.go @@ -0,0 +1,214 @@ +package githubapp + +import ( + "bytes" + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newAppServer serves the two endpoints a MultiProvider uses: the installation +// directory and the per-installation token exchange. installations maps an +// account login to its installation ID. +func newAppServer(t *testing.T, installations map[string]int64, listCalls *atomic.Int64) *httptest.Server { + t.Helper() + + type account struct { + Login string `json:"login"` + } + type installation struct { + ID int64 `json:"id"` + Account account `json:"account"` + } + + mux := http.NewServeMux() + mux.HandleFunc("/app/installations", func(w http.ResponseWriter, r *http.Request) { + if listCalls != nil { + listCalls.Add(1) + } + if r.URL.Query().Get("page") != "1" { + require.NoError(t, json.NewEncoder(w).Encode([]installation{})) + return + } + body := make([]installation, 0, len(installations)) + for login, id := range installations { + body = append(body, installation{ID: id, Account: account{Login: login}}) + } + require.NoError(t, json.NewEncoder(w).Encode(body)) + }) + mux.HandleFunc("/app/installations/", func(w http.ResponseWriter, r *http.Request) { + // The token names its installation so tests can assert which one a + // request was routed to. Parsing the ID rather than echoing the path + // keeps untrusted input out of the response body. + raw := strings.TrimSuffix(strings.TrimPrefix(r.URL.Path, "/app/installations/"), "/access_tokens") + id, err := strconv.ParseInt(raw, 10, 64) + if err != nil { + w.WriteHeader(http.StatusNotFound) + return + } + w.WriteHeader(http.StatusCreated) + body := map[string]string{ + "token": "token-for-" + strconv.FormatInt(id, 10), + "expires_at": "2999-01-01T00:00:00Z", + } + require.NoError(t, json.NewEncoder(w).Encode(body)) + }) + + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + return server +} + +func newTestMultiProvider(t *testing.T, baseURL string) *MultiProvider { + t.Helper() + provider, err := NewMultiProvider(MultiConfig{ + AppID: "123", + PrivateKeyPEM: pkcs1PEM(t, newTestKey(t)), + BaseRESTURL: baseURL, + }, slog.New(slog.NewTextHandler(io.Discard, nil))) + require.NoError(t, err) + return provider +} + +func TestMultiConfigValidate(t *testing.T) { + key := pkcs1PEM(t, newTestKey(t)) + + tests := []struct { + name string + cfg MultiConfig + wantErr string + }{ + { + name: "valid", + cfg: MultiConfig{AppID: "1", PrivateKeyPEM: key, BaseRESTURL: "https://api.github.com/"}, + }, + { + name: "missing app ID", + cfg: MultiConfig{PrivateKeyPEM: key, BaseRESTURL: "https://api.github.com/"}, + wantErr: "GITHUB_APP_ID", + }, + { + name: "missing private key", + cfg: MultiConfig{AppID: "1", BaseRESTURL: "https://api.github.com/"}, + wantErr: "private key is required", + }, + { + name: "missing base URL", + cfg: MultiConfig{AppID: "1", PrivateKeyPEM: key}, + wantErr: "REST base URL is required", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := tc.cfg.validate() + if tc.wantErr == "" { + require.NoError(t, err) + return + } + require.ErrorContains(t, err, tc.wantErr) + }) + } +} + +// TestMultiProviderTokenForOwner verifies that each owner is routed to the +// installation that owns it, and that an owner the app is not installed on +// yields no token rather than another installation's. +func TestMultiProviderTokenForOwner(t *testing.T) { + server := newAppServer(t, map[string]int64{"octo-org": 11, "other-org": 22}, nil) + provider := newTestMultiProvider(t, server.URL+"/") + + assert.Equal(t, "token-for-11", provider.TokenForOwner("octo-org")) + assert.Equal(t, "token-for-22", provider.TokenForOwner("other-org")) + assert.Empty(t, provider.TokenForOwner("unknown-org")) + assert.Empty(t, provider.TokenForOwner("")) +} + +// TestMultiProviderOwnerLookupIsCaseInsensitive covers owners whose casing in a +// tool argument differs from the account login GitHub returns. +func TestMultiProviderOwnerLookupIsCaseInsensitive(t *testing.T) { + server := newAppServer(t, map[string]int64{"Octo-Org": 11}, nil) + provider := newTestMultiProvider(t, server.URL+"/") + + assert.Equal(t, "token-for-11", provider.TokenForOwner("octo-ORG")) +} + +// TestMultiProviderCachesInstallationDirectory verifies that a resolvable owner +// does not re-list installations on every request, and that an unresolvable one +// does not re-list until the refresh interval elapses. +func TestMultiProviderCachesInstallationDirectory(t *testing.T) { + var listCalls atomic.Int64 + server := newAppServer(t, map[string]int64{"octo-org": 11}, &listCalls) + provider := newTestMultiProvider(t, server.URL+"/") + + for range 3 { + assert.Equal(t, "token-for-11", provider.TokenForOwner("octo-org")) + } + assert.Equal(t, int64(1), listCalls.Load()) + + for range 3 { + assert.Empty(t, provider.TokenForOwner("unknown-org")) + } + assert.Equal(t, int64(1), listCalls.Load(), "a miss within the refresh interval should not re-list") +} + +// TestMultiProviderRefreshesOnMiss verifies that an owner added to the app after +// the directory was cached resolves once the cached copy is stale. +func TestMultiProviderRefreshesOnMiss(t *testing.T) { + installations := map[string]int64{"octo-org": 11} + server := newAppServer(t, installations, nil) + provider := newTestMultiProvider(t, server.URL+"/") + + require.Equal(t, "token-for-11", provider.TokenForOwner("octo-org")) + require.Empty(t, provider.TokenForOwner("late-org")) + + installations["late-org"] = 33 + // Age the cached directory past the refresh interval. + provider.mu.Lock() + provider.listedAt = provider.listedAt.Add(-2 * installationsRefreshInterval) + provider.mu.Unlock() + + assert.Equal(t, "token-for-33", provider.TokenForOwner("late-org")) +} + +// TestMultiProviderTokenForRequest exercises the routing the transport relies +// on, over both REST paths and GraphQL bodies. +func TestMultiProviderTokenForRequest(t *testing.T) { + server := newAppServer(t, map[string]int64{"octo-org": 11, "other-org": 22}, nil) + provider := newTestMultiProvider(t, server.URL+"/") + + restReq, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos/octo-org/repo/issues", nil) + require.NoError(t, err) + assert.Equal(t, "token-for-11", provider.TokenForRequest(restReq)) + + body := `{"query":"query($owner:String!){}","variables":{"owner":"other-org"}}` + gqlReq, err := http.NewRequest(http.MethodPost, "https://api.github.com/graphql", strings.NewReader(body)) + require.NoError(t, err) + assert.Equal(t, "token-for-22", provider.TokenForRequest(gqlReq)) + + unscopedReq, err := http.NewRequest(http.MethodGet, "https://api.github.com/rate_limit", nil) + require.NoError(t, err) + assert.Empty(t, provider.TokenForRequest(unscopedReq)) +} + +// TestMultiProviderListError verifies that a failing directory listing degrades +// to an empty token rather than panicking or returning a wrong one. +func TestMultiProviderListError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = io.Copy(w, bytes.NewBufferString(`{"message":"Bad credentials"}`)) + })) + t.Cleanup(server.Close) + + provider := newTestMultiProvider(t, server.URL+"/") + assert.Empty(t, provider.TokenForOwner("octo-org")) +} diff --git a/internal/githubapp/owner.go b/internal/githubapp/owner.go new file mode 100644 index 0000000000..d13cc9aacf --- /dev/null +++ b/internal/githubapp/owner.go @@ -0,0 +1,98 @@ +package githubapp + +import ( + "encoding/json" + "io" + "net/http" + "strings" +) + +// maxGraphQLBodyPeek caps how much of a GraphQL request body is buffered while +// looking for the owner variable. Queries are small; a body larger than this is +// not worth inspecting. +const maxGraphQLBodyPeek = 1 << 20 + +// graphQLOwnerVariables are the GraphQL variable names that carry an account +// login, in priority order. +var graphQLOwnerVariables = []string{"owner", "org", "organization", "login", "repositoryOwner"} + +// OwnerFromRequest reports the account login that owns the resource an outbound +// GitHub API request addresses, or "" when it cannot be determined. +// +// REST paths are read directly: /repos/{owner}/..., /orgs/{org}/... and +// /users/{user}/... all name their owner, and the leading segments of GitHub +// Enterprise Server paths (/api/v3/...) are skipped by scanning for the first +// segment that introduces an owner. GraphQL requests carry the owner in their +// variables instead, so the body is inspected for one of the conventional +// variable names. +// +// Endpoints that are not owner-scoped (/user, /rate_limit, /repositories/{id}) +// yield "". So does a GraphQL query that names no owner. +func OwnerFromRequest(req *http.Request) string { + if req == nil || req.URL == nil { + return "" + } + if owner := ownerFromPath(req.URL.Path); owner != "" { + return owner + } + if isGraphQL(req.URL.Path) { + return ownerFromGraphQLBody(req) + } + return "" +} + +func ownerFromPath(path string) string { + segments := strings.Split(strings.Trim(path, "/"), "/") + for i, segment := range segments { + switch segment { + case "repos", "orgs", "users": + if i+1 < len(segments) && segments[i+1] != "" { + return segments[i+1] + } + return "" + } + } + return "" +} + +func isGraphQL(path string) bool { + trimmed := strings.Trim(path, "/") + return trimmed == "graphql" || strings.HasSuffix(trimmed, "/graphql") +} + +// ownerFromGraphQLBody reads the request body through GetBody so the original +// body stays intact for the transport below. Requests without GetBody (a +// streamed body) are skipped rather than consumed. +func ownerFromGraphQLBody(req *http.Request) string { + if req.GetBody == nil { + return "" + } + body, err := req.GetBody() + if err != nil || body == nil { + return "" + } + defer func() { _ = body.Close() }() + + raw, err := io.ReadAll(io.LimitReader(body, maxGraphQLBodyPeek)) + if err != nil { + return "" + } + + var payload struct { + Variables map[string]json.RawMessage `json:"variables"` + } + if err := json.Unmarshal(raw, &payload); err != nil { + return "" + } + for _, name := range graphQLOwnerVariables { + value, ok := payload.Variables[name] + if !ok { + continue + } + var login string + if err := json.Unmarshal(value, &login); err == nil && login != "" { + return login + } + } + return "" +} diff --git a/internal/githubapp/owner_test.go b/internal/githubapp/owner_test.go new file mode 100644 index 0000000000..5372b58484 --- /dev/null +++ b/internal/githubapp/owner_test.go @@ -0,0 +1,81 @@ +package githubapp + +import ( + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOwnerFromRequestRESTPaths(t *testing.T) { + tests := []struct { + name string + url string + want string + }{ + {name: "repo", url: "https://api.github.com/repos/octo-org/repo", want: "octo-org"}, + {name: "repo sub-resource", url: "https://api.github.com/repos/octo-org/repo/issues/1/comments", want: "octo-org"}, + {name: "org", url: "https://api.github.com/orgs/octo-org/repos", want: "octo-org"}, + {name: "user", url: "https://api.github.com/users/octocat/repos", want: "octocat"}, + {name: "GHES prefix", url: "https://ghes.example.com/api/v3/repos/octo-org/repo", want: "octo-org"}, + {name: "repo named like a keyword", url: "https://api.github.com/repos/octo-org/orgs/contents", want: "octo-org"}, + {name: "query string ignored", url: "https://api.github.com/orgs/octo-org/repos?per_page=100", want: "octo-org"}, + {name: "not owner scoped", url: "https://api.github.com/rate_limit", want: ""}, + {name: "authenticated user", url: "https://api.github.com/user", want: ""}, + {name: "repository by ID", url: "https://api.github.com/repositories/1300192", want: ""}, + {name: "truncated path", url: "https://api.github.com/repos", want: ""}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req, err := http.NewRequest(http.MethodGet, tc.url, nil) + require.NoError(t, err) + assert.Equal(t, tc.want, OwnerFromRequest(req)) + }) + } +} + +func TestOwnerFromRequestGraphQL(t *testing.T) { + tests := []struct { + name string + body string + want string + }{ + {name: "owner variable", body: `{"variables":{"owner":"octo-org","name":"repo"}}`, want: "octo-org"}, + {name: "login variable", body: `{"variables":{"login":"octo-org"}}`, want: "octo-org"}, + {name: "owner preferred over login", body: `{"variables":{"login":"other-org","owner":"octo-org"}}`, want: "octo-org"}, + {name: "no owner variable", body: `{"variables":{"first":10}}`, want: ""}, + {name: "no variables", body: `{"query":"query{viewer{login}}"}`, want: ""}, + {name: "non-string owner", body: `{"variables":{"owner":42}}`, want: ""}, + {name: "malformed body", body: `not json`, want: ""}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req, err := http.NewRequest(http.MethodPost, "https://api.github.com/graphql", strings.NewReader(tc.body)) + require.NoError(t, err) + assert.Equal(t, tc.want, OwnerFromRequest(req)) + }) + } +} + +// TestOwnerFromRequestGraphQLLeavesBodyIntact guards the transport contract: +// inspecting the body must not consume it before the request is sent. +func TestOwnerFromRequestGraphQLLeavesBodyIntact(t *testing.T) { + body := `{"variables":{"owner":"octo-org"}}` + req, err := http.NewRequest(http.MethodPost, "https://api.github.com/graphql", strings.NewReader(body)) + require.NoError(t, err) + + require.Equal(t, "octo-org", OwnerFromRequest(req)) + + buf := make([]byte, len(body)) + n, _ := req.Body.Read(buf) + assert.Equal(t, body, string(buf[:n])) +} + +func TestOwnerFromRequestNilSafe(t *testing.T) { + assert.Empty(t, OwnerFromRequest(nil)) + assert.Empty(t, OwnerFromRequest(&http.Request{})) +} diff --git a/pkg/github/server.go b/pkg/github/server.go index b8f0197889..2e14ac8d39 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "log/slog" + "net/http" "strings" "time" @@ -72,6 +73,13 @@ type MCPServerConfig struct { // request instead of the static Token. TokenProvider func() string + // RequestTokenProvider, when non-nil, supplies the GitHub token for each API + // request based on the request itself, taking precedence over TokenProvider + // and Token. It carries GitHub App authentication that spans several + // installations, where the token depends on which account owns the resource + // being addressed. + RequestTokenProvider func(*http.Request) string + // ToolHandlerMiddleware wraps every registered tool handler. Unlike MCP // receiving middleware, these wrappers execute inside Server.callTool, so // SDK result finalization still runs on results they return. diff --git a/pkg/http/transport/bearer.go b/pkg/http/transport/bearer.go index 522f4c6753..8c9d75b1f9 100644 --- a/pkg/http/transport/bearer.go +++ b/pkg/http/transport/bearer.go @@ -16,6 +16,13 @@ type BearerAuthTransport struct { // and takes precedence over Token. TokenProvider func() string + // RequestTokenProvider, when non-nil, supplies the bearer token for each + // request based on the request itself, and takes precedence over both + // TokenProvider and Token. It exists for credentials that are scoped to the + // resource being addressed, such as GitHub App installation tokens across + // several organizations. + RequestTokenProvider func(*http.Request) string + // AllowedHosts, when non-empty, restricts the hosts the Authorization // header is attached to. The token is set only when the request host // and port exactly match one of these entries (case-insensitive). This @@ -36,7 +43,10 @@ type BearerAuthTransport struct { func (t *BearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, error) { req = req.Clone(req.Context()) token := t.Token - if t.TokenProvider != nil { + switch { + case t.RequestTokenProvider != nil: + token = t.RequestTokenProvider(req) + case t.TokenProvider != nil: token = t.TokenProvider() } if !t.hostAllowed(req.URL.Host) { diff --git a/pkg/http/transport/bearer_test.go b/pkg/http/transport/bearer_test.go index 49f50710d0..4aa5adb023 100644 --- a/pkg/http/transport/bearer_test.go +++ b/pkg/http/transport/bearer_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" ghcontext "github.com/github/github-mcp-server/pkg/context" @@ -393,3 +394,69 @@ func TestBearerAuthTransport_RemovesAuthorizationFromDisallowedHost(t *testing.T assert.Empty(t, rec.authByHost[req.URL.Host]) assert.NotEmpty(t, req.Header.Get(headers.AuthorizationHeader), "original request must not be mutated") } + +// TestBearerAuthTransport_RequestTokenProvider verifies that a request-scoped +// provider sees the outbound request and takes precedence over the other two +// token sources. GitHub App authentication across several installations relies +// on this to pick the installation that owns the resource being addressed. +func TestBearerAuthTransport_RequestTokenProvider(t *testing.T) { + t.Parallel() + + var gotAuth string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAuth = r.Header.Get(headers.AuthorizationHeader) + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + rt := &BearerAuthTransport{ + Transport: newIsolatedTransport(t), + Token: "static-token", + TokenProvider: func() string { return "provider-token" }, + RequestTokenProvider: func(req *http.Request) string { + return "token-for" + strings.ReplaceAll(req.URL.Path, "/", "-") + }, + } + + do := func(path string) { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL+path, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + defer resp.Body.Close() + } + + do("/repos/octo-org/repo") + assert.Equal(t, "Bearer token-for-repos-octo-org-repo", gotAuth) + + do("/repos/other-org/repo") + assert.Equal(t, "Bearer token-for-repos-other-org-repo", gotAuth, "the token is resolved per request, not cached across them") +} + +// TestBearerAuthTransport_RequestTokenProviderEmptyToken verifies that a +// request the provider cannot resolve is sent unauthenticated rather than +// falling back to another credential. +func TestBearerAuthTransport_RequestTokenProviderEmptyToken(t *testing.T) { + t.Parallel() + + var gotAuth string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAuth = r.Header.Get(headers.AuthorizationHeader) + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + rt := &BearerAuthTransport{ + Transport: newIsolatedTransport(t), + Token: "static-token", + RequestTokenProvider: func(*http.Request) string { return "" }, + } + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Empty(t, gotAuth) +}