From 50ee2fd4c954ee96584595cdced1a82a7ae72347 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 29 Aug 2026 21:43:36 -0700 Subject: [PATCH 1/2] Invalidate cached api key validations on revoke --- .../apikey-revoke-cache-invalidation.md | 7 ++ .../api-key-validation-cache.node.test.ts | 109 ++++++++++++++++-- apps/cloud/src/auth/api-keys.ts | 82 +++++++++---- apps/cloud/src/mcp/agent-handler.ts | 27 ++--- 4 files changed, 184 insertions(+), 41 deletions(-) create mode 100644 .changeset/apikey-revoke-cache-invalidation.md diff --git a/.changeset/apikey-revoke-cache-invalidation.md b/.changeset/apikey-revoke-cache-invalidation.md new file mode 100644 index 0000000000..1f6b8077bb --- /dev/null +++ b/.changeset/apikey-revoke-cache-invalidation.md @@ -0,0 +1,7 @@ +--- +"@executor-js/cloud": patch +--- + +**Revoking an API key now invalidates its cached validation** + +The per-isolate validation cache made a revoked key keep authenticating for up to 60 seconds. Two fixes close that: the cache map now lives at module scope, shared by every build of the key service (the account middleware rebuilds it per request, so a per-build map left the revoke invalidating a fresh empty cache), and both revoke paths drop the revoked key's entries from it. A revoked key is refused on the next request served by the isolate that processed the revoke; other isolates still age the entry out within the TTL. diff --git a/apps/cloud/src/auth/api-key-validation-cache.node.test.ts b/apps/cloud/src/auth/api-key-validation-cache.node.test.ts index 6c1b32eca9..63365369cd 100644 --- a/apps/cloud/src/auth/api-key-validation-cache.node.test.ts +++ b/apps/cloud/src/auth/api-key-validation-cache.node.test.ts @@ -171,10 +171,46 @@ describe("makeCachedApiKeyValidate", () => { expect(cached.cacheKeys()).not.toContain(rawKey); }), ); + + it.effect("invalidating a key id drops its entries and forces revalidation", () => + Effect.gen(function* () { + let calls = 0; + const cached = makeCachedApiKeyValidate(() => + Effect.sync(() => { + calls += 1; + return userOwner("api_key_1"); + }), + ); + + yield* cached.validate("sk_test_abc"); + cached.invalidateKeyId("api_key_1"); + expect(cached.cacheKeys()).toHaveLength(0); + + yield* cached.validate("sk_test_abc"); + expect(calls).toBe(2); + }), + ); + + it.effect("invalidating one key id leaves other keys' entries cached", () => + Effect.gen(function* () { + const cached = makeCachedApiKeyValidate((value) => + Effect.succeed(userOwner(`key_for_${value}`)), + ); + + yield* cached.validate("sk_1"); + yield* cached.validate("sk_2"); + cached.invalidateKeyId("key_for_sk_1"); + + expect(cached.cacheKeys()).toHaveLength(1); + }), + ); }); +// These tests build the REAL WorkOS layer, whose cache map is module-scope — +// entries written by one test are visible to the next within this file. Each +// test therefore uses key values of its own; none may reuse another's. describe("ApiKeyService.WorkOS validation cache", () => { - it.effect("validates a repeated key through one upstream call per layer build", () => + it.effect("serves a key cached by one layer build from a second build", () => Effect.gen(function* () { let calls = 0; const layer = ApiKeyService.WorkOS.pipe( @@ -185,7 +221,7 @@ describe("ApiKeyService.WorkOS validation cache", () => { calls += 1; return { apiKey: { - id: "api_key_123", + id: "api_key_shared", owner: { type: "user", id: "user_123", organizationId: "org_123" }, }, }; @@ -194,17 +230,74 @@ describe("ApiKeyService.WorkOS validation cache", () => { ), ); - const { first, second } = yield* Effect.gen(function* () { + // Two separate `Effect.provide(layer)` runs are two separate layer + // builds — the account middleware rebuilds this layer per request, so + // cross-build reuse is exactly what production needs the cache to do. + const first = yield* Effect.gen(function* () { const apiKeys = yield* ApiKeyService; - return { - first: yield* apiKeys.validate("sk_test_abc"), - second: yield* apiKeys.validate("sk_test_abc"), - }; + return yield* apiKeys.validate("sk_test_cross_build"); + }).pipe(Effect.provide(layer)); + const second = yield* Effect.gen(function* () { + const apiKeys = yield* ApiKeyService; + return yield* apiKeys.validate("sk_test_cross_build"); }).pipe(Effect.provide(layer)); expect(calls).toBe(1); - expect(first?.keyId).toBe("api_key_123"); + expect(first?.keyId).toBe("api_key_shared"); expect(second).toEqual(first); }), ); + + it.effect("a revoke in one layer build refuses the cached value in every build", () => + Effect.gen(function* () { + // Live until revoked: the stub answers with the key while `revoked` is + // false and with a null apiKey afterwards, the same flip the WorkOS + // backend performs. The revoke runs in its OWN layer build — the shape + // of the real console flow, where the account middleware rebuilds the + // layer per request — and must still drop the entry the validating + // plane cached. + let revoked = false; + let calls = 0; + const layer = ApiKeyService.WorkOS.pipe( + Layer.provide( + stubWorkOS({ + validateApiKey: () => + Effect.sync(() => { + calls += 1; + return { + apiKey: revoked + ? null + : { + id: "api_key_revoked", + owner: { type: "user", id: "user_123", organizationId: "org_123" }, + }, + }; + }), + deleteApiKey: () => + Effect.sync(() => { + revoked = true; + return {}; + }), + }), + ), + ); + + const before = yield* Effect.gen(function* () { + const apiKeys = yield* ApiKeyService; + return yield* apiKeys.validate("sk_test_revocable"); + }).pipe(Effect.provide(layer)); + yield* Effect.gen(function* () { + const apiKeys = yield* ApiKeyService; + return yield* apiKeys.revokeUserKey({ keyId: "api_key_revoked" }); + }).pipe(Effect.provide(layer)); + const after = yield* Effect.gen(function* () { + const apiKeys = yield* ApiKeyService; + return yield* apiKeys.validate("sk_test_revocable"); + }).pipe(Effect.provide(layer)); + + expect(before?.keyId).toBe("api_key_revoked"); + expect(after, "the revoke dropped the cached entry, not just the upstream key").toBeNull(); + expect(calls).toBe(2); + }), + ); }); diff --git a/apps/cloud/src/auth/api-keys.ts b/apps/cloud/src/auth/api-keys.ts index 9ff7a1c66a..b8b3e28971 100644 --- a/apps/cloud/src/auth/api-keys.ts +++ b/apps/cloud/src/auth/api-keys.ts @@ -296,10 +296,14 @@ const createdFromResponse = (value: unknown): CreatedApiKey | null => // failure always miss: an attacker probing bad keys cannot poison the map // (the size bound alone handles memory), and a just-created key works // immediately instead of waiting out a stale negative entry. -// - REVOCATION WINDOW: a revoked key keeps working for up to the TTL (60s) -// in any isolate that validated it before revocation. That is the price -// of the cache, and it is far tighter than the 1h JWKS rotation window -// the JWT path already accepts. +// - REVOCATION WINDOW: the isolate that serves the revoke drops its own +// entries for that key id immediately (`invalidateKeyId`, called by both +// revoke paths below), so revoking a key through the console refuses it +// on the next request wherever the same isolate validates. OTHER isolates +// that validated the key before revocation keep serving it for up to the +// TTL (60s). That residual window is the price of the cache, and it is +// far tighter than the 1h JWKS rotation window the JWT path already +// accepts. // - Concurrent misses for the same key each call WorkOS; there is no // in-flight dedupe. The duplicate window is one round trip per key per // TTL per isolate — exactly the cost EVERY request paid before this @@ -312,18 +316,34 @@ const API_KEY_VALIDATION_CACHE_TTL_MS = 60_000; // map unbounded (mirrors BALANCE_CACHE_MAX_ENTRIES in execution-gate.ts). const API_KEY_VALIDATION_CACHE_MAX_ENTRIES = 10_000; +type ApiKeyValidationCacheEntry = { + readonly owner: ApiKeyOwner; + readonly expiresAtMs: number; +}; + +// The map lives at MODULE scope — one per isolate — not inside +// `makeCachedApiKeyValidate`, because the `ApiKeyService.WorkOS` layer is NOT +// built once per isolate on every plane: the account middleware rebuilds it +// per request. A map owned by each build would give the revoke paths a fresh +// empty cache to invalidate while the long-lived identity plane kept serving +// the revoked key from its own. Every build shares this map instead (entries +// are pure validation RESULTS, so which build's WorkOS client wrote them does +// not matter), which is also what makes the entries survive between requests +// on the per-request plane at all. +const isolateApiKeyValidationCache = new Map(); + export type ApiKeyValidate = ( value: string, ) => Effect.Effect; /** - * Wrap a `validate` function with the per-isolate success cache described - * above. Exported for tests only — production use is the single instance the - * `ApiKeyService.WorkOS` layer builds, which lives in `boot` and is therefore - * constructed once per isolate. + * Wrap a `validate` function with the success cache described above. Exported + * for tests only — production use is the `ApiKeyService.WorkOS` layer below, + * every build of which shares the module-scope map. * - * The `ttlMs` / `maxEntries` / `now` knobs exist so tests can exercise expiry - * and the size bound without waiting on wall time; production passes nothing. + * The `ttlMs` / `maxEntries` / `now` / `cache` knobs exist so tests can + * exercise expiry, the size bound, and map sharing without wall time or + * module state; production passes only `cache`. */ export const makeCachedApiKeyValidate = ( validate: ApiKeyValidate, @@ -331,9 +351,19 @@ export const makeCachedApiKeyValidate = ( readonly ttlMs?: number; readonly maxEntries?: number; readonly now?: () => number; + /** The map to cache into. Production passes the module-scope + * `isolateApiKeyValidationCache`; tests default to a private map. */ + readonly cache?: Map; }, ): { readonly validate: ApiKeyValidate; + /** + * Drop every cached entry that resolved to this key id. The cache is keyed + * by the digest of the presented VALUE and a revoke only knows the key ID, + * so this walks the map — bounded by `maxEntries`, and revocation is a rare + * human action, not a hot path. + */ + readonly invalidateKeyId: (keyId: string) => void; /** Test seam: the digests currently cached, so tests can assert the raw * credential never appears as a map key. */ readonly cacheKeys: () => ReadonlyArray; @@ -341,7 +371,7 @@ export const makeCachedApiKeyValidate = ( const ttlMs = options?.ttlMs ?? API_KEY_VALIDATION_CACHE_TTL_MS; const maxEntries = options?.maxEntries ?? API_KEY_VALIDATION_CACHE_MAX_ENTRIES; const now = options?.now ?? Date.now; - const cache = new Map(); + const cache = options?.cache ?? new Map(); const writeCache = (digest: string, owner: ApiKeyOwner, nowMs: number): void => { if (cache.size >= maxEntries) { @@ -356,6 +386,11 @@ export const makeCachedApiKeyValidate = ( return { cacheKeys: () => [...cache.keys()], + invalidateKeyId: (keyId) => { + for (const [digest, entry] of cache) { + if (entry.owner.keyId === keyId) cache.delete(digest); + } + }, validate: (value) => Effect.gen(function* () { const digest = yield* sha256Hex(value); @@ -414,12 +449,17 @@ export class ApiKeyService extends Context.Service< static WorkOS = Layer.effect(this)( Effect.gen(function* () { const workos = yield* WorkOSClient; - // Built once with the boot layer, so the success cache is per-isolate. - const cachedValidate = makeCachedApiKeyValidate((value) => - workos.validateApiKey(value).pipe( - Effect.map(ownerFromResponse), - Effect.mapError((cause) => new ApiKeyValidationError({ cause })), - ), + // The cache MAP is the module-scope per-isolate one (see its comment); + // this build only contributes the validate function that fills it, so + // however many times a plane rebuilds this layer, hits, misses, and + // invalidations all land in the same map. + const cachedValidate = makeCachedApiKeyValidate( + (value) => + workos.validateApiKey(value).pipe( + Effect.map(ownerFromResponse), + Effect.mapError((cause) => new ApiKeyValidationError({ cause })), + ), + { cache: isolateApiKeyValidationCache }, ); return { validate: cachedValidate.validate, @@ -439,9 +479,10 @@ export class ApiKeyService extends Context.Service< }), ), revokeUserKey: ({ keyId }) => - workos - .deleteApiKey(keyId) - .pipe(Effect.mapError((cause) => new ApiKeyManagementError({ cause }))), + workos.deleteApiKey(keyId).pipe( + Effect.mapError((cause) => new ApiKeyManagementError({ cause })), + Effect.tap(() => Effect.sync(() => cachedValidate.invalidateKeyId(keyId))), + ), listOrgKeys: ({ organizationId }) => workos.listOrgApiKeys(organizationId).pipe( Effect.map((response) => orgListFromResponse(response, organizationId)), @@ -475,6 +516,7 @@ export class ApiKeyService extends Context.Service< yield* workos .deleteApiKey(keyId) .pipe(Effect.mapError((cause) => new ApiKeyManagementError({ cause }))); + cachedValidate.invalidateKeyId(keyId); }), }; }), diff --git a/apps/cloud/src/mcp/agent-handler.ts b/apps/cloud/src/mcp/agent-handler.ts index 59a6e86f79..a789ecaf85 100644 --- a/apps/cloud/src/mcp/agent-handler.ts +++ b/apps/cloud/src/mcp/agent-handler.ts @@ -133,25 +133,26 @@ const authenticate = (request: Request) => // ONE auth layer per isolate. `authenticate` used to end in // `Effect.provide(cloudMcpAuth)`, which rebuilt the entire auth stack — the -// WorkOS client, `ApiKeyService` and with it the api-key validation success -// cache — on every MCP request, so that cache never survived a request on this -// plane (the /api/* plane resolves the same service from the app's `boot` -// layer, built once per isolate). A lazily created ManagedRuntime builds -// `cloudMcpAuth` on the first request and memoizes it; `runTraced` runs every -// program on it. Nothing inside the layer is request-scoped: the WorkOS client -// holds no sockets (just config), the JWKS cache is already module-scope, and -// `McpOrganizationAuthLive` builds its postgres socket FRESH inside every -// `authorize` call precisely so the service itself can outlive a request -// (Cloudflare Workers' I/O isolation — see makeMcpOrganizationAuthServices). +// WorkOS client and `ApiKeyService` — on every MCP request. A lazily created +// ManagedRuntime builds `cloudMcpAuth` on the first request and memoizes it; +// `runTraced` runs every program on it. Nothing inside the layer is +// request-scoped: the WorkOS client holds no sockets (just config), the JWKS +// cache is already module-scope, and `McpOrganizationAuthLive` builds its +// postgres socket FRESH inside every `authorize` call precisely so the +// service itself can outlive a request (Cloudflare Workers' I/O isolation — +// see makeMcpOrganizationAuthServices). // // The memo is keyed on the WorkOS binding values the layer captures at build, // NOT held forever: Cloudflare reuses warm isolates across binding-only // deployments, so a bare `??=` would keep authenticating with a rotated-out // WORKOS_API_KEY until isolate eviction. `runTraced` fingerprints the // request's own `env` (a string compare per request) and the cache swaps in a -// fresh runtime — fresh ApiKeyService validation cache included, as a rotated -// key must not serve cached validations — when the bindings changed. See -// auth-runtime.ts for the drop-vs-dispose reasoning. +// fresh runtime when the bindings changed. The api-key validation success +// cache is NOT swapped with it — it is module-scope in api-keys.ts (shared +// with the /api/* plane so revocation can invalidate it everywhere), so after +// a WORKOS_API_KEY rotation its remaining entries age out within the 60s TTL +// rather than dropping instantly. See auth-runtime.ts for the drop-vs-dispose +// reasoning. const mcpAuthRuntimeFor: ( fingerprint: string, ) => ManagedRuntime.ManagedRuntime = makeBindingKeyedRuntime(() => From c385591363043a00d6b8754ac999c4983d8a6800 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sun, 30 Aug 2026 02:49:02 -0700 Subject: [PATCH 2/2] Reset the shared validation cache between unit tests --- apps/cloud/src/auth/api-keys.node.test.ts | 9 +++++-- apps/cloud/src/auth/api-keys.ts | 6 +++++ apps/cloud/src/mcp/auth-runtime.node.test.ts | 26 ++++++++++++++------ apps/cloud/src/mcp/auth-runtime.ts | 8 +++--- 4 files changed, 37 insertions(+), 12 deletions(-) diff --git a/apps/cloud/src/auth/api-keys.node.test.ts b/apps/cloud/src/auth/api-keys.node.test.ts index c55a2f90e6..30fb45ee2d 100644 --- a/apps/cloud/src/auth/api-keys.node.test.ts +++ b/apps/cloud/src/auth/api-keys.node.test.ts @@ -1,9 +1,14 @@ -import { describe, expect, it } from "@effect/vitest"; +import { beforeEach, describe, expect, it } from "@effect/vitest"; import { Effect, Layer } from "effect"; -import { ApiKeyService } from "./api-keys"; +import { ApiKeyService, resetApiKeyValidationCacheForTest } from "./api-keys"; import { WorkOSClient, type WorkOSClientService } from "./workos"; +// Every case validates the same "test_key" value through the REAL layer, whose +// success cache is module-scope — without a reset, one case's decoded owner +// would be served to the next in place of its own stubbed response. +beforeEach(() => resetApiKeyValidationCacheForTest()); + const stubWorkOS = (overrides: Partial) => Layer.succeed( WorkOSClient, diff --git a/apps/cloud/src/auth/api-keys.ts b/apps/cloud/src/auth/api-keys.ts index b8b3e28971..3b66b0ccd9 100644 --- a/apps/cloud/src/auth/api-keys.ts +++ b/apps/cloud/src/auth/api-keys.ts @@ -332,6 +332,12 @@ type ApiKeyValidationCacheEntry = { // on the per-request plane at all. const isolateApiKeyValidationCache = new Map(); +/** Test-only: module state outlives a single test case (mirrors + * `resetBuildSlotsForTest` in mcp/session-build-semaphore.ts). */ +export const resetApiKeyValidationCacheForTest = (): void => { + isolateApiKeyValidationCache.clear(); +}; + export type ApiKeyValidate = ( value: string, ) => Effect.Effect; diff --git a/apps/cloud/src/mcp/auth-runtime.node.test.ts b/apps/cloud/src/mcp/auth-runtime.node.test.ts index 6fe8a13bda..76d2b4e632 100644 --- a/apps/cloud/src/mcp/auth-runtime.node.test.ts +++ b/apps/cloud/src/mcp/auth-runtime.node.test.ts @@ -1,7 +1,7 @@ -import { describe, expect, it } from "@effect/vitest"; +import { beforeEach, describe, expect, it } from "@effect/vitest"; import { Effect, Layer, ManagedRuntime } from "effect"; -import { ApiKeyService } from "../auth/api-keys"; +import { ApiKeyService, resetApiKeyValidationCacheForTest } from "../auth/api-keys"; import { WorkOSClient, type WorkOSClientService } from "../auth/workos"; import { makeBindingKeyedRuntime, @@ -64,6 +64,11 @@ const bindings = (overrides?: Partial): McpAuthBindings => ({ }); describe("makeBindingKeyedRuntime", () => { + // The validation cache is module-scope in api-keys.ts, so it outlives both + // a test case and (deliberately) a runtime swap; reset it per case so each + // test observes only its own upstream calls. + beforeEach(() => resetApiKeyValidationCacheForTest()); + it.effect("reuses the runtime — and its validation cache — while bindings are unchanged", () => Effect.gen(function* () { const { counters, runtimeFor, request } = makeCountingAuthRuntime(); @@ -80,7 +85,7 @@ describe("makeBindingKeyedRuntime", () => { }), ); - it.effect("rebuilds a fresh runtime — resetting the validation cache — on a binding change", () => + it.effect("rebuilds a fresh runtime on a binding change; cached validations survive", () => Effect.gen(function* () { const { counters, runtimeFor, request } = makeCountingAuthRuntime(); @@ -88,11 +93,15 @@ describe("makeBindingKeyedRuntime", () => { yield* request("fingerprint_a"); expect(counters.upstreamCalls).toBe(1); - // Rotated bindings: a fresh runtime (fresh ApiKeyService, empty cache), - // so the same key must be re-validated upstream. + // Rotated bindings: a fresh runtime and auth stack — but NOT a fresh + // validation cache. The cache map is module-scope (shared with the + // /api/* plane so revocation can invalidate it everywhere; see + // agent-handler.ts), so an already-validated key keeps hitting it and + // its entries age out within the TTL rather than dropping with the + // runtime. yield* request("fingerprint_b"); expect(counters.builds).toBe(2); - expect(counters.upstreamCalls).toBe(2); + expect(counters.upstreamCalls).toBe(1); expect(runtimeFor("fingerprint_b")).not.toBe(before); }), ); @@ -105,8 +114,11 @@ describe("makeBindingKeyedRuntime", () => { yield* request("fingerprint_b"); yield* request("fingerprint_a"); + // Three runtime builds (the memo holds one entry), one upstream call: + // the module-scope validation cache serves the repeat validations + // across every swap. expect(counters.builds).toBe(3); - expect(counters.upstreamCalls).toBe(3); + expect(counters.upstreamCalls).toBe(1); }), ); }); diff --git a/apps/cloud/src/mcp/auth-runtime.ts b/apps/cloud/src/mcp/auth-runtime.ts index 3328c554c3..755d033563 100644 --- a/apps/cloud/src/mcp/auth-runtime.ts +++ b/apps/cloud/src/mcp/auth-runtime.ts @@ -9,9 +9,11 @@ // outage once the old key is revoked. So the memo is keyed on a fingerprint // of exactly the binding values the layer captures at build: same // fingerprint -> reuse (one string compare per request), changed fingerprint -// -> build fresh. The rebuild also resets the ApiKeyService validation cache -// living inside the layer, which is required — a rotated key must not serve -// cached validations. +// -> build fresh. The ApiKeyService validation cache is NOT tied to the +// runtime — its map is module-scope in api-keys.ts (shared with the /api/* +// plane so revocation can invalidate it everywhere), so after a rotation its +// remaining entries age out within the 60s TTL rather than dropping with the +// rebuild. // // The superseded runtime is DROPPED, not disposed. `ManagedRuntime.dispose` // tears down the runtime's scope, and requests already in flight may still be