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) +}