fix(lockdown): scope repo-access cache per identity via entry keys

Isolating identities by deriving a cache2go table name per token grew a
process-wide registry that is never reclaimed: cache2go creates each named
table on first use and never evicts it, so every distinct bearer token —
including invalid ones, since the table was built before GitHub validated
the token — permanently added a table.

Keep a single cache table and scope entries instead. WithIdentity stores a
SHA-256 digest of the identity and prefixes each entry key with it, so
different identities still cannot observe each other's trust decisions,
while per-identity state is reclaimed by the table's ordinary TTL cleanup.
WithCacheName stays for tenant/test isolation, with docs warning against
deriving names from request data.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: SamMorrowDrums

pkg/github/dependencies.go

@@ -455,20 +455,15 @@ func (d *RequestDeps) GetRepoAccessCache(ctx context.Context) (*lockdown.RepoAcc
 		return nil, err
 	}
 
-	// Scope the cache table to the requesting identity so a trust decision
+	// Scope cache entries to the requesting identity so a trust decision
 	// computed under one caller's credentials is never served to another.
-	// cache2go.Cache(name) returns a process-wide singleton keyed by name, so
-	// without this every request sharing d.RepoAccessOpts (built once at
-	// server startup) would hit the same default-named table regardless of
-	// which token issued the request. Deriving the name from the token keeps
-	// repeated requests from the same identity on a warm cache while
-	// isolating different identities from one another. Copy RepoAccessOpts
-	// before appending so concurrent requests never mutate the shared slice.
+	// RepoAccessOpts is built once at server startup and shared by every
+	// request, so identity scoping has to be applied per request here. Copy
+	// the slice before appending so concurrent requests never mutate the
+	// shared backing array.
 	opts := d.RepoAccessOpts
-	if tokenInfo, ok := ghcontext.GetTokenInfo(ctx); ok {
-		if name := lockdown.CacheNameForIdentity(tokenInfo.Token); name != "" {
-			opts = append(append([]lockdown.RepoAccessOption{}, d.RepoAccessOpts...), lockdown.WithCacheName(name))
-		}
+	if tokenInfo, ok := ghcontext.GetTokenInfo(ctx); ok && tokenInfo.Token != "" {
+		opts = append(append([]lockdown.RepoAccessOption{}, d.RepoAccessOpts...), lockdown.WithIdentity(tokenInfo.Token))
 	}
 
 	// Create repo access cache

pkg/lockdown/lockdown.go

@@ -28,6 +28,10 @@ type RepoAccessCache struct {
 	logger           *slog.Logger
 	trustedBotLogins map[string]struct{}
 
+	// identityDigest scopes this instance's entry keys to a single request
+	// identity. Empty means entries are unscoped. See WithIdentity.
+	identityDigest string
+
 	// now returns the current time and defaults to time.Now. Tests override it
 	// to exercise bounded expiry deterministically without sleeping.
 	now func() time.Time
@@ -84,12 +88,13 @@ 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.Cache(name) returns a process-wide singleton table keyed by name,
-// so any two RepoAccessCache instances constructed with the same name share
-// every cached trust decision. In deployments that serve multiple request
-// identities from one process (e.g. the HTTP server), callers MUST derive a
-// distinct name per identity — see CacheNameForIdentity — or one identity's
-// cached decision can be served to another.
+// cache2go.Cache(name) returns a process-wide singleton table that is created
+// on first use and never reclaimed, so the set of names a process passes here
+// must be bounded and known ahead of time. Never derive a name from
+// request-supplied data such as an auth token: the table registry would grow
+// without bound, retaining every distinct value seen for the lifetime of the
+// process. To isolate cached decisions per request identity, use WithIdentity,
+// which keeps a single table and scopes individual entries instead.
 func WithCacheName(name string) RepoAccessOption {
 	return func(c *RepoAccessCache) {
 		if name != "" {
@@ -98,22 +103,29 @@ func WithCacheName(name string) RepoAccessOption {
 	}
 }
 
-// CacheNameForIdentity derives a stable cache table name scoped to a single
-// request identity (typically an auth token). Two calls with the same
-// identity always return the same name, so repeated requests from the same
-// identity keep sharing a warm cache; two calls with different identities
-// always return different names, so their cached trust decisions cannot mix.
+// WithIdentity scopes this cache's entries to a single request identity
+// (typically an auth token), so a trust decision computed under one caller's
+// credentials is never served to another. Two instances configured with the
+// same identity share a warm cache; instances with different identities
+// cannot observe each other's entries.
+//
+// Isolation is applied to the entry key rather than the cache table: entries
+// are stored in the shared table under a key prefixed with a digest of the
+// identity. This keeps storage bounded, because per-identity entries are
+// reclaimed by the same TTL cleanup as any other entry. Allocating a table
+// per identity instead would leak, since cache2go never evicts tables.
 //
-// The identity is hashed so it never appears verbatim in cache-table names,
-// logs, or metrics. An empty identity returns an empty string, which
-// WithCacheName treats as a no-op (falling back to the default shared name);
-// callers that need isolation must ensure a non-empty identity is supplied.
-func CacheNameForIdentity(identity string) string {
-	if identity == "" {
-		return ""
-	}
-	sum := sha256.Sum256([]byte(identity))
-	return "repo-access:" + hex.EncodeToString(sum[:])
+// The identity is hashed so it never appears verbatim in cache keys, logs, or
+// metrics. An empty identity is a no-op, leaving this instance's entries
+// unscoped; callers that need isolation must supply a non-empty identity.
+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.
@@ -222,7 +234,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,
@@ -378,6 +390,14 @@ 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 returns the entry key for owner/repo, prefixed with this cache's
+// identity digest when one is configured. Instances sharing a cache table are
+// kept isolated by this prefix rather than by separate tables, so every
+// identity's entries remain subject to the table's ordinary TTL cleanup.
+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
 }

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"
 )
@@ -231,54 +233,100 @@ func TestRepoAccessCacheIsolatesViewerPerInstance(t *testing.T) {
 	require.True(t, safe)
 }
 
-func TestCacheNameForIdentity(t *testing.T) {
-	t.Run("deterministic for the same identity", func(t *testing.T) {
-		require.Equal(t, CacheNameForIdentity("token-a"), CacheNameForIdentity("token-a"))
-	})
+// TestRepoAccessCacheIdentityScopedKeys covers the key derivation that keeps
+// identities isolated inside a single shared cache table.
+func TestRepoAccessCacheIdentityScopedKeys(t *testing.T) {
+	restClient := newMockRESTServer(t, "write")
+	gqlClient, _ := newMockGQLClient(testUser, false)
+
+	newCache := func(opts ...RepoAccessOption) *RepoAccessCache {
+		return NewRepoAccessCache(gqlClient, restClient, opts...)
+	}
 
-	t.Run("distinct for different identities", func(t *testing.T) {
-		require.NotEqual(t, CacheNameForIdentity("token-a"), CacheNameForIdentity("token-b"))
-	})
+	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)
 
-	t.Run("empty identity yields empty name", func(t *testing.T) {
-		require.Empty(t, CacheNameForIdentity(""))
-	})
+	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")
 
-	t.Run("never contains the raw identity", func(t *testing.T) {
-		name := CacheNameForIdentity("super-secret-token")
-		require.NotContains(t, name, "super-secret-token")
-	})
+	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")
 }
 
-// TestRepoAccessCacheIdentityScopedNamesPreventCrossIdentityLeakage is a
-// regression test for issue #3107. It mirrors how the HTTP server must
-// construct a RepoAccessCache per request: reusing the same
-// lockdown.RepoAccessOption slice across requests but scoping the cache table
-// name to CacheNameForIdentity(token). Two different identities querying the
-// same owner/repo/author must each hit their own upstream clients rather than
-// one being served from the other's cached decision.
-func TestRepoAccessCacheIdentityScopedNamesPreventCrossIdentityLeakage(t *testing.T) {
+// TestRepoAccessCacheIdentityScopingIsolatesWithinOneTable is a regression
+// test for issue #3107. Isolating identities by allocating a cache2go table
+// per token grows a process-wide registry that is never reclaimed, so
+// isolation must instead come from the entry key inside a single table. This
+// asserts both halves: different identities cannot see each other's trust
+// decisions, and their entries share one table so ordinary TTL cleanup can
+// reclaim them.
+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)
-	aliceCache := NewRepoAccessCache(aliceGQL, restClient, WithCacheName(CacheNameForIdentity("token-alice")))
-	_, err := aliceCache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
+	_, 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)
-	bobCache := NewRepoAccessCache(bobGQL, restClient, WithCacheName(CacheNameForIdentity("token-bob")))
-	_, err = bobCache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
+	_, 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, 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")
 
-	// The same identity repeating a request must still hit the warm cache.
-	aliceCacheAgain := NewRepoAccessCache(aliceGQL, restClient, WithCacheName(CacheNameForIdentity("token-alice")))
-	_, err = aliceCacheAgain.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
+	// Repeating the same identity must hit the warm cache and must not
+	// allocate additional storage.
+	_, 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")
+}
+
+// TestRepoAccessCacheIdentityScopedEntriesAreReclaimed proves the storage held
+// for distinct identities is bounded: because identity scoping lives in the
+// entry key, per-identity state is removed by the cache table's ordinary TTL
+// cleanup. A table-per-identity design could not shrink this way, since
+// cache2go retains every named table for the life of the process.
+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 TTL cleanup so cache storage stays bounded")
 }
 
 type flakyTransport struct {