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>
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>
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 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>
* chore(deps): bump go-github v87 -> v89 and resolve breaking changes
Bumps google/go-github from v87 to v89 across the module and fixes the
resulting breaking changes. No tool or behavior changes.
- Rewrite all import paths go-github/v87 -> go-github/v89.
- gists.go: Gists.Create now takes CreateGistRequest by value and
Gists.Edit is renamed to Gists.Update taking UpdateGistRequest.
- repositories_test.go: adapt to RepositoryRelease fields that became
value types in v89 (ID, TagName, Draft).
- Regenerate third-party license files for the new module path.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Don't clear gist description on update when omitted
update_gist always sent Description as a pointer to the OptionalParam
zero value (""), so omitting description would overwrite an existing gist
description with an empty string. Only set UpdateGistRequest.Description
when the caller actually provided the argument; an explicit empty string
still clears it. Adds a test asserting the description key is absent from
the PATCH body when omitted and present when set.
This addresses a pre-existing behavior surfaced while migrating to the
v89 gist request types.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Breaking changes addressed:
- raw.NewClient: Use WithHTTPClient/WithEnterpriseURLs options, pass ctx to
NewRequest, return (*Client, error)
- internal/ghmcp/server.go: Use functional options for REST client creation,
replace UserAgent field mutation with UserAgentTransport wrapper, add
restUATransp field to githubClients struct
- pkg/github/dependencies.go: Use functional options for REST client creation,
handle raw.NewClient error return
- pkg/github/actions.go: Handle new WorkflowDispatchRunDetails return value
from CreateWorkflowDispatchEventByID/ByFileName
- pkg/github/issues.go: Replace IssueListOptions with ListOptions for
SubIssue.ListByIssue
- pkg/github/notifications.go: MarkThreadDone now takes string instead of
int64; remove ParseInt and strconv import
- pkg/github/projects.go: Remove pointer indirection from
ListProjectsPaginationOptions and ListProjectsOptions fields
- pkg/github/issues_granular.go: Pass ctx to NewRequest, remove ctx from Do
- Test files: Add mustNewGHClient helper, replace all NewClient calls,
fix stubClientFnFromHTTP signature, fix lockdown_test.go BaseURL handling,
fix raw_test.go, remove invalid threadID test case
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CodeQL / Analyze (actions) (push) Has been cancelled
CodeQL / Analyze (javascript) (push) Has been cancelled
Docker / build (push) Has been cancelled
Build and Test Go Project / build (macos-latest) (push) Has been cancelled
Build and Test Go Project / build (ubuntu-latest) (push) Has been cancelled
Build and Test Go Project / build (windows-latest) (push) Has been cancelled
GoReleaser Release / release (push) Has been cancelled
MCP Server Diff / mcp-diff (push) Has been cancelled
Publish to MCP Registry / publish (push) Has been cancelled
* use REST API for permission checks
* update tests
* skip API call for bots and add github-action[bot] to trusted logins
* improve tests
* add nil guard to IsSafeContent
* add comment clarifying maintain mapping
---------
Co-authored-by: Sam Morrow <info@sam-morrow.com>
Sam Morrow