diff --git a/apps/journal/app/routes/api.v1.uploads.test.ts b/apps/journal/app/routes/api.v1.uploads.test.ts new file mode 100644 index 0000000..77c19bb --- /dev/null +++ b/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(); + 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"); + }); +}); diff --git a/apps/journal/app/routes/api.v1.uploads.ts b/apps/journal/app/routes/api.v1.uploads.ts index 4998bdc..4d76ffb 100644 --- a/apps/journal/app/routes/api.v1.uploads.ts +++ b/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({ diff --git a/packages/api/src/index.ts b/packages/api/src/index.ts index 3d50b31..fc88341 100644 --- a/packages/api/src/index.ts +++ b/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"; diff --git a/packages/api/src/uploads.test.ts b/packages/api/src/uploads.test.ts new file mode 100644 index 0000000..1c28009 --- /dev/null +++ b/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); + }); +}); diff --git a/packages/api/src/uploads.ts b/packages/api/src/uploads.ts index 37da2f1..fb1fd15 100644 --- a/packages/api/src/uploads.ts +++ b/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(), - contentType: z.string(), + filename: z.string().min(1).max(255), + 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(),