Merge pull request #521 from trails-cool/sec-upload-validation

security: authorize & validate presigned upload requests

apps/journal/app/routes/api.v1.uploads.test.ts

@@ -0,0 +1,84 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { TERMS_VERSION } from "~/lib/legal";
+
+const mockUser = {
+  id: "user-1", email: "t@t.com", username: "t", domain: "localhost",
+  displayName: null, bio: null, createdAt: new Date(), termsVersion: TERMS_VERSION,
+};
+const OWN_ID = "11111111-1111-4111-8111-111111111111";
+const OTHER_ID = "22222222-2222-4222-8222-222222222222";
+
+const mockGetAuthenticatedUser = vi.fn();
+const loadOwnedRoute = vi.fn();
+const loadOwnedActivity = vi.fn();
+
+vi.mock("~/lib/db", () => ({ getDb: vi.fn() }));
+vi.mock("~/lib/auth.server", () => ({ getSessionUser: vi.fn() }));
+vi.mock("~/lib/oauth.server", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("~/lib/oauth.server")>();
+  return { ...actual, getAuthenticatedUser: mockGetAuthenticatedUser };
+});
+vi.mock("~/lib/ownership.server", () => ({
+  loadOwnedRoute: (...a: unknown[]) => loadOwnedRoute(...a),
+  loadOwnedActivity: (...a: unknown[]) => loadOwnedActivity(...a),
+}));
+
+function authRequest(body: unknown) {
+  return new Request("http://localhost:3000/api/v1/uploads", {
+    method: "POST",
+    headers: { Authorization: "Bearer valid-token", "Content-Type": "application/json" },
+    body: JSON.stringify(body),
+  });
+}
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function routeArgs(request: Request): any {
+  return { request, params: {}, context: {}, unstable_url: new URL(request.url), unstable_pattern: "api/v1/uploads" };
+}
+
+const validBody = {
+  filename: "photo.jpg",
+  contentType: "image/jpeg",
+  resourceType: "activity" as const,
+  resourceId: OWN_ID,
+};
+
+describe("POST /api/v1/uploads", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mockGetAuthenticatedUser.mockResolvedValue(mockUser);
+  });
+
+  it("mints a key scoped to the owned resource, with a sanitized filename", async () => {
+    loadOwnedActivity.mockResolvedValue({ ok: true, entity: { id: OWN_ID, ownerId: "user-1" } });
+    const { action } = await import("./api.v1.uploads.ts");
+    const resp = await action(routeArgs(authRequest({ ...validBody, filename: "../../evil name.jpg" }))) as Response;
+
+    expect(resp.status).toBe(200);
+    const json = await resp.json();
+    expect(loadOwnedActivity).toHaveBeenCalledWith(OWN_ID, "user-1");
+    expect(json.key).toMatch(new RegExp(`^activity/${OWN_ID}/[0-9a-f-]+-evil_name\\.jpg$`));
+    expect(json.key).not.toContain("..");
+  });
+
+  it("rejects uploads to a resource the caller does not own (404, no leak)", async () => {
+    loadOwnedActivity.mockResolvedValue({ ok: false, reason: "not_owner" });
+    const { action } = await import("./api.v1.uploads.ts");
+    const resp = await action(routeArgs(authRequest({ ...validBody, resourceId: OTHER_ID }))) as Response;
+    expect(resp.status).toBe(404);
+  });
+
+  it("rejects a disallowed content type before any ownership work", async () => {
+    const { action } = await import("./api.v1.uploads.ts");
+    const resp = await action(routeArgs(authRequest({ ...validBody, contentType: "text/html" }))) as Response;
+    expect(resp.status).toBe(400);
+    expect(loadOwnedActivity).not.toHaveBeenCalled();
+  });
+
+  it("routes route-type uploads through the route ownership check", async () => {
+    loadOwnedRoute.mockResolvedValue({ ok: true, entity: { id: OWN_ID, ownerId: "user-1" } });
+    const { action } = await import("./api.v1.uploads.ts");
+    const resp = await action(routeArgs(authRequest({ ...validBody, resourceType: "route" }))) as Response;
+    expect(resp.status).toBe(200);
+    expect(loadOwnedRoute).toHaveBeenCalledWith(OWN_ID, "user-1");
+  });
+});

apps/journal/app/routes/api.v1.uploads.ts

@@ -1,7 +1,13 @@
 import type { Route } from "./+types/api.v1.uploads";
 import { requireApiUser, apiError } from "~/lib/api-guard.server";
-import { PresignedUploadRequestSchema, ERROR_CODES, zodIssuesToFieldErrors } from "@trails-cool/api";
+import {
+  PresignedUploadRequestSchema,
+  sanitizeUploadFilename,
+  ERROR_CODES,
+  zodIssuesToFieldErrors,
+} from "@trails-cool/api";
 import { randomUUID } from "node:crypto";
+import { loadOwnedRoute, loadOwnedActivity } from "~/lib/ownership.server";
 
 const S3_ENDPOINT = process.env.S3_ENDPOINT ?? "http://localhost:3902";
 const S3_BUCKET = process.env.S3_BUCKET ?? "trails-cool";
@@ -10,9 +16,11 @@ const S3_PUBLIC_URL = process.env.S3_PUBLIC_URL ?? S3_ENDPOINT;
 /** POST /api/v1/uploads — generate presigned upload URL */
 export async function action({ request }: Route.ActionArgs) {
   if (request.method !== "POST") return new Response(null, { status: 405 });
-  await requireApiUser(request);
+  const user = await requireApiUser(request);
 
   const body = await request.json().catch(() => null);
+  // The schema enforces a content-type allowlist (no active content that
+  // could execute if served inline) and a bounded filename.
   const parsed = PresignedUploadRequestSchema.safeParse(body);
   if (!parsed.success) {
     return apiError(400, ERROR_CODES.VALIDATION_ERROR, "Validation failed",
@@ -20,7 +28,21 @@ export async function action({ request }: Route.ActionArgs) {
   }
 
   const { filename, resourceType, resourceId } = parsed.data;
-  const key = `${resourceType}/${resourceId}/${randomUUID()}-${filename}`;
+
+  // Authorization: a caller may only mint upload URLs for a route or
+  // activity they own. Without this, anyone could write objects into the
+  // key-space of another user's resource. 404 (not 403) so a guessed id
+  // doesn't reveal the resource exists.
+  const owned = resourceType === "route"
+    ? await loadOwnedRoute(resourceId, user.id)
+    : await loadOwnedActivity(resourceId, user.id);
+  if (!owned.ok) {
+    return apiError(404, ERROR_CODES.NOT_FOUND, `${resourceType} not found`);
+  }
+
+  // Sanitize the client filename before it becomes part of the S3 key.
+  const safeName = sanitizeUploadFilename(filename);
+  const key = `${resourceType}/${resourceId}/${randomUUID()}-${safeName}`;
 
   // Return the upload URL and the final public URL
   return Response.json({

packages/api/src/index.ts

@@ -55,5 +55,6 @@ export {
 // Uploads
 export {
   PresignedUploadRequestSchema, PresignedUploadResponseSchema,
+  ALLOWED_UPLOAD_CONTENT_TYPES, sanitizeUploadFilename,
   type PresignedUploadRequest, type PresignedUploadResponse,
 } from "./uploads.ts";

packages/api/src/uploads.test.ts

@@ -0,0 +1,63 @@
+import { describe, it, expect } from "vitest";
+import {
+  PresignedUploadRequestSchema,
+  sanitizeUploadFilename,
+} from "./uploads.ts";
+
+const ROUTE_ID = "11111111-1111-4111-8111-111111111111";
+
+describe("PresignedUploadRequestSchema", () => {
+  it("accepts an allowed content type", () => {
+    const r = PresignedUploadRequestSchema.safeParse({
+      filename: "photo.jpg",
+      contentType: "image/jpeg",
+      resourceType: "activity",
+      resourceId: ROUTE_ID,
+    });
+    expect(r.success).toBe(true);
+  });
+
+  it("rejects active/disallowed content types", () => {
+    for (const contentType of ["text/html", "image/svg+xml", "application/javascript"]) {
+      const r = PresignedUploadRequestSchema.safeParse({
+        filename: "x", contentType, resourceType: "route", resourceId: ROUTE_ID,
+      });
+      expect(r.success, contentType).toBe(false);
+    }
+  });
+
+  it("rejects a non-uuid resourceId and empty filename", () => {
+    expect(PresignedUploadRequestSchema.safeParse({
+      filename: "a.png", contentType: "image/png", resourceType: "route", resourceId: "nope",
+    }).success).toBe(false);
+    expect(PresignedUploadRequestSchema.safeParse({
+      filename: "", contentType: "image/png", resourceType: "route", resourceId: ROUTE_ID,
+    }).success).toBe(false);
+  });
+});
+
+describe("sanitizeUploadFilename", () => {
+  it("strips path components (no traversal into the key)", () => {
+    expect(sanitizeUploadFilename("../../etc/passwd")).toBe("passwd");
+    expect(sanitizeUploadFilename("/abs/path/img.png")).toBe("img.png");
+    expect(sanitizeUploadFilename("a\\b\\c.jpg")).toBe("c.jpg");
+  });
+
+  it("restricts to a conservative charset and collapses runs", () => {
+    expect(sanitizeUploadFilename("my photo (1)!.jpg")).toBe("my_photo_1_.jpg");
+  });
+
+  it("strips leading dots so dotfiles / .. can't form the segment", () => {
+    expect(sanitizeUploadFilename("...hidden")).toBe("hidden");
+    expect(sanitizeUploadFilename("..")).toBe("upload");
+  });
+
+  it("never returns empty", () => {
+    expect(sanitizeUploadFilename("")).toBe("upload");
+    expect(sanitizeUploadFilename("////")).toBe("upload");
+  });
+
+  it("bounds the length", () => {
+    expect(sanitizeUploadFilename("a".repeat(500)).length).toBeLessThanOrEqual(128);
+  });
+});

packages/api/src/uploads.ts

@@ -1,12 +1,40 @@
 import { z } from "zod";
 
+/**
+ * Content types accepted for uploads. Constrained so a caller can't
+ * stash active content (HTML/SVG/JS) that would execute if the object
+ * were ever served inline. Keep in sync with what the upload UI sends.
+ */
+export const ALLOWED_UPLOAD_CONTENT_TYPES = [
+  "image/jpeg",
+  "image/png",
+  "image/webp",
+  "image/gif",
+  "application/gpx+xml",
+] as const;
+
 export const PresignedUploadRequestSchema = z.object({
-  filename: z.string(),
+  filename: z.string().min(1).max(255),
-  contentType: z.string(),
+  contentType: z.enum(ALLOWED_UPLOAD_CONTENT_TYPES),
   resourceType: z.enum(["route", "activity"]),
   resourceId: z.uuid(),
 });
 
+/**
+ * Reduce a client-supplied filename to a safe S3 object-key segment:
+ * basename only (drop any path), restrict to a conservative charset,
+ * collapse runs, strip leading dots, and bound the length. Never empty.
+ */
+export function sanitizeUploadFilename(filename: string): string {
+  const base = filename.split(/[/\\]/).pop() ?? "";
+  const cleaned = base
+    .replace(/[^A-Za-z0-9._-]/g, "_")
+    .replace(/_{2,}/g, "_")
+    .replace(/^\.+/, "")
+    .slice(0, 128);
+  return cleaned.length > 0 ? cleaned : "upload";
+}
+
 export const PresignedUploadResponseSchema = z.object({
   uploadUrl: z.url(),
   storageKey: z.string(),