Skip to content

Commit ed37e19

Browse files
claude[bot]Trigger.dev RepoOps
authored andcommitted
fix(sdk): build the offloaded trigger payload path from a generated id
**Before:** triggering a task whose id contained a slash could fail once the payload was large enough to be stored separately. The upload request came back as a 400 with `Invalid packet storage path`, so the trigger never completed. Whether an id hit it depended on where its slashes fell: it affected ids that started or ended with a slash, that contained two slashes in a row, or that contained a path component of exactly `.` or `..`. **After:** the task id takes no part in the storage path any more, so no task id can produce one that fails. A large trigger payload is uploaded to object storage rather than sent inline, and the path used to be built from the task id. It was percent-encoded so that it would stay inside a single path segment, but the escape was decoded again when the upload request was handled, so the slash came back as a real separator and the id spread across several segments of the path. Some of the results were not valid storage paths, and those were rejected. The payload is now named with a generated id instead, so no task id can produce an unusable storage path. Payloads that are already stored are unaffected: the storage location is recorded when a payload is uploaded and read back from there, rather than rebuilt from the task id. Mono-RevId: 267fcbb3eb0f69db41e67c24b82a25f454d3052a
1 parent f78261a commit ed37e19

4 files changed

Lines changed: 162 additions & 15 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@trigger.dev/sdk": patch
3+
---
4+
5+
Fixes storage of large trigger payloads for task ids containing a slash, which could fail the trigger with an "Invalid packet storage path" error. It affected ids that started or ended with a slash, contained two slashes in a row, or contained a `.` or `..` path component. The storage path is now built from a generated id rather than from the task id, so no task id can produce an unusable one, and payloads that are already stored are still read from where they were written.

apps/webapp/test/objectStore.test.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { postgresAndMinioTest } from "@internal/testcontainers";
2-
import { type IOPacket } from "@trigger.dev/core/v3";
2+
import { generateFriendlyId, type IOPacket } from "@trigger.dev/core/v3";
33
import { type PrismaClient } from "@trigger.dev/database";
44
import { afterAll, describe, expect, it, vi } from "vitest";
55
import { env } from "~/env.server";
@@ -163,6 +163,22 @@ describe("Object Storage", () => {
163163
});
164164
});
165165

