diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index dfa5d96ed5..d52b5f4aeb 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -463,8 +463,15 @@ func (d *RequestDeps) GetRepoAccessCache(ctx context.Context) (*lockdown.RepoAcc return nil, err } + // RepoAccessOpts is shared across requests, so copy before appending the + // per-request identity scope. + opts := d.RepoAccessOpts + if tokenInfo, ok := ghcontext.GetTokenInfo(ctx); ok && tokenInfo.Token != "" { + opts = append(append([]lockdown.RepoAccessOption{}, d.RepoAccessOpts...), lockdown.WithIdentity(tokenInfo.Token)) + } + // Create repo access cache - instance := lockdown.NewRepoAccessCache(gqlClient, restClient, d.RepoAccessOpts...) + instance := lockdown.NewRepoAccessCache(gqlClient, restClient, opts...) return instance, nil } diff --git a/pkg/github/dependencies_test.go b/pkg/github/dependencies_test.go index 7b7abaa62d..223fc8aca4 100644 --- a/pkg/github/dependencies_test.go +++ b/pkg/github/dependencies_test.go @@ -7,6 +7,8 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" + "sync" "testing" ghcontext "github.com/github/github-mcp-server/pkg/context" @@ -122,6 +124,80 @@ func TestRequestDepsScopesTokensToConfiguredHosts(t *testing.T) { assert.Empty(t, foreignAuth, "GraphQL redirect must not authenticate to a foreign host") } +// Regression test for #3107: RequestDeps is built once at startup and shared, +// so identity scoping has to happen per request in GetRepoAccessCache. +func TestGetRepoAccessCacheIsolatesTrustDecisionsPerIdentity(t *testing.T) { + t.Parallel() + + var mu sync.Mutex + var gqlCalls, restCalls int + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set(headers.ContentTypeHeader, headers.ContentTypeJSON) + mu.Lock() + defer mu.Unlock() + if strings.Contains(r.URL.Path, "/collaborators/") { + restCalls++ + _, _ = w.Write([]byte(`{"permission":"write"}`)) + return + } + gqlCalls++ + _, _ = w.Write([]byte(`{"data":{"viewer":{"login":"someone"},"repository":{"isPrivate":false}}}`)) + })) + defer server.Close() + + callCounts := func() (int, int) { + mu.Lock() + defer mu.Unlock() + return gqlCalls, restCalls + } + + // Built as pkg/http/server.go does: no per-identity options. + deps := github.NewRequestDeps( + newRequestDepsAPIHostResolver(t, server.URL), + "test", + true, // lockdownMode + nil, // RepoAccessOpts + translations.NullTranslationHelper, + 0, + nil, + testExporters(), + ) + + ctxAlice := ghcontext.WithTokenInfo(context.Background(), &ghcontext.TokenInfo{Token: "token-for-alice"}) + cacheAlice, err := deps.GetRepoAccessCache(ctxAlice) + require.NoError(t, err) + require.NotNil(t, cacheAlice) + + _, err = cacheAlice.IsSafeContent(ctxAlice, "mallory", "owner", "repo") + require.NoError(t, err) + + gqlN, restN := callCounts() + require.Equal(t, 1, gqlN) + require.Equal(t, 1, restN) + + ctxBob := ghcontext.WithTokenInfo(context.Background(), &ghcontext.TokenInfo{Token: "token-for-bob"}) + cacheBob, err := deps.GetRepoAccessCache(ctxBob) + require.NoError(t, err) + require.NotNil(t, cacheBob) + + _, err = cacheBob.IsSafeContent(ctxBob, "mallory", "owner", "repo") + require.NoError(t, err) + + gqlN, restN = callCounts() + require.Equal(t, 2, gqlN, "a different identity's request must not be served from another identity's cached trust decision") + require.Equal(t, 2, restN, "a different identity's request must not be served from another identity's cached trust decision") + + cacheAliceAgain, err := deps.GetRepoAccessCache(ctxAlice) + require.NoError(t, err) + _, err = cacheAliceAgain.IsSafeContent(ctxAlice, "mallory", "owner", "repo") + require.NoError(t, err) + + gqlN, restN = callCounts() + require.Equal(t, 2, gqlN, "repeated requests from the same identity should reuse the warm cache") + require.Equal(t, 2, restN, "repeated requests from the same identity should reuse the warm cache") +} + func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) { t.Parallel() diff --git a/pkg/lockdown/lockdown.go b/pkg/lockdown/lockdown.go index e9231414a9..8625073116 100644 --- a/pkg/lockdown/lockdown.go +++ b/pkg/lockdown/lockdown.go @@ -2,6 +2,8 @@ package lockdown import ( "context" + "crypto/sha256" + "encoding/hex" "fmt" "log/slog" "maps" @@ -25,6 +27,7 @@ type RepoAccessCache struct { ttl time.Duration logger *slog.Logger trustedBotLogins map[string]struct{} + identityDigest string viewerMu sync.Mutex viewerLogin string @@ -66,6 +69,10 @@ func WithLogger(logger *slog.Logger) RepoAccessOption { // WithCacheName overrides the cache table name used for storing entries. // Use this to isolate cache entries between tenants or in tests. +// +// cache2go never reclaims a named table, so names must come from a bounded, +// known set; never derive one from request data. Use WithIdentity instead to +// isolate per request identity. func WithCacheName(name string) RepoAccessOption { return func(c *RepoAccessCache) { if name != "" { @@ -74,6 +81,24 @@ func WithCacheName(name string) RepoAccessOption { } } +// WithIdentity scopes cache entries to a single request identity, typically an +// auth token, so a decision computed under one caller's credentials is never +// served to another. Equal identities share a warm cache; an empty one is a +// no-op. +// +// Scoping lives in the entry key rather than the table so per-identity state +// stays bounded and is reclaimed by ordinary idle-TTL cleanup. The identity is +// hashed so it never appears verbatim in a key. +func WithIdentity(identity string) RepoAccessOption { + return func(c *RepoAccessCache) { + if identity == "" { + return + } + sum := sha256.Sum256([]byte(identity)) + c.identityDigest = hex.EncodeToString(sum[:]) + } +} + // NewRepoAccessCache creates a RepoAccessCache bound to the supplied clients. func NewRepoAccessCache(client *githubv4.Client, restClient *github.Client, opts ...RepoAccessOption) *RepoAccessCache { c := &RepoAccessCache{ @@ -180,7 +205,7 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner return RepoAccessInfo{}, fmt.Errorf("nil repo access cache") } - key := cacheKey(owner, repo) + key := c.cacheKey(owner, repo) userKey := strings.ToLower(username) // Entries are immutable once added: the cache table is shared across instances, @@ -305,6 +330,12 @@ func (c *RepoAccessCache) isTrustedBot(username string) bool { return ok } -func cacheKey(owner, repo string) string { - return fmt.Sprintf("%s/%s", strings.ToLower(owner), strings.ToLower(repo)) +// cacheKey scopes the owner/repo key to this cache's identity, so identities +// sharing a table cannot observe each other's entries. +func (c *RepoAccessCache) cacheKey(owner, repo string) string { + key := fmt.Sprintf("%s/%s", strings.ToLower(owner), strings.ToLower(repo)) + if c.identityDigest == "" { + return key + } + return c.identityDigest + ":" + key } diff --git a/pkg/lockdown/lockdown_test.go b/pkg/lockdown/lockdown_test.go index 887fcfde6f..6254533682 100644 --- a/pkg/lockdown/lockdown_test.go +++ b/pkg/lockdown/lockdown_test.go @@ -5,12 +5,14 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "sync" "testing" "time" "github.com/github/github-mcp-server/internal/githubv4mock" gogithub "github.com/google/go-github/v89/github" + "github.com/muesli/cache2go" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/require" ) @@ -152,6 +154,90 @@ func TestRepoAccessCacheIsolatesViewerPerInstance(t *testing.T) { require.True(t, safe) } +func TestRepoAccessCacheIdentityScopedKeys(t *testing.T) { + restClient := newMockRESTServer(t, "write") + gqlClient, _ := newMockGQLClient(testUser, false) + + newCache := func(opts ...RepoAccessOption) *RepoAccessCache { + return NewRepoAccessCache(gqlClient, restClient, opts...) + } + + unscoped := newCache().cacheKey(testOwner, testRepo) + alice := newCache(WithIdentity("token-alice")).cacheKey(testOwner, testRepo) + aliceAgain := newCache(WithIdentity("token-alice")).cacheKey(testOwner, testRepo) + bob := newCache(WithIdentity("token-bob")).cacheKey(testOwner, testRepo) + + require.Equal(t, alice, aliceAgain, "the same identity must map to the same key so it keeps a warm cache") + require.NotEqual(t, alice, bob, "different identities must map to different keys") + require.NotEqual(t, alice, unscoped, "a scoped identity must not collide with unscoped entries") + require.NotContains(t, alice, "token-alice", "the raw identity must never appear in a cache key") + + require.Equal(t, unscoped, newCache(WithIdentity("")).cacheKey(testOwner, testRepo), + "an empty identity must leave entries unscoped") + require.Equal(t, alice, newCache(WithIdentity("token-alice")).cacheKey(strings.ToUpper(testOwner), strings.ToUpper(testRepo)), + "identity scoping must preserve owner/repo case-insensitivity") +} + +// Regression test for #3107: a table per identity leaks, so isolation must come +// from the entry key inside one table. +func TestRepoAccessCacheIdentityScopingIsolatesWithinOneTable(t *testing.T) { + ctx := t.Context() + + restClient := newMockRESTServer(t, "write") + table := cache2go.Cache(t.Name()) + t.Cleanup(table.Flush) + + newCache := func(gqlClient *githubv4.Client, identity string) *RepoAccessCache { + return NewRepoAccessCache(gqlClient, restClient, WithCacheName(t.Name()), WithIdentity(identity)) + } + + aliceGQL, aliceTransport := newMockGQLClient("alice", true) + _, err := newCache(aliceGQL, "token-alice").getRepoAccessInfo(ctx, testUser, testOwner, testRepo) + require.NoError(t, err) + require.EqualValues(t, 1, aliceTransport.CallCount()) + + bobGQL, bobTransport := newMockGQLClient("bob", true) + _, err = newCache(bobGQL, "token-bob").getRepoAccessInfo(ctx, testUser, testOwner, testRepo) + require.NoError(t, err) + require.EqualValues(t, 1, bobTransport.CallCount(), + "a different identity must fetch its own trust decision, not reuse another identity's cached entry") + + require.EqualValues(t, 2, table.Count(), + "per-identity entries must be stored in one shared table rather than a table per identity") + + _, err = newCache(aliceGQL, "token-alice").getRepoAccessInfo(ctx, testUser, testOwner, testRepo) + require.NoError(t, err) + require.EqualValues(t, 1, aliceTransport.CallCount(), "repeated requests from the same identity should reuse the warm cache") + require.EqualValues(t, 2, table.Count(), "a repeated request from a known identity must not add another entry") +} + +// Key-scoped entries stay bounded because ordinary idle-TTL cleanup reclaims +// them; a table per identity could not shrink this way. +func TestRepoAccessCacheIdentityScopedEntriesAreReclaimed(t *testing.T) { + ctx := t.Context() + + restClient := newMockRESTServer(t, "write") + table := cache2go.Cache(t.Name()) + t.Cleanup(table.Flush) + + identities := []string{"token-a", "token-b", "token-c"} + for _, identity := range identities { + gqlClient, _ := newMockGQLClient(testUser, false) + cache := NewRepoAccessCache(gqlClient, restClient, + WithCacheName(t.Name()), + WithIdentity(identity), + WithTTL(500*time.Millisecond), + ) + _, err := cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) + require.NoError(t, err) + } + + require.EqualValues(t, len(identities), table.Count(), "each identity should hold exactly one entry in the shared table") + + require.Eventually(t, func() bool { return table.Count() == 0 }, 30*time.Second, 10*time.Millisecond, + "per-identity entries must be reclaimed by ordinary idle-TTL cleanup so cache storage stays bounded") +} + type flakyTransport struct { mu sync.Mutex failN int