fix(webapp): validate packet storage paths (#3830)

## Summary

This PR adds packet path validation before key construction and
presigning. Invalid paths are rejected before reaching either
object-store client implementation, ensuring consistent behavior
regardless of the underlying storage configuration.
This commit is contained in:
Katia Bulatova
2026-06-05 10:28:08 +02:00
committed by GitHub
parent 8c9fee3933
commit db4074df54
6 changed files with 360 additions and 34 deletions
@@ -0,0 +1,6 @@
---
area: webapp
type: fix
---
Validate packet-relative storage paths before building object-store keys or presigned URLs.
+3 -3
View File
@@ -3,7 +3,7 @@ import { json } from "@remix-run/server-runtime";
import { z } from "zod";
import { authenticateApiRequest } from "~/services/apiAuth.server";
import { createLoaderApiRoute } from "~/services/routeBuilders/apiBuilder.server";
import { generatePresignedUrl } from "~/v3/objectStore.server";
import { generatePresignedUrl, jsonPacketPresignFailure } from "~/v3/objectStore.server";
const ParamsSchema = z.object({
"*": z.string(),
@@ -34,7 +34,7 @@ export async function action({ request, params }: ActionFunctionArgs) {
);
if (!signed.success) {
return json({ error: `Failed to generate presigned URL: ${signed.error}` }, { status: 500 });
return jsonPacketPresignFailure(signed);
}
// Caller can now use this URL to upload to that object.
@@ -59,7 +59,7 @@ export const loader = createLoaderApiRoute(
);
if (!signed.success) {
return json({ error: `Failed to generate presigned URL: ${signed.error}` }, { status: 500 });
return jsonPacketPresignFailure(signed);
}
// Caller can now use this URL to fetch that object.
+2 -2
View File
@@ -2,7 +2,7 @@ import type { ActionFunctionArgs } from "@remix-run/server-runtime";
import { json } from "@remix-run/server-runtime";
import { z } from "zod";
import { authenticateApiRequest } from "~/services/apiAuth.server";
import { generatePresignedUrl } from "~/v3/objectStore.server";
import { generatePresignedUrl, jsonPacketPresignFailure } from "~/v3/objectStore.server";
const ParamsSchema = z.object({
"*": z.string(),
@@ -34,7 +34,7 @@ export async function action({ request, params }: ActionFunctionArgs) {
);
if (!signed.success) {
return json({ error: `Failed to generate presigned URL: ${signed.error}` }, { status: 500 });
return jsonPacketPresignFailure(signed);
}
if (signed.storagePath === undefined) {
+152 -26
View File
@@ -1,9 +1,15 @@
import { json } from "@remix-run/server-runtime";
import { type IOPacket } from "@trigger.dev/core/v3";
import { env } from "~/env.server";
import { type AuthenticatedEnvironment } from "~/services/apiAuth.server";
import { logger } from "~/services/logger.server";
import { ServiceValidationError } from "~/v3/services/common.server";
import { singleton } from "~/utils/singleton";
import { ObjectStoreClient, type ObjectStoreClientConfig } from "./objectStoreClient.server";
import {
normalizeObjectStoreLogicalKeyPathname,
ObjectStoreClient,
type ObjectStoreClientConfig,
} from "./objectStoreClient.server";
/**
* Parsed storage URI with optional protocol prefix
@@ -47,6 +53,115 @@ export function formatStorageUri(path: string, protocol?: string): string {
return path;
}
export const INVALID_PACKET_STORAGE_PATH = "Invalid packet storage path";
export type PacketPresignFailure = {
success: false;
error: string;
status?: number;
};
const PACKET_RELATIVE_PATH_BASE = "/__packet_base__";
function throwInvalidPacketStoragePath(): never {
throw new ServiceValidationError(INVALID_PACKET_STORAGE_PATH, 400);
}
function assertRawPacketRelativePathSegments(path: string): void {
if (!path || path.includes("\\") || path.startsWith("/")) {
throwInvalidPacketStoragePath();
}
for (const segment of path.split("/")) {
if (segment === "" || segment === "." || segment === "..") {
throwInvalidPacketStoragePath();
}
if (segment.includes("%")) {
let decoded: string;
try {
decoded = decodeURIComponent(segment);
} catch {
throwInvalidPacketStoragePath();
}
if (decoded === "." || decoded === ".." || decoded.includes("/")) {
throwInvalidPacketStoragePath();
}
}
}
}
/**
* Normalize a packet-relative path using the same URL pathname resolution as object-store clients.
*/
export function normalizePacketRelativePath(path: string): string {
const url = new URL("https://trigger.invalid");
url.pathname = `${PACKET_RELATIVE_PATH_BASE}/${path.replace(/^\/+/, "")}`;
const prefix = `${PACKET_RELATIVE_PATH_BASE}/`;
if (!url.pathname.startsWith(prefix)) {
throwInvalidPacketStoragePath();
}
return url.pathname.slice(prefix.length);
}
/**
* Ensure a full logical object-store key resolves under the packet prefix after URL normalization.
*/
export function assertPacketObjectStoreKeyUnderPrefix(key: string, packetPrefix: string): void {
const normalizedKeyPath = normalizeObjectStoreLogicalKeyPathname(key);
const normalizedPrefixPath = normalizeObjectStoreLogicalKeyPathname(packetPrefix);
if (
normalizedKeyPath !== normalizedPrefixPath &&
!normalizedKeyPath.startsWith(`${normalizedPrefixPath}/`)
) {
throwInvalidPacketStoragePath();
}
}
/**
* Validate a packet-relative path and return the canonical form used for object-store keys.
*/
export function resolveSafePacketRelativePath(path: string): string {
assertRawPacketRelativePathSegments(path);
const normalized = normalizePacketRelativePath(path);
assertRawPacketRelativePathSegments(normalized);
return normalized;
}
/**
* Reject path traversal and other unsafe packet-relative storage paths before
* building object-store keys or presigned URLs.
*/
export function assertSafePacketRelativePath(path: string): void {
resolveSafePacketRelativePath(path);
}
function buildPacketObjectStoreKey(
projectRef: string,
envSlug: string,
relativePath: string
): string {
const safeRelativePath = resolveSafePacketRelativePath(relativePath);
const prefix = `packets/${projectRef}/${envSlug}`;
const key = `${prefix}/${safeRelativePath}`;
assertPacketObjectStoreKeyUnderPrefix(key, prefix);
return key;
}
/** JSON response for packet presign failures (400 client error vs 500 internal). */
export function jsonPacketPresignFailure(failure: PacketPresignFailure) {
const status = failure.status ?? 500;
if (status === 400) {
return json({ error: failure.error }, { status: 400 });
}
return json({ error: `Failed to generate presigned URL: ${failure.error}` }, { status: 500 });
}
/**
* Get object storage configuration for a given protocol.
* Returns a config if baseUrl is set, even without explicit credentials —
@@ -134,14 +249,20 @@ export async function uploadPacketToObjectStore(
throw new Error(`Object store is not configured for protocol: ${protocol || "default"}`);
}
const key = `packets/${environment.project.externalRef}/${environment.slug}/${filename}`;
const { path } = parseStorageUri(filename);
const safePath = resolveSafePacketRelativePath(path);
const key = buildPacketObjectStoreKey(
environment.project.externalRef,
environment.slug,
safePath
);
logger.debug("Uploading to object store", { key, protocol: protocol || "default" });
await client.putObject(key, data, contentType);
// Return filename with protocol prefix if specified
return formatStorageUri(filename, protocol);
// Return canonical storage URI (path only in the key; protocol prefix applied here)
return formatStorageUri(safePath, protocol);
}
export async function downloadPacketFromObjectStore(
@@ -162,14 +283,18 @@ export async function downloadPacketFromObjectStore(
}
const { protocol, path } = parseStorageUri(packet.data);
const key = buildPacketObjectStoreKey(
environment.project.externalRef,
environment.slug,
path
);
const client = getObjectStoreClient(protocol);
if (!client) {
throw new Error(`Object store is not configured for protocol: ${protocol || "default"}`);
}
const key = `packets/${environment.project.externalRef}/${environment.slug}/${path}`;
logger.debug("Downloading from object store", { key, protocol: protocol || "default" });
const data = await client.getObject(key);
@@ -220,10 +345,7 @@ export async function generatePresignedRequest(
method: "PUT" | "GET" = "PUT",
options?: GeneratePacketPresignOptions
): Promise<
| {
success: false;
error: string;
}
| PacketPresignFailure
| {
success: true;
request: Request;
@@ -237,6 +359,21 @@ export async function generatePresignedRequest(
options?.forceNoPrefix
);
let safePath: string;
try {
safePath = resolveSafePacketRelativePath(path);
} catch (error) {
if (error instanceof ServiceValidationError) {
return {
success: false,
error: error.message,
status: error.status ?? 400,
};
}
throw error;
}
const config = getObjectStoreConfig(storeProtocol);
if (!config?.baseUrl) {
return {
@@ -253,7 +390,7 @@ export async function generatePresignedRequest(
};
}
const key = `packets/${projectRef}/${envSlug}/${path}`;
const key = buildPacketObjectStoreKey(projectRef, envSlug, safePath);
try {
const url = await client.presign(key, method, 300); // 5 minutes
@@ -266,7 +403,7 @@ export async function generatePresignedRequest(
protocol: storeProtocol || "default",
});
const storagePath = method === "PUT" ? formatStorageUri(path, storeProtocol) : undefined;
const storagePath = method === "PUT" ? formatStorageUri(safePath, storeProtocol) : undefined;
return {
success: true,
@@ -276,9 +413,7 @@ export async function generatePresignedRequest(
} catch (error) {
return {
success: false,
error: `Failed to generate presigned URL: ${
error instanceof Error ? error.message : String(error)
}`,
error: error instanceof Error ? error.message : String(error),
};
}
}
@@ -289,23 +424,14 @@ export async function generatePresignedUrl(
filename: string,
method: "PUT" | "GET" = "PUT",
options?: GeneratePacketPresignOptions
): Promise<
| {
success: false;
error: string;
}
| {
success: true;
url: string;
storagePath?: string;
}
> {
): Promise<PacketPresignFailure | { success: true; url: string; storagePath?: string }> {
const signed = await generatePresignedRequest(projectRef, envSlug, filename, method, options);
if (!signed.success) {
return {
success: false,
error: signed.error,
status: signed.status,
};
}
+13 -3
View File
@@ -2,6 +2,16 @@ import { AwsClient } from "aws4fetch";
import { GetObjectCommand, PutObjectCommand, S3Client } from "@aws-sdk/client-s3";
import { getSignedUrl } from "@aws-sdk/s3-request-presigner";
/**
* Normalize a logical object-store key the same way Aws4FetchClient assigns URL pathnames.
* Decodes percent-escapes and resolves `.` / `..` segments before the request is signed.
*/
export function normalizeObjectStoreLogicalKeyPathname(logicalKey: string): string {
const url = new URL("https://trigger.invalid");
url.pathname = `/${logicalKey.replace(/^\/+/, "")}`;
return url.pathname;
}
interface IObjectStoreClient {
putObject(key: string, body: ReadableStream | string, contentType: string): Promise<string>;
getObject(key: string): Promise<string>;
@@ -32,7 +42,7 @@ class Aws4FetchClient implements IObjectStoreClient {
private buildUrl(key: string): string {
const url = new URL(this.config.baseUrl);
url.pathname = `/${key}`;
url.pathname = normalizeObjectStoreLogicalKeyPathname(key);
return url.toString();
}
@@ -59,7 +69,7 @@ class Aws4FetchClient implements IObjectStoreClient {
async presign(key: string, method: "PUT" | "GET", expiresIn: number): Promise<string> {
const url = new URL(this.config.baseUrl);
url.pathname = `/${key}`;
url.pathname = normalizeObjectStoreLogicalKeyPathname(key);
url.searchParams.set("X-Amz-Expires", String(expiresIn));
const signed = await this.awsClient.sign(new Request(url, { method }), {
@@ -100,7 +110,7 @@ class AwsSdkClient implements IObjectStoreClient {
private logicalObjectUrl(logicalKey: string): string {
const url = new URL(this.config.baseUrl);
url.pathname = `/${logicalKey}`;
url.pathname = normalizeObjectStoreLogicalKeyPathname(logicalKey);
return url.href;
}
+184
View File
@@ -4,13 +4,21 @@ import { type PrismaClient } from "@trigger.dev/database";
import { afterAll, describe, expect, it, vi } from "vitest";
import { env } from "~/env.server";
import { processWaitpointCompletionPacket } from "~/runEngine/concerns/waitpointCompletionPacket.server";
import { ServiceValidationError } from "~/v3/services/common.server";
import { normalizeObjectStoreLogicalKeyPathname } from "~/v3/objectStoreClient.server";
import {
assertPacketObjectStoreKeyUnderPrefix,
assertSafePacketRelativePath,
downloadPacketFromObjectStore,
formatStorageUri,
generatePresignedRequest,
generatePresignedUrl,
hasObjectStoreClient,
INVALID_PACKET_STORAGE_PATH,
jsonPacketPresignFailure,
normalizePacketRelativePath,
parseStorageUri,
resolveSafePacketRelativePath,
resolveStoreProtocolForPacketPresign,
uploadPacketToObjectStore,
} from "~/v3/objectStore.server";
@@ -106,6 +114,182 @@ describe("Object Storage", () => {
});
});
describe("assertSafePacketRelativePath", () => {
const expectInvalidPath = (path: string) => {
try {
assertSafePacketRelativePath(path);
expect.unreachable("expected path validation to throw");
} catch (error) {
expect(error).toBeInstanceOf(ServiceValidationError);
expect((error as ServiceValidationError).message).toBe(INVALID_PACKET_STORAGE_PATH);
expect((error as ServiceValidationError).status).toBe(400);
}
};
it.each([
"../file.json",
"../../other-env/file.json",
"foo/../bar.json",
"/absolute/path.json",
"foo//bar.json",
"foo\\bar.json",
".",
"..",
])("rejects unsafe path %s with 400 ServiceValidationError", (path) => {
expectInvalidPath(path);
});
it.each([
"run/%2e%2e/secret.json",
"%2e%2e/secret.json",
"%2E%2E/secret.json",
"run/%2e./payload.json",
"run/%2e%2e",
"foo/%2e/bar.json",
"foo/%2E/bar.json",
])("rejects percent-encoded traversal %s", (path) => {
expectInvalidPath(path);
});
it("allows normal packet paths", () => {
expect(() => assertSafePacketRelativePath("run_123/payload.json")).not.toThrow();
expect(resolveSafePacketRelativePath("run_123/payload.json")).toBe("run_123/payload.json");
});
it("rejects traversal in protocol-prefixed storage URIs", () => {
expect(() => assertSafePacketRelativePath(parseStorageUri("s3://../evil.json").path)).toThrow(
ServiceValidationError
);
});
});
describe("normalizePacketRelativePath", () => {
it("collapses redundant segments in safe paths", () => {
expect(normalizePacketRelativePath("run/./payload.json")).toBe("run/payload.json");
});
it("collapses encoded parent segments when used without segment decoding (why decode is required)", () => {
expect(normalizePacketRelativePath("run/%2e%2e/secret.json")).toBe("secret.json");
});
});
describe("double-encoded traversal segments", () => {
it("does not decode %252e%252e to .. in URL pathname (stays literal; prefix check still holds)", () => {
const path = "%252e%252e/secret.json";
expect(() => assertSafePacketRelativePath(path)).not.toThrow();
const key = `packets/proj_ref/dev/${path}`;
const normalized = normalizeObjectStoreLogicalKeyPathname(key);
expect(normalized).toBe(`/packets/proj_ref/dev/${path}`);
expect(() => assertPacketObjectStoreKeyUnderPrefix(key, "packets/proj_ref/dev")).not.toThrow();
});
});
describe("assertPacketObjectStoreKeyUnderPrefix", () => {
const prefix = "packets/proj_ref/dev";
it("accepts keys under the packet prefix after URL normalization", () => {
expect(() =>
assertPacketObjectStoreKeyUnderPrefix(`${prefix}/run_123/payload.json`, prefix)
).not.toThrow();
});
it("rejects keys whose normalized pathname escapes the packet prefix", () => {
const traversalKey = `${prefix}/%2e%2e/secret.json`;
const normalized = normalizeObjectStoreLogicalKeyPathname(traversalKey);
expect(normalized).toBe("/packets/proj_ref/secret.json");
expect(() => assertPacketObjectStoreKeyUnderPrefix(traversalKey, prefix)).toThrow(
ServiceValidationError
);
});
it("rejects env-level traversal via encoded parent segments", () => {
const traversalKey = `${prefix}/%2e%2e/secret.json`;
expect(normalizeObjectStoreLogicalKeyPathname(traversalKey)).toBe("/packets/proj_ref/secret.json");
expect(() => assertPacketObjectStoreKeyUnderPrefix(traversalKey, prefix)).toThrow(
ServiceValidationError
);
});
});
describe("normalizeObjectStoreLogicalKeyPathname (Aws4FetchClient behavior)", () => {
it("decodes %2e%2e segments into parent directory traversal", () => {
const key = "packets/proj_ref/dev/run/%2e%2e/secret.json";
expect(normalizeObjectStoreLogicalKeyPathname(key)).toBe(
"/packets/proj_ref/dev/secret.json"
);
});
});
describe("jsonPacketPresignFailure", () => {
it("returns 400 for invalid packet path validation failures", async () => {
const response = jsonPacketPresignFailure({
success: false,
error: INVALID_PACKET_STORAGE_PATH,
status: 400,
});
expect(response.status).toBe(400);
expect(await response.json()).toEqual({ error: INVALID_PACKET_STORAGE_PATH });
});
it("returns 500 for other presign failures", async () => {
const response = jsonPacketPresignFailure({
success: false,
error: "Object store is not configured for protocol: default",
});
expect(response.status).toBe(500);
expect(await response.json()).toEqual({
error: "Failed to generate presigned URL: Object store is not configured for protocol: default",
});
});
it("prefixes unprefixed internal errors exactly once (no double prefix)", async () => {
const response = jsonPacketPresignFailure({
success: false,
error: "Access Denied",
});
const body = await response.json();
expect(body).toEqual({ error: "Failed to generate presigned URL: Access Denied" });
expect(body.error).not.toMatch(/Failed to generate presigned URL: Failed to generate/);
});
});
describe("generatePresignedUrl path validation", () => {
it.each([
"../file.json",
"../../other-env/file.json",
"foo/../bar.json",
"/absolute/path.json",
"run/%2e%2e/secret.json",
"%2e%2e/secret.json",
"%2E%2E/secret.json",
])(
"returns 400 failure for unsafe path %s without calling object store",
async (filename) => {
const result = await generatePresignedUrl("proj_test", "dev", filename, "PUT");
expect(result.success).toBe(false);
if (result.success) throw new Error("expected presign to fail");
expect(result.error).toBe(INVALID_PACKET_STORAGE_PATH);
expect(result.status).toBe(400);
}
);
it("allows presign for valid packet paths when object store is not configured", async () => {
env.OBJECT_STORE_BASE_URL = undefined;
env.OBJECT_STORE_ACCESS_KEY_ID = undefined;
env.OBJECT_STORE_SECRET_ACCESS_KEY = undefined;
env.OBJECT_STORE_DEFAULT_PROTOCOL = undefined;
const result = await generatePresignedUrl("proj_test", "dev", "run_123/payload.json", "PUT");
expect(result.success).toBe(false);
if (result.success) throw new Error("expected presign to fail");
expect(result.error).toContain("Object store is not configured");
expect(result.status).toBeUndefined();
});
});
describe("resolveStoreProtocolForPacketPresign", () => {
afterEach(() => {
env.OBJECT_STORE_DEFAULT_PROTOCOL = originalEnvObj.OBJECT_STORE_DEFAULT_PROTOCOL;