journal: branded ownership loading for routes and activities

Ownership was checked ad hoc: some handlers loaded-and-compared
ownerId, some lib mutators enforced it in WHERE clauses and silently
no-op'd for non-owners, and nothing tied the two together. Two real
authorization bugs hid in the gaps: linkActivityToRoute ignored its
ownerId parameter entirely (any logged-in user could relink any
activity), and createRouteFromActivity loaded any activity without an
ownership check (a non-owner could copy a private activity's GPX into
their own route). PUT /api/v1/routes/:id also returned ok:true for
non-owners without updating anything.

ownership.server.ts is now the single enforcement point:

- loadOwnedRoute / loadOwnedActivity (non-throwing, for callers with
  their own error vocabulary) and requireOwnedRoute /
  requireOwnedActivity (throwing data() 404/403 for web handlers; 404
  by default so guessed ids don't leak existence)
- the returned entities carry an Owned<> brand; mutators (updateRoute,
  deleteRoute, deleteActivity, updateActivityVisibility,
  linkActivityToRoute, createRouteFromActivity) now require an
  OwnedRef, so skipping the check is a compile error
- vouchOwnership is the explicit, greppable escape hatch for the one
  non-session authorization path (the Planner JWT callback)
- WHERE ownerId clauses stay in the mutators as defense in depth

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Ullrich Schäfer 2026-06-10 02:35:00 +02:00
parent 1443a82f2b
commit 7a1dca378f
No known key found for this signature in database
GPG key ID: A32FF691A0F752D9
13 changed files with 256 additions and 48 deletions

View file

@ -7,6 +7,7 @@ import type { Visibility } from "@trails-cool/db/schema/journal";
import { validateGpx, writeGeom } from "./gpx-save.server.ts";
import type { GpxData } from "./gpx-save.server.ts";
import { enqueueOptional } from "./boss.server.ts";
import type { OwnedRef } from "./ownership.server.ts";
import {
enqueueActivityDeliveries,
visibilityTransitionAction,
@ -25,10 +26,10 @@ export interface ActivityInput {
}
export async function updateActivityVisibility(
id: string,
ownerId: string,
ownedActivity: OwnedRef,
visibility: Visibility,
): Promise<boolean> {
const { id, ownerId } = ownedActivity;
const db = getDb();
// Read the previous visibility first: the federation action depends on
// the *transition*, not the new value alone (a gratuitous Delete
@ -130,8 +131,11 @@ export async function getActivity(id: string) {
return { ...activity, geojson, importSource };
}
export async function deleteActivity(id: string, ownerId: string): Promise<boolean> {
export async function deleteActivity(ownedActivity: OwnedRef): Promise<boolean> {
const { id, ownerId } = ownedActivity;
const db = getDb();
// The WHERE ownerId clause stays as defense in depth even though the
// OwnedRef brand already proves ownership.
const [activity] = await db.select({ id: activities.id, visibility: activities.visibility }).from(activities)
.where(and(eq(activities.id, id), eq(activities.ownerId, ownerId)));
if (!activity) return false;
@ -311,17 +315,21 @@ export async function listRecentPublicActivities(limit: number = 20) {
return rows.map((r) => ({ ...r, geojson: geojsonMap.get(r.id) ?? null }));
}
export async function linkActivityToRoute(activityId: string, routeId: string, _ownerId: string) {
export async function linkActivityToRoute(ownedActivity: OwnedRef, ownedRoute: OwnedRef) {
const db = getDb();
await db
.update(activities)
.set({ routeId })
.where(eq(activities.id, activityId));
.set({ routeId: ownedRoute.id })
.where(and(eq(activities.id, ownedActivity.id), eq(activities.ownerId, ownedActivity.ownerId)));
}
export async function createRouteFromActivity(activityId: string, ownerId: string): Promise<string | null> {
export async function createRouteFromActivity(ownedActivity: OwnedRef): Promise<string | null> {
const { id: activityId, ownerId } = ownedActivity;
const db = getDb();
const [activity] = await db.select().from(activities).where(eq(activities.id, activityId));
const [activity] = await db
.select()
.from(activities)
.where(and(eq(activities.id, activityId), eq(activities.ownerId, ownerId)));
if (!activity?.gpx) return null;
const parsed = await validateGpx(activity.gpx);

View file

@ -10,7 +10,7 @@ import { desc, eq } from "drizzle-orm";
import { parseGpxAsync } from "@trails-cool/gpx";
import { routeVersions } from "@trails-cool/db/schema/journal";
import { getDb } from "../db.ts";
import { getRoute } from "../routes.server.ts";
import { loadOwnedRoute } from "../ownership.server.ts";
import {
ConnectionNotActiveError,
NeedsRelinkError,
@ -49,9 +49,11 @@ export async function pushRouteToProvider(
if (!manifest) return { status: "not_found" };
if (!manifest.routePusher) return { status: "unsupported_provider" };
const route = await getRoute(routeId);
if (!route) return { status: "not_found" };
if (route.ownerId !== userId) return { status: "not_owner" };
const loaded = await loadOwnedRoute(routeId, userId);
if (!loaded.ok) {
return { status: loaded.reason === "not_found" ? "not_found" : "not_owner" };
}
const route = loaded.entity;
const service = await getService(userId, providerId);
if (!service) return { status: "no_connection" };

View file

@ -0,0 +1,91 @@
import { describe, it, expect, vi, beforeEach } from "vitest";
const getRoute = vi.fn();
vi.mock("./routes.server.ts", () => ({
getRoute: (...args: unknown[]) => getRoute(...args),
}));
const getActivity = vi.fn();
vi.mock("./activities.server.ts", () => ({
getActivity: (...args: unknown[]) => getActivity(...args),
}));
import {
loadOwnedRoute,
loadOwnedActivity,
requireOwnedRoute,
requireOwnedActivity,
vouchOwnership,
} from "./ownership.server.ts";
const USER = "user-1";
describe("loadOwnedRoute / loadOwnedActivity", () => {
beforeEach(() => vi.clearAllMocks());
it("returns not_found for a missing route", async () => {
getRoute.mockResolvedValue(null);
expect(await loadOwnedRoute("r1", USER)).toEqual({ ok: false, reason: "not_found" });
});
it("returns not_owner for someone else's route", async () => {
getRoute.mockResolvedValue({ id: "r1", ownerId: "someone-else" });
expect(await loadOwnedRoute("r1", USER)).toEqual({ ok: false, reason: "not_owner" });
});
it("returns the entity when owned", async () => {
const route = { id: "r1", ownerId: USER, name: "Tour" };
getRoute.mockResolvedValue(route);
const result = await loadOwnedRoute("r1", USER);
expect(result.ok).toBe(true);
if (result.ok) expect(result.entity).toBe(route);
});
it("treats remote-ingested activities (ownerId null) as not owned", async () => {
getActivity.mockResolvedValue({ id: "a1", ownerId: null });
expect(await loadOwnedActivity("a1", USER)).toEqual({ ok: false, reason: "not_owner" });
});
});
describe("requireOwnedRoute / requireOwnedActivity", () => {
beforeEach(() => vi.clearAllMocks());
async function statusOf(promise: Promise<unknown>): Promise<number> {
try {
await promise;
throw new Error("expected a throw");
} catch (thrown) {
// react-router data() throws a Response-like with init status
return (thrown as { init?: { status: number }; status?: number }).init?.status
?? (thrown as { status: number }).status;
}
}
it("throws 404 for missing entities", async () => {
getRoute.mockResolvedValue(null);
expect(await statusOf(requireOwnedRoute("r1", USER))).toBe(404);
});
it("throws 404 for not-owner by default (no existence leak)", async () => {
getRoute.mockResolvedValue({ id: "r1", ownerId: "someone-else" });
expect(await statusOf(requireOwnedRoute("r1", USER))).toBe(404);
});
it("throws 403 for not-owner when the surface opts in", async () => {
getActivity.mockResolvedValue({ id: "a1", ownerId: "someone-else" });
expect(await statusOf(requireOwnedActivity("a1", USER, { notOwnerStatus: 403 }))).toBe(403);
});
it("returns the entity when owned", async () => {
const activity = { id: "a1", ownerId: USER };
getActivity.mockResolvedValue(activity);
expect(await requireOwnedActivity("a1", USER)).toBe(activity);
});
});
describe("vouchOwnership", () => {
it("passes the entity through unchanged (brand is type-level)", () => {
const route = { id: "r1", ownerId: "u9" };
expect(vouchOwnership(route)).toBe(route);
});
});

View file

@ -0,0 +1,88 @@
// The single enforcement point for "does this user own this entity".
//
// Mutators in routes.server.ts / activities.server.ts take an `OwnedRef`
// — a branded {id, ownerId} that can only be produced here (or by the
// explicit `vouchOwnership` escape hatch). A handler that forgets the
// ownership check no longer compiles, instead of silently no-op'ing or
// leaking another user's data.
import { data } from "react-router";
import { getRoute } from "./routes.server.ts";
import { getActivity } from "./activities.server.ts";
declare const ownedBrand: unique symbol;
/** An entity verified to belong to the acting user (or an equivalent grant). */
export type Owned<T> = T & { readonly [ownedBrand]: true };
/** The minimal shape mutators need; any Owned entity satisfies it. */
export type OwnedRef = Owned<{ id: string; ownerId: string }>;
export type OwnershipFailure = "not_found" | "not_owner";
type LoadResult<T> = { ok: true; entity: Owned<T> } | { ok: false; reason: OwnershipFailure };
function check<T extends { ownerId: string | null }>(
entity: T | null,
userId: string,
): LoadResult<T & { ownerId: string }> {
if (!entity) return { ok: false, reason: "not_found" };
// ownerId === null covers remote-ingested content, which nobody owns locally.
if (entity.ownerId === null || entity.ownerId !== userId) {
return { ok: false, reason: "not_owner" };
}
return { ok: true, entity: entity as Owned<T & { ownerId: string }> };
}
/** Non-throwing core for callers with their own error vocabulary
* (discriminated results, apiError JSON shapes). */
export async function loadOwnedRoute(routeId: string, userId: string) {
return check(await getRoute(routeId), userId);
}
export async function loadOwnedActivity(activityId: string, userId: string) {
return check(await getActivity(activityId), userId);
}
interface RequireOptions {
/**
* Status for the not-owner case. Defaults to 404 so a guessed id
* doesn't leak that the entity exists; pass 403 on surfaces where
* the entity is already known to the viewer (e.g. the edit page).
*/
notOwnerStatus?: 403 | 404;
}
function throwFor(reason: OwnershipFailure, label: string, opts?: RequireOptions): never {
if (reason === "not_owner" && (opts?.notOwnerStatus ?? 404) === 403) {
throw data({ error: "Not authorized" }, { status: 403 });
}
throw data({ error: `${label} not found` }, { status: 404 });
}
/** Throwing wrapper for web loaders/actions: 404 / 403 as `data()`. */
export async function requireOwnedRoute(routeId: string, userId: string, opts?: RequireOptions) {
const result = await loadOwnedRoute(routeId, userId);
if (!result.ok) throwFor(result.reason, "Route", opts);
return result.entity;
}
export async function requireOwnedActivity(
activityId: string,
userId: string,
opts?: RequireOptions,
) {
const result = await loadOwnedActivity(activityId, userId);
if (!result.ok) throwFor(result.reason, "Activity", opts);
return result.entity;
}
/**
* Escape hatch for callers whose authorization does not come from a
* session today only the Planner JWT callback, where a single-use
* token scoped to the route stands in for the owner. Every use should
* be greppable and justified by a comment at the call site.
*/
export function vouchOwnership<T extends { id: string; ownerId: string }>(entity: T): Owned<T> {
return entity as Owned<T>;
}

View file

@ -6,6 +6,7 @@ import type { Visibility } from "@trails-cool/db/schema/journal";
import { sql } from "drizzle-orm";
import { validateGpx, writeGeom } from "./gpx-save.server.ts";
import type { GpxData } from "./gpx-save.server.ts";
import type { OwnedRef } from "./ownership.server.ts";
export interface RouteInput {
name: string;
@ -130,11 +131,8 @@ export async function listPublicRoutesForOwner(ownerId: string, limit: number =
return rows.map((r) => ({ ...r, geojson: geojsonMap.get(r.id) ?? null }));
}
export async function updateRoute(
id: string,
ownerId: string,
input: Partial<RouteInput>,
) {
export async function updateRoute(route: OwnedRef, input: Partial<RouteInput>) {
const { id, ownerId } = route;
const db = getDb();
let parsed: GpxData | null = null;
@ -188,11 +186,13 @@ export async function updateRoute(
});
}
export async function deleteRoute(id: string, ownerId: string) {
export async function deleteRoute(route: OwnedRef) {
const db = getDb();
// The WHERE ownerId clause stays as defense in depth even though the
// OwnedRef brand already proves ownership.
const result = await db
.delete(routes)
.where(and(eq(routes.id, id), eq(routes.ownerId, ownerId)))
.where(and(eq(routes.id, route.id), eq(routes.ownerId, route.ownerId)))
.returning({ id: routes.id });
return result.length > 0;
}

View file

@ -12,6 +12,7 @@ import {
} from "~/lib/activities.server";
import { deleteImportByActivity } from "~/lib/sync/imports.server";
import { listRoutes } from "~/lib/routes.server";
import { requireOwnedActivity, requireOwnedRoute } from "~/lib/ownership.server";
import type { Visibility } from "@trails-cool/db/schema/journal";
const VISIBILITY_VALUES = new Set<Visibility>(["private", "unlisted", "public"]);
@ -65,24 +66,27 @@ export async function activityDetailAction(request: Request, id: string | undefi
const intent = formData.get("intent");
if (intent === "link-route") {
const routeId = formData.get("routeId") as string;
if (routeId) {
await linkActivityToRoute(activityId, routeId, user.id);
const activity = await requireOwnedActivity(activityId, user.id);
const routeIdRaw = formData.get("routeId") as string;
if (routeIdRaw) {
const route = await requireOwnedRoute(routeIdRaw, user.id);
await linkActivityToRoute(activity, route);
}
return redirect(`/activities/${activityId}`);
}
if (intent === "create-route") {
const routeId = await createRouteFromActivity(activityId, user.id);
const activity = await requireOwnedActivity(activityId, user.id);
const routeId = await createRouteFromActivity(activity);
if (routeId) return redirect(`/routes/${routeId}`);
return data({ error: "No GPX data to create route from" }, { status: 400 });
}
if (intent === "delete") {
const activity = await requireOwnedActivity(activityId, user.id);
await deleteImportByActivity(activityId);
const deleted = await deleteActivity(activityId, user.id);
if (deleted) return redirect("/activities");
return data({ error: "Activity not found" }, { status: 404 });
await deleteActivity(activity);
return redirect("/activities");
}
if (intent === "set-visibility") {
@ -90,8 +94,8 @@ export async function activityDetailAction(request: Request, id: string | undefi
if (!raw || !VISIBILITY_VALUES.has(raw as Visibility)) {
return data({ error: "Invalid visibility" }, { status: 400 });
}
const ok = await updateActivityVisibility(activityId, user.id, raw as Visibility);
if (!ok) return data({ error: "Activity not found" }, { status: 404 });
const activity = await requireOwnedActivity(activityId, user.id);
await updateActivityVisibility(activity, raw as Visibility);
return redirect(`/activities/${activityId}`);
}

View file

@ -2,6 +2,7 @@ import { data } from "react-router";
import type { Route } from "./+types/api.routes.$id.callback";
import { verifyRouteToken } from "~/lib/jwt.server";
import { updateRoute, getRoute } from "~/lib/routes.server";
import { vouchOwnership } from "~/lib/ownership.server";
import { GpxValidationError } from "~/lib/gpx-save.server";
const PLANNER_ORIGIN = process.env.PLANNER_URL ?? "http://localhost:3001";
@ -61,8 +62,10 @@ export async function action({ params, request }: Route.ActionArgs) {
return data({ error: "Missing GPX data" }, { status: 400, headers: corsHeaders() });
}
// Update route with new GPX (creates new version)
await updateRoute(params.id, route.ownerId, { gpx });
// Update route with new GPX (creates new version). Authorization
// here comes from the verified single-use route token, not a
// session — vouchOwnership marks that explicitly.
await updateRoute(vouchOwnership(route), { gpx });
return data({ success: true, routeId: params.id }, { headers: corsHeaders() });
} catch (e) {

View file

@ -2,16 +2,14 @@ import { data } from "react-router";
import { getOrigin } from "~/lib/config.server";
import type { Route } from "./+types/api.routes.$id.edit-in-planner";
import { getSessionUser } from "~/lib/auth/session.server";
import { getRouteWithVersions } from "~/lib/routes.server";
import { requireOwnedRoute } from "~/lib/ownership.server";
import { createRouteToken } from "~/lib/jwt.server";
export async function action({ params, request }: Route.ActionArgs) {
const user = await getSessionUser(request);
if (!user) return data({ error: "Not authenticated" }, { status: 401 });
const route = await getRouteWithVersions(params.id);
if (!route) return data({ error: "Route not found" }, { status: 404 });
if (route.ownerId !== user.id) return data({ error: "Not authorized" }, { status: 403 });
const route = await requireOwnedRoute(params.id, user.id, { notOwnerStatus: 403 });
const token = await createRouteToken(params.id);
const origin = getOrigin();

View file

@ -1,6 +1,7 @@
import type { Route } from "./+types/api.v1.activities.$id";
import { requireApiUser, apiError } from "~/lib/api-guard.server";
import { getActivity, deleteActivity } from "~/lib/activities.server";
import { loadOwnedActivity } from "~/lib/ownership.server";
import { ERROR_CODES } from "@trails-cool/api";
/** GET /api/v1/activities/:id — full activity detail */
@ -33,9 +34,10 @@ export async function action({ request, params }: Route.ActionArgs) {
if (request.method !== "DELETE") return new Response(null, { status: 405 });
const user = await requireApiUser(request);
const deleted = await deleteActivity(params.id, user.id);
if (!deleted) {
const result = await loadOwnedActivity(params.id, user.id);
if (!result.ok) {
return apiError(404, ERROR_CODES.NOT_FOUND, "Activity not found");
}
await deleteActivity(result.entity);
return new Response(null, { status: 204 });
}

View file

@ -1,6 +1,7 @@
import type { Route } from "./+types/api.v1.routes.$id";
import { requireApiUser, apiError } from "~/lib/api-guard.server";
import { getRouteWithVersions, updateRoute, deleteRoute } from "~/lib/routes.server";
import { loadOwnedRoute } from "~/lib/ownership.server";
import { UpdateRouteRequestSchema, ERROR_CODES, zodIssuesToFieldErrors } from "@trails-cool/api";
/** GET /api/v1/routes/:id — full route detail */
@ -48,15 +49,20 @@ export async function action({ request, params }: Route.ActionArgs) {
zodIssuesToFieldErrors(parsed.error));
}
await updateRoute(params.id, user.id, parsed.data);
const result = await loadOwnedRoute(params.id, user.id);
if (!result.ok) {
return apiError(404, ERROR_CODES.NOT_FOUND, "Route not found");
}
await updateRoute(result.entity, parsed.data);
return Response.json({ ok: true });
}
if (request.method === "DELETE") {
const deleted = await deleteRoute(params.id, user.id);
if (!deleted) {
const result = await loadOwnedRoute(params.id, user.id);
if (!result.ok) {
return apiError(404, ERROR_CODES.NOT_FOUND, "Route not found");
}
await deleteRoute(result.entity);
return new Response(null, { status: 204 });
}

View file

@ -7,6 +7,7 @@ const mockGetAuthenticatedUser = vi.fn();
const mockListRoutes = vi.fn();
const mockCreateRoute = vi.fn();
const mockGetRouteWithVersions = vi.fn();
const mockGetRoute = vi.fn();
const mockDeleteRoute = vi.fn();
vi.mock("~/lib/db", () => ({ getDb: vi.fn() }));
@ -19,6 +20,7 @@ vi.mock("~/lib/routes.server", () => ({
listRoutes: mockListRoutes,
createRoute: mockCreateRoute,
getRouteWithVersions: mockGetRouteWithVersions,
getRoute: mockGetRoute,
updateRoute: vi.fn(),
deleteRoute: mockDeleteRoute,
}));
@ -146,6 +148,7 @@ describe("GET /api/v1/routes/:id", () => {
describe("DELETE /api/v1/routes/:id", () => {
it("returns 204 on success", async () => {
mockGetRoute.mockResolvedValue({ id: "r1", ownerId: "user-1" });
mockDeleteRoute.mockResolvedValue(true);
const { action } = await import("./api.v1.routes.$id.ts");
const resp = await action(routeArgs(
@ -156,7 +159,7 @@ describe("DELETE /api/v1/routes/:id", () => {
});
it("returns 404 if not found", async () => {
mockDeleteRoute.mockResolvedValue(false);
mockGetRoute.mockResolvedValue(null);
const { action } = await import("./api.v1.routes.$id.ts");
const resp = await action(routeArgs(
authRequest("/api/v1/routes/missing", { method: "DELETE" }), { id: "missing" }, "api/v1/routes/:id",

View file

@ -1,8 +1,9 @@
// Server-only loader/action for /routes/:id/edit. See `home.server.ts`.
import { data, redirect } from "react-router";
import { redirect } from "react-router";
import { requireSessionUser } from "~/lib/auth/session.server";
import { getRoute, updateRoute } from "~/lib/routes.server";
import { updateRoute } from "~/lib/routes.server";
import { requireOwnedRoute } from "~/lib/ownership.server";
import type { Visibility } from "@trails-cool/db/schema/journal";
const VISIBILITY_VALUES = new Set<Visibility>(["private", "unlisted", "public"]);
@ -10,9 +11,7 @@ const VISIBILITY_VALUES = new Set<Visibility>(["private", "unlisted", "public"])
export async function loadRouteEdit(request: Request, id: string | undefined) {
const user = await requireSessionUser(request);
const route = await getRoute(id ?? "");
if (!route) throw data({ error: "Route not found" }, { status: 404 });
if (route.ownerId !== user.id) throw data({ error: "Not authorized" }, { status: 403 });
const route = await requireOwnedRoute(id ?? "", user.id, { notOwnerStatus: 403 });
return {
route: {
@ -44,6 +43,7 @@ export async function routeEditAction(request: Request, id: string | undefined)
input.visibility = visibilityRaw as Visibility;
}
await updateRoute(routeId, user.id, input);
const route = await requireOwnedRoute(routeId, user.id, { notOwnerStatus: 403 });
await updateRoute(route, input);
return redirect(`/routes/${routeId}`);
}

View file

@ -5,6 +5,7 @@ import { and, eq } from "drizzle-orm";
import { canView } from "~/lib/auth.server";
import { getSessionUser, requireSessionUser } from "~/lib/auth/session.server";
import { getRoute, getRouteWithVersions, deleteRoute, updateRoute } from "~/lib/routes.server";
import { requireOwnedRoute } from "~/lib/ownership.server";
import { getDb } from "~/lib/db";
import { syncPushes } from "@trails-cool/db/schema/journal";
import { getService } from "~/lib/connected-services";
@ -146,7 +147,8 @@ export async function routeDetailAction(request: Request, id: string | undefined
const intent = formData.get("intent");
if (intent === "delete") {
await deleteRoute(routeId, user.id);
const route = await requireOwnedRoute(routeId, user.id);
await deleteRoute(route);
return redirect("/routes");
}
@ -162,7 +164,8 @@ export async function routeDetailAction(request: Request, id: string | undefined
input.gpx = await gpxFile.text();
}
await updateRoute(routeId, user.id, input as { name?: string; description?: string; gpx?: string });
const route = await requireOwnedRoute(routeId, user.id);
await updateRoute(route, input as { name?: string; description?: string; gpx?: string });
return redirect(`/routes/${routeId}`);
}