fix(planner): validate callback/returnUrl + cap session URL-param payloads
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) <noreply@anthropic.com>
This commit is contained in:
parent
e35a8e27c8
commit
51e6b8a0d7
4 changed files with 244 additions and 10 deletions
82
apps/planner/app/lib/url-validation.server.test.ts
Normal file
82
apps/planner/app/lib/url-validation.server.test.ts
Normal file
|
|
@ -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", () => {
|
||||
// `<a href="//evil.example">` 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"]);
|
||||
});
|
||||
});
|
||||
91
apps/planner/app/lib/url-validation.server.ts
Normal file
91
apps/planner/app/lib/url-validation.server.ts
Normal file
|
|
@ -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 `<a href>`. 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);
|
||||
}
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<T>(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 `<a href>`.
|
||||
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<Array<{ lat: number; lon: number; name?: string; isDayBreak?: boolean }>>(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<Array<{ points: Array<{ lat: number; lon: number }> }>>(noGoParam, MAX_NOGO_LEN)
|
||||
: undefined;
|
||||
const notesRaw = searchParams.get("notes");
|
||||
const initialNotes = notesRaw && notesRaw.length <= MAX_NOTES_LEN ? notesRaw : undefined;
|
||||
|
||||
return (
|
||||
<div className="flex h-full flex-col">
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue