From 51e6b8a0d7893a4e62e1d631bfd2c7303a2bb783 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ullrich=20Sch=C3=A4fer?= Date: Tue, 26 May 2026 00:05:47 +0200 Subject: [PATCH] fix(planner): validate callback/returnUrl + cap session URL-param payloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses planner audit #3 (SSRF via callbackUrl) and #7 (URL-param size). Two attack surfaces hardened: 1. /new loader — \`callback\`, \`token\`, \`returnUrl\`, \`gpx\` query params now validated: - callbackUrl: must be a valid absolute http(s) URL ≤ 2048 chars. If \`PLANNER_CALLBACK_ALLOWED_HOSTS\` is set (comma-separated), the host must match — defense-in-depth SSRF guard for self- hosted instances. Unset = no allowlist (dev / open self-host). - token: max 2048 chars. - returnUrl: must be a same-origin path or absolute http(s) URL ≤ 2048 chars. Rejects \`javascript:\`, \`data:\`, and protocol-relative \`//host\` (which would resolve to a remote origin on HTTPS pages). - gpx: ≤ 2 MB encoded. Invalid input throws 400 from the loader. 2. /session/:id default-export component — \`waypoints\`, \`noGoAreas\`, \`notes\`, \`returnUrl\` URL params now bounded before \`JSON.parse\` / use: - waypoints / noGoAreas: ≤ 50KB each; over-cap returns undefined (component starts with empty initial state, same as malformed). - notes: ≤ 10KB. - returnUrl: ≤ 2KB + same scheme rules as #1. Pulled the URL validation into \`lib/url-validation.server.ts\` so both routes (and any future caller) share the same rules. Tests: \`url-validation.server.test.ts\` (14 cases — schemes, allowlist, length caps, protocol-relative guards, env parsing). Full repo: pnpm typecheck / lint / test all green. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../app/lib/url-validation.server.test.ts | 82 +++++++++++++++++ apps/planner/app/lib/url-validation.server.ts | 91 +++++++++++++++++++ apps/planner/app/routes/new.tsx | 35 +++++++ apps/planner/app/routes/session.$id.tsx | 46 ++++++++-- 4 files changed, 244 insertions(+), 10 deletions(-) create mode 100644 apps/planner/app/lib/url-validation.server.test.ts create mode 100644 apps/planner/app/lib/url-validation.server.ts diff --git a/apps/planner/app/lib/url-validation.server.test.ts b/apps/planner/app/lib/url-validation.server.test.ts new file mode 100644 index 0000000..ad5dc07 --- /dev/null +++ b/apps/planner/app/lib/url-validation.server.test.ts @@ -0,0 +1,82 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { + validateFetchUrl, + validateRedirectUrl, + getCallbackAllowedHosts, +} from "./url-validation.server.ts"; + +describe("validateFetchUrl", () => { + it("accepts a plain https URL", () => { + expect(validateFetchUrl("https://journal.trails.cool/api/cb").ok).toBe(true); + }); + + it("rejects javascript: scheme", () => { + const r = validateFetchUrl("javascript:alert(1)"); + expect(r.ok).toBe(false); + expect(r.reason).toMatch(/scheme/); + }); + + it("rejects file: scheme", () => { + expect(validateFetchUrl("file:///etc/passwd").ok).toBe(false); + }); + + it("rejects relative paths (must be absolute)", () => { + expect(validateFetchUrl("/foo/bar").ok).toBe(false); + }); + + it("rejects malformed input", () => { + expect(validateFetchUrl("not a url").ok).toBe(false); + }); + + it("rejects oversized input", () => { + expect(validateFetchUrl("https://" + "x".repeat(3000) + ".test").ok).toBe(false); + }); + + it("enforces the host allowlist when provided", () => { + const allowed = ["journal.trails.cool"]; + expect(validateFetchUrl("https://journal.trails.cool/x", { allowedHosts: allowed }).ok).toBe(true); + expect(validateFetchUrl("https://evil.example/x", { allowedHosts: allowed }).ok).toBe(false); + }); + + it("ignores the allowlist when it's empty/undefined", () => { + expect(validateFetchUrl("https://random.example/x").ok).toBe(true); + expect(validateFetchUrl("https://random.example/x", { allowedHosts: [] }).ok).toBe(true); + }); +}); + +describe("validateRedirectUrl", () => { + it("accepts an absolute https URL", () => { + expect(validateRedirectUrl("https://trails.cool/r/123").ok).toBe(true); + }); + + it("accepts a same-origin relative path", () => { + expect(validateRedirectUrl("/routes/abc").ok).toBe(true); + }); + + it("rejects javascript: scheme", () => { + expect(validateRedirectUrl("javascript:alert(1)").ok).toBe(false); + }); + + it("rejects protocol-relative //host URLs", () => { + // `` would resolve to https://evil.example + // when the page is on HTTPS. Explicitly reject to keep the + // "same-origin path" branch tight. + expect(validateRedirectUrl("//evil.example/x").ok).toBe(false); + }); +}); + +describe("getCallbackAllowedHosts", () => { + beforeEach(() => { + vi.unstubAllEnvs(); + }); + + it("returns undefined when the env is unset", () => { + delete process.env.PLANNER_CALLBACK_ALLOWED_HOSTS; + expect(getCallbackAllowedHosts()).toBeUndefined(); + }); + + it("splits, trims, and filters empty entries", () => { + vi.stubEnv("PLANNER_CALLBACK_ALLOWED_HOSTS", "a.test , b.test,, c.test "); + expect(getCallbackAllowedHosts()).toEqual(["a.test", "b.test", "c.test"]); + }); +}); diff --git a/apps/planner/app/lib/url-validation.server.ts b/apps/planner/app/lib/url-validation.server.ts new file mode 100644 index 0000000..84e2964 --- /dev/null +++ b/apps/planner/app/lib/url-validation.server.ts @@ -0,0 +1,91 @@ +// Shared validation for query-param URLs that flow into either an +// outbound fetch (callbackUrl) or a rendered link (returnUrl). +// Keeps the new-session and session-detail loaders honest without +// scattering ad-hoc string checks. + +const SAFE_SCHEMES = new Set(["http:", "https:"]); + +/** + * Result type so callers can decide whether to reject the request or + * just drop the value (e.g. returnUrl is optional UX; callbackUrl is + * load-bearing). + */ +export interface UrlValidationResult { + ok: boolean; + reason?: string; + url?: URL; +} + +/** + * Validate a URL string for use as a fetch target. Requires absolute + * http(s) URL. If `allowedHosts` is provided, the host must be on the + * list — matches behavior of standard "open redirect" allowlists. + */ +export function validateFetchUrl( + raw: string, + opts: { allowedHosts?: string[]; maxLength?: number } = {}, +): UrlValidationResult { + const maxLength = opts.maxLength ?? 2048; + if (raw.length > maxLength) { + return { ok: false, reason: `url exceeds ${maxLength} chars` }; + } + let parsed: URL; + try { + parsed = new URL(raw); + } catch { + return { ok: false, reason: "not a valid absolute URL" }; + } + if (!SAFE_SCHEMES.has(parsed.protocol)) { + return { ok: false, reason: `disallowed scheme ${parsed.protocol}` }; + } + if (opts.allowedHosts && opts.allowedHosts.length > 0) { + if (!opts.allowedHosts.includes(parsed.host)) { + return { ok: false, reason: `host ${parsed.host} not on allowlist` }; + } + } + return { ok: true, url: parsed }; +} + +/** + * Validate a URL string for use as a rendered ``. Accepts + * either a same-origin relative path (starts with `/` and doesn't + * start with `//`) or an absolute http(s) URL. The browser still + * follows the link, so the goal is to refuse `javascript:` and + * `data:` schemes which would execute on click. + */ +export function validateRedirectUrl( + raw: string, + opts: { maxLength?: number } = {}, +): UrlValidationResult { + const maxLength = opts.maxLength ?? 2048; + if (raw.length > maxLength) { + return { ok: false, reason: `url exceeds ${maxLength} chars` }; + } + // Same-origin relative path. `//host` would be protocol-relative and + // bypass the scheme check, so reject those explicitly. + if (raw.startsWith("/") && !raw.startsWith("//")) { + return { ok: true }; + } + let parsed: URL; + try { + parsed = new URL(raw); + } catch { + return { ok: false, reason: "not a valid absolute or relative URL" }; + } + if (!SAFE_SCHEMES.has(parsed.protocol)) { + return { ok: false, reason: `disallowed scheme ${parsed.protocol}` }; + } + return { ok: true, url: parsed }; +} + +/** + * Parse the `PLANNER_CALLBACK_ALLOWED_HOSTS` env (comma-separated + * hostnames). When set, callbackUrl hosts must match. When unset, + * no allowlist is applied — useful in dev/self-hosted where the + * journal lives somewhere unpredictable. + */ +export function getCallbackAllowedHosts(): string[] | undefined { + const raw = process.env.PLANNER_CALLBACK_ALLOWED_HOSTS; + if (!raw) return undefined; + return raw.split(",").map((s) => s.trim()).filter(Boolean); +} diff --git a/apps/planner/app/routes/new.tsx b/apps/planner/app/routes/new.tsx index 121e0f6..739b6d9 100644 --- a/apps/planner/app/routes/new.tsx +++ b/apps/planner/app/routes/new.tsx @@ -3,6 +3,16 @@ import type { Route } from "./+types/new"; import { createSession, initializeSessionWithWaypoints, initializeSessionWithNoGoAreas } from "~/lib/sessions"; import { parseGpxAsync, extractWaypoints } from "@trails-cool/gpx"; import { checkRateLimit } from "~/lib/rate-limit"; +import { + validateFetchUrl, + validateRedirectUrl, + getCallbackAllowedHosts, +} from "~/lib/url-validation.server"; + +// Query-param hard caps so an attacker can't shovel multi-MB strings +// into our URL parser / session-storage path. +const MAX_GPX_QUERY_LEN = 2_000_000; // GPX intentionally allowed to be large (full route) +const MAX_URL_PARAM_LEN = 2048; export async function loader({ request }: Route.LoaderArgs) { const url = new URL(request.url); @@ -25,6 +35,31 @@ export async function loader({ request }: Route.LoaderArgs) { const returnUrl = url.searchParams.get("returnUrl"); const gpxEncoded = url.searchParams.get("gpx"); + // Validate the callback URL — it will be used as a fetch target on + // save-to-journal. Reject non-http(s) schemes (no `file:` / `javascript:`), + // and if PLANNER_CALLBACK_ALLOWED_HOSTS is set, restrict to those hosts. + if (callbackUrl) { + const v = validateFetchUrl(callbackUrl, { + allowedHosts: getCallbackAllowedHosts(), + maxLength: MAX_URL_PARAM_LEN, + }); + if (!v.ok) { + throw data({ error: `Invalid callback URL: ${v.reason}` }, { status: 400 }); + } + } + if (token && token.length > MAX_URL_PARAM_LEN) { + throw data({ error: "callback token too long" }, { status: 400 }); + } + if (returnUrl) { + const v = validateRedirectUrl(returnUrl, { maxLength: MAX_URL_PARAM_LEN }); + if (!v.ok) { + throw data({ error: `Invalid returnUrl: ${v.reason}` }, { status: 400 }); + } + } + if (gpxEncoded && gpxEncoded.length > MAX_GPX_QUERY_LEN) { + throw data({ error: "GPX payload too large" }, { status: 413 }); + } + // Create a session with callback info const session = await createSession({ callbackUrl: callbackUrl ?? undefined, diff --git a/apps/planner/app/routes/session.$id.tsx b/apps/planner/app/routes/session.$id.tsx index fdcf518..99979e7 100644 --- a/apps/planner/app/routes/session.$id.tsx +++ b/apps/planner/app/routes/session.$id.tsx @@ -28,21 +28,47 @@ export async function loader({ params }: Route.LoaderArgs) { }); } +// URL-param caps prevent a hostile link from feeding multi-MB JSON +// into JSON.parse on the client (and from there into the Yjs doc). +// Picked generously enough to accept realistic multi-day routes +// (~hundreds of waypoints) but small enough to refuse abuse. +const MAX_WAYPOINTS_LEN = 50_000; +const MAX_NOGO_LEN = 50_000; +const MAX_NOTES_LEN = 10_000; +const MAX_RETURN_URL_LEN = 2048; + +function safeParseJson(raw: string, maxLen: number): T | undefined { + if (raw.length > maxLen) return undefined; + try { return JSON.parse(raw) as T; } catch { return undefined; } +} + +function safeReturnUrl(raw: string | null): string | undefined { + if (!raw || raw.length > MAX_RETURN_URL_LEN) return undefined; + // Same-origin relative path or http(s) absolute URL only. Rejects + // `javascript:` and protocol-relative `//host` so the value is safe + // to drop into an ``. + if (raw.startsWith("/") && !raw.startsWith("//")) return raw; + try { + const parsed = new URL(raw); + if (parsed.protocol === "http:" || parsed.protocol === "https:") return raw; + } catch { /* fall through */ } + return undefined; +} + export default function SessionPage({ loaderData }: Route.ComponentProps) { const { id } = useParams(); const [searchParams] = useSearchParams(); - const returnUrl = searchParams.get("returnUrl") ?? undefined; + const returnUrl = safeReturnUrl(searchParams.get("returnUrl")); const waypointsParam = searchParams.get("waypoints"); - let initialWaypoints: Array<{ lat: number; lon: number; name?: string; isDayBreak?: boolean }> | undefined; - if (waypointsParam) { - try { initialWaypoints = JSON.parse(waypointsParam); } catch { /* ignore */ } - } + const initialWaypoints = waypointsParam + ? safeParseJson>(waypointsParam, MAX_WAYPOINTS_LEN) + : undefined; const noGoParam = searchParams.get("noGoAreas"); - let initialNoGoAreas: Array<{ points: Array<{ lat: number; lon: number }> }> | undefined; - if (noGoParam) { - try { initialNoGoAreas = JSON.parse(noGoParam); } catch { /* ignore */ } - } - const initialNotes = searchParams.get("notes") ?? undefined; + const initialNoGoAreas = noGoParam + ? safeParseJson }>>(noGoParam, MAX_NOGO_LEN) + : undefined; + const notesRaw = searchParams.get("notes"); + const initialNotes = notesRaw && notesRaw.length <= MAX_NOTES_LEN ? notesRaw : undefined; return (