security: authorize and validate presigned upload requests
POST /api/v1/uploads minted an upload key from caller input with no ownership check, no content-type allowlist, and the raw filename interpolated into the S3 key. - Ownership: a caller may only mint upload URLs for a route/activity they own, enforced via the branded loadOwnedRoute/loadOwnedActivity (404 on miss, no existence leak). Closes writing into another user's resource key-space. - Content type: the request schema now constrains contentType to an image/gpx allowlist, so active content (HTML/SVG/JS) that would execute if served inline is rejected at the request boundary. - Filename: sanitizeUploadFilename() reduces the client filename to a safe basename (charset-restricted, no path components, no leading dots, length-bounded) before it becomes part of the key. Tests: schema allow/deny + filename sanitization in @trails-cool/api; handler tests covering owned-success, not-owner 404, disallowed content-type, and the route-vs-activity ownership branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
e64155b490
commit
01d4832edb
5 changed files with 203 additions and 5 deletions
84
apps/journal/app/routes/api.v1.uploads.test.ts
Normal file
84
apps/journal/app/routes/api.v1.uploads.test.ts
Normal file
|
|
@ -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");
|
||||
});
|
||||
});
|
||||
|
|
@ -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({
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue