fix(cache): scope Actor definition cache to its owner#985
Merged
Conversation
The process-wide actorDefinitionCache was keyed by Actor name only, so on a multi-tenant HTTP server one token could read another tenant's private Actor definition (input schema, README, run options) straight from cache with no authorization check. Gate cache hits: public Actors are served to everyone, private Actors only to the owner (effective userId == owner userId, via the already-cached getUserInfoCached); everyone else re-fetches under their own token, which the platform authorizes. Drop the separate mcpServerCache and derive the MCP URL from the now-gated definition cache instead — getActorMCPServerURL does no I/O, so the extra cache guarded nothing and re-introduced the same cross-tenant leak.
Council review nits on the cache-isolation gate: - Document the two load-bearing invariants (info.userId is the platform-set owner; caller identity must match the platform's auth identity, null stays the sole sentinel) so a future refactor can't silently invert the gate. - Add TTLLRUCache.clear() and reset the shared cache in beforeEach so tests don't pass by virtue of unique names. - Cover getActorMcpUrlCached (was untested): owner-visible -> URL, non-owner private -> re-fetch + false (no leak of the cached URL), missing -> false no throw, non-MCP -> false. Strengthen the non-owner definition test to assert the re-fetched object is returned, never the cached one, plus fail-closed-on-identity-error and no-poisoning-after-shared-access cases. - Delete the now-dead MCP_SERVER_CACHE_* constants left in const.ts.
Council review on test minimalism — cut the cases that duplicate another or only assert the mock, keeping the complete authorization truth table: - drop the 'fail-closed on identity error' case (byte-identical to the anonymous case; the mock can't throw), - drop the 'no corruption after shared re-fetch' case (no product path writes info.userId, so it only re-asserts the mock and duplicates the non-owner case), - drop the 'non-MCP -> false' case (false-branch already covered by the missing -> false case and mcp.actors.test.ts). Also remove the test-only TTLLRUCache.clear() (every case uses a unique key) and tighten the gate comments to the two load-bearing invariants.
github-actions
Bot
added
t-ai
master's CachedUserInfo gained a required isOrganization field; update the mocked return values to match after merging master.
RobertCrupa
approved these changes
Jun 15, 2026
jirispilka
approved these changes
Jun 16, 2026
jirispilka
left a comment
jirispilka
left a comment
Collaborator
There was a problem hiding this comment.
Nice PR.
As Claude puts it: This is a textbook "delete the layer" move, not a "polish the layer" move
Co-authored-by: Jiří Spilka <jiri.spilka@apify.com>
…olation Resolve src/const.ts conflict: master (#1000) removed the unused cache constants; this branch defines them locally in state.ts/userid_cache.ts, so take master's deletion. Strip trailing whitespace in actor.ts comment (oxfmt).
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
4 participants
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.
What
Gate
actorDefinitionCachehits by ownership: public Actors are served to any caller, private Actors only to their owner; everyone else falls through to a fresh fetch under their own token. Drop the separatemcpServerCacheand derive the MCP URL from the now-gated definition cache.Why
The cache is process-wide and was keyed by Actor name only. On a multi-tenant HTTP deployment, token B requesting an Actor that token A already cached got a cache hit that bypassed B's authorization check — leaking a private Actor's definition (input schema, README, run options) across tenants.
getActorMCPServerURLdoes no I/O, somcpServerCacheguarded nothing expensive and re-introduced the same leak for the MCP URL; removing it closes that vector and deletes a global. Owner check reuses the already-cachedgetUserInfoCached, so no extra API call on the hot path. Found in an internal security review of the public package.The gate hinges on two invariants (documented in
callerMaySeeCachedActor):info.userIdis the platform-set owner, and the caller's identity is resolved with the sameuser('me')identity the platform authorizes with. Verified against the live Apify API: a private Actor returnsisPublic: falseanduserIdequal to its owner'suser('me').id; a public Actor returnsisPublic: true. So a non-owner token resolves to a differentuser('me').id, the predicate denies, and the cache can never grant more than a bare re-fetch would.Testing
tests/unit/utils.actor.cache.test.ts(7 cases) pinning the authorization truth table: public→served to all (asserts nouser('me')lookup), private→owner served, private→non-owner returns the re-fetched object (never the cached one), private→anonymous denied, and thegetActorMcpUrlCachedgate (owner→URL, non-owner private→false/re-fetch, missing→falseno throw).pnpm type-check,pnpm lint,pnpm format:check,pnpm test:unit(870 pass / 2 skipped) all green.