Skip to content

Commit 01f3958

Browse files
d-csclaude
andcommitted
fix(webapp): populate run.id on mollify synthetic result (cross-tenant leak fix)
Devin review flagged a security + correctness bug: the mollify path returned \`MollifySyntheticResult\` with run shape \`{ friendlyId, spanId }\` and the call site cast it to \`TriggerTaskServiceResult\` (which expects \`run: TaskRun\` with an \`id: string\`). Downstream, the trigger route calls \`saveRequestIdempotency(requestKey, "trigger", result.run.id)\` — so the cache stored \`undefined\` as the entity id. On SDK retry the request-idempotency flow then ran \`prisma.taskRun.findFirst({ where: { id: undefined } })\`. Prisma strips \`undefined\` from where clauses, so the query degenerates to an unfiltered \`findFirst\` and returns an arbitrary TaskRun row — potentially from another env / user. Fix: - Add \`id: string\` to \`MollifySyntheticResult.run\`. - Compute it via \`RunId.fromFriendlyId(...)\` on both branches: the happy-accept path (id from \`args.runFriendlyId\`) and the \`duplicate_idempotency\` race-loser path (id from \`result.existingRunId\` so the response carries the WINNER's id). - Add regression tests pinning both branches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent baa6f17 commit 01f3958

2 files changed

Lines changed: 73 additions & 15 deletions

File tree

apps/webapp/app/v3/mollifier/mollifierMollify.server.ts

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { RunId } from "@trigger.dev/core/v3/isomorphic";
12
import type { MollifierBuffer } from "@trigger.dev/redis-worker";
23
import { serialiseMollifierSnapshot, type MollifierSnapshot } from "./mollifierSnapshot.server";
34
import type { TripDecision } from "./mollifierGate.server";
@@ -9,12 +10,20 @@ export type MollifyNotice = {
910
};
1011

1112
export type MollifySyntheticResult = {
12-
// `spanId` is the root-span id allocated at gate-accept time and stored
13-
// in the snapshot. Callers like the dashboard's Test action use it to
14-
// build a `v3RunSpanPath` URL that auto-opens the right details panel
15-
// — without it, the buffered run lands on the run-detail page with no
16-
// span selected (parity gap with PG-resident runs).
17-
run: { friendlyId: string; spanId: string };
13+
// `id` is the canonical TaskRun primary key derived from `friendlyId`
14+
// via `RunId.fromFriendlyId`. Downstream consumers in the trigger
15+
// route — notably `saveRequestIdempotency` (Q3) — index the request-
16+
// idempotency cache by this id; without it the cache stores
17+
// `undefined` and Prisma's `findFirst({ where: { id: undefined } })`
18+
// on retry strips the predicate and returns an arbitrary TaskRun
19+
// (potential cross-tenant leak). Always populated.
20+
//
21+
// `spanId` is the root-span id allocated at gate-accept time and
22+
// stored in the snapshot. Callers like the dashboard's Test action
23+
// use it to build a `v3RunSpanPath` URL that auto-opens the right
24+
// details panel — without it, the buffered run lands on the
25+
// run-detail page with no span selected (parity gap with PG runs).
26+
run: { id: string; friendlyId: string; spanId: string };
1827
error: undefined;
1928
// The race-loser path (Q5): if accept's SETNX hit an existing
2029
// buffered run with the same (env, task, idempotencyKey), the
@@ -60,7 +69,11 @@ export async function mollifyTrigger(args: {
6069
// causes `v3RunSpanPath` to omit the `?span=` param, which matches
6170
// current behaviour for cached PG responses.
6271
return {
63-
run: { friendlyId: result.existingRunId, spanId: "" },
72+
run: {
73+
id: RunId.fromFriendlyId(result.existingRunId),
74+
friendlyId: result.existingRunId,
75+
spanId: "",
76+
},
6477
error: undefined,
6578
isCached: true,
6679
};
@@ -73,7 +86,11 @@ export async function mollifyTrigger(args: {
7386
const rawSpanId = args.engineTriggerInput.spanId;
7487
const spanId = typeof rawSpanId === "string" ? rawSpanId : "";
7588
return {
76-
run: { friendlyId: args.runFriendlyId, spanId },
89+
run: {
90+
id: RunId.fromFriendlyId(args.runFriendlyId),
91+
friendlyId: args.runFriendlyId,
92+
spanId,
93+
},
7794
error: undefined,
7895
isCached: false,
7996
notice: NOTICE,

apps/webapp/test/mollifierMollify.test.ts

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ vi.mock("~/db.server", () => ({
66
}));
77

88
import { mollifyTrigger } from "~/v3/mollifier/mollifierMollify.server";
9+
import { RunId } from "@trigger.dev/core/v3/isomorphic";
910
import type { MollifierBuffer } from "@trigger.dev/redis-worker";
1011

1112
function fakeBuffer(
@@ -22,7 +23,7 @@ describe("mollifyTrigger", () => {
2223
it("writes the snapshot to buffer and returns synthesised result", async () => {
2324
const { buffer, accept } = fakeBuffer();
2425
const result = await mollifyTrigger({
25-
runFriendlyId: "run_friendly_1",
26+
runFriendlyId: "run_abc123def456",
2627
environmentId: "env_a",
2728
organizationId: "org_1",
2829
engineTriggerInput: { taskIdentifier: "my-task", payload: '{"x":1}' },
@@ -37,14 +38,14 @@ describe("mollifyTrigger", () => {
3738

3839
expect(accept).toHaveBeenCalledOnce();
3940
expect(accept).toHaveBeenCalledWith({
40-
runId: "run_friendly_1",
41+
runId: "run_abc123def456",
4142
envId: "env_a",
4243
orgId: "org_1",
4344
payload: expect.any(String),
4445
idempotencyKey: undefined,
4546
taskIdentifier: undefined,
4647
});
47-
expect(result.run.friendlyId).toBe("run_friendly_1");
48+
expect(result.run.friendlyId).toBe("run_abc123def456");
4849
expect(result.error).toBeUndefined();
4950
expect(result.isCached).toBe(false);
5051
expect(result.notice).toEqual({
@@ -57,10 +58,10 @@ describe("mollifyTrigger", () => {
5758
it("echoes the winner's runId with isCached=true on duplicate_idempotency", async () => {
5859
const { buffer } = fakeBuffer({
5960
kind: "duplicate_idempotency",
60-
existingRunId: "run_winner",
61+
existingRunId: "run_winner12345",
6162
});
6263
const result = await mollifyTrigger({
63-
runFriendlyId: "run_loser",
64+
runFriendlyId: "run_loser56789a",
6465
environmentId: "env_a",
6566
organizationId: "org_1",
6667
engineTriggerInput: { taskIdentifier: "t", payload: "{}" },
@@ -69,16 +70,56 @@ describe("mollifyTrigger", () => {
6970
idempotencyKey: "key",
7071
taskIdentifier: "t",
7172
});
72-
expect(result.run.friendlyId).toBe("run_winner");
73+
expect(result.run.friendlyId).toBe("run_winner12345");
7374
expect(result.isCached).toBe(true);
7475
expect(result.notice).toBeUndefined();
7576
});
7677

78+
// Regression: the synthetic result MUST carry a populated `run.id`
79+
// derived from the friendlyId. Without it, the route handler's
80+
// `saveRequestIdempotency(…, result.run.id)` stores `undefined` as
81+
// the cached entity id, and on SDK retry Prisma's
82+
// `findFirst({ where: { id: undefined } })` silently drops the
83+
// predicate and returns an arbitrary TaskRun — a cross-tenant leak
84+
// path. (See Devin review on PR #3753.)
85+
it("populates run.id from friendlyId on the happy-accept path", async () => {
86+
const { buffer } = fakeBuffer();
87+
const result = await mollifyTrigger({
88+
runFriendlyId: "run_pri456789ab",
89+
environmentId: "env_a",
90+
organizationId: "org_1",
91+
engineTriggerInput: { taskIdentifier: "t", payload: "{}" },
92+
decision: { divert: true, reason: "per_env_rate", count: 1, threshold: 1 },
93+
buffer,
94+
});
95+
expect(result.run.id).toBe(RunId.fromFriendlyId("run_pri456789ab"));
96+
expect(result.run.id).toMatch(/^[a-z0-9]+$/); // non-undefined, non-empty
97+
});
98+
99+
it("populates run.id from the WINNER's friendlyId on duplicate_idempotency", async () => {
100+
const { buffer } = fakeBuffer({
101+
kind: "duplicate_idempotency",
102+
existingRunId: "run_winnerdup12",
103+
});
104+
const result = await mollifyTrigger({
105+
runFriendlyId: "run_loser56789a",
106+
environmentId: "env_a",
107+
organizationId: "org_1",
108+
engineTriggerInput: { taskIdentifier: "t", payload: "{}" },
109+
decision: { divert: true, reason: "per_env_rate", count: 1, threshold: 1 },
110+
buffer,
111+
idempotencyKey: "key",
112+
taskIdentifier: "t",
113+
});
114+
expect(result.run.id).toBe(RunId.fromFriendlyId("run_winnerdup12"));
115+
expect(result.run.id).not.toBe(RunId.fromFriendlyId("run_loser56789a"));
116+
});
117+
77118
it("snapshot is round-trippable: payload field is parseable JSON of engineTriggerInput", async () => {
78119
const { buffer, accept } = fakeBuffer();
79120
const engineInput = { taskIdentifier: "t", payload: "{}", tags: ["a", "b"] };
80121
await mollifyTrigger({
81-
runFriendlyId: "run_x",
122+
runFriendlyId: "run_xabcde12345",
82123
environmentId: "env_a",
83124
organizationId: "org_1",
84125
engineTriggerInput: engineInput,

0 commit comments

Comments
 (0)