fix(lockdown): bound repo-access cache expiry and isolate it per identity - #3113
Open
SamMorrowDrums wants to merge 6 commits into
Open
Conversation
…tity 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 <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Bounds lockdown cache expiry and isolates trust decisions by request identity.
Changes:
- Adds absolute TTL enforcement with deterministic tests.
- Derives hashed, identity-scoped cache names.
- Applies identity isolation in HTTP request dependencies.
Show a summary per file
| File | Description |
|---|---|
pkg/lockdown/lockdown.go |
Implements bounded expiry and identity cache naming. |
pkg/lockdown/lockdown_test.go |
Tests expiry and identity isolation. |
pkg/github/dependencies.go |
Scopes request caches by token. |
pkg/github/dependencies_test.go |
Tests request-level isolation. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
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>
The golangci-lint action downloads a JSON schema from golangci-lint.run on every run to verify .golangci.yml. A blip reaching that host fails the job before any linter runs, as it did on this PR. Linting should depend only on the checked-out code, which is also what script/lint does locally.
…ed-expiry-for-lockdown-c56a37
This reverts commit d7d8dd2. The lint job failed on a transient timeout fetching the golangci-lint config schema, which is a CI infrastructure concern rather than a defect in this change. Disabling schema verification to work around it does not belong in a cache-hardening PR: it weakens a check for every future run, and its root cause is out of scope here. Leaving CI configuration untouched keeps this PR to the lockdown cache redesign. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The cache changes carried explanatory comments that restated the code or narrated what each step did. Drop them and keep only what the code cannot express: that cache2go never reclaims a named table, that its own expiry slides on every read, that createdAt survives entry updates, and that RepoAccessOpts is shared across requests. Exported options keep a short doc comment. Comment-only; no behavior change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The lockdown repo-access cache (
pkg/lockdown) had two related issues:cache2go, which extends an entry's TTL on every read (Value()callsKeepAlive()). A frequently-accessed entry — e.g. because it's driving repeated lockdown checks — never actually expires, so a stale trust decision (like since-revoked push access) can be kept alive indefinitely instead of being refreshed after its TTL.cache2go.Cache(name)returns a process-wide singleton table keyed by name.RequestDeps.GetRepoAccessCachebuilds a new*lockdown.RepoAccessCacheper request, but all requests reused the same default-named table (noWithCacheName), so one caller's cached trust decision for a repo could be served to a completely different caller — without that second caller's own credentials ever being checked. I verified this concretely with a throwaway reproduction before fixing it: a "bob" request was served "alice"'s cached decision without bob's own REST client ever being invoked.Fix
repoAccessCacheEntrynow tracks its originalcreatedAt. A bounded, absolute max age is enforced from that fixed point (not last access), so entries refresh after the TTL regardless of read frequency. Learning about a newly-seen author on an existing entry preserves the originalcreatedAtrather than resetting it.lockdown.CacheNameForIdentity(identity string) string, which derives a stable, hashed cache-table name per identity. Same identity → same name (keeps a warm cache across a session's repeated requests); different identities → different names (no shared state). The raw identity/token never appears in the derived name.RequestDeps.GetRepoAccessCachenow scopes each request's cache viaCacheNameForIdentity(token), closing the cross-identity leak for HTTP/multi-tenant deployments. Stdio mode is untouched — it builds a singleRepoAccessCachefor the whole process lifetime, as before.Tests
All new tests are deterministic (no sleeps/flaky timing):
TestRepoAccessCacheBoundedExpiryIgnoresRepeatedAccess/TestRepoAccessCacheNewUserDoesNotResetEntryAge— bounded expiry via an injectable clock.TestCacheNameForIdentity/TestRepoAccessCacheIdentityScopedNamesPreventCrossIdentityLeakage— naming helper + cross-identity isolation at thelockdownpackage level.TestGetRepoAccessCacheIsolatesTrustDecisionsPerIdentity(pkg/github) — mirrors the HTTP server's exact construction pattern end-to-end. I confirmed this test fails against the pre-fixdependencies.goand passes with the fix.script/lintandscript/test(go test -race ./...) both pass.Fixes #3107