Skip to content

Commit a429a87

Browse files
SamMorrowDrumsCopilot
SamMorrowDrums
and
Copilot
committed
fix(lockdown): bound repo-access cache expiry and isolate it per identity
The repo-access cache used by lockdown mode relied on cache2go's sliding expiry: every read extends an entry's life, so a frequently-accessed entry could keep a stale trust decision (e.g. revoked push access) alive indefinitely instead of refreshing after its TTL. Separately, cache2go.Cache(name) returns a process-wide singleton table keyed by name. In HTTP mode, RequestDeps.GetRepoAccessCache built a new RepoAccessCache per request but always reused the same default-named table, so trust decisions computed under one caller's credentials could be served to a different caller for the same owner/repo, without ever validating the second caller's own access. Fixes: - Track each cache entry's original creation time and bound its maximum age from that fixed point, not from last access, so entries are refreshed after a fixed TTL regardless of read frequency. - Add lockdown.CacheNameForIdentity, which derives a stable, hashed cache-table name from a request identity (e.g. auth token). Two calls for the same identity return the same name (reusing a warm cache across a session's repeated requests); different identities always get different names (no shared cache state). - RequestDeps.GetRepoAccessCache now scopes each request's cache to the requesting token's identity via CacheNameForIdentity, closing the cross-identity leak in HTTP/multi-tenant deployments. Stdio mode is unaffected: it constructs a single RepoAccessCache for the whole process lifetime, as before. Tests added: - TestRepoAccessCacheBoundedExpiryIgnoresRepeatedAccess and TestRepoAccessCacheNewUserDoesNotResetEntryAge exercise bounded expiry deterministically via an injectable clock (no sleeps). - TestCacheNameForIdentity and TestRepoAccessCacheIdentityScopedNamesPreventCrossIdentityLeakage cover the naming helper and cross-identity isolation at the lockdown package level. - TestGetRepoAccessCacheIsolatesTrustDecisionsPerIdentity in pkg/github mirrors the HTTP server's exact construction pattern end-to-end and fails without the dependencies.go fix. Fixes #3107 Co-authored-by: Copilot App <[email protected]>
1 parent b3ecab4 commit a429a87

4 files changed

Lines changed: 332 additions & 29 deletions

File tree

‎pkg/github/dependencies.go‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -463,8 +463,24 @@ func (d *RequestDeps) GetRepoAccessCache(ctx context.Context) (*lockdown.RepoAcc
463463
return nil, err
464464
}
465465

466+
// Scope the cache table to the requesting identity so a trust decision
467+
// computed under one caller's credentials is never served to another.
468+
// cache2go.Cache(name) returns a process-wide singleton keyed by name, so
469+
// without this every request sharing d.RepoAccessOpts (built once at
470+
// server startup) would hit the same default-named table regardless of
471+
// which token issued the request. Deriving the name from the token keeps
472+
// repeated requests from the same identity on a warm cache while
473+
// isolating different identities from one another. Copy RepoAccessOpts
474+
// before appending so concurrent requests never mutate the shared slice.
475+
opts := d.RepoAccessOpts
476+
if tokenInfo, ok := ghcontext.GetTokenInfo(ctx); ok {
477+
if name := lockdown.CacheNameForIdentity(tokenInfo.Token); name != "" {
478+
opts = append(append([]lockdown.RepoAccessOption{}, d.RepoAccessOpts...), lockdown.WithCacheName(name))
479+
}
480+
}
481+
466482
// Create repo access cache
467-
instance := lockdown.NewRepoAccessCache(gqlClient, restClient, d.RepoAccessOpts...)
483+
instance := lockdown.NewRepoAccessCache(gqlClient, restClient, opts...)
468484
return instance, nil
469485
}
470486

‎pkg/github/dependencies_test.go‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import (
77
"net/http"
88
"net/http/httptest"
99
"net/url"
10+
"strings"
11+
"sync"
1012
"testing"
1113

1214
ghcontext "github.com/github/github-mcp-server/pkg/context"
@@ -122,6 +124,89 @@ func TestRequestDepsScopesTokensToConfiguredHosts(t *testing.T) {
122124
assert.Empty(t, foreignAuth, "GraphQL redirect must not authenticate to a foreign host")
123125
}
124126

127+
// TestGetRepoAccessCacheIsolatesTrustDecisionsPerIdentity is a regression test
128+
// for issue #3107. It mirrors exactly how the HTTP server builds RequestDeps:
129+
// a single RepoAccessOpts slice is constructed once at startup (with no
130+
// per-identity WithCacheName) and reused across every request, and
131+
// GetRepoAccessCache is called fresh per request. Two different token
132+
// identities querying the same owner/repo/author must each perform their own
133+
// upstream lookups instead of one being served from the other's cached
134+
// decision, while repeated requests from the same identity must reuse a warm
135+
// cache.
136+
func TestGetRepoAccessCacheIsolatesTrustDecisionsPerIdentity(t *testing.T) {
137+
t.Parallel()
138+
139+
var mu sync.Mutex
140+
var gqlCalls, restCalls int
141+
142+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
143+
w.Header().Set(headers.ContentTypeHeader, headers.ContentTypeJSON)
144+
mu.Lock()
145+
defer mu.Unlock()
146+
if strings.Contains(r.URL.Path, "/collaborators/") {
147+
restCalls++
148+
_, _ = w.Write([]byte(`{"permission":"write"}`))
149+
return
150+
}
151+
gqlCalls++
152+
_, _ = w.Write([]byte(`{"data":{"viewer":{"login":"someone"},"repository":{"isPrivate":false}}}`))
153+
}))
154+
defer server.Close()
155+
156+
callCounts := func() (int, int) {
157+
mu.Lock()
158+
defer mu.Unlock()
159+
return gqlCalls, restCalls
160+
}
161+
162+
// Built once, exactly as pkg/http/server.go does today: no WithCacheName.
163+
deps := github.NewRequestDeps(
164+
newRequestDepsAPIHostResolver(t, server.URL),
165+
"test",
166+
true, // lockdownMode
167+
nil, // RepoAccessOpts
168+
translations.NullTranslationHelper,
169+
0,
170+
nil,
171+
testExporters(),
172+
)
173+
174+
ctxAlice := ghcontext.WithTokenInfo(context.Background(), &ghcontext.TokenInfo{Token: "token-for-alice"})
175+
cacheAlice, err := deps.GetRepoAccessCache(ctxAlice)
176+
require.NoError(t, err)
177+
require.NotNil(t, cacheAlice)
178+
179+
_, err = cacheAlice.IsSafeContent(ctxAlice, "mallory", "owner", "repo")
180+
require.NoError(t, err)
181+
182+
gqlN, restN := callCounts()
183+
require.Equal(t, 1, gqlN)
184+
require.Equal(t, 1, restN)
185+
186+
ctxBob := ghcontext.WithTokenInfo(context.Background(), &ghcontext.TokenInfo{Token: "token-for-bob"})
187+
cacheBob, err := deps.GetRepoAccessCache(ctxBob)
188+
require.NoError(t, err)
189+
require.NotNil(t, cacheBob)
190+
191+
_, err = cacheBob.IsSafeContent(ctxBob, "mallory", "owner", "repo")
192+
require.NoError(t, err)
193+
194+
gqlN, restN = callCounts()
195+
require.Equal(t, 2, gqlN, "a different identity's request must not be served from another identity's cached trust decision")
196+
require.Equal(t, 2, restN, "a different identity's request must not be served from another identity's cached trust decision")
197+
198+
// Repeating the same identity's token must reuse the warm per-identity
199+
// cache without any additional upstream calls.
200+
cacheAliceAgain, err := deps.GetRepoAccessCache(ctxAlice)
201+
require.NoError(t, err)
202+
_, err = cacheAliceAgain.IsSafeContent(ctxAlice, "mallory", "owner", "repo")
203+
require.NoError(t, err)
204+
205+
gqlN, restN = callCounts()
206+
require.Equal(t, 2, gqlN, "repeated requests from the same identity should reuse the warm cache")
207+
require.Equal(t, 2, restN, "repeated requests from the same identity should reuse the warm cache")
208+
}
209+
125210
func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) {
126211
t.Parallel()
127212

‎pkg/lockdown/lockdown.go‎

Lines changed: 99 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package lockdown
22

33
import (
44
"context"
5+
"crypto/sha256"
6+
"encoding/hex"
57
"fmt"
68
"log/slog"
79
"maps"
@@ -26,13 +28,23 @@ type RepoAccessCache struct {
2628
logger *slog.Logger
2729
trustedBotLogins map[string]struct{}
2830

31+
// now returns the current time and defaults to time.Now. Tests override it
32+
// to exercise bounded expiry deterministically without sleeping.
33+
now func() time.Time
34+
2935
viewerMu sync.Mutex
3036
viewerLogin string
3137
}
3238

3339
type repoAccessCacheEntry struct {
3440
isPrivate bool
3541
knownUsers map[string]bool // normalized login -> has push access
42+
43+
// createdAt is the wall-clock time this repository's trust decision was
44+
// first fetched. It is preserved across every subsequent update to the
45+
// entry (e.g. learning about a newly-seen author), so an entry's maximum
46+
// age is bounded from its original creation rather than reset by access.
47+
createdAt time.Time
3648
}
3749

3850
// RepoAccessInfo captures repository metadata needed for lockdown decisions.
@@ -49,8 +61,13 @@ const (
4961
// RepoAccessOption configures RepoAccessCache at construction time.
5062
type RepoAccessOption func(*RepoAccessCache)
5163

52-
// WithTTL overrides the default TTL applied to cache entries. A non-positive
53-
// duration disables expiration.
64+
// WithTTL overrides the default maximum age applied to cache entries. A
65+
// non-positive duration disables expiration.
66+
//
67+
// The TTL is a bounded, absolute age measured from when an entry's trust
68+
// decision was first fetched: repeated reads never extend it. This ensures
69+
// an actively-read entry is still refreshed once it reaches the maximum age,
70+
// rather than sliding its expiration forward indefinitely.
5471
func WithTTL(ttl time.Duration) RepoAccessOption {
5572
return func(c *RepoAccessCache) {
5673
c.ttl = ttl
@@ -66,6 +83,13 @@ func WithLogger(logger *slog.Logger) RepoAccessOption {
6683

6784
// WithCacheName overrides the cache table name used for storing entries.
6885
// Use this to isolate cache entries between tenants or in tests.
86+
//
87+
// cache2go.Cache(name) returns a process-wide singleton table keyed by name,
88+
// so any two RepoAccessCache instances constructed with the same name share
89+
// every cached trust decision. In deployments that serve multiple request
90+
// identities from one process (e.g. the HTTP server), callers MUST derive a
91+
// distinct name per identity — see CacheNameForIdentity — or one identity's
92+
// cached decision can be served to another.
6993
func WithCacheName(name string) RepoAccessOption {
7094
return func(c *RepoAccessCache) {
7195
if name != "" {
@@ -74,6 +98,24 @@ func WithCacheName(name string) RepoAccessOption {
7498
}
7599
}
76100

101+
// CacheNameForIdentity derives a stable cache table name scoped to a single
102+
// request identity (typically an auth token). Two calls with the same
103+
// identity always return the same name, so repeated requests from the same
104+
// identity keep sharing a warm cache; two calls with different identities
105+
// always return different names, so their cached trust decisions cannot mix.
106+
//
107+
// The identity is hashed so it never appears verbatim in cache-table names,
108+
// logs, or metrics. An empty identity returns an empty string, which
109+
// WithCacheName treats as a no-op (falling back to the default shared name);
110+
// callers that need isolation must ensure a non-empty identity is supplied.
111+
func CacheNameForIdentity(identity string) string {
112+
if identity == "" {
113+
return ""
114+
}
115+
sum := sha256.Sum256([]byte(identity))
116+
return "repo-access:" + hex.EncodeToString(sum[:])
117+
}
118+
77119
// NewRepoAccessCache creates a RepoAccessCache bound to the supplied clients.
78120
func NewRepoAccessCache(client *githubv4.Client, restClient *github.Client, opts ...RepoAccessOption) *RepoAccessCache {
79121
c := &RepoAccessCache{
@@ -187,37 +229,45 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner
187229
// so we publish a fresh entry with a cloned knownUsers map on every miss.
188230
if cacheItem, err := c.cache.Value(key); err == nil {
189231
entry := cacheItem.Data().(*repoAccessCacheEntry)
190-
if cachedHasPush, known := entry.knownUsers[userKey]; known {
191-
c.logDebug(ctx, fmt.Sprintf("repo access cache hit for user %s to %s/%s", username, owner, repo))
232+
233+
if !c.entryExpired(entry) {
234+
if cachedHasPush, known := entry.knownUsers[userKey]; known {
235+
c.logDebug(ctx, fmt.Sprintf("repo access cache hit for user %s to %s/%s", username, owner, repo))
236+
return RepoAccessInfo{
237+
IsPrivate: entry.isPrivate,
238+
HasPushAccess: cachedHasPush,
239+
}, nil
240+
}
241+
242+
c.logDebug(ctx, "known users cache miss, fetching permission")
243+
244+
hasPush, pushErr := c.checkPushAccess(ctx, username, owner, repo)
245+
if pushErr != nil {
246+
return RepoAccessInfo{}, pushErr
247+
}
248+
249+
users := make(map[string]bool, len(entry.knownUsers)+1)
250+
maps.Copy(users, entry.knownUsers)
251+
users[userKey] = hasPush
252+
// Preserve the entry's original createdAt: learning about a newly
253+
// seen author must not reset the entry's bounded maximum age.
254+
c.cache.Add(key, c.ttl, &repoAccessCacheEntry{
255+
isPrivate: entry.isPrivate,
256+
knownUsers: users,
257+
createdAt: entry.createdAt,
258+
})
259+
192260
return RepoAccessInfo{
193261
IsPrivate: entry.isPrivate,
194-
HasPushAccess: cachedHasPush,
262+
HasPushAccess: hasPush,
195263
}, nil
196264
}
197265

198-
c.logDebug(ctx, "known users cache miss, fetching permission")
199-
200-
hasPush, pushErr := c.checkPushAccess(ctx, username, owner, repo)
201-
if pushErr != nil {
202-
return RepoAccessInfo{}, pushErr
203-
}
204-
205-
users := make(map[string]bool, len(entry.knownUsers)+1)
206-
maps.Copy(users, entry.knownUsers)
207-
users[userKey] = hasPush
208-
c.cache.Add(key, c.ttl, &repoAccessCacheEntry{
209-
isPrivate: entry.isPrivate,
210-
knownUsers: users,
211-
})
212-
213-
return RepoAccessInfo{
214-
IsPrivate: entry.isPrivate,
215-
HasPushAccess: hasPush,
216-
}, nil
266+
c.logDebug(ctx, fmt.Sprintf("repo access cache entry for %s/%s exceeded max age, refreshing", owner, repo))
267+
} else {
268+
c.logDebug(ctx, fmt.Sprintf("repo access cache miss for user %s to %s/%s", username, owner, repo))
217269
}
218270

219-
c.logDebug(ctx, fmt.Sprintf("repo access cache miss for user %s to %s/%s", username, owner, repo))
220-
221271
isPrivate, viewerLogin, queryErr := c.queryRepoAccessInfo(ctx, owner, repo)
222272
if queryErr != nil {
223273
return RepoAccessInfo{}, queryErr
@@ -232,6 +282,7 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner
232282
c.cache.Add(key, c.ttl, &repoAccessCacheEntry{
233283
knownUsers: map[string]bool{userKey: hasPush},
234284
isPrivate: isPrivate,
285+
createdAt: c.clock(),
235286
})
236287

237288
return RepoAccessInfo{
@@ -240,6 +291,28 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner
240291
}, nil
241292
}
242293

294+
// entryExpired reports whether entry has reached the cache's bounded maximum
295+
// age, measured from its original creation time rather than its last access
296+
// time. Unlike the underlying cache2go table's own sliding expiry (which
297+
// resets on every read), this check ensures a frequently-accessed entry is
298+
// still forced to refresh once it is old enough, so stale trust decisions
299+
// cannot be kept alive indefinitely by repeated reads.
300+
func (c *RepoAccessCache) entryExpired(entry *repoAccessCacheEntry) bool {
301+
if c.ttl <= 0 {
302+
return false
303+
}
304+
return c.clock().Sub(entry.createdAt) >= c.ttl
305+
}
306+
307+
// clock returns the current time, using the injected now function if set
308+
// (tests use this to exercise bounded expiry deterministically).
309+
func (c *RepoAccessCache) clock() time.Time {
310+
if c.now != nil {
311+
return c.now()
312+
}
313+
return time.Now()
314+
}
315+
243316
// queryRepoAccessInfo fetches repository visibility and the viewer login in a single GraphQL round-trip.
244317
func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, owner, repo string) (bool, string, error) {
245318
if c.client == nil {

0 commit comments

Comments
 (0)