Skip to content

[Fix] Modal snapshot resume auth and MCP job-token principal checks - #80

Merged
mrubens merged 3 commits into
developfrom
fix/modal-resume-and-mcp-token-principal
Jul 10, 2026
Merged

[Fix] Modal snapshot resume auth and MCP job-token principal checks#80
mrubens merged 3 commits into
developfrom
fix/modal-resume-and-mcp-token-principal

Conversation

@mrubens

@mrubens mrubens commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Two production bugs found while testing the new data model on the nightly deployment (roomote.roomote.ai).

Modal snapshot resume fails with "Profile is missing token_id or token_secret"

resumeFromSnapshot loaded snapshot images through the Modal SDK's static Image.fromId, which resolves auth from the default profile/process env — not from the client instance we construct with explicit credentials. When Modal credentials come from the encrypted deployment env vars (the setup-flow storage) instead of process env, fresh spawns worked but every snapshot resume failed after 3 retries. Fixed by routing the load through this.sdk.images.fromId so the explicit tokenId/tokenSecret travel with it.

Slack MCP calls 403 after a steer or follow-up ("MCP token principal does not match cloud job")

Job tokens snapshot the run's acting user at mint time, but task_runs.actingUserId is deliberately mutable: web steer and follow-up delivery both reassign it to the latest human. The MCP handlers (and findCloudJobByJobTokenClaims, which gates the worker's entire sandbox→API RPC surface) required strict equality between the two — so the moment anyone steered or replied in-thread (including a human replying in an automation run's thread), every subsequent MCP/RPC call from that worker 403'd. Observed as a worker unable to post its Slack closeout.

The run-scoped cloudJobId binding is the actual authorization (only that sandbox holds the token; Slack posts go out via the deployment bot token, so there is no impersonation vector). This PR removes the exact-user equality everywhere the token is already run-bound — the 5 sites in mcp/slack.ts, the shared proxy-utils validator (renamed to assertCloudJobTokenJobExists to match what it now does), the inline copies in the vercel/grafana/snowflake/asana integrations (all resolve credentials from deployment-scoped connections; token userId played no role), and the SDK-side findCloudJobByJobTokenClaims. Capability gates by principal kind (human vs deployment) and all live-actingUserId attribution reads are unchanged.

Tests cover both switch scenarios (user→user and null-principal→human) across the integration auth suites and the Slack channel-post path.

Both commits were validated with the full pnpm check gate (lint, types, all tests, knip).

roomote added 2 commits July 10, 2026 08:10
The static Image.fromId resolves auth from the default profile/env vars,
which are absent when Modal credentials come from the encrypted
deployment env vars (the setup-flow storage) instead of process env —
fresh spawns worked but every snapshot resume failed with 'Profile is
missing token_id or token_secret'. Route the load through
this.sdk.images.fromId so the explicit tokenId/tokenSecret travel with
it.
Job tokens are minted once with the run's acting user at MINT time, but
task_runs.actingUserId is deliberately mutable mid-run: web steer and
follow-up delivery switch it to the latest human. The MCP/RPC handlers
required strict equality (run.actingUserId ?? null) === token.userId, so
the moment anyone steered or followed up — including a human replying in
an automation run's thread — every subsequent MCP call from that worker
403'd ("MCP token principal does not match cloud job"). Observed on
nightly: the worker could not post its Slack closeout.

The token's cloudJobId binding IS the authorization: only that run's
sandbox holds the run-scoped token, handlers verify the token targets
its own run, and Slack/integration calls go out via deployment-scoped
credentials (bot token / user_id IS NULL connections), so the exact-user
equality added no security and was wrong against a mutable column.

Removed the equality (run lookup + 404 stays) at: the five Slack MCP
endpoints, the shared proxy validator (verifyCloudJobTokenMatchesJob ->
verifyCloudJobTokenJobExists, assertCloudJobTokenMatchesCloudJobUser ->
assertCloudJobTokenJobExists, callers roomote.ts/manageSourceControl),
the four integration MCP handlers (Vercel/Grafana/Snowflake/Asana), and
the SDK's findCloudJobByJobTokenClaims (gating the artifacts route and
the tRPC jobScoped middleware — the same divergence broke those too).

Deliberately left: capability gates by principal KIND (human vs
deployment service principal, e.g. resolveActingUserId's 403), live
actor resolution that READS actingUserId for attribution/credentials,
the worker sandbox-server's token-cloudJobId-vs-sandbox check, and the
tRPC middlewares' token.cloudJobId === target id checks — those are the
run binding itself.

Tests now assert the divergence works: a token minted for user A keeps
working after actingUserId switches to user B, and a deployment-
principal (null) token keeps working after a human becomes the acting
user — including the Slack channel-post closeout path. auth.md updated:
token userId is mint-time attribution, never an authorization key.
@roomote-roomote

roomote-roomote Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

No new code issues found. See task

@roomote-roomote

Copy link
Copy Markdown
Contributor

Review (Opus)

Reviewed both commits against develop (head b527f9a, base 4a519a2). Verdict: the two fixes are correct and the security reasoning for the principal-check removal holds for every site this PR touches. One pre-existing platform-level concern is worth noting but is out of scope for this PR, plus a couple of tiny cleanups.

Commit 1 — Modal snapshot resume (1d7f004)

Correct root-cause and fix. The static Image.fromId resolves auth from the default profile/process env, which is empty when Modal creds come from the encrypted deployment env vars — so fresh spawns worked but resumes failed. Routing through this.sdk.images.fromId carries the explicit tokenId/tokenSecret. The test mock was moved from the static Image.fromId onto the client's images.fromId to match, which is the right shape. Clean.

Commit 2 — Drop principal equality on run-bound job tokens (b527f9a)

The diagnosis is right: task_runs.actingUserId is genuinely mutable mid-run (web steer, follow-up delivery, and web takeover all reassign it), so comparing it against the token's mint-time userId produces legitimate 403s after any steer/reply. Removing the equality check is sound at each changed site because the run-scoped cloudJobId binding is the real authorization and none of these paths select user-scoped credentials off the token:

  • Slack MCP (5 sites): all egress via the deployment bot token — no impersonation vector. actingUserId is still read where it's genuine attribution (reaction_add membership user, thread_reply subject), just not for authz.
  • asana / grafana / snowflake / vercel: each resolves credentials from a deployment-scoped connection (mcpConnections.userId IS NULL); the token's userId never fed credential selection.
  • proxy-utils / SDK findCloudJobByJobTokenClaims: now validate run existence only. Rename to assertCloudJobTokenJobExists matches the new behavior; no stale references to the old names remain.
  • roomote.ts capability gating by principal kind is unchanged (deployment vs human resolution via resolveActingUserIdOrNull).

New tests cover both switch scenarios (user→user and null-principal→human) plus the modal client routing. Typecheck passes on all three changed packages; lint is clean on apps/api.

Non-blocking notes

  1. Pre-existing (not introduced here): a job token can already write an arbitrary actingUserId onto its own run via the job-scoped cloudJobs.update (actingUserId: z.string().optional()), and resolveActorScopedUserContext prefers that mutable field when resolving the effective user for user-scoped credential routes (e.g. userApiKeys.getDecryptedKey, user-scoped mcpConnections). That actor-confusion surface predates this PR (both existed at base 4a519a2) and lives in different code paths than the ones changed here, so it's not a regression. Worth a separate hardening pass: don't let ordinary job-token callers set actingUserId to an arbitrary user, and add a regression test that a token can't self-reassign and then read another user's API key.
  2. Tiny cleanup: thread_reply and channel_post still columns: { actingUserId: true } but no longer read it after the check removal — dead selects. (reaction_add still legitimately uses it.)
  3. Stale comments: user-api-keys.ts still says "userId is embedded in the token" and resolve-actor-scoped-user.ts says tokens stay authorized as the "original job owner" — both slightly out of date given this change; a doc-only follow-up.

Note: slack-channel-post.test.ts > posts inside existing threads and includes image blocks fails locally, but it also fails on the base commit (unmocked HTTP in the image-block path), so it's a pre-existing/environmental flake, not caused by this PR.

LGTM to merge; the noted hardening is best handled as a follow-up.

channel_post no longer reads actingUserId (thread_reply still does, for
the attribution subject, so its select stays). The user-api-keys and
resolve-actor-scoped-user comments now describe run-scoped token
authorization with mint-time attribution instead of the old
owner-equality rule.
@mrubens
mrubens merged commit fefff6b into develop Jul 10, 2026
1 check passed
@mrubens
mrubens deleted the fix/modal-resume-and-mcp-token-principal branch July 12, 2026 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants