fix(credentialsource): bound token-exchange cache TTL and evict on upstream 401/403

AGENTS.md

@@ -102,6 +102,15 @@ go vet ./...
 go build -o gatekeeper ./cmd/gatekeeper/
 ```
 
+## Testing
+
+Always work test-first. Write the test, run it and confirm it fails, then write the implementation and confirm it passes. This holds even for small bug fixes where the fix looks obvious — a test written after the fix can pass without ever having demonstrated the bug. The failing run is the evidence that the test is testing something.
+
+- **Confirm the test fails for the right reason.** Prefer a failure that demonstrates the defect over one that is merely a compile error. If asserting against a not-yet-existing constant would only produce `undefined: X`, pin the expected value in the test instead, so it compiles and fails on behavior. A red state of `cached TTL = 7h56m12s, want <= 1m0s` names the bug; `undefined: maxTokenTTL` does not.
+- **If you wrote the fix first, back it out.** `git checkout -- <file>`, write the test, watch it fail, then re-apply.
+- **When a clean red is impossible** — adding a new struct field or method makes the compile error unavoidable — mutation-check afterward: neuter the implementation (make the new method a no-op), confirm the test fails, restore. This catches tests that pass vacuously.
+- **Regression tests should encode the incident.** Name the real values from the bug report in the test, so the failure output reads like the original symptom.
+
 ## Code Style
 
 - Follow standard Go conventions and `go fmt` formatting
@@ -135,3 +144,24 @@ This module (`github.com/majorcontext/gatekeeper`) was extracted from moat's `in
 - Use `gh pr create` with default flags only (no `--base`, `--head`, etc.)
 - If `gh pr create` fails, report the error to the operator immediately
 - Do not attempt to work around failures by adding flags or changing configuration
+
+## Responding to Review Feedback
+
+- **Resolve a review thread once its finding is addressed.** Reply explaining what changed (or why nothing did), then resolve it. A thread left open after the fix has landed reads as unaddressed.
+- Resolving requires GraphQL — `gh pr review` cannot do it:
+
+```bash
+# List threads and their IDs
+gh api graphql -f query='{repository(owner:"majorcontext",name:"gatekeeper"){
+  pullRequest(number:NN){reviewThreads(first:20){nodes{id isResolved}}}}}'
+
+# Reply to a thread, then resolve it
+gh api graphql -f query='mutation($tid:ID!,$body:String!){
+  addPullRequestReviewThreadReply(input:{pullRequestReviewThreadId:$tid,body:$body}){comment{url}}}' \
+  -f tid=THREAD_ID -f body='...'
+gh api graphql -f query='mutation($tid:ID!){
+  resolveReviewThread(input:{threadId:$tid}){thread{isResolved}}}' -f tid=THREAD_ID
+```
+
+- Verify a reviewer's claim before acting on it, and say so if the suggested fix is wrong. A reviewer can correctly identify a bug while proposing a remedy that does not fix it.
+- Do not silently expand a PR's scope to fix pre-existing bugs a review surfaces. Note them, and ask whether to fold them in or track separately.

CHANGELOG.md

@@ -4,12 +4,16 @@ Gatekeeper is a standalone credential-injecting TLS-intercepting proxy. It trans
 
 Gatekeeper is pre-1.0. The configuration schema and credential source interface may change between minor versions.
 
-## Unreleased
+## v0.15.0 — 2026-07-09
 
 ### Added
 
 - **`process` credential source** — run a host command (`sh -c`) and use its stdout as the credential value; any helper that prints a credential works (OS keychain CLIs, `pass`, 1Password's `op`, AWS `credential_process` helpers). Implements `RefreshingSource`: when the output is AWS `credential_process`-format JSON (exact-case `Version`/`AccessKeyId`/`Expiration` keys, so unrelated JSON can't hijack the schedule), the credential refreshes on the embedded `Expiration`, and already-expired output fails the fetch (engaging retry backoff) instead of installing credentials that would 401; other output reports a configurable `ttl` (default 5m; gatekeeper re-fetches at the standard 75%-of-TTL schedule). Header-invalid control characters are stripped from the output, with a warning (count only, never the value) when non-whitespace control bytes were present; stderr is included (truncated) in fetch errors for diagnosability, stdout never; the command string is config-owned and must not be accepted from untrusted config by embedding operators ([#38](https://github.com/majorcontext/gatekeeper/pull/38))
 
+### Fixed
+
+- **Token-exchange credentials no longer serve a rotated token until its advertised expiry** — `TokenExchangeSource.Resolve` cached each exchanged token for the full `expires_in` returned by the STS, with no invalidation path. When an STS reports the remaining lifetime of the underlying credential (e.g. a GitHub user-to-server token, ~8h), a credential that was revoked, rotated, or re-authorized upstream kept being injected for the rest of that window: every request failed with a `403` from the destination, and the only remediation was restarting the process to flush the in-memory cache. Two changes bound this. First, cache TTL is now capped at 1 minute regardless of the advertised `expires_in` — a long `expires_in` only means the token *may* live that long, not that it stays valid, and `Resolve` is singleflighted so the extra exchanges coalesce. Second, a `401` or `403` from the destination now drops the cache entry for that `(subject, actor)` so the next request exchanges afresh; the failed request is **not** retried (its body is already consumed, and requests like a git push are not idempotent). Because gatekeeper sees only a status code — a GitHub `403` covers "re-authorize the app", secondary rate limits, and plain permission denials alike — evictions are rate-limited to one per key per 10 seconds, so a client looping on a failing request cannot drive one STS exchange per request. An `Invalidate` hook on `proxy.CredentialHeader` carries this signal; it is nil for credentials with no cache behind them, and only the credential actually injected into the rejected request is evicted — when several credentials for a host share a header name, the ones that lost de-duplication keep their cache entries. Note that `expires_in` values above the cap no longer reduce STS request volume ([#39](https://github.com/majorcontext/gatekeeper/pull/39))
+
 ## v0.14.1 — 2026-06-23
 
 ### Fixed

credentialsource/tokenexchange.go

@@ -34,7 +34,8 @@ type TokenExchangeResponse struct {
 }
 
 // TokenExchangeSource exchanges a subject token for an access token via
-// RFC 8693. It caches tokens per subject with TTL from the STS response.
+// RFC 8693. It caches tokens per subject with TTL from the STS response,
+// capped at maxTokenTTL.
 type TokenExchangeSource struct {
 	endpoint         string
 	clientID         string
@@ -44,9 +45,19 @@ type TokenExchangeSource struct {
 	actorTokenType   string
 	client           *http.Client
 
+	// invalidateCooldown bounds how often a given key may be evicted by
+	// Invalidate. Overridden in tests.
+	invalidateCooldown time.Duration
+
 	mu    sync.Mutex
 	cache map[tokenCacheKey]cachedToken
-	sf    singleflight.Group
+	// cacheGen increments on every Invalidate. A Resolve that captured an
+	// older generation must not write its result: the exchange may have been
+	// issued before the upstream credential rotated, so caching it would
+	// re-stale the entry the invalidation just cleared.
+	cacheGen        uint64
+	lastInvalidated map[tokenCacheKey]time.Time
+	sf              singleflight.Group
 }
 
 type tokenCacheKey struct {
@@ -70,14 +81,16 @@ func NewTokenExchangeSource(cfg TokenExchangeConfig) *TokenExchangeSource {
 		actorTokenType = "urn:ietf:params:oauth:token-type:access_token"
 	}
 	return &TokenExchangeSource{
-		endpoint:         cfg.Endpoint,
-		clientID:         cfg.ClientID,
-		clientSecret:     cfg.ClientSecret,
-		resource:         cfg.Resource,
-		subjectTokenType: subjectTokenType,
-		actorTokenType:   actorTokenType,
-		client:           &http.Client{Timeout: 30 * time.Second},
-		cache:            make(map[tokenCacheKey]cachedToken),
+		endpoint:           cfg.Endpoint,
+		clientID:           cfg.ClientID,
+		clientSecret:       cfg.ClientSecret,
+		resource:           cfg.Resource,
+		subjectTokenType:   subjectTokenType,
+		actorTokenType:     actorTokenType,
+		client:             &http.Client{Timeout: 30 * time.Second},
+		cache:              make(map[tokenCacheKey]cachedToken),
+		lastInvalidated:    make(map[tokenCacheKey]time.Time),
+		invalidateCooldown: defaultInvalidateCooldown,
 	}
 }
 
@@ -130,10 +143,62 @@ func (s *TokenExchangeSource) Exchange(ctx context.Context, subjectToken, actorT
 	return &result, nil
 }
 
-const defaultTokenTTL = 5 * time.Minute
+// maxTokenTTL caps how long an exchanged token is cached, regardless of the
+// expires_in the STS advertises. A long expires_in only means the token may
+// live that long, not that it stays valid: the upstream credential behind the
+// exchange can be revoked or rotated at any time, and gatekeeper has no way to
+// learn of it. Capping bounds how long a stale credential keeps being injected
+// after such a change. Resolve is singleflighted, so the extra STS calls are
+// coalesced and cheap.
+const maxTokenTTL = time.Minute
+
+// defaultInvalidateCooldown bounds how often one key may force a re-exchange.
+//
+// Invalidate's trigger is an upstream rejection, which gatekeeper can only see
+// as a status code — a GitHub 403 means "re-authorize the app", but equally
+// "secondary rate limit" or "no write access to this repo". A client looping on
+// a request that always fails would otherwise drive one STS exchange per
+// request, so evictions for a given key are rate-limited. The cost is bounded
+// recovery latency: at worst one cooldown passes before a genuinely rotated
+// credential is picked up.
+const defaultInvalidateCooldown = 10 * time.Second
+
+// Invalidate drops the cached token for the given subject and actor, so the
+// next Resolve performs a fresh exchange. Callers invoke it when the
+// destination rejects an injected credential, which usually means the upstream
+// credential behind the exchange was rotated or re-authorized and the cached
+// token predates that change.
+//
+// Evictions are rate-limited per key (see defaultInvalidateCooldown); calls
+// within the cooldown are no-ops. Invalidate is safe to call when no entry is
+// cached — it still bars any in-flight exchange from caching a result that
+// predates it.
+func (s *TokenExchangeSource) Invalidate(subjectToken, actorToken string) {
+	ck := tokenCacheKey{subject: subjectToken, actor: actorToken}
+
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	now := time.Now()
+	if last, ok := s.lastInvalidated[ck]; ok && now.Sub(last) < s.invalidateCooldown {
+		return
+	}
+	s.lastInvalidated[ck] = now
+
+	delete(s.cache, ck)
+	s.cacheGen++
+
+	// Bound the bookkeeping map: entries past their cooldown carry no meaning.
+	for k, t := range s.lastInvalidated {
+		if now.Sub(t) >= s.invalidateCooldown {
+			delete(s.lastInvalidated, k)
+		}
+	}
+}
 
 // Resolve returns a credential for the given subject, using the cache when
-// possible. Concurrent requests for the same subject are coalesced into a
+// possible. Cache entries live for the STS-advertised expires_in, capped at
+// maxTokenTTL. Concurrent requests for the same subject are coalesced into a
 // single STS call via singleflight. When actorToken is non-empty, it is
 // forwarded to the STS as the RFC 8693 actor_token parameter and included
 // in the cache key. When requestID is non-empty, it is forwarded as
@@ -156,6 +221,7 @@ func (s *TokenExchangeSource) Resolve(ctx context.Context, subjectToken, actorTo
 			s.mu.Unlock()
 			return cached.accessToken, nil
 		}
+		gen := s.cacheGen
 		s.mu.Unlock()
 
 		// WithoutCancel strips both cancellation and deadline from the parent.
@@ -171,8 +237,8 @@ func (s *TokenExchangeSource) Resolve(ctx context.Context, subjectToken, actorTo
 		}
 
 		ttl := time.Duration(result.ExpiresIn) * time.Second
-		if ttl <= 0 {
-			ttl = defaultTokenTTL
+		if ttl <= 0 || ttl > maxTokenTTL {
+			ttl = maxTokenTTL
 		}
 
 		s.mu.Lock()
@@ -182,9 +248,15 @@ func (s *TokenExchangeSource) Resolve(ctx context.Context, subjectToken, actorTo
 				delete(s.cache, k)
 			}
 		}
-		s.cache[ck] = cachedToken{
-			accessToken: result.AccessToken,
-			expiresAt:   now.Add(ttl),
+		// Only cache when no Invalidate ran while the exchange was in flight.
+		// Otherwise this token may predate the rotation that prompted the
+		// invalidation, and writing it would re-stale the entry that was just
+		// cleared. The caller still gets this token; only the caching is skipped.
+		if s.cacheGen == gen {
+			s.cache[ck] = cachedToken{
+				accessToken: result.AccessToken,
+				expiresAt:   now.Add(ttl),
+			}
 		}
 		s.mu.Unlock()