github-featured/github--github-mcp-server
2
0
Fork 0
Code Issues Pull Requests Deployments Models Agent Notes Packages Projects Releases Wiki Activity

fix(lockdown): drop fixed-age expiry, keep per-identity cache isolation

Browse Source 
The cache's idle/sliding TTL is cache2go's documented behaviour and was
deliberate in both the original hand-rolled cache and the cache2go
migration: a hot repo keeps serving from cache and only idle entries are
reclaimed. Replacing it with a fixed max age traded that away for a
periodic refetch on every hot repo, which is a freshness change rather
than the isolation fix this issue is about.

Remove createdAt, the injected clock, entryExpired, the createdAt
preservation on entry updates, and the tests that only existed to prove
bounded non-sliding expiry. Restore the original sliding semantics.

Keep the per-caller isolation, which is the actual defect: entries were
keyed on owner/repo alone in a process-wide table, so a trust decision
computed under one caller's credentials could be served to another
caller whose own credentials were never checked. Entry keys now carry a
SHA-256 digest of the request identity, inside a single bounded table.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Sam Morrow
2026-08-19 16:04:07 +02:00
parent acd2afc766
commit 95b347e945
2 changed files with 32 additions and 123 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/lockdown/lockdown.go
+27 -57
View File
@@ -28,7 +28,6 @@ type RepoAccessCache struct {
logger *slog.Logger
trustedBotLogins map[string]struct{}
identityDigest string
now func() time.Time
viewerMu sync.Mutex
viewerLogin string
@@ -37,9 +36,6 @@ type RepoAccessCache struct {
type repoAccessCacheEntry struct {
isPrivate bool
knownUsers map[string]bool // normalized login -> has push access
// Preserved across entry updates, so age is bounded from the first fetch.
createdAt time.Time
}
// RepoAccessInfo captures repository metadata needed for lockdown decisions.
@@ -56,9 +52,8 @@ const (
// RepoAccessOption configures RepoAccessCache at construction time.
type RepoAccessOption func(*RepoAccessCache)
// WithTTL overrides the default maximum age applied to cache entries. A
// non-positive duration disables expiration. The age is absolute, measured
// from an entry's first fetch: repeated reads never extend it.
// WithTTL overrides the default TTL applied to cache entries. A non-positive
// duration disables expiration.
func WithTTL(ttl time.Duration) RepoAccessOption {
return func(c *RepoAccessCache) {
c.ttl = ttl
@@ -92,7 +87,7 @@ func WithCacheName(name string) RepoAccessOption {
// no-op.
//
// Scoping lives in the entry key rather than the table so per-identity state
// stays bounded and is reclaimed by ordinary TTL cleanup. The identity is
// 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) {
@@ -217,44 +212,37 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner
// so we publish a fresh entry with a cloned knownUsers map on every miss.
if cacheItem, err := c.cache.Value(key); err == nil {
entry := cacheItem.Data().(*repoAccessCacheEntry)
if !c.entryExpired(entry) {
if cachedHasPush, known := entry.knownUsers[userKey]; known {
c.logDebug(ctx, fmt.Sprintf("repo access cache hit for user %s to %s/%s", username, owner, repo))
return RepoAccessInfo{
IsPrivate: entry.isPrivate,
HasPushAccess: cachedHasPush,
}, nil
}
c.logDebug(ctx, "known users cache miss, fetching permission")
hasPush, pushErr := c.checkPushAccess(ctx, username, owner, repo)
if pushErr != nil {
return RepoAccessInfo{}, pushErr
}
users := make(map[string]bool, len(entry.knownUsers)+1)
maps.Copy(users, entry.knownUsers)
users[userKey] = hasPush
// Preserve createdAt: a new author must not reset the entry's age.
c.cache.Add(key, c.ttl, &repoAccessCacheEntry{
isPrivate: entry.isPrivate,
knownUsers: users,
createdAt: entry.createdAt,
})
if cachedHasPush, known := entry.knownUsers[userKey]; known {
c.logDebug(ctx, fmt.Sprintf("repo access cache hit for user %s to %s/%s", username, owner, repo))
return RepoAccessInfo{
IsPrivate: entry.isPrivate,
HasPushAccess: hasPush,
HasPushAccess: cachedHasPush,
}, nil
}
c.logDebug(ctx, fmt.Sprintf("repo access cache entry for %s/%s exceeded max age, refreshing", owner, repo))
} else {
c.logDebug(ctx, fmt.Sprintf("repo access cache miss for user %s to %s/%s", username, owner, repo))
c.logDebug(ctx, "known users cache miss, fetching permission")
hasPush, pushErr := c.checkPushAccess(ctx, username, owner, repo)
if pushErr != nil {
return RepoAccessInfo{}, pushErr
}
users := make(map[string]bool, len(entry.knownUsers)+1)
maps.Copy(users, entry.knownUsers)
users[userKey] = hasPush
c.cache.Add(key, c.ttl, &repoAccessCacheEntry{
isPrivate: entry.isPrivate,
knownUsers: users,
})
return RepoAccessInfo{
IsPrivate: entry.isPrivate,
HasPushAccess: hasPush,
}, nil
}
c.logDebug(ctx, fmt.Sprintf("repo access cache miss for user %s to %s/%s", username, owner, repo))
isPrivate, viewerLogin, queryErr := c.queryRepoAccessInfo(ctx, owner, repo)
if queryErr != nil {
return RepoAccessInfo{}, queryErr
@@ -269,7 +257,6 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner
c.cache.Add(key, c.ttl, &repoAccessCacheEntry{
knownUsers: map[string]bool{userKey: hasPush},
isPrivate: isPrivate,
createdAt: c.clock(),
})
return RepoAccessInfo{
@@ -278,23 +265,6 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner
}, nil
}
// entryExpired reports whether entry has reached the cache's maximum age,
// measured from creation. cache2go's own expiry instead slides on every read,
// which would let repeated reads keep a stale decision alive indefinitely.
func (c *RepoAccessCache) entryExpired(entry *repoAccessCacheEntry) bool {
if c.ttl <= 0 {
return false
}
return c.clock().Sub(entry.createdAt) >= c.ttl
}
func (c *RepoAccessCache) clock() time.Time {
if c.now != nil {
return c.now()
}
return time.Now()
}
// queryRepoAccessInfo fetches repository visibility and the viewer login in a single GraphQL round-trip.
func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, owner, repo string) (bool, string, error) {
if c.client == nil {
pkg/lockdown/lockdown_test.go
+5 -66
View File
@@ -115,17 +115,14 @@ func newMockRepoAccessCache(t *testing.T, ttl time.Duration) (*RepoAccessCache,
func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) {
ctx := t.Context()
cache, transport := newMockRepoAccessCache(t, time.Minute)
start := time.Now()
cache.now = func() time.Time { return start }
cache, transport := newMockRepoAccessCache(t, 5*time.Millisecond)
info, err := cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
require.NoError(t, err)
require.False(t, info.IsPrivate)
require.True(t, info.HasPushAccess)
require.EqualValues(t, 1, transport.CallCount())
cache.now = func() time.Time { return start.Add(2 * time.Minute) }
time.Sleep(20 * time.Millisecond)
info, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
require.NoError(t, err)
@@ -134,64 +131,6 @@ func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) {
require.EqualValues(t, 2, transport.CallCount())
}
// Regression test for #3107: sliding expiry would let a frequently-read entry
// outlive revoked access, so age must be bounded from creation.
func TestRepoAccessCacheBoundedExpiryIgnoresRepeatedAccess(t *testing.T) {
ctx := t.Context()
const ttl = 100 * time.Second
cache, transport := newMockRepoAccessCache(t, ttl)
current := time.Now()
cache.now = func() time.Time { return current }
info, err := cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
require.NoError(t, err)
require.True(t, info.HasPushAccess)
require.EqualValues(t, 1, transport.CallCount())
// Each read lands well inside the TTL; only their sum exceeds it.
for range 4 {
current = current.Add(20 * time.Second)
_, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
require.NoError(t, err)
}
require.EqualValues(t, 1, transport.CallCount(), "repeated access within the bounded window must still be served from cache")
current = current.Add(30 * time.Second)
info, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
require.NoError(t, err)
require.True(t, info.HasPushAccess)
require.EqualValues(t, 2, transport.CallCount(), "entry must refresh once its absolute age exceeds the TTL, regardless of access frequency")
}
// A "known users" miss updates an existing entry, a second path that must not
// reset its age.
func TestRepoAccessCacheNewUserDoesNotResetEntryAge(t *testing.T) {
ctx := t.Context()
const ttl = 100 * time.Second
gqlClient, transport := newMockGQLClient(testUser, false)
restClient := newMockRESTServer(t, "write")
cache := NewRepoAccessCache(gqlClient, restClient, WithTTL(ttl), WithCacheName(t.Name()))
start := time.Now()
cache.now = func() time.Time { return start }
_, err := cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
require.NoError(t, err)
require.EqualValues(t, 1, transport.CallCount())
cache.now = func() time.Time { return start.Add(50 * time.Second) }
_, err = cache.getRepoAccessInfo(ctx, "someone-else", testOwner, testRepo)
require.NoError(t, err)
require.EqualValues(t, 1, transport.CallCount(), "checking a new user against a cached repo entry must not re-query repo metadata")
cache.now = func() time.Time { return start.Add(120 * time.Second) }
_, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo)
require.NoError(t, err)
require.EqualValues(t, 2, transport.CallCount(), "entry age must be bounded from its original creation, not reset by learning about a new user")
}
func TestRepoAccessCacheIsolatesViewerPerInstance(t *testing.T) {
ctx := t.Context()
@@ -272,8 +211,8 @@ func TestRepoAccessCacheIdentityScopingIsolatesWithinOneTable(t *testing.T) {
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 TTL cleanup reclaims them;
// a table per identity could not shrink this way.
// 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()
@@ -296,7 +235,7 @@ func TestRepoAccessCacheIdentityScopedEntriesAreReclaimed(t *testing.T) {
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")
"per-identity entries must be reclaimed by ordinary idle-TTL cleanup so cache storage stays bounded")
}
type flakyTransport struct {