From 195394a7833827a0c5334dea15d0e0ae6fa85652 Mon Sep 17 00:00:00 2001 From: Timur Mashara <62305878+tmashara@users.noreply.github.com> Date: Mon, 2 Nov 2026 13:59:05 +0100 Subject: [PATCH] Config: reject unknown keys, bad widths, non-http origins, blank selectors Unknown top-level and masks[i] keys exit 2 with the valid keys listed, so a `urlpath` typo no longer turns a scoped mask global. The key lists are type-checked against RenderdiffConfig and Mask. widths must be a non-empty array of positive integers. The resolved left/right (config or flags) must be http(s) URLs, and probe uses the same check. hideSelectors entries and mask selectors must be non-blank. Co-Authored-By: Claude Opus 5.5 --- src/cli.ts | 3 +- src/config.ts | 57 +++++++++++++++++++++++++++++++--- tests/unit/cli-args.test.ts | 9 ++++++ tests/unit/config-load.test.ts | 19 ++++++++++-- 4 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index bd8686a..ce473ef 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -16,6 +16,7 @@ import { ACCEPTED_FILE } from "./accept.ts"; import type { Results } from "./report.ts"; import { alive, + isHttpUrl, label, loadConfig, name, @@ -509,7 +510,7 @@ async function probe(a: Args, loads: number): Promise { if ((a.widths?.length ?? 0) > 1) die("probe takes one --width"); if (a.selector) die("probe does not take --selector; it walks the whole page"); if (a.only) die("probe does not take --only"); - if (!URL.canParse(a.left)) die(`not a URL: ${a.left}`); + if (!isHttpUrl(a.left)) die(`not an http(s) URL: ${a.left}`); const u = new URL(a.left); let route: Route; try { diff --git a/src/config.ts b/src/config.ts index 3b8f313..fca9645 100644 --- a/src/config.ts +++ b/src/config.ts @@ -390,6 +390,37 @@ const CONFIG_NAMES = [ const isStrArray = (v: unknown): v is string[] => Array.isArray(v) && v.every((x) => typeof x === "string"); +export const isHttpUrl = (s: string) => URL.canParse(s) && /^https?:$/.test(new URL(s).protocol); + +type KeysOf = T extends unknown ? keyof T : never; +const CONFIG_KEYS = Object.keys({ + left: 0, + right: 0, + routes: 0, + before: 0, + beforeShot: 0, + storageState: 0, + widths: 0, + threshold: 0, + outDir: 0, + concurrency: 0, + blockUrlPatterns: 0, + hideSelectors: 0, + extraCss: 0, + masks: 0, + computed: 0, + aria: 0, + maskMedia: 0, + freezeVideo: 0, + lottieFrame: 0, +} satisfies Record); +const MASK_KEYS = Object.keys({ + selector: 0, + locate: 0, + reason: 0, + urlPath: 0, +} satisfies Record, 0>); + /** * Load a config file. An explicit `--config` path must exist; otherwise cwd is probed * and finding nothing is fine — the CLI works with flags alone. @@ -447,6 +478,11 @@ function validate(c: Record, file: string) { throw new Error(`${file}: \`${key}\` must be ${expected}`); }; + const unknown = Object.keys(c).find((k) => !CONFIG_KEYS.includes(k)); + if (unknown) { + throw new Error(`${file}: \`${unknown}\` is not a config key (${CONFIG_KEYS.join(", ")})`); + } + for (const key of ["left", "right", "outDir", "extraCss"]) { if (c[key] !== undefined && typeof c[key] !== "string") bad(key, "a string"); } @@ -455,6 +491,9 @@ function validate(c: Record, file: string) { bad(key, "an array of strings"); } } + if ((c.hideSelectors as string[] | undefined)?.some((s) => !s.trim())) { + bad("hideSelectors", "an array of non-empty strings"); + } const hook = "a function (page, { side, route, width }) => Promise"; if (c.before !== undefined && typeof c.before !== "function") bad("before", hook); if (c.beforeShot !== undefined && typeof c.beforeShot !== "function") bad("beforeShot", hook); @@ -517,9 +556,10 @@ function validate(c: Record, file: string) { if (c.widths !== undefined) { if ( !Array.isArray(c.widths) || - !c.widths.every((w) => typeof w === "number" && Number.isFinite(w) && w > 0) + !c.widths.length || + !c.widths.every((w) => Number.isInteger(w) && w > 0) ) { - bad("widths", "an array of positive numbers"); + bad("widths", "a non-empty array of positive integers"); } } if (c.computed !== undefined && typeof c.computed !== "boolean") bad("computed", "a boolean"); @@ -544,6 +584,12 @@ function validate(c: Record, file: string) { bad(`masks[${i}]`, "an object"); return; } + const unknown = Object.keys(o).find((k) => !MASK_KEYS.includes(k)); + if (unknown) { + throw new Error( + `${file}: \`masks[${i}].${unknown}\` is not a mask key (${MASK_KEYS.join(", ")})`, + ); + } if (o.urlPath !== undefined && typeof o.urlPath !== "string") { bad(`masks[${i}].urlPath`, "a string"); } @@ -551,8 +597,8 @@ function validate(c: Record, file: string) { if ("selector" in o === "locate" in o) { throw new Error(`${file}: \`masks[${i}]\` needs exactly one of \`selector\` or \`locate\``); } - if (o.selector !== undefined && typeof o.selector !== "string") { - bad(`masks[${i}].selector`, "a string"); + if (o.selector !== undefined && (typeof o.selector !== "string" || !o.selector.trim())) { + bad(`masks[${i}].selector`, "a non-empty string"); } if (o.locate !== undefined && typeof o.locate !== "function") { bad(`masks[${i}].locate`, "a function (page) => Locator"); @@ -604,6 +650,9 @@ export function resolveConfig(file: RenderdiffConfig, cli: CliOverrides): Resolv : "right origin is required (second positional URL, --right, or right in renderdiff.config.*)", ); } + for (const [side, url] of Object.entries({ left, right })) { + if (!isHttpUrl(url)) throw new Error(`${side} origin must be an http(s) URL, got ${url}`); + } const fileState = typeof file.storageState === "string" diff --git a/tests/unit/cli-args.test.ts b/tests/unit/cli-args.test.ts index 295d3fe..0933a4d 100644 --- a/tests/unit/cli-args.test.ts +++ b/tests/unit/cli-args.test.ts @@ -128,6 +128,12 @@ describe("required arguments", () => { test("a missing --left is named, in the singular", () => rejects(["/a", "--right", R, "--dry-run"], "left origin is required (first positional URL")); + test("an origin without a scheme is refused", () => + rejects( + ["/a", "--left", "localhost:3000", "--right", R, "--dry-run"], + "left origin must be an http(s) URL, got localhost:3000", + )); + test("a missing --right is named", () => rejects(["/a", "--left", L, "--dry-run"], "right origin is required (second positional URL")); @@ -463,6 +469,9 @@ describe("probe", () => { await rejects(["probe", U, "/b"], "probe takes one URL"); }); + test("refuses a URL without a scheme", () => + rejects(["probe", "--left", "localhost:3000"], "not an http(s) URL: localhost:3000")); + test("refuses --only", () => rejects(["probe", U, "--only", "/a"], "probe does not take --only")); test("--loads is an integer of at least 2", async () => { diff --git a/tests/unit/config-load.test.ts b/tests/unit/config-load.test.ts index 541f295..c319340 100644 --- a/tests/unit/config-load.test.ts +++ b/tests/unit/config-load.test.ts @@ -112,7 +112,7 @@ describe("validation", () => { "bad-routes.mjs", /`routes\[0\]` must be a string or an object \{ left, right\?, selector\?, before\?, beforeShot\? \}/, ], - ["bad-widths.mjs", /`widths` must be an array of positive numbers/], + ["bad-widths.mjs", /`widths` must be a non-empty array of positive integers/], ["bad-threshold.mjs", /`threshold` must be a number between 0 and 1/], ["bad-concurrency.mjs", /`concurrency` must be a positive integer/], ["bad-mask-shape.mjs", /`masks` must be an array/], @@ -153,7 +153,22 @@ describe("validation", () => { '{ masks: [{ reason: "x", selector: "a", locate: (p) => p.locator("a") }] }', /`masks\[0\]` needs exactly one of `selector` or `locate`/, ], - ['{ masks: [{ reason: "x", selector: 1 }] }', /`masks\[0\]\.selector` must be a string/], + [ + '{ masks: [{ reason: "x", selector: 1 }] }', + /`masks\[0\]\.selector` must be a non-empty string/, + ], + [ + '{ masks: [{ reason: "x", selector: " " }] }', + /`masks\[0\]\.selector` must be a non-empty string/, + ], + [ + '{ masks: [{ reason: "x", selector: "a", urlpath: "/a" }] }', + /`masks\[0\]\.urlpath` is not a mask key \(selector, locate, reason, urlPath\)/, + ], + ['{ lfet: "http://x.test" }', /`lfet` is not a config key \(left, right, routes,/], + ['{ hideSelectors: [" "] }', /`hideSelectors` must be an array of non-empty strings/], + ["{ widths: [] }", /`widths` must be a non-empty array of positive integers/], + ["{ widths: [375.5] }", /`widths` must be a non-empty array of positive integers/], [ '{ routes: [{ left: "/a", selector: 1 }] }', /`routes\[0\]\.selector` must be a non-empty string or \{ left, right \}/,