166+
describe("offloaded trigger payload paths", () => {
167+
/**
168+
* The SDK names an offloaded trigger payload with the same generated id, so this
169+
* pins the half of that contract the server owns: the guard must keep accepting
170+
* every character the id generator can emit.
171+
*/
172+
it("accepts the path shape the SDK builds for an offloaded payload", () => {
173+
const id = generateFriendlyId("packet");
174+
const path = `trigger/${id}/payload.json`;
175+
176+
expect(id).toMatch(/^packet_[123456789abcdefghijkmnopqrstuvwxyz]{21}$/);
177+
expect(() => assertSafePacketRelativePath(path)).not.toThrow();
178+
expect(resolveSafePacketRelativePath(path)).toBe(path);
179+
});
180+
});
181+
166182
describe("normalizePacketRelativePath", () => {
167183
it("collapses redundant segments in safe paths", () => {
168184
expect(normalizePacketRelativePath("run/./payload.json")).toBe("run/payload.json");

packages/trigger-sdk/src/v3/shared.test.ts

Lines changed: 124 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
import { ApiClient } from "@trigger.dev/core/v3";
2-
import { describe, it, expect } from "vitest";
2+
import { createServer, type Server } from "node:http";
3+
import type { AddressInfo } from "node:net";
4+
import { afterEach, describe, it, expect } from "vitest";
35
import {
46
offloadBatchItemPayloads,
57
readableStreamToAsyncIterable,
8+
trigger,
69
uniqueBatchTaskIdentifiers,
710
} from "./shared.js";
811

@@ -69,6 +72,126 @@ describe("offloadBatchItemPayloads", () => {
6972
});
7073
});
7174

75+
describe("offloaded trigger payload paths", () => {
76+
let server: Server | undefined;
77+
78+
afterEach(async () => {
79+
const running = server;
80+
server = undefined;
81+
if (running) {
82+
running.closeAllConnections?.();
83+
await new Promise<void>((resolve) => running.close(() => resolve()));
84+
}
85+
});
86+
87+
/**
88+
* Stand in for the presign route and the object store, recording the packet path
89+
* the SDK actually puts on the wire.
90+
*/
91+
const startPacketServer = async (): Promise<{ origin: string; requestedPaths: string[] }> => {
92+
const requestedPaths: string[] = [];
93+
let origin = "";
94+
95+
server = createServer((request, response) => {
96+
const url = new URL(request.url ?? "/", origin);
97+
98+
if (url.pathname.startsWith("/api/v2/packets/")) {
99+
const encoded = url.pathname.slice("/api/v2/packets/".length);
100+
const storagePath = decodeURIComponent(encoded);
101+
requestedPaths.push(storagePath);
102+
103+
response.writeHead(200, { "Content-Type": "application/json" });
104+
response.end(JSON.stringify({ presignedUrl: `${origin}/upload`, storagePath }));
105+
return;
106+
}
107+
108+
request.resume();
109+
request.on("end", () => {
110+
response.writeHead(200, { "Content-Type": "application/json" });
111+
response.end(JSON.stringify({ id: "run_test" }));
112+
});
113+
});
114+
115+
await new Promise<void>((resolve) => server!.listen(0, "127.0.0.1", resolve));
116+
origin = `http://127.0.0.1:${(server!.address() as AddressInfo).port}`;
117+
118+
return { origin, requestedPaths };
119+
};
120+
121+
const NASTY_TASK_IDS = [
122+
"/my-task",
123+
"jobs/my-task",
124+
"my-task/",
125+
"a//b",
126+
"jobs/../x",
127+
"..",
128+
"my task",
129+
"caf\u00e9",
130+
"\ud800",
131+
];
132+
133+
it("builds a path from generated ids only, whatever the task id is", async () => {
134+
const { origin, requestedPaths } = await startPacketServer();
135+
const apiClient = new ApiClient(origin, "tr_dev_test");
136+
const payload = JSON.stringify({ blob: "x".repeat(200_000) });
137+
138+
const items = NASTY_TASK_IDS.map((task, index) => ({
139+
index,
140+
task,
141+
payload,
142+
options: { payloadType: "application/json" },
143+
}));
144+
145+
const result = await offloadBatchItemPayloads(items, apiClient);
146+
147+
expect(requestedPaths).toHaveLength(NASTY_TASK_IDS.length);
148+
149+
for (const path of requestedPaths) {
150+
expect(path).toMatch(
151+
/^trigger\/packet_[123456789abcdefghijkmnopqrstuvwxyz]{21}\/payload\.json$/
152+
);
153+
expect(path).not.toContain("%");
154+
}
155+
156+
expect(new Set(requestedPaths).size).toBe(NASTY_TASK_IDS.length);
157+
expect(new Set(result.map((item) => item.payload))).toEqual(new Set(requestedPaths));
158+
159+
for (const [index, item] of result.entries()) {
160+
expect(item.options?.payloadType).toBe("application/store");
161+
expect(item.task).toBe(NASTY_TASK_IDS[index]);
162+
}
163+
});
164+
165+
/**
166+
* A lone surrogate is excluded here only because the trigger endpoint's own URL
167+
* builder throws on it. That happens after the payload has been offloaded, so the
168+
* offload path handles the id fine, as the batch case above shows; the uploaded
169+
* object is simply orphaned when the trigger call fails.
170+
*/
171+
const TRIGGERABLE_NASTY_TASK_IDS = NASTY_TASK_IDS.filter((taskId) => taskId !== "\ud800");
172+
173+
it.each(TRIGGERABLE_NASTY_TASK_IDS)(
174+
"keeps the task id out of the path for trigger() of %j",
175+
async (taskId) => {
176+
const { origin, requestedPaths } = await startPacketServer();
177+
178+
const handle = await trigger(
179+
taskId as never,
180+
{ blob: "x".repeat(200_000) } as never,
181+
undefined,
182+
{ clientConfig: { baseURL: origin, accessToken: "tr_dev_test" } }
183+
);
184+
185+
expect(handle.id).toBe("run_test");
186+
expect(requestedPaths).toHaveLength(1);
187+
expect(requestedPaths[0]).toMatch(
188+
/^trigger\/packet_[123456789abcdefghijkmnopqrstuvwxyz]{21}\/payload\.json$/
189+
);
190+
expect(requestedPaths[0]).not.toContain("%");
191+
}
192+
);
193+
});
194+
72195
describe("readableStreamToAsyncIterable", () => {
73196
it("yields all values from the stream", async () => {
74197
const values = [1, 2, 3, 4, 5];

packages/trigger-sdk/src/v3/shared.ts

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
createErrorTaskError,
1818
defaultRetryOptions,
1919
flattenIdempotencyKey,
20+
generateFriendlyId,
2021
getIdempotencyKeyOptions,
2122
getSchemaParseFn,
2223
lifecycleHooks,
@@ -1770,7 +1771,7 @@ async function offloadBatchItemPayload(
17701771

17711772
const exported = await conditionallyExportPacket(
17721773
packet,
1773-
createTriggerPayloadPathPrefix(item.task),
1774+
createTriggerPayloadPathPrefix(),
17741775
undefined,
17751776
apiClient
17761777
);
@@ -2301,8 +2302,7 @@ async function trigger_internal<TRunTypes extends AnyRunTypes>(
23012302
const parsedPayload = parsePayload ? await parsePayload(payload) : payload;
23022303
const { packet: triggerPayloadPacket, payloadSize } = await prepareTriggerPayload(
23032304
parsedPayload,
2304-
apiClient,
2305-
id
2305+
apiClient
23062306
);
23072307

23082308
// Process idempotency key and extract options for storage
@@ -2566,8 +2566,7 @@ async function triggerAndWait_internal<TIdentifier extends string, TPayload, TOu
25662566
const parsedPayload = parsePayload ? await parsePayload(payload) : payload;
25672567
const { packet: triggerPayloadPacket, payloadSize } = await prepareTriggerPayload(
25682568
parsedPayload,
2569-
apiClient,
2570-
id
2569+
apiClient
25712570
);
25722571

25732572
// Process idempotency key and extract options for storage
@@ -2657,8 +2656,7 @@ async function triggerAndSubscribe_internal<TIdentifier extends string, TPayload
26572656
const parsedPayload = parsePayload ? await parsePayload(payload) : payload;
26582657
const { packet: triggerPayloadPacket, payloadSize } = await prepareTriggerPayload(
26592658
parsedPayload,
2660-
apiClient,
2661-
id
2659+
apiClient
26622660
);
26632661

26642662
const processedIdempotencyKey = await makeIdempotencyKey(options?.idempotencyKey);
@@ -3183,23 +3181,28 @@ function registerTaskLifecycleHooks<
31833181

31843182
async function prepareTriggerPayload(
31853183
payload: unknown,
3186-
apiClient: ApiClient,
3187-
taskId: string
3184+
apiClient: ApiClient
31883185
): Promise<{ packet: IOPacket; payloadSize: number }> {
31893186
const payloadPacket = await stringifyIO(payload);
31903187
// Measure the serialized size before any offload, so it reflects the real payload
31913188
// size rather than the small "application/store" reference we may send instead.
31923189
const { size: payloadSize } = packetRequiresOffloading(payloadPacket);
31933190
const packet = await conditionallyExportPacket(
31943191
payloadPacket,
3195-
createTriggerPayloadPathPrefix(taskId),
3192+
createTriggerPayloadPathPrefix(),
31963193
undefined,
31973194
apiClient
31983195
);
31993196
return { packet, payloadSize };
32003197
}
32013198

3202-
function createTriggerPayloadPathPrefix(taskId: string): string {
3203-
const safeTaskId = encodeURIComponent(taskId);
3204-
return `trigger/${safeTaskId}/${Date.now()}-${Math.random().toString(36).slice(2)}/payload`;
3199+
/**
3200+
* Build the object-storage path prefix for an offloaded trigger payload.
3201+
*
3202+
* Every segment is generated here, so nothing caller-controlled reaches the key.
3203+
* The id's alphabet is lowercase alphanumeric, which no layer between here and the
3204+
* object store rewrites, so the path the SDK asks for is the path that gets stored.
3205+
*/
3206+
function createTriggerPayloadPathPrefix(): string {
3207+
return `trigger/${generateFriendlyId("packet")}/payload`;
32053208
}

0 commit comments

Comments
 (0)