From c15218e655a37ae87ff468174ae996f50f1956a5 Mon Sep 17 00:00:00 2001 From: Madraka <8898640+Madraka@users.noreply.github.com> Date: Sun, 8 Nov 2026 21:28:04 +0300 Subject: [PATCH 1/3] fix(security): second ReDoS sweep (#174) - api: the reset-request and invite address check is linear and checks the length first (isEmailAddress); public token endpoints refuse JSON bodies over 64 KiB with 413, streamed or not. - core-ts: linear redirect path trims and ancestorPaths below the first `//`; searchText fences, links, images and tags; SVG CSS comments, url() values and lengths (css-scan.ts). - schedules: linear CDATA, element and media url scanners (xml-scan.ts) and urlPath. - agent-runtime: linear SKILL.md frontmatter line parsers. - ui, docs: linear newline trims in the editor; isPagePath by lastIndexOf. - AGENTS.md rule 13 and a CONTRIBUTING note on regexes over external input. Each rewrite has an equivalence test against the old regex and a 100k-character adversarial case. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Madraka <8898640+Madraka@users.noreply.github.com> --- .changeset/redos-sweep-174.md | 15 ++ AGENTS.md | 1 + CONTRIBUTING.md | 1 + apps/docs/src/worker/paths.ts | 8 +- apps/docs/test/paths.test.ts | 28 +++ .../agent-runtime/src/frontmatter-lines.ts | 42 +++++ packages/agent-runtime/src/skills.ts | 9 +- .../test/frontmatter-lines.test.ts | 49 ++++++ packages/api/src/email.ts | 21 +++ packages/api/src/routes/invites.ts | 5 +- packages/api/src/routes/password-reset.ts | 4 +- packages/api/src/tokens.ts | 53 +++++- packages/api/test/public-input.test.ts | 133 ++++++++++++++ packages/core-ts/src/media/css-scan.ts | 84 +++++++++ packages/core-ts/src/media/svg.ts | 18 +- packages/core-ts/src/redirects.ts | 16 +- packages/core-ts/src/search-rows.ts | 15 +- packages/core-ts/src/search-text-scan.ts | 119 +++++++++++++ packages/core-ts/test/redos-174.test.ts | 164 ++++++++++++++++++ packages/schedules/src/feed-check.ts | 37 ++-- packages/schedules/src/xml-scan.ts | 158 +++++++++++++++++ packages/schedules/test/xml-scan.test.ts | 94 ++++++++++ .../ui/src/blocks/editor/markdown-blocks.ts | 6 +- .../ui/test/markdown-blocks-redos.test.ts | 28 +++ 24 files changed, 1047 insertions(+), 61 deletions(-) create mode 100644 .changeset/redos-sweep-174.md create mode 100644 apps/docs/test/paths.test.ts create mode 100644 packages/agent-runtime/src/frontmatter-lines.ts create mode 100644 packages/agent-runtime/test/frontmatter-lines.test.ts create mode 100644 packages/api/src/email.ts create mode 100644 packages/api/test/public-input.test.ts create mode 100644 packages/core-ts/src/media/css-scan.ts create mode 100644 packages/core-ts/src/search-text-scan.ts create mode 100644 packages/core-ts/test/redos-174.test.ts create mode 100644 packages/schedules/src/xml-scan.ts create mode 100644 packages/schedules/test/xml-scan.test.ts create mode 100644 packages/ui/test/markdown-blocks-redos.test.ts diff --git a/.changeset/redos-sweep-174.md b/.changeset/redos-sweep-174.md new file mode 100644 index 00000000..d1371743 --- /dev/null +++ b/.changeset/redos-sweep-174.md @@ -0,0 +1,15 @@ +--- +"@cloudhal/api": patch +"@cloudhal/core-ts": patch +"@cloudhal/schedules": patch +"@cloudhal/agent-runtime": patch +"@cloudhal/cli": patch +--- + +Second ReDoS sweep (#174). Input that made a regex backtrack is now handled in linear time; results are unchanged. + +- **Unauthenticated password-reset request.** `POST /api/password-reset/request` (and invite creation) checks the address length before anything else, and the address check no longer backtracks: `a@` followed by many dots used to cost seconds of CPU per request. Public token endpoints (`/api/password-reset/*`, invite acceptance) refuse a JSON body over 64 KiB with `413 payload_too_large`, whether or not it declares a `content-length`. +- **core-ts.** Redirect path normalising and matching (on every site request): `normalisePath`, `joinTarget`, and `ancestorPaths`, which no longer lists ancestors holding `//` that no redirect can match. Search text built at publish time: fenced code, links, images and tags. SVG upload checks: CSS comments, `url(…)` references (about cubic on `url(` followed by spaces) and width/height lengths (new `css-scan.ts`). +- **schedules.** The feed check's CDATA, element and ``/`` url scans, and `urlPath`. +- **agent-runtime.** SKILL.md frontmatter lines (a line of spaces was quadratic). +- **Studio** (in `@cloudhal/cli`): the markdown body editor's trailing-newline trims. diff --git a/AGENTS.md b/AGENTS.md index 04ec7fc0..eb10029d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -56,6 +56,7 @@ pnpm i18n:export / pnpm i18n:import [--locale xx] # translations as nested ICU 10. **UI strings** go through `useT()` with keys in `en.ts` (only English is edited by hand; every other `.ts` is generated by `pnpm i18n:import`, ADR-0034) and every language's `.ts` (or every file of a lazy namespace, read with `useT("")`; languages are registered in `apps/studio/src/i18n/locales.ts`); no hard-coded words in `apps/studio/src` or `packages/ui/src` — visible text, `aria-label`/`title`/`placeholder`/`alt`, toasts, banners, label tables and block defaults (blocks take every word as a prop, no English fallback; API errors through `apiErrorText`). Enforced by the `i18n`, `i18n-plural`, `i18n-concat` and `i18n-format` rules (and `rtl-logical` for physical CSS utilities, ADR-0033) of `pnpm design:lint` (`apps/studio/scripts/i18n-lint.ts`, run in `pnpm check` by `test/design-rules.test.ts` and on every agent edit); legitimate literals go in its `ALLOWED` list or carry an `i18n-ignore: ` comment. 11. Conventional commits, `git commit -s` (DCO). Changeset for published-package changes. 12. **Size ratchet** ([docs/engineering/code-structure.md](docs/engineering/code-structure.md)): files ≤ 800 lines (soft 400; `.astro` 500, tests 1500; i18n dictionaries are data and exempt — the Studio bundle budget guards them), functions ≤ 150 lines, depth ≤ 5, ≤ 4 positional params (use an options object beyond 3); one responsibility per module, dispatch tables instead of giant switches. `pnpm size:check` (in `pnpm check`) fails on a new violation or when a file in `scripts/size-baseline.json` grows — put new code in a new module next to it; after shrinking one, run `node scripts/size-check.ts --update`. +13. **Regexes over external input** (request paths and bodies, agent or editor markdown, uploads, fetched feeds) must be linear (#168, #174): no unanchored or end-anchored quantified runs (`/-+$/`, `/\s+$/`, `/\/+$/`), no lazy scans that rescan to the end when the closer is missing (`/[\s\S]*?<\/x>/`, `/<[^>]*>/`), no adjacent quantifiers over overlapping classes (`\s+[^:]+`, `\s*(.*)`). Use `trimChar`/`trimEndChar` (`@cloudhal/core-ts/names`), `trimEnd()`, `indexOf`/`lastIndexOf` scanners, and check lengths before patterns. A rewrite gets an equivalence test against the old regex and a 100k-character adversarial case. ## Versions (pinned via pnpm catalog / Cargo workspace) Node 24 · pnpm 12 · TypeScript 7.0 · Biome 2.6 · Vitest 5 · Turborepo 2.11 · Rust 2024 edition · json-schema-to-typescript 16.0.0 · cargo-typify 0.8.0 · ajv 8.20 · jsonschema 0.60 · better-sqlite3 13 · miniflare 4.20260730 (last stable 4.x; its sharp overridden to 0.35.5; see ADR-0012) · wrangler 4.158 · @astrojs/cloudflare 14.4 · Astro 7.4 · Tailwind 4.3 · marked 18 · Hono 4.13 · Better Auth 1.7.12 · Kumo 2.14 · React 19.3 · TanStack Router 1.170 / Query 5.104 · Vite 8.3. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6496455f..898352cf 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -40,6 +40,7 @@ Commit the regenerated files; CI fails on drift. Add a valid and an invalid fixt - Add a changeset (`pnpm changeset`) when a published package changes behaviour. - A PR that changes what the renderer gives themes freezes the last released starter in the same PR: copy it under `packages/cli/test/fixtures/theme-v/` and add that version to `FROZEN` in `packages/cli/test/old-theme-compat.test.ts` (see `theme-v0.5.0/README.md` there), so the release that ships the change is already tested against the theme existing sites have. - Keep modules small and single-purpose: [`docs/engineering/code-structure.md`](docs/engineering/code-structure.md) sets file/function limits that `pnpm size:check` enforces with a shrink-only baseline. +- Regexes over input from outside (requests, agent or editor content, uploads, feeds) must not backtrack: prefer `indexOf` scanners and the linear trims in `@cloudhal/core-ts/names`, check lengths first, and test a rewrite against the old regex plus a 100k-character adversarial input (AGENTS.md rule 13). - CI must be green: lint, size check, typecheck, tests (TS + Rust), contract drift. ## Releasing diff --git a/apps/docs/src/worker/paths.ts b/apps/docs/src/worker/paths.ts index fa4e7c9b..03a51195 100644 --- a/apps/docs/src/worker/paths.ts +++ b/apps/docs/src/worker/paths.ts @@ -38,5 +38,9 @@ export function pageId(input: string, selfHost?: string): string | undefined { export const markdownPath = (id: string) => (id === "index" ? "/index.md" : `/${id}.md`); -/** A path that may be a page: no file extension in its last segment. */ -export const isPagePath = (pathname: string) => !/\.[^/]*$/.test(pathname); +/** + * A path that may be a page: no file extension in its last segment, that is no "." after the last + * "/". Two `lastIndexOf`s, not `/\.[^/]*$/`, which backtracks on a long run of dots (#174). + */ +export const isPagePath = (pathname: string) => + pathname.lastIndexOf(".") <= pathname.lastIndexOf("/"); diff --git a/apps/docs/test/paths.test.ts b/apps/docs/test/paths.test.ts new file mode 100644 index 00000000..bd8c6cc9 --- /dev/null +++ b/apps/docs/test/paths.test.ts @@ -0,0 +1,28 @@ +/** #174 item 3: `isPagePath` answers as `/\.[^/]*$/` did, in linear time. */ +import { describe, expect, it } from "vitest"; +import { isPagePath } from "../src/worker/paths.ts"; + +describe("isPagePath (#174)", () => { + it("answers as the regex did", () => { + const old = (p: string) => !/\.[^/]*$/.test(p); + let s = 174; + const next = () => { + s = (s * 1103515245 + 12345) & 0x7fffffff; + return s; + }; + for (let n = 0; n < 5000; n++) { + let p = ""; + for (let i = next() % 10; i > 0; i--) p += ["/", ".", "a", "md"][next() % 4]; + expect(isPagePath(p), p).toBe(old(p)); + } + expect(isPagePath("/guides/search")).toBe(true); + expect(isPagePath("/llms.txt")).toBe(false); + expect(isPagePath("/v0.6/guide")).toBe(true); + }); + + it("stays linear on a long run of dots", () => { + const start = performance.now(); + expect(isPagePath(`/${".".repeat(100_000)}/`)).toBe(true); + expect(performance.now() - start).toBeLessThan(100); + }); +}); diff --git a/packages/agent-runtime/src/frontmatter-lines.ts b/packages/agent-runtime/src/frontmatter-lines.ts new file mode 100644 index 00000000..efb1caaa --- /dev/null +++ b/packages/agent-runtime/src/frontmatter-lines.ts @@ -0,0 +1,42 @@ +/** + * The two line shapes of SKILL.md frontmatter, matched in one linear pass each (#174). They give + * the groups `/^\s+([^:]+):\s*(.*)$/` and `/^([A-Za-z][\w-]*):\s*(.*)$/` gave, including where + * those regexes backtracked: the first one's `\s+` and `[^:]+` both take whitespace, which made a + * line of spaces quadratic, and each `\s*` overlapped `(.*)` before a lone `\r`. + */ + +const isSpace = (ch: string | undefined) => ch !== undefined && /\s/.test(ch); +/** What `.` does not match without the `s` flag. */ +const LINE_END = /[\n\r\u2028\u2029]/; + +/** After the `:` at `colon`: whitespace skipped; undefined when the rest holds a line end. */ +function valueAfter(raw: string, colon: number): string | undefined { + let i = colon + 1; + while (isSpace(raw[i])) i++; + const rest = raw.slice(i); + return LINE_END.test(rest) ? undefined : rest; +} + +/** An indented `key: value` line (a nested map entry): `[key, value]`, or undefined. */ +export function nestedLine(raw: string): [string, string] | undefined { + let lead = 0; + while (isSpace(raw[lead])) lead++; + if (lead === 0) return undefined; + const colon = raw.indexOf(":"); + if (colon < 0) return undefined; + // The key needs a character: when the indent runs up to the `:`, its last space is the key. + const start = lead < colon ? lead : lead >= 2 ? lead - 1 : -1; + if (start < 0) return undefined; + const value = valueAfter(raw, colon); + return value === undefined ? undefined : [raw.slice(start, colon), value]; +} + +/** A top-level `key: value` line, the key a letter then letters, digits, `_` or `-`. */ +export function topLine(raw: string): [string, string] | undefined { + if (!/^[A-Za-z]/.test(raw)) return undefined; + let j = 1; + while (j < raw.length && /[\w-]/.test(raw[j] as string)) j++; + if (raw[j] !== ":") return undefined; + const value = valueAfter(raw, j); + return value === undefined ? undefined : [raw.slice(0, j), value]; +} diff --git a/packages/agent-runtime/src/skills.ts b/packages/agent-runtime/src/skills.ts index a78b109f..623a7a48 100644 --- a/packages/agent-runtime/src/skills.ts +++ b/packages/agent-runtime/src/skills.ts @@ -1,6 +1,7 @@ import { readdir, readFile, stat } from "node:fs/promises"; import path from "node:path"; import { assertValid, type SkillManifest } from "@cloudhal/core-contracts"; +import { nestedLine, topLine } from "./frontmatter-lines.ts"; export interface LoadedSkill { manifest: SkillManifest; @@ -36,14 +37,14 @@ export function parseFrontmatter(markdown: string): { let currentMap: Record | undefined; for (const raw of (m[1] as string).split(/\r?\n/)) { if (!raw.trim() || raw.trim().startsWith("#")) continue; - const nested = raw.match(/^\s+([^:]+):\s*(.*)$/); + const nested = nestedLine(raw); if (nested && currentMap) { - currentMap[nested[1] as string] = unquote((nested[2] as string).trim()); + currentMap[nested[0]] = unquote(nested[1].trim()); continue; } - const top = raw.match(/^([A-Za-z][\w-]*):\s*(.*)$/); + const top = topLine(raw); if (!top) throw new SkillError(`invalid frontmatter line: ${raw}`); - const [, key, value] = top as unknown as [string, string, string]; + const [key, value] = top; if (value.trim() === "") { currentMap = {}; fm[key] = currentMap; diff --git a/packages/agent-runtime/test/frontmatter-lines.test.ts b/packages/agent-runtime/test/frontmatter-lines.test.ts new file mode 100644 index 00000000..b26d0bcd --- /dev/null +++ b/packages/agent-runtime/test/frontmatter-lines.test.ts @@ -0,0 +1,49 @@ +/** + * #174 item 5: the frontmatter line parsers give the regexes' groups (seeded random lines over the + * characters that matter) and stay linear on 100k-character lines that made them backtrack. + */ +import { describe, expect, it } from "vitest"; +import { nestedLine, topLine } from "../src/frontmatter-lines.ts"; +import { parseFrontmatter } from "../src/skills.ts"; + +function strings(alphabet: readonly string[], count: number, maxLength: number, seed = 174) { + let s = seed; + const next = () => { + s = (s * 1103515245 + 12345) & 0x7fffffff; + return s; + }; + const out: string[] = []; + for (let n = 0; n < count; n++) { + let v = ""; + const len = next() % (maxLength + 1); + for (let i = 0; i < len; i++) v += alphabet[next() % alphabet.length]; + out.push(v); + } + return out; +} + +const groups = (m: RegExpMatchArray | null) => (m ? [m[1], m[2]] : undefined); + +describe("frontmatter lines (#174 item 5)", () => { + it("match exactly what the regexes matched", () => { + const parts = [" ", " ", "\t", ":", "a", "B", "-", "_", "1", "\r", "\u2028", "'"]; + for (const raw of strings(parts, 10_000, 9)) { + expect(nestedLine(raw), JSON.stringify(raw)).toEqual( + groups(raw.match(/^\s+([^:]+):\s*(.*)$/)), + ); + expect(topLine(raw), JSON.stringify(raw)).toEqual( + groups(raw.match(/^([A-Za-z][\w-]*):\s*(.*)$/)), + ); + } + }); + + it("stays linear on lines that made the regexes backtrack", () => { + const start = performance.now(); + expect(nestedLine(" ".repeat(100_000))).toBeUndefined(); + expect(nestedLine(` key:${" ".repeat(100_000)}x\r`)).toBeUndefined(); + expect(topLine(`name:${" ".repeat(100_000)}x\r`)).toBeUndefined(); + const fm = `---\nmetadata:\n${" ".repeat(100_000)}\n---\nbody`; + expect(() => parseFrontmatter(fm)).not.toThrow(); + expect(performance.now() - start).toBeLessThan(100); + }); +}); diff --git a/packages/api/src/email.ts b/packages/api/src/email.ts new file mode 100644 index 00000000..be2905bd --- /dev/null +++ b/packages/api/src/email.ts @@ -0,0 +1,21 @@ +/** + * The address check of the invite and password-reset routes (#174). It accepts exactly what + * `/^[^\s@]+@[^\s@]+\.[^\s@]+$/` accepted, in one linear pass, and checks the length first. That + * regex backtracked quadratically on `a@` followed by many dots (the domain part is ambiguous on + * "."), and it ran before the length cap on the unauthenticated `POST /api/password-reset/request`. + */ + +/** RFC 5321's limit on a forward path: longer is never an address. */ +export const EMAIL_MAX_LENGTH = 254; + +/** + * One `@` with something before it; after it, no `@` and a `.` with something on both sides; no + * whitespace anywhere; at most `EMAIL_MAX_LENGTH` characters. + */ +export function isEmailAddress(value: string): boolean { + if (value.length > EMAIL_MAX_LENGTH || /\s/.test(value)) return false; + const at = value.indexOf("@"); + if (at < 1 || value.indexOf("@", at + 1) !== -1) return false; + // `[^\s@]+\.[^\s@]+`: a dot that is neither the domain's first nor its last character. + return value.slice(at + 2, -1).includes("."); +} diff --git a/packages/api/src/routes/invites.ts b/packages/api/src/routes/invites.ts index 53a3618d..ae66b269 100644 --- a/packages/api/src/routes/invites.ts +++ b/packages/api/src/routes/invites.ts @@ -29,6 +29,7 @@ import type { Actor } from "@cloudhal/core-contracts"; import { type Context, Hono } from "hono"; import { actorIdForUser, type StaffService } from "../auth.ts"; import type { Env } from "../context.ts"; +import { isEmailAddress } from "../email.ts"; import { apiError, badRequest, notFound } from "../errors.ts"; import { MailError } from "../mail.ts"; import { inviteMail } from "../mail-messages.ts"; @@ -82,8 +83,6 @@ export function inviteView(i: InviteRow): InviteView { }; } -export const EMAIL = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; - function gone() { return apiError( 410, @@ -210,7 +209,7 @@ export function inviteRoutes(options: { const { person, actor, staff } = me(c); const body = ((await c.req.json().catch(() => ({}))) ?? {}) as Record; const email = typeof body.email === "string" ? body.email.trim().toLowerCase() : ""; - if (!EMAIL.test(email) || email.length > 254) throw badRequest("a valid email is required"); + if (!isEmailAddress(email)) throw badRequest("a valid email is required"); const { role, sites } = await inviteTarget(c, person, body); const hours = body.expiresInHours === undefined ? inviteDefaultHours(role) : body.expiresInHours; diff --git a/packages/api/src/routes/password-reset.ts b/packages/api/src/routes/password-reset.ts index ea468b26..a7895bb5 100644 --- a/packages/api/src/routes/password-reset.ts +++ b/packages/api/src/routes/password-reset.ts @@ -18,6 +18,7 @@ import type { Actor } from "@cloudhal/core-contracts"; import { type Context, Hono } from "hono"; import { actorIdForUser, type StaffService } from "../auth.ts"; import type { Env } from "../context.ts"; +import { isEmailAddress } from "../email.ts"; import { apiError, badRequest, notFound } from "../errors.ts"; import { MailError, type MailSender } from "../mail.ts"; import { passwordChangedMail, resetMail } from "../mail-messages.ts"; @@ -30,7 +31,6 @@ import { RESET_TOKEN_PREFIX, studioOrigin, } from "../tokens.ts"; -import { EMAIL } from "./invites.ts"; import { auditEvent, RESET_LINK_MINUTES } from "./members.ts"; /** RFC-0011: reset mails one address can receive per hour, whoever asks. */ @@ -120,7 +120,7 @@ export function passwordResetRoutes(options: { await limitPublic(c, "reset-request"); const raw = (await jsonBody(c)).email; const email = typeof raw === "string" ? raw.trim().toLowerCase() : ""; - if (!EMAIL.test(email) || email.length > 254) throw badRequest("a valid email is required"); + if (!isEmailAddress(email)) throw badRequest("a valid email is required"); c.get("defer")( mailResetLink({ staff, diff --git a/packages/api/src/tokens.ts b/packages/api/src/tokens.ts index 0f325ce7..2eaca5cb 100644 --- a/packages/api/src/tokens.ts +++ b/packages/api/src/tokens.ts @@ -37,7 +37,50 @@ export function hashToken(token: string): Promise { return sha256Hex(token); } -/** Public token endpoints take JSON bodies only (a cross-site form post needs a preflight). */ +/** + * The largest body a public token endpoint reads (#174): an email, a token, a password and a name + * fit many times over. Anything larger is refused before it is parsed or checked. + */ +export const JSON_BODY_MAX_BYTES = 64 * 1024; + +function tooLarge() { + return apiError( + 413, + "payload_too_large", + `the body must be at most ${JSON_BODY_MAX_BYTES} bytes`, + ); +} + +/** The body as text, or a 413 once more than `max` bytes arrive (a chunked body has no length). */ +async function cappedText(request: Request, max: number): Promise { + if (Number(request.headers.get("content-length") ?? 0) > max) throw tooLarge(); + const reader = request.body?.getReader(); + if (!reader) return ""; + const chunks: Uint8Array[] = []; + let size = 0; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + size += value.byteLength; + if (size > max) { + await reader.cancel().catch(() => undefined); + throw tooLarge(); + } + chunks.push(value); + } + const all = new Uint8Array(size); + let at = 0; + for (const chunk of chunks) { + all.set(chunk, at); + at += chunk.byteLength; + } + return new TextDecoder().decode(all); +} + +/** + * Public token endpoints take JSON bodies only (a cross-site form post needs a preflight), of at + * most `JSON_BODY_MAX_BYTES`. + */ export async function jsonBody(c: Context): Promise> { if (!(c.req.header("content-type") ?? "").toLowerCase().startsWith("application/json")) throw apiError( @@ -45,7 +88,13 @@ export async function jsonBody(c: Context): Promise "unsupported_media_type", "send a JSON body (content-type: application/json)", ); - const body = (await c.req.json().catch(() => undefined)) as unknown; + const text = await cappedText(c.req.raw, JSON_BODY_MAX_BYTES); + let body: unknown; + try { + body = JSON.parse(text); + } catch { + body = undefined; + } if (!body || typeof body !== "object" || Array.isArray(body)) throw apiError(400, "bad_request", "the body must be a JSON object"); return body as Record; diff --git a/packages/api/test/public-input.test.ts b/packages/api/test/public-input.test.ts new file mode 100644 index 00000000..8229b757 --- /dev/null +++ b/packages/api/test/public-input.test.ts @@ -0,0 +1,133 @@ +/** + * #174 item 1: the unauthenticated reset request checks the address in linear time with the + * length first, and public token endpoints refuse bodies above `JSON_BODY_MAX_BYTES` with 413. + */ +import { SiteGraphStore } from "@cloudhal/core-ts"; +import { MemoryBlobStore } from "@cloudhal/storage"; +import { BetterSqlite3Driver } from "@cloudhal/storage/sqlite"; +import Database from "better-sqlite3"; +import { describe, expect, it } from "vitest"; +import { createApi } from "../src/app.ts"; +import { createAuth } from "../src/auth.ts"; +import { EMAIL_MAX_LENGTH, isEmailAddress } from "../src/email.ts"; +import { JSON_BODY_MAX_BYTES } from "../src/tokens.ts"; + +/** What the routes checked before #174. */ +const OLD_EMAIL = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; +const oldCheck = (s: string) => OLD_EMAIL.test(s) && s.length <= EMAIL_MAX_LENGTH; + +describe("isEmailAddress (#174)", () => { + it("accepts exactly what the old regex and length cap accepted", () => { + const samples = [ + "a@b.c", + "owner@example.com", + "first.last+tag@sub.example.co.uk", + "a@b..c", + "a@.b.c", + "a@..", + "a@...", + "a@b.", + "a@.b", + "a@bc", + "@b.c", + "a@@b.c", + "a@b@c.d", + "a b@c.d", + "a@b.c ", + "a@b.\tc", + "", + "@", + "a@", + `${"a".repeat(250)}@b.c`, + `${"a".repeat(251)}@b.c`, + ]; + for (const s of samples) expect(isEmailAddress(s), JSON.stringify(s)).toBe(oldCheck(s)); + // Every string of up to 6 characters over the characters that matter. + const alphabet = ["a", ".", "@", " "]; + let all = [""]; + for (let len = 1; len <= 6; len++) { + all = all.flatMap((s) => alphabet.map((c) => s + c)); + for (const s of all) expect(isEmailAddress(s), JSON.stringify(s)).toBe(oldCheck(s)); + } + }); + + it("answers in linear time on input that made the old regex backtrack", () => { + // The old regex took about a second on 40k dots and grew with the square of the length. + const start = performance.now(); + expect(isEmailAddress(`a@${".".repeat(100_000)}@`)).toBe(false); + expect(isEmailAddress(`a@b${".".repeat(100_000)}`)).toBe(false); + expect(isEmailAddress(`a@${".".repeat(200)}@`)).toBe(false); + expect(performance.now() - start).toBeLessThan(100); + }); +}); + +describe("public token endpoints (#174)", () => { + const ORIGIN = "http://public.test"; + const setup = async () => { + const database = new Database(":memory:"); + const sql = new BetterSqlite3Driver(database); + const store = new SiteGraphStore({ sql, blobs: new MemoryBlobStore() }); + await store.init(); + const auth = createAuth({ + database, + sql, + baseURL: ORIGIN, + secret: "public-input-test-secret-0123456789abcde", + signUp: "first-user", + }); + return createApi({ + store, + resolvePrincipal: auth.resolvePrincipal, + authHandler: auth.handler, + staff: auth.staff, + signUp: auth.signUp, + mail: { id: "test", send: async () => undefined }, + defer: () => undefined, + }); + }; + const post = (app: Awaited>, path: string, body: string) => + app.request(`${ORIGIN}${path}`, { + method: "POST", + headers: { "content-type": "application/json" }, + body, + }); + + it("refuses a 100k-character address quickly, before any regex sees it", async () => { + const app = await setup(); + const email = `a@${".".repeat(60_000)}@`; // under the body cap; the old regex took ~2 s + const start = performance.now(); + const res = await post(app, "/api/password-reset/request", JSON.stringify({ email })); + expect(res.status).toBe(400); + expect(performance.now() - start).toBeLessThan(500); + const ok = await post(app, "/api/password-reset/request", JSON.stringify({ email: "x@y.z" })); + expect(ok.status).toBe(202); + }); + + it("answers 413 for a body above the cap, with or without content-length", async () => { + const app = await setup(); + const big = JSON.stringify({ email: `a@${".".repeat(JSON_BODY_MAX_BYTES)}` }); + const res = await post(app, "/api/password-reset/request", big); + expect(res.status).toBe(413); + expect(await res.json()).toMatchObject({ error: "payload_too_large" }); + // A streamed body (no content-length) is cut off once it passes the cap. + const encoder = new TextEncoder(); + const stream = new ReadableStream({ + start(controller) { + for (let i = 0; i < 10; i++) controller.enqueue(encoder.encode(" ".repeat(10_000))); + controller.enqueue(encoder.encode("{}")); + controller.close(); + }, + }); + const streamed = await app.request(`${ORIGIN}/api/password-reset/request`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: stream, + duplex: "half", + } as RequestInit); + expect(streamed.status).toBe(413); + // At the cap the body is read and judged as usual. + const atCap = JSON.stringify({ email: "x@y.z", pad: " ".repeat(JSON_BODY_MAX_BYTES - 40) }); + expect(new TextEncoder().encode(atCap).byteLength).toBeLessThanOrEqual(JSON_BODY_MAX_BYTES); + expect((await post(app, "/api/password-reset/request", atCap)).status).toBe(202); + }); +}); diff --git a/packages/core-ts/src/media/css-scan.ts b/packages/core-ts/src/media/css-scan.ts new file mode 100644 index 00000000..dd0fbb9c --- /dev/null +++ b/packages/core-ts/src/media/css-scan.ts @@ -0,0 +1,84 @@ +/** + * Linear scanners for the SVG checks (#174). They return what the regexes they replace returned, + * without the backtracking: `/\/\*[\s\S]*?\*\//g` rescanned to the end for every unclosed `/*`, + * and `/url\s*\(\s*(['"]?)([^'")]*)\1\s*\)/gi` was about cubic on `url(` followed by spaces + * (its two `\s*` and the value class overlap). Uploaded SVGs reach both. + */ + +const isSpace = (ch: string | undefined) => ch !== undefined && /\s/.test(ch); + +/** `css` with every `/* … *\/` comment removed, as `css.replace(/\/\*[\s\S]*?\*\//g, "")`. */ +export function stripCssComments(css: string): string { + let out = ""; + let from = 0; + for (;;) { + const open = css.indexOf("/*", from); + if (open < 0) break; + const close = css.indexOf("*/", open + 2); + // No closer after this opener means none after any later one either. + if (close < 0) break; + out += css.slice(from, open); + from = close + 2; + } + return from === 0 ? css : out + css.slice(from); +} + +/** + * The values of `url(…)` references, as group 2 of + * `css.matchAll(/url\s*\(\s*(['"]?)([^'")]*)\1\s*\)/gi)`: quoted, the text between the quotes; + * unquoted, everything after the opening spaces up to `)`. + */ +export function cssUrlValues(css: string): string[] { + const out: string[] = []; + // The first `'`, `"` or `)` at or after a position; positions only grow, so the cache holds. + let special = -1; + const nextSpecial = (from: number) => { + if (special >= from) return special; + special = css.length; + for (let i = from; i < css.length; i++) { + const ch = css[i]; + if (ch === "'" || ch === '"' || ch === ")") { + special = i; + break; + } + } + return special; + }; + let end = 0; + for (const m of css.matchAll(/url/gi)) { + const at = m.index ?? 0; + if (at < end) continue; + let i = at + 3; + while (isSpace(css[i])) i++; + if (css[i] !== "(") continue; + i++; + while (isSpace(css[i])) i++; + const q = css[i]; + if (q === "'" || q === '"') { + const close = nextSpecial(i + 1); + if (css[close] !== q) continue; + let j = close + 1; + while (isSpace(css[j])) j++; + if (css[j] !== ")") continue; + out.push(css.slice(i + 1, close)); + end = j + 1; + } else { + const close = nextSpecial(i); + if (css[close] !== ")") continue; + out.push(css.slice(i, close)); + end = close + 1; + } + } + return out; +} + +/** + * An SVG length as `/^\s*([0-9]*\.?[0-9]+)\s*(px)?\s*$/` read it: a number (`12`, `1.5`, `.5`), + * optionally followed by `px`, with whitespace around; undefined otherwise. + */ +export function svgPixels(value: string | undefined): number | undefined { + if (!value) return undefined; + let v = value.trim(); + if (v.endsWith("px")) v = v.slice(0, -2).trimEnd(); + return /^(?:[0-9]+|[0-9]*\.[0-9]+)$/.test(v) ? Number(v) : undefined; +} diff --git a/packages/core-ts/src/media/svg.ts b/packages/core-ts/src/media/svg.ts index 8bc0d9e1..f382d2be 100644 --- a/packages/core-ts/src/media/svg.ts +++ b/packages/core-ts/src/media/svg.ts @@ -4,6 +4,7 @@ * attributes is reported, and the upload is refused with the list. SVG is served under a CSP * sandbox as well (§7.2), and only people may upload it in v1 (answer 2). Worker-safe. */ +import { cssUrlValues, stripCssComments, svgPixels } from "./css-scan.ts"; const SVG_NS = "http://www.w3.org/2000/svg"; const XLINK_NS = "http://www.w3.org/1999/xlink"; @@ -141,29 +142,26 @@ function bump(problems: Map, what: string, n = 1): void { /** CSS (a `style` attribute or element) without imports, expressions or non-fragment URLs. */ function checkCss(css: string, problems: Map): void { - const flat = css.replace(/\/\*[\s\S]*?\*\//g, ""); + const flat = stripCssComments(css); if (/\\/.test(flat)) bump(problems, "CSS escapes"); if (/@import/i.test(flat)) bump(problems, "CSS @import rules"); if (/expression\s*\(|behavior\s*:|-moz-binding|javascript:/i.test(flat)) bump(problems, "CSS expressions or script URLs"); - for (const m of flat.matchAll(/url\s*\(\s*(['"]?)([^'")]*)\1\s*\)/gi)) - if (!FRAGMENT.test((m[2] ?? "").trim())) bump(problems, "external references"); + for (const url of cssUrlValues(flat)) + if (!FRAGMENT.test(url.trim())) bump(problems, "external references"); if (/@font-face|@namespace/i.test(flat)) bump(problems, "CSS @font-face or @namespace rules"); } function checkValue(name: string, value: string, problems: Map): void { if (/javascript:|vbscript:|data:text\/html/i.test(value.replace(/\s+/g, ""))) bump(problems, "script URLs"); - for (const m of value.matchAll(/url\s*\(\s*(['"]?)([^'")]*)\1\s*\)/gi)) - if (!FRAGMENT.test((m[2] ?? "").trim())) bump(problems, "external references"); + for (const url of cssUrlValues(value)) + if (!FRAGMENT.test(url.trim())) bump(problems, "external references"); if (name === "style") checkCss(value, problems); } -function length(value: string | undefined): number | undefined { - if (!value) return undefined; - const m = /^\s*([0-9]*\.?[0-9]+)\s*(px)?\s*$/.exec(value); - return m ? Number(m[1]) : undefined; -} +/** Width and height values: a number, optionally in `px`. */ +const length = svgPixels; /** Validates an SVG document. Never throws on bad input; read `ok` and `problems`. */ export function checkSvg(source: string): SvgCheck { diff --git a/packages/core-ts/src/redirects.ts b/packages/core-ts/src/redirects.ts index 1422adef..3b263507 100644 --- a/packages/core-ts/src/redirects.ts +++ b/packages/core-ts/src/redirects.ts @@ -11,13 +11,14 @@ import { } from "@cloudhal/core-contracts"; import { InvariantError } from "./errors.ts"; import { ID_PREFIX } from "./ids.ts"; +import { trimEndChar } from "./names.ts"; import type { OpBatch } from "./node-sql.ts"; import type { Creatable } from "./nodes.ts"; /** `/a/b/` → `/a/b`; the root stays `/`. Query and fragment are not part of a request path here. */ export function normalisePath(path: string): string { const p = path.split(/[?#]/, 1)[0] ?? "/"; - return p.length > 1 ? p.replace(/\/+$/, "") || "/" : p || "/"; + return p.length > 1 ? trimEndChar(p, "/") || "/" : p || "/"; } /** A target split into its site path and the `?query#fragment` suffix; `undefined` for https URLs. */ @@ -33,13 +34,22 @@ export function joinTarget(to: string, rest: string): string { const i = to.search(/[?#]/); const head = i < 0 ? to : to.slice(0, i); const tail = i < 0 ? "" : to.slice(i); - return `${head.replace(/\/+$/, "")}${rest}${tail}`; + return `${trimEndChar(head, "/")}${rest}${tail}`; } -/** The proper ancestors of a path, longest first, including `/`. */ +/** + * The proper ancestors of a path that a redirect's `from` can be, longest first, including `/`. + * A `from` has no empty segment, so no ancestor that holds `//` can match: the walk starts below + * the first `//` (#174; a run of slashes used to yield one useless ancestor per slash). + */ export function ancestorPaths(path: string): string[] { const out: string[] = []; let p = path; + const cut = path.indexOf("//"); + if (cut >= 0) { + p = path.slice(0, cut) || "/"; + out.push(p); + } while (p.length > 1) { p = p.slice(0, p.lastIndexOf("/")) || "/"; out.push(p); diff --git a/packages/core-ts/src/search-rows.ts b/packages/core-ts/src/search-rows.ts index d1ccd91c..deb81fcb 100644 --- a/packages/core-ts/src/search-rows.ts +++ b/packages/core-ts/src/search-rows.ts @@ -10,6 +10,7 @@ import { termName } from "./names.ts"; import { entryPath, type PermalinkSite } from "./permalinks.ts"; import { MARK_CLOSE, MARK_OPEN, SEARCH_TEXT_MAX, type SearchRow } from "./search-common.ts"; import { searchableFieldTexts } from "./search-fields.ts"; +import { linkTexts, stripFences, stripTags } from "./search-text-scan.ts"; import { termSlug } from "./term-locales.ts"; const iso = (value: string | undefined) => { @@ -38,20 +39,16 @@ const clip = (text: string) => export function searchText(body: Pick): string { let text = body.value; if (body.format === "markdown") { - text = text - // fenced code and ```surface fences - .replace(/^(`{3,}|~{3,})[^\n]*\n[\s\S]*?^\1[ \t]*$/gm, " ") + // fenced code and ```surface fences; linear scanners where a regex would backtrack (#174) + text = stripFences(text) // directive lines: keep the text of quoted attributes .replace(/^:{2,}[a-z][\w-]*(\{[^\n]*\})?[ \t]*$/gim, (_m, attrs: string | undefined) => attrs ? [...attrs.matchAll(/="((?:[^"\\]|\\.)*)"/g)].map((a) => a[1]).join(" ") : " ", ) - .replace(/^:{3,}[ \t]*$/gm, " ") - .replace(/!\[([^\]]*)\]\([^)]*\)/g, "$1") - .replace(/\[([^\]]*)\]\([^)]*\)/g, "$1") - .replace(/`([^`]*)`/g, "$1"); + .replace(/^:{3,}[ \t]*$/gm, " "); + text = linkTexts(linkTexts(text, true), false).replace(/`([^`]*)`/g, "$1"); } - text = text - .replace(/<[^>]*>/g, " ") + text = stripTags(text) .replace(/&(?:amp|lt|gt|quot|#39|nbsp);/g, (m) => ENTITIES[m] ?? m) .replace(/\bhttps?:\/\/\S+/g, " ") .replace(/[#>*_~|\\]+/g, " ") diff --git a/packages/core-ts/src/search-text-scan.ts b/packages/core-ts/src/search-text-scan.ts new file mode 100644 index 00000000..9c39fb8e --- /dev/null +++ b/packages/core-ts/src/search-text-scan.ts @@ -0,0 +1,119 @@ +/** + * Linear scanners for `searchText` (#174). Each returns what the regex replace it stands for + * returned, without rescanning to the end of the body for every opener whose closer is missing. + * The regexes were quadratic on such input, and article bodies come from agents and editors: + * + * - `/^(`{3,}|~{3,})[^\n]*\n[\s\S]*?^\1[ \t]*$/gm` → `" "` (`stripFences`); + * - `/!\[([^\]]*)\]\([^)]*\)/g` → `"$1"` and `/\[([^\]]*)\]\([^)]*\)/g` → `"$1"` (`linkTexts`); + * - `/<[^>]*>/g` → `" "` (`stripTags`). + */ + +/** The line terminators `^` and `$` see under the `m` flag. */ +const isLineEnd = (ch: string | undefined) => + ch === "\n" || ch === "\r" || ch === "\u2028" || ch === "\u2029"; + +/** The first value in sorted `list` that is at least `min`, or undefined. */ +function firstAtLeast(list: readonly number[], min: number): number | undefined { + let lo = 0; + let hi = list.length; + while (lo < hi) { + const mid = (lo + hi) >> 1; + if ((list[mid] as number) < min) lo = mid + 1; + else hi = mid; + } + return list[lo]; +} + +/** + * Fenced blocks replaced by one space. An opening line starts with three or more backticks (or + * tildes); the block ends at the nearest later line that is exactly that fence, optionally + * followed by spaces or tabs. Like the regex, a longer opening fence falls back to shorter ones + * (```` ```` ```` may close at ```` ``` ````), longest first. + */ +export function stripFences(text: string): string { + const starts = [0]; + for (let i = 0; i < text.length; i++) if (isLineEnd(text[i])) starts.push(i + 1); + // Closing lines by fence (character and length): each line closes exactly one kind. + const closers = new Map(); + const fenceAt = (s: number) => { + const c = text[s]; + if (c !== "`" && c !== "~") return undefined; + let q = s; + while (text[q] === c) q++; + return q - s >= 3 ? { c, length: q - s } : undefined; + }; + for (const s of starts) { + const f = fenceAt(s); + if (!f) continue; + let e = s + f.length; + while (text[e] === " " || text[e] === "\t") e++; + if (e < text.length && !isLineEnd(text[e])) continue; + const key = f.c + f.length; + const list = closers.get(key); + if (list) list.push(s); + else closers.set(key, [s]); + } + let out = ""; + let from = 0; + for (const s of starts) { + if (s < from) continue; + const f = fenceAt(s); + if (!f) continue; + const nl = text.indexOf("\n", s); + if (nl < 0) continue; + for (let m = f.length; m >= 3; m--) { + const t = firstAtLeast(closers.get(f.c + m) ?? [], nl + 1); + if (t === undefined) continue; + let e = t + m; + while (text[e] === " " || text[e] === "\t") e++; + out += `${text.slice(from, s)} `; + from = e; + break; + } + } + return from === 0 ? text : out + text.slice(from); +} + +/** + * Markdown links (`[text](url)`) or, with `image`, images (`![alt](url)`) replaced by their text: + * the bracket text runs to the first `]`, which must be followed by `(`, and the target to the + * first `)` after it. + */ +export function linkTexts(text: string, image: boolean): string { + const open = image ? "![" : "["; + let out = ""; + let from = 0; + let search = 0; + for (;;) { + const j = text.indexOf(open, search); + if (j < 0) break; + const k = text.indexOf("]", j + open.length); + if (k < 0) break; // no `]` left: no later link can close either + if (text[k + 1] !== "(") { + // Every opener before this `]` reaches the same `]`, so it fails the same way. + search = k + 1; + continue; + } + const end = text.indexOf(")", k + 2); + if (end < 0) break; // no `)` left: no later link can end either + out += text.slice(from, j) + text.slice(j + open.length, k); + from = end + 1; + search = from; + } + return from === 0 ? text : out + text.slice(from); +} + +/** Every `<…>` (up to the first `>`) replaced by one space. */ +export function stripTags(text: string): string { + let out = ""; + let from = 0; + for (;;) { + const j = text.indexOf("<", from); + if (j < 0) break; + const k = text.indexOf(">", j + 1); + if (k < 0) break; // no `>` left: no later tag can close either + out += `${text.slice(from, j)} `; + from = k + 1; + } + return from === 0 ? text : out + text.slice(from); +} diff --git a/packages/core-ts/test/redos-174.test.ts b/packages/core-ts/test/redos-174.test.ts new file mode 100644 index 00000000..1c1dd90b --- /dev/null +++ b/packages/core-ts/test/redos-174.test.ts @@ -0,0 +1,164 @@ +/** + * #174: every linear rewrite answers like the regex it replaced (seeded random strings over the + * characters that matter, plus hand-picked cases), and stays fast on input that made the regex + * backtrack (100k characters or more). + */ +import type { Redirect } from "@cloudhal/core-contracts"; +import { describe, expect, it } from "vitest"; +import { cssUrlValues, stripCssComments, svgPixels } from "../src/media/css-scan.ts"; +import { checkSvg } from "../src/media/svg.ts"; +import { ancestorPaths, joinTarget, matchRedirect, normalisePath } from "../src/redirects.ts"; +import { searchText } from "../src/search-rows.ts"; +import { linkTexts, stripFences, stripTags } from "../src/search-text-scan.ts"; + +/** Deterministic strings: every run of the suite checks the same inputs. */ +function strings(alphabet: readonly string[], count: number, maxLength: number, seed = 174) { + let s = seed; + const next = () => { + s = (s * 1103515245 + 12345) & 0x7fffffff; + return s; + }; + const out: string[] = []; + for (let n = 0; n < count; n++) { + let v = ""; + const len = next() % (maxLength + 1); + for (let i = 0; i < len; i++) v += alphabet[next() % alphabet.length]; + out.push(v); + } + return out; +} + +/** Runs `f` and fails when it takes longer than `ms` (generous: CI machines are busy). */ +function fast(ms: number, f: () => T): T { + const start = performance.now(); + const value = f(); + expect(performance.now() - start).toBeLessThan(ms); + return value; +} + +const N = 100_000; + +describe("redirect paths (#174 item 2)", () => { + const oldNormalise = (path: string) => { + const p = path.split(/[?#]/, 1)[0] ?? "/"; + return p.length > 1 ? p.replace(/\/+$/, "") || "/" : p || "/"; + }; + const oldAncestors = (path: string) => { + const out: string[] = []; + let p = path; + while (p.length > 1) { + p = p.slice(0, p.lastIndexOf("/")) || "/"; + out.push(p); + } + return out; + }; + /** What a redirect `from` can be (the contract's pattern). */ + const FROM = /^\/(?:[^\s?#/\\]+(?:\/[^\s?#/\\]+)*)?$/; + + it("normalisePath and joinTarget answer as before", () => { + for (const p of strings(["/", "a", "?", "#", "b"], 3000, 12)) + expect(normalisePath(p), p).toBe(oldNormalise(p)); + for (const to of strings(["/", "x", "?", "#"], 500, 8)) + for (const rest of ["", "/r", "/r/s"]) + expect(joinTarget(to, rest)).toBe( + (() => { + if (!rest) return to; + const i = to.search(/[?#]/); + const head = i < 0 ? to : to.slice(0, i); + return `${head.replace(/\/+$/, "")}${rest}${i < 0 ? "" : to.slice(i)}`; + })(), + ); + }); + + it("ancestorPaths keeps every ancestor a redirect could match", () => { + for (const raw of strings(["/", "a", "b"], 3000, 12)) { + const p = `/${raw}`; + expect(ancestorPaths(p), p).toEqual(oldAncestors(p).filter((a) => FROM.test(a))); + expect( + ancestorPaths(p).every((a) => FROM.test(a)), + p, + ).toBe(true); + } + const redirects = ["/a", "/a/b", "/"].map( + (from) => + ({ from, to: `/new${from}`, match: "prefix", locale: undefined }) as unknown as Redirect, + ); + expect(matchRedirect(redirects, "/a//x")?.redirect.from).toBe("/a"); + expect(matchRedirect(redirects, "/a/b//c")?.redirect.from).toBe("/a/b"); + expect(matchRedirect(redirects, "//x")?.redirect.from).toBe("/"); + }); + + it("stays linear on long runs of slashes", () => { + fast(100, () => expect(normalisePath(`/a${"/".repeat(N)}b/`)).toBe(`/a${"/".repeat(N)}b`)); + fast(100, () => expect(joinTarget(`/t${"/".repeat(N)}`, "/r")).toBe("/t/r")); + fast(100, () => expect(ancestorPaths(`/a${"/".repeat(N)}b`)).toEqual(["/a", "/"])); + }); +}); + +describe("SVG CSS (#174 item 4)", () => { + const OLD_URL = /url\s*\(\s*(['"]?)([^'")]*)\1\s*\)/gi; + const oldLength = (value: string) => { + const m = /^\s*([0-9]*\.?[0-9]+)\s*(px)?\s*$/.exec(value); + return m ? Number(m[1]) : undefined; + }; + + it("answers as the regexes did", () => { + for (const css of strings(["/", "*", "a", " ", "\n"], 3000, 14)) + expect(stripCssComments(css), css).toBe(css.replace(/\/\*[\s\S]*?\*\//g, "")); + const urlParts = ["url", "URL", "(", ")", "'", '"', " ", "#a", "x", "\t"]; + for (const css of strings(urlParts, 5000, 9)) + expect(cssUrlValues(css), css).toEqual([...css.matchAll(OLD_URL)].map((m) => m[2])); + for (const v of strings(["1", ".", "p", "x", " ", "\t"], 3000, 8)) + expect(svgPixels(v), JSON.stringify(v)).toBe(oldLength(v)); + expect(svgPixels(undefined)).toBeUndefined(); + expect(svgPixels("")).toBeUndefined(); + }); + + it("stays linear on unclosed comments, url( runs and long lengths", () => { + fast(100, () => stripCssComments("/* ".repeat(N / 3))); + fast(100, () => cssUrlValues(`url(${" ".repeat(N)}`)); + fast(100, () => cssUrlValues("url(".repeat(N / 4))); + fast(100, () => cssUrlValues(`url('${"a".repeat(N)}`)); + fast(100, () => svgPixels(`${"1".repeat(N)}x`)); + fast(100, () => svgPixels(`1${" ".repeat(N)}x`)); + // The old `url(` check took minutes on 20k spaces; a whole document with them is quick now + // (attribute values are read from a 64 KiB window, so the document case stays below it). + const svg = ``; + expect(fast(500, () => checkSvg(svg)).ok).toBe(true); + }); +}); + +describe("search text (#174 item 4)", () => { + const OLD_FENCE = /^(`{3,}|~{3,})[^\n]*\n[\s\S]*?^\1[ \t]*$/gm; + + it("answers as the regexes did", () => { + const fenceParts = ["`", "`", "`", "~", "~", "a", " ", "\t", "\n", "\n", "\r"]; + for (const t of strings(fenceParts, 8000, 24)) + expect(stripFences(t), JSON.stringify(t)).toBe(t.replace(OLD_FENCE, " ")); + for (const t of [ + "```\ncode\n```", + "````\ncode\n```\nafter", + "~~~ js\na\n~~~ \nb", + "```\r\ncode\r\n```\r\n", + "```\nnever closed", + "text\n```a\n```\n```\n", + ]) + expect(stripFences(t), JSON.stringify(t)).toBe(t.replace(OLD_FENCE, " ")); + const linkParts = ["!", "[", "]", "(", ")", "a", "\n"]; + for (const t of strings(linkParts, 5000, 14)) { + expect(linkTexts(t, true), t).toBe(t.replace(/!\[([^\]]*)\]\([^)]*\)/g, "$1")); + expect(linkTexts(t, false), t).toBe(t.replace(/\[([^\]]*)\]\([^)]*\)/g, "$1")); + } + for (const t of strings(["<", ">", "a", "\n"], 3000, 12)) + expect(stripTags(t), t).toBe(t.replace(/<[^>]*>/g, " ")); + }); + + it("stays linear on openers without closers", () => { + fast(150, () => stripFences("```a\n".repeat(N / 5))); + fast(100, () => linkTexts("![a".repeat(N / 3), true)); + fast(100, () => linkTexts("[a](".repeat(N / 4), false)); + fast(100, () => linkTexts("[a]x".repeat(N / 4), false)); + fast(100, () => stripTags(" searchText({ format: "markdown", value: `${"```a\n[x](<".repeat(N / 10)}` })); + }); +}); diff --git a/packages/schedules/src/feed-check.ts b/packages/schedules/src/feed-check.ts index 698eb2da..888b7293 100644 --- a/packages/schedules/src/feed-check.ts +++ b/packages/schedules/src/feed-check.ts @@ -12,6 +12,7 @@ import type { FeedFinding, FindingSeverity, GateStateName } from "./findings.ts"; import { instantMs, MINUTE_MS } from "./instant.ts"; import { plainLine } from "./text.ts"; +import { cdataParts, elementContents, mediaUrlValues } from "./xml-scan.ts"; /** Documents larger than this are not parsed (`feed-invalid`). */ export const MAX_DOCUMENT_CHARS = 5_000_000; @@ -153,15 +154,10 @@ const ENTITIES: Record = { /** Element text: CDATA unwrapped, the five XML entities and numeric references decoded. */ export function xmlText(raw: string): string { - const parts: string[] = []; - const re = //g; - let last = 0; - for (const m of raw.matchAll(re)) { - parts.push(decode(raw.slice(last, m.index)), m[1] as string); - last = (m.index ?? 0) + m[0].length; - } - parts.push(decode(raw.slice(last))); - return parts.join("").trim(); + return cdataParts(raw) + .map((p) => decode(p.text) + (p.cdata ?? "")) + .join("") + .trim(); } function decode(s: string): string { @@ -178,30 +174,20 @@ function safeChar(code: number): string | undefined { : undefined; } -function escapeName(name: string): string { - return name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); -} - /** Every `…` block (non-nesting elements such as `item`, `url`). */ function blocks(text: string, name: string): string[] { - const n = escapeName(name); - return [...text.matchAll(new RegExp(`<${n}(?:\\s[^>]*)?>([\\s\\S]*?)`, "g"))].map( - (m) => m[1] as string, - ); + return elementContents(text, name); } /** The text of the first `` child, or undefined. */ function child(block: string, name: string): string | undefined { - const n = escapeName(name); - const m = new RegExp(`<${n}(?:\\s[^>]*)?>([\\s\\S]*?)`).exec(block); - return m ? xmlText(m[1] as string) : undefined; + const first = elementContents(block, name, 1)[0]; + return first === undefined ? undefined : xmlText(first); } /** `url="…"` attributes of `` and ``. */ function mediaAttributes(block: string): string[] { - return [...block.matchAll(/<(?:enclosure|media:content)\b[^>]*?\burl\s*=\s*(["'])(.*?)\1/g)].map( - (m) => decode(m[2] as string), - ); + return mediaUrlValues(block).map(decode); } /** A URL's path as the site routes it (no query, fragment or trailing slash), or undefined. */ @@ -214,7 +200,10 @@ export function urlPath(url: string): string | undefined { } catch { // keep the encoded form } - return p.length > 1 ? p.replace(/\/+$/, "") || "/" : p || "/"; + // A linear trim of trailing "/" (`/\/+$/` backtracks on a long run of "/" inside the path). + let end = p.length; + while (end > 1 && p[end - 1] === "/") end--; + return p.length > 1 ? p.slice(0, end) : p || "/"; } catch { return undefined; } diff --git a/packages/schedules/src/xml-scan.ts b/packages/schedules/src/xml-scan.ts new file mode 100644 index 00000000..5975bbeb --- /dev/null +++ b/packages/schedules/src/xml-scan.ts @@ -0,0 +1,158 @@ +/** + * Linear scanners for the feed check (#174). Each returns what the regex it replaces returned, + * without rescanning to the end of the document for every opener whose closer is missing: + * `//g`, `]*)?>([\s\S]*?)` and + * `/<(?:enclosure|media:content)\b[^>]*?\burl\s*=\s*(["'])(.*?)\1/g` were quadratic on such input. + */ + +const isSpace = (ch: string | undefined) => ch !== undefined && /\s/.test(ch); +const isWord = (ch: string | undefined) => ch !== undefined && /\w/.test(ch); +/** What `.` does not match without the `s` flag. */ +const isLineEnd = (ch: string) => ch === "\n" || ch === "\r" || ch === "\u2028" || ch === "\u2029"; + +const CDATA_OPEN = ""; + +/** `raw` split into text outside CDATA sections and their contents, in order. */ +export function cdataParts(raw: string): { text: string; cdata?: string }[] { + const out: { text: string; cdata?: string }[] = []; + let from = 0; + for (;;) { + const open = raw.indexOf(CDATA_OPEN, from); + if (open < 0) break; + const close = raw.indexOf(CDATA_CLOSE, open + CDATA_OPEN.length); + // No closer after this opener means none after any later one either. + if (close < 0) break; + out.push({ text: raw.slice(from, open), cdata: raw.slice(open + CDATA_OPEN.length, close) }); + from = close + CDATA_CLOSE.length; + } + out.push({ text: raw.slice(from) }); + return out; +} + +/** + * The contents of every `…` element (an opening tag `` or ``, the + * nearest `` closer, whitespace allowed before its `>`), as group 1 of the global regex. + * `limit` stops after that many (1 for the first, as a non-global `exec` finds it). + */ +export function elementContents(text: string, name: string, limit = Infinity): string[] { + const out: string[] = []; + const open = `<${name}`; + const close = `` left + const nextGt = (from: number) => { + if (gt === undefined || (gt >= 0 && gt < from)) gt = text.indexOf(">", from); + return gt; + }; + let closer: { at: number; end: number } | undefined | null; + const nextCloser = (from: number): { at: number; end: number } | undefined => { + if (closer === null) return undefined; + if (closer && closer.at >= from) return closer; + for (let c = text.indexOf(close, from); c >= 0; c = text.indexOf(close, c + 1)) { + let j = c + close.length; + while (isSpace(text[j])) j++; + if (text[j] === ">") { + closer = { at: c, end: j + 1 }; + return closer; + } + } + closer = null; + return undefined; + }; + for (let at = text.indexOf(open); at >= 0 && out.length < limit; ) { + const after = at + open.length; + let start: number | undefined; + if (text[after] === ">") start = after + 1; + else if (isSpace(text[after])) { + const g = nextGt(after + 1); + if (g < 0) break; // no `>` left: no later opening tag can end either + start = g + 1; + } + if (start === undefined) { + at = text.indexOf(open, at + 1); + continue; + } + const c = nextCloser(start); + if (!c) break; // no closer left: no later element can end either + out.push(text.slice(start, c.at)); + at = text.indexOf(open, c.end); + } + return out; +} + +const MEDIA_TAGS = ["(); // -1: none left + const find = (tag: string, from: number) => { + const known = next.get(tag); + if (known !== undefined && (known < 0 || known >= from)) return known; + let at = text.indexOf(tag, from); + while (at >= 0 && isWord(text[at + tag.length])) at = text.indexOf(tag, at + 1); + next.set(tag, at); + return at; + }; + return (from: number): { at: number; nameEnd: number } | undefined => { + let best: { at: number; nameEnd: number } | undefined; + for (const tag of MEDIA_TAGS) { + const at = find(tag, from); + if (at >= 0 && (!best || at < best.at)) best = { at, nameEnd: at + tag.length }; + } + return best; + }; +} + +/** At `p`: `url`, optional spaces, `=`, optional spaces, a quote; the value and where it ends. */ +function urlAttribute(text: string, p: number): { value: string; end: number } | undefined { + if (isWord(text[p - 1]) || !text.startsWith("url", p)) return undefined; + let i = p + 3; + while (isSpace(text[i])) i++; + if (text[i] !== "=") return undefined; + i++; + while (isSpace(text[i])) i++; + const q = text[i]; + if (q !== '"' && q !== "'") return undefined; + for (let j = i + 1; j < text.length; j++) { + const ch = text[j] as string; + if (ch === q) return { value: text.slice(i + 1, j), end: j + 1 }; + if (isLineEnd(ch)) return undefined; + } + return undefined; +} + +/** + * The `url="…"` values of `` and `` tags, as group 2 of the regex: the + * first `url` attribute inside the tag (before its `>`) whose quoted value closes on its line. + */ +export function mediaUrlValues(text: string): string[] { + const out: string[] = []; + const nextTag = mediaTagFinder(text); + let from = 0; + for (let tag = nextTag(from); tag; tag = nextTag(from)) { + const gt = text.indexOf(">", tag.nameEnd); + const stop = gt < 0 ? text.length : gt; + let found: { value: string; end: number } | undefined; + for ( + let p = text.indexOf("url", tag.nameEnd); + p >= 0 && p < stop; + p = text.indexOf("url", p + 1) + ) { + found = urlAttribute(text, p); + if (found) break; + } + if (found) { + out.push(found.value); + from = found.end; + continue; + } + // Every later tag that starts before this `>` sees the same candidates, which all failed. + if (gt < 0) break; + from = gt + 1; + } + return out; +} diff --git a/packages/schedules/test/xml-scan.test.ts b/packages/schedules/test/xml-scan.test.ts new file mode 100644 index 00000000..862b34d7 --- /dev/null +++ b/packages/schedules/test/xml-scan.test.ts @@ -0,0 +1,94 @@ +/** + * #174: the feed check's linear scanners answer like the regexes they replaced (seeded random + * strings plus hand-picked cases) and stay fast on 100k characters of openers without closers. + */ +import { describe, expect, it } from "vitest"; +import { urlPath, xmlText } from "../src/feed-check.ts"; +import { cdataParts, elementContents, mediaUrlValues } from "../src/xml-scan.ts"; + +function strings(alphabet: readonly string[], count: number, maxLength: number, seed = 174) { + let s = seed; + const next = () => { + s = (s * 1103515245 + 12345) & 0x7fffffff; + return s; + }; + const out: string[] = []; + for (let n = 0; n < count; n++) { + let v = ""; + const len = next() % (maxLength + 1); + for (let i = 0; i < len; i++) v += alphabet[next() % alphabet.length]; + out.push(v); + } + return out; +} + +function fast(ms: number, f: () => T): T { + const start = performance.now(); + const value = f(); + expect(performance.now() - start).toBeLessThan(ms); + return value; +} + +const N = 100_000; +const OLD_CDATA = //g; +const OLD_MEDIA = /<(?:enclosure|media:content)\b[^>]*?\burl\s*=\s*(["'])(.*?)\1/g; +const oldBlocks = (text: string, n: string) => + [...text.matchAll(new RegExp(`<${n}(?:\\s[^>]*)?>([\\s\\S]*?)`, "g"))].map( + (m) => m[1], + ); + +describe("feed-check scanners (#174 item 4)", () => { + it("cdataParts splits as the CDATA regex did", () => { + for (const raw of strings(["", "a", "]", "<"], 3000, 8)) { + const parts: { text: string; cdata?: string }[] = []; + let last = 0; + for (const m of raw.matchAll(OLD_CDATA)) { + parts.push({ text: raw.slice(last, m.index), cdata: m[1] as string }); + last = (m.index ?? 0) + m[0].length; + } + parts.push({ text: raw.slice(last) }); + expect(cdataParts(raw), raw).toEqual(parts); + } + expect(xmlText("a & &]]> c")).toBe("a & & c"); + }); + + it("elementContents finds the blocks the element regex found", () => { + const parts = ["", " ", "\n", "a", "s", "<"]; + for (const text of strings(parts, 6000, 10)) { + expect(elementContents(text, "item"), text).toEqual(oldBlocks(text, "item")); + expect(elementContents(text, "item", 1), text).toEqual(oldBlocks(text, "item").slice(0, 1)); + } + expect(elementContents("xyz", "item")).toEqual(["x", "z"]); + }); + + it("mediaUrlValues finds the url attributes the media regex found", () => { + const parts = ["", "\n", "a", "x"]; + for (const text of strings(parts, 8000, 9)) + expect(mediaUrlValues(text), text).toEqual([...text.matchAll(OLD_MEDIA)].map((m) => m[2])); + expect(mediaUrlValues('')).toEqual([ + "https://x/a.mp3", + ]); + }); + + it("urlPath trims trailing slashes as before", () => { + const old = (url: string) => { + const p = new URL(url.trim(), "https://site.invalid").pathname; + return p.length > 1 ? p.replace(/\/+$/, "") || "/" : p || "/"; + }; + for (const raw of strings(["/", "a", "b"], 2000, 10)) { + const url = `https://x/${raw}`; + expect(urlPath(url), url).toBe(old(url)); + } + }); + + it("stays linear on openers without closers", () => { + fast(100, () => cdataParts(" elementContents("".repeat(N / 6), "item")); + fast(100, () => elementContents(" elementContents(`${" mediaUrlValues(" mediaUrlValues(''.repeat(N / 19))); + fast(100, () => expect(urlPath(`https://x/a${"/".repeat(N)}b/`)).toBe(`/a${"/".repeat(N)}b`)); + }); +}); diff --git a/packages/ui/src/blocks/editor/markdown-blocks.ts b/packages/ui/src/blocks/editor/markdown-blocks.ts index 0384fff0..7766d227 100644 --- a/packages/ui/src/blocks/editor/markdown-blocks.ts +++ b/packages/ui/src/blocks/editor/markdown-blocks.ts @@ -15,6 +15,7 @@ import { type SplitMarkdown, splitMarkdown, } from "@cloudhal/core-ts/body-blocks"; +import { trimEndChar } from "@cloudhal/core-ts/names"; import { Marked } from "marked"; export type { MarkdownUnit, SplitMarkdown } from "@cloudhal/core-ts/body-blocks"; @@ -66,12 +67,13 @@ export function mergeBlocks( } // A block that was not last before keeps the blank lines it had in the middle: end it with one // newline, like a new block. An untouched body ends exactly as it did. - if (previous !== source.units.length - 1 && out !== "") out = out.replace(/\n*$/, "\n"); + if (previous !== source.units.length - 1 && out !== "") out = `${trimEndChar(out, "\n")}\n`; return out; } /** `text` ready for another block: nothing, or ending in one blank line. */ function separated(text: string): string { if (text === "") return ""; - return `${text.replace(/\n*$/, "")}\n\n`; + // `trimEndChar`, not `/\n*$/`: that backtracks on a long run of newlines inside the text (#174). + return `${trimEndChar(text, "\n")}\n\n`; } diff --git a/packages/ui/test/markdown-blocks-redos.test.ts b/packages/ui/test/markdown-blocks-redos.test.ts new file mode 100644 index 00000000..4fb919f0 --- /dev/null +++ b/packages/ui/test/markdown-blocks-redos.test.ts @@ -0,0 +1,28 @@ +/** #174 item 6: the editor's newline trims answer as `/\n*$/` did, in linear time. */ +import { trimEndChar } from "@cloudhal/core-ts/names"; +import { describe, expect, it } from "vitest"; +import { mergeBlocks, splitMarkdown } from "../src/blocks/editor/markdown-blocks.ts"; + +describe("markdown block trims (#174)", () => { + it("trimEndChar on newlines is the old /\\n*$/ replace", () => { + let s = 174; + const next = () => { + s = (s * 1103515245 + 12345) & 0x7fffffff; + return s; + }; + for (let n = 0; n < 5000; n++) { + let t = ""; + for (let i = next() % 10; i > 0; i--) t += ["\n", "\n", "a", "\r", " "][next() % 5]; + expect(`${trimEndChar(t, "\n")}\n`, JSON.stringify(t)).toBe(t.replace(/\n*$/, "\n")); + expect(trimEndChar(t, "\n"), JSON.stringify(t)).toBe(t.replace(/\n*$/, "")); + } + }); + + it("merges a block with a long run of blank lines in linear time", () => { + const tall = `x${"\n".repeat(100_000)}y`; + const start = performance.now(); + const out = mergeBlocks(splitMarkdown(""), [], ["a", tall, "b"]); + expect(performance.now() - start).toBeLessThan(500); + expect(out).toBe(`a\n\n${tall}\n\nb\n`); + }); +}); From 26756ca2c53886484e240b757509c5aaeae1cea7 Mon Sep 17 00:00:00 2001 From: Madraka <8898640+Madraka@users.noreply.github.com> Date: Sun, 8 Nov 2026 21:42:25 +0300 Subject: [PATCH 2/3] fix(security): review round 2 for the ReDoS sweep (#174) - ancestorPaths starts at the longest ancestor a redirect from can be (1024 characters), so many short segments stay bounded. - searchText's directive braces stay on one line (CR line ends were quadratic); stripFences keeps the next "\n" between openers. - mediaUrlValues keeps the next ">" and "url" (forwardFinder). - The editor trim test is renamed to .test.tsx so ui's vitest collects it. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Madraka <8898640+Madraka@users.noreply.github.com> --- .changeset/redos-sweep-174.md | 2 +- packages/core-ts/src/redirects.ts | 21 +++++++--- packages/core-ts/src/search-rows.ts | 8 +++- packages/core-ts/src/search-text-scan.ts | 7 +++- packages/core-ts/test/redos-174.test.ts | 40 +++++++++++++++++-- packages/schedules/src/xml-scan.ts | 27 ++++++++++--- packages/schedules/test/xml-scan.test.ts | 7 ++++ ...test.ts => markdown-blocks-redos.test.tsx} | 0 8 files changed, 94 insertions(+), 18 deletions(-) rename packages/ui/test/{markdown-blocks-redos.test.ts => markdown-blocks-redos.test.tsx} (100%) diff --git a/.changeset/redos-sweep-174.md b/.changeset/redos-sweep-174.md index d1371743..f4657ab6 100644 --- a/.changeset/redos-sweep-174.md +++ b/.changeset/redos-sweep-174.md @@ -9,7 +9,7 @@ Second ReDoS sweep (#174). Input that made a regex backtrack is now handled in linear time; results are unchanged. - **Unauthenticated password-reset request.** `POST /api/password-reset/request` (and invite creation) checks the address length before anything else, and the address check no longer backtracks: `a@` followed by many dots used to cost seconds of CPU per request. Public token endpoints (`/api/password-reset/*`, invite acceptance) refuse a JSON body over 64 KiB with `413 payload_too_large`, whether or not it declares a `content-length`. -- **core-ts.** Redirect path normalising and matching (on every site request): `normalisePath`, `joinTarget`, and `ancestorPaths`, which no longer lists ancestors holding `//` that no redirect can match. Search text built at publish time: fenced code, links, images and tags. SVG upload checks: CSS comments, `url(…)` references (about cubic on `url(` followed by spaces) and width/height lengths (new `css-scan.ts`). +- **core-ts.** Redirect path normalising and matching (on every site request): `normalisePath`, `joinTarget`, and `ancestorPaths`, which no longer lists ancestors that no redirect can match: those longer than a `from` may be (1024 characters) or holding `//`. Directive lines in search text keep their braces on one line, so a directive whose `{…}` spans a lone `\r` is no longer read as one. Search text built at publish time: fenced code, links, images and tags. SVG upload checks: CSS comments, `url(…)` references (about cubic on `url(` followed by spaces) and width/height lengths (new `css-scan.ts`). - **schedules.** The feed check's CDATA, element and ``/`` url scans, and `urlPath`. - **agent-runtime.** SKILL.md frontmatter lines (a line of spaces was quadratic). - **Studio** (in `@cloudhal/cli`): the markdown body editor's trailing-newline trims. diff --git a/packages/core-ts/src/redirects.ts b/packages/core-ts/src/redirects.ts index 3b263507..dbb98014 100644 --- a/packages/core-ts/src/redirects.ts +++ b/packages/core-ts/src/redirects.ts @@ -37,19 +37,30 @@ export function joinTarget(to: string, rest: string): string { return `${trimEndChar(head, "/")}${rest}${tail}`; } +/** The contract's `maxLength` of a redirect's `from` (`redirect.schema.json`). */ +export const REDIRECT_FROM_MAX_LENGTH = 1024; + /** * The proper ancestors of a path that a redirect's `from` can be, longest first, including `/`. - * A `from` has no empty segment, so no ancestor that holds `//` can match: the walk starts below - * the first `//` (#174; a run of slashes used to yield one useless ancestor per slash). + * A `from` is at most `REDIRECT_FROM_MAX_LENGTH` characters and has no empty segment, so the walk + * starts at the longest ancestor within that length and below the first `//` (#174: a long + * request path used to yield one ancestor per segment, quadratic in size, on every 404). */ export function ancestorPaths(path: string): string[] { const out: string[] = []; let p = path; - const cut = path.indexOf("//"); + let first: string | undefined; + if (p.length > REDIRECT_FROM_MAX_LENGTH) { + const slash = p.lastIndexOf("/", REDIRECT_FROM_MAX_LENGTH); + p = slash > 0 ? p.slice(0, slash) : "/"; + first = p; + } + const cut = p.indexOf("//"); if (cut >= 0) { - p = path.slice(0, cut) || "/"; - out.push(p); + p = p.slice(0, cut) || "/"; + first = p; } + if (first !== undefined) out.push(first); while (p.length > 1) { p = p.slice(0, p.lastIndexOf("/")) || "/"; out.push(p); diff --git a/packages/core-ts/src/search-rows.ts b/packages/core-ts/src/search-rows.ts index deb81fcb..1ca67d6d 100644 --- a/packages/core-ts/src/search-rows.ts +++ b/packages/core-ts/src/search-rows.ts @@ -31,6 +31,12 @@ const ENTITIES: Record = { const clip = (text: string) => text.length > SEARCH_TEXT_MAX ? text.slice(0, SEARCH_TEXT_MAX) : text; +/** + * A directive line (`::name{…}`, `:::name`). Its braces stay on one line: under `m` every line + * terminator starts a line, and `[^\n]*` running across `\r` was quadratic (#174). + */ +export const DIRECTIVE_LINE = /^:{2,}[a-z][\w-]*(\{[^\n\r\u2028\u2029]*\})?[ \t]*$/gim; + /** * An article body as plain text for the index: code and generated UI dropped, links and images * reduced to their words, URLs and markup removed. Directive attributes keep their quoted text @@ -42,7 +48,7 @@ export function searchText(body: Pick) // fenced code and ```surface fences; linear scanners where a regex would backtrack (#174) text = stripFences(text) // directive lines: keep the text of quoted attributes - .replace(/^:{2,}[a-z][\w-]*(\{[^\n]*\})?[ \t]*$/gim, (_m, attrs: string | undefined) => + .replace(DIRECTIVE_LINE, (_m, attrs: string | undefined) => attrs ? [...attrs.matchAll(/="((?:[^"\\]|\\.)*)"/g)].map((a) => a[1]).join(" ") : " ", ) .replace(/^:{3,}[ \t]*$/gm, " "); diff --git a/packages/core-ts/src/search-text-scan.ts b/packages/core-ts/src/search-text-scan.ts index 9c39fb8e..1b529c77 100644 --- a/packages/core-ts/src/search-text-scan.ts +++ b/packages/core-ts/src/search-text-scan.ts @@ -55,12 +55,15 @@ export function stripFences(text: string): string { } let out = ""; let from = 0; + // The opening line ends at the next "\n" (not at "\r" or U+2028, which also start lines): + // openers come in order, so the answer is kept until an opener passes it. + let nl: number | undefined; for (const s of starts) { if (s < from) continue; const f = fenceAt(s); if (!f) continue; - const nl = text.indexOf("\n", s); - if (nl < 0) continue; + if (nl === undefined || nl < s) nl = text.indexOf("\n", s); + if (nl < 0) break; // no "\n" left: no later opener has a body either for (let m = f.length; m >= 3; m--) { const t = firstAtLeast(closers.get(f.c + m) ?? [], nl + 1); if (t === undefined) continue; diff --git a/packages/core-ts/test/redos-174.test.ts b/packages/core-ts/test/redos-174.test.ts index 1c1dd90b..4de2474a 100644 --- a/packages/core-ts/test/redos-174.test.ts +++ b/packages/core-ts/test/redos-174.test.ts @@ -7,8 +7,14 @@ import type { Redirect } from "@cloudhal/core-contracts"; import { describe, expect, it } from "vitest"; import { cssUrlValues, stripCssComments, svgPixels } from "../src/media/css-scan.ts"; import { checkSvg } from "../src/media/svg.ts"; -import { ancestorPaths, joinTarget, matchRedirect, normalisePath } from "../src/redirects.ts"; -import { searchText } from "../src/search-rows.ts"; +import { + ancestorPaths, + joinTarget, + matchRedirect, + normalisePath, + REDIRECT_FROM_MAX_LENGTH, +} from "../src/redirects.ts"; +import { DIRECTIVE_LINE, searchText } from "../src/search-rows.ts"; import { linkTexts, stripFences, stripTags } from "../src/search-text-scan.ts"; /** Deterministic strings: every run of the suite checks the same inputs. */ @@ -54,6 +60,7 @@ describe("redirect paths (#174 item 2)", () => { }; /** What a redirect `from` can be (the contract's pattern). */ const FROM = /^\/(?:[^\s?#/\\]+(?:\/[^\s?#/\\]+)*)?$/; + const validFrom = (a: string) => FROM.test(a) && a.length <= REDIRECT_FROM_MAX_LENGTH; it("normalisePath and joinTarget answer as before", () => { for (const p of strings(["/", "a", "?", "#", "b"], 3000, 12)) @@ -73,7 +80,7 @@ describe("redirect paths (#174 item 2)", () => { it("ancestorPaths keeps every ancestor a redirect could match", () => { for (const raw of strings(["/", "a", "b"], 3000, 12)) { const p = `/${raw}`; - expect(ancestorPaths(p), p).toEqual(oldAncestors(p).filter((a) => FROM.test(a))); + expect(ancestorPaths(p), p).toEqual(oldAncestors(p).filter(validFrom)); expect( ancestorPaths(p).every((a) => FROM.test(a)), p, @@ -92,6 +99,20 @@ describe("redirect paths (#174 item 2)", () => { fast(100, () => expect(normalisePath(`/a${"/".repeat(N)}b/`)).toBe(`/a${"/".repeat(N)}b`)); fast(100, () => expect(joinTarget(`/t${"/".repeat(N)}`, "/r")).toBe("/t/r")); fast(100, () => expect(ancestorPaths(`/a${"/".repeat(N)}b`)).toEqual(["/a", "/"])); + // Many short segments: only ancestors within a `from`'s length are listed, so the output + // stays bounded (it used to be one ancestor per segment, quadratic in total size). + const long = "/a".repeat(N / 2); + const ancestors = fast(100, () => ancestorPaths(long)); + expect(ancestors[0]).toBe("/a".repeat(REDIRECT_FROM_MAX_LENGTH / 2)); + expect(ancestors).toEqual(oldAncestors(long.slice(0, 4000)).filter(validFrom)); + expect(ancestors.reduce((n, a) => n + a.length, 0)).toBeLessThanOrEqual( + REDIRECT_FROM_MAX_LENGTH * (REDIRECT_FROM_MAX_LENGTH / 2 + 1), + ); + // Long fuzzed paths still give the old answer filtered to what a `from` can be. + for (const raw of strings(["/", "a", "bb"], 200, 900)) { + const p = `/${raw}`; + expect(ancestorPaths(p)).toEqual(oldAncestors(p).filter(validFrom)); + } }); }); @@ -151,10 +172,23 @@ describe("search text (#174 item 4)", () => { } for (const t of strings(["<", ">", "a", "\n"], 3000, 12)) expect(stripTags(t), t).toBe(t.replace(/<[^>]*>/g, " ")); + // The directive pattern now keeps its braces on one line; without "\r" or U+2028/2029 in + // the body (markdown from Studio and agents is "\n"-only) it matches as before. + const OLD_DIRECTIVE = /^:{2,}[a-z][\w-]*(\{[^\n]*\})?[ \t]*$/gim; + for (const t of strings([":", ":", "a", "{", "}", " ", "\n", "x", '"', "="], 4000, 14)) + expect(t.replace(DIRECTIVE_LINE, "<$1>"), JSON.stringify(t)).toBe( + t.replace(OLD_DIRECTIVE, "<$1>"), + ); }); it("stays linear on openers without closers", () => { fast(150, () => stripFences("```a\n".repeat(N / 5))); + // Line ends without "\n": every opener used to search to the end for one. + fast(150, () => stripFences("```a\r".repeat(N / 5))); + fast(150, () => stripFences("```a\u2028".repeat(N / 5))); + fast(150, () => stripFences(`${"```a\r".repeat(N / 5)}\n`)); + // Directive lines with CR endings: `[^\n]*` used to run across every following line. + fast(150, () => searchText({ format: "markdown", value: "::a{}x\r".repeat(N / 7) })); fast(100, () => linkTexts("![a".repeat(N / 3), true)); fast(100, () => linkTexts("[a](".repeat(N / 4), false)); fast(100, () => linkTexts("[a]x".repeat(N / 4), false)); diff --git a/packages/schedules/src/xml-scan.ts b/packages/schedules/src/xml-scan.ts index 5975bbeb..ed36bcab 100644 --- a/packages/schedules/src/xml-scan.ts +++ b/packages/schedules/src/xml-scan.ts @@ -107,6 +107,21 @@ function mediaTagFinder(text: string) { }; } +/** + * `indexOf(needle, from)` that keeps its last answer: a query at or after the last one, and not + * past the kept position, is answered without searching again (-1 stays -1). + */ +function forwardFinder(text: string, needle: string): (from: number) => number { + let asked = -1; + let found = -1; + return (from) => { + if (asked >= 0 && from >= asked && (found < 0 || found >= from)) return found; + asked = from; + found = text.indexOf(needle, from); + return found; + }; +} + /** At `p`: `url`, optional spaces, `=`, optional spaces, a quote; the value and where it ends. */ function urlAttribute(text: string, p: number): { value: string; end: number } | undefined { if (isWord(text[p - 1]) || !text.startsWith("url", p)) return undefined; @@ -132,16 +147,16 @@ function urlAttribute(text: string, p: number): { value: string; end: number } | export function mediaUrlValues(text: string): string[] { const out: string[] = []; const nextTag = mediaTagFinder(text); + // Tags are visited in order, so the next `>` and the next `url` are each found once and kept + // until a query passes them: a per-tag `indexOf` made `` × n quadratic. + const nextGt = forwardFinder(text, ">"); + const nextUrl = forwardFinder(text, "url"); let from = 0; for (let tag = nextTag(from); tag; tag = nextTag(from)) { - const gt = text.indexOf(">", tag.nameEnd); + const gt = nextGt(tag.nameEnd); const stop = gt < 0 ? text.length : gt; let found: { value: string; end: number } | undefined; - for ( - let p = text.indexOf("url", tag.nameEnd); - p >= 0 && p < stop; - p = text.indexOf("url", p + 1) - ) { + for (let p = nextUrl(tag.nameEnd); p >= 0 && p < stop; p = nextUrl(p + 1)) { found = urlAttribute(text, p); if (found) break; } diff --git a/packages/schedules/test/xml-scan.test.ts b/packages/schedules/test/xml-scan.test.ts index 862b34d7..ceb59550 100644 --- a/packages/schedules/test/xml-scan.test.ts +++ b/packages/schedules/test/xml-scan.test.ts @@ -89,6 +89,13 @@ describe("feed-check scanners (#174 item 4)", () => { fast(100, () => mediaUrlValues(" mediaUrlValues(''.repeat(N / 19))); + // Per-tag searches for `>` and `url` used to run to the end of the feed for every tag. + fast(100, () => mediaUrlValues(' mediaUrlValues("".repeat(N / 11))); + fast(100, () => + mediaUrlValues(`${'>`), + ); + expect(mediaUrlValues(' expect(urlPath(`https://x/a${"/".repeat(N)}b/`)).toBe(`/a${"/".repeat(N)}b`)); }); }); diff --git a/packages/ui/test/markdown-blocks-redos.test.ts b/packages/ui/test/markdown-blocks-redos.test.tsx similarity index 100% rename from packages/ui/test/markdown-blocks-redos.test.ts rename to packages/ui/test/markdown-blocks-redos.test.tsx From d3d98c9a833e9bb231ad5acf1239c21b4cc1d230 Mon Sep 17 00:00:00 2001 From: Madraka <8898640+Madraka@users.noreply.github.com> Date: Mon, 9 Nov 2026 00:40:45 +0300 Subject: [PATCH 3/3] test: align ReDoS timing bounds; code-point cap in ancestorPaths (#174) - Every new timing assertion is 1.5 s on 200k inputs (more where the old regex grew slowly), as in #168's tests; the public reset test warms the app first. - ancestorPaths caps at 2 x 1024 UTF-16 units: a from's maxLength is counted in code points, so astral froms longer than 512 characters still match. The `//` cut is found on the whole path, so the first ancestor never ends in "/". - Cases for U+2029 and CRLF fences, U+2028/2029 directives, and a multi-enclosure feed (data-url kept as the regex read it). Co-Authored-By: Claude Opus 5.5 Signed-off-by: Madraka <8898640+Madraka@users.noreply.github.com> --- .changeset/redos-sweep-174.md | 2 +- apps/docs/test/paths.test.ts | 4 +- .../test/frontmatter-lines.test.ts | 10 +- packages/api/test/public-input.test.ts | 26 +++-- packages/core-ts/src/redirects.ts | 33 ++++--- packages/core-ts/test/redos-174.test.ts | 95 ++++++++++++------- packages/schedules/test/xml-scan.test.ts | 51 +++++++--- .../ui/test/markdown-blocks-redos.test.tsx | 4 +- 8 files changed, 145 insertions(+), 80 deletions(-) diff --git a/.changeset/redos-sweep-174.md b/.changeset/redos-sweep-174.md index f4657ab6..c493751d 100644 --- a/.changeset/redos-sweep-174.md +++ b/.changeset/redos-sweep-174.md @@ -9,7 +9,7 @@ Second ReDoS sweep (#174). Input that made a regex backtrack is now handled in linear time; results are unchanged. - **Unauthenticated password-reset request.** `POST /api/password-reset/request` (and invite creation) checks the address length before anything else, and the address check no longer backtracks: `a@` followed by many dots used to cost seconds of CPU per request. Public token endpoints (`/api/password-reset/*`, invite acceptance) refuse a JSON body over 64 KiB with `413 payload_too_large`, whether or not it declares a `content-length`. -- **core-ts.** Redirect path normalising and matching (on every site request): `normalisePath`, `joinTarget`, and `ancestorPaths`, which no longer lists ancestors that no redirect can match: those longer than a `from` may be (1024 characters) or holding `//`. Directive lines in search text keep their braces on one line, so a directive whose `{…}` spans a lone `\r` is no longer read as one. Search text built at publish time: fenced code, links, images and tags. SVG upload checks: CSS comments, `url(…)` references (about cubic on `url(` followed by spaces) and width/height lengths (new `css-scan.ts`). +- **core-ts.** Redirect path normalising and matching (on every site request): `normalisePath`, `joinTarget`, and `ancestorPaths`, which no longer lists ancestors that no redirect can match: those longer than a `from` may be (1024 code points) or holding `//`. Directive lines in search text keep their braces on one line, so a directive whose `{…}` spans a lone `\r` is no longer read as one. Search text built at publish time: fenced code, links, images and tags. SVG upload checks: CSS comments, `url(…)` references (about cubic on `url(` followed by spaces) and width/height lengths (new `css-scan.ts`). - **schedules.** The feed check's CDATA, element and ``/`` url scans, and `urlPath`. - **agent-runtime.** SKILL.md frontmatter lines (a line of spaces was quadratic). - **Studio** (in `@cloudhal/cli`): the markdown body editor's trailing-newline trims. diff --git a/apps/docs/test/paths.test.ts b/apps/docs/test/paths.test.ts index bd8c6cc9..74db59fe 100644 --- a/apps/docs/test/paths.test.ts +++ b/apps/docs/test/paths.test.ts @@ -22,7 +22,7 @@ describe("isPagePath (#174)", () => { it("stays linear on a long run of dots", () => { const start = performance.now(); - expect(isPagePath(`/${".".repeat(100_000)}/`)).toBe(true); - expect(performance.now() - start).toBeLessThan(100); + expect(isPagePath(`/${".".repeat(200_000)}/`)).toBe(true); + expect(performance.now() - start).toBeLessThan(1500); }); }); diff --git a/packages/agent-runtime/test/frontmatter-lines.test.ts b/packages/agent-runtime/test/frontmatter-lines.test.ts index b26d0bcd..bb751038 100644 --- a/packages/agent-runtime/test/frontmatter-lines.test.ts +++ b/packages/agent-runtime/test/frontmatter-lines.test.ts @@ -39,11 +39,11 @@ describe("frontmatter lines (#174 item 5)", () => { it("stays linear on lines that made the regexes backtrack", () => { const start = performance.now(); - expect(nestedLine(" ".repeat(100_000))).toBeUndefined(); - expect(nestedLine(` key:${" ".repeat(100_000)}x\r`)).toBeUndefined(); - expect(topLine(`name:${" ".repeat(100_000)}x\r`)).toBeUndefined(); - const fm = `---\nmetadata:\n${" ".repeat(100_000)}\n---\nbody`; + expect(nestedLine(" ".repeat(200_000))).toBeUndefined(); + expect(nestedLine(` key:${" ".repeat(200_000)}x\r`)).toBeUndefined(); + expect(topLine(`name:${" ".repeat(200_000)}x\r`)).toBeUndefined(); + const fm = `---\nmetadata:\n${" ".repeat(200_000)}\n---\nbody`; expect(() => parseFrontmatter(fm)).not.toThrow(); - expect(performance.now() - start).toBeLessThan(100); + expect(performance.now() - start).toBeLessThan(1500); }); }); diff --git a/packages/api/test/public-input.test.ts b/packages/api/test/public-input.test.ts index 8229b757..8fa55cfe 100644 --- a/packages/api/test/public-input.test.ts +++ b/packages/api/test/public-input.test.ts @@ -54,10 +54,10 @@ describe("isEmailAddress (#174)", () => { it("answers in linear time on input that made the old regex backtrack", () => { // The old regex took about a second on 40k dots and grew with the square of the length. const start = performance.now(); - expect(isEmailAddress(`a@${".".repeat(100_000)}@`)).toBe(false); - expect(isEmailAddress(`a@b${".".repeat(100_000)}`)).toBe(false); + expect(isEmailAddress(`a@${".".repeat(200_000)}@`)).toBe(false); + expect(isEmailAddress(`a@b${".".repeat(200_000)}`)).toBe(false); expect(isEmailAddress(`a@${".".repeat(200)}@`)).toBe(false); - expect(performance.now() - start).toBeLessThan(100); + expect(performance.now() - start).toBeLessThan(1500); }); }); @@ -92,15 +92,23 @@ describe("public token endpoints (#174)", () => { body, }); - it("refuses a 100k-character address quickly, before any regex sees it", async () => { + it("refuses an adversarial address quickly, before any regex sees it", async () => { const app = await setup(); - const email = `a@${".".repeat(60_000)}@`; // under the body cap; the old regex took ~2 s - const start = performance.now(); - const res = await post(app, "/api/password-reset/request", JSON.stringify({ email })); - expect(res.status).toBe(400); - expect(performance.now() - start).toBeLessThan(500); + // A valid request first, so the timed ones measure the check and not the app's first call. const ok = await post(app, "/api/password-reset/request", JSON.stringify({ email: "x@y.z" })); expect(ok.status).toBe(202); + // 60k dots fit under the body cap (the old regex took about 2 s); 200k do not (it took + // about 23 s, and the old body had no cap). + for (const [dots, status] of [ + [60_000, 400], + [200_000, 413], + ] as const) { + const email = `a@${".".repeat(dots)}@`; + const start = performance.now(); + const res = await post(app, "/api/password-reset/request", JSON.stringify({ email })); + expect(res.status).toBe(status); + expect(performance.now() - start).toBeLessThan(1500); + } }); it("answers 413 for a body above the cap, with or without content-length", async () => { diff --git a/packages/core-ts/src/redirects.ts b/packages/core-ts/src/redirects.ts index dbb98014..8aa6f7ab 100644 --- a/packages/core-ts/src/redirects.ts +++ b/packages/core-ts/src/redirects.ts @@ -37,30 +37,33 @@ export function joinTarget(to: string, rest: string): string { return `${trimEndChar(head, "/")}${rest}${tail}`; } -/** The contract's `maxLength` of a redirect's `from` (`redirect.schema.json`). */ +/** The contract's `maxLength` of a redirect's `from` (`redirect.schema.json`), in code points. */ export const REDIRECT_FROM_MAX_LENGTH = 1024; /** * The proper ancestors of a path that a redirect's `from` can be, longest first, including `/`. - * A `from` is at most `REDIRECT_FROM_MAX_LENGTH` characters and has no empty segment, so the walk - * starts at the longest ancestor within that length and below the first `//` (#174: a long - * request path used to yield one ancestor per segment, quadratic in size, on every 404). + * A `from` has at most `REDIRECT_FROM_MAX_LENGTH` code points, so at most twice that many UTF-16 + * units, and no empty segment: the walk starts at the longest ancestor within that many units + * and below the first `//` (#174: a long request path used to yield one ancestor per segment, + * quadratic in size, on every 404). */ export function ancestorPaths(path: string): string[] { const out: string[] = []; - let p = path; - let first: string | undefined; - if (p.length > REDIRECT_FROM_MAX_LENGTH) { - const slash = p.lastIndexOf("/", REDIRECT_FROM_MAX_LENGTH); - p = slash > 0 ? p.slice(0, slash) : "/"; - first = p; + const maxUnits = 2 * REDIRECT_FROM_MAX_LENGTH; + // The longest ancestor worth walking from ends before `end`. + let end = path.length; + if (end > maxUnits) { + const slash = path.lastIndexOf("/", maxUnits); + end = slash > 0 ? slash : 0; } - const cut = p.indexOf("//"); - if (cut >= 0) { - p = p.slice(0, cut) || "/"; - first = p; + // On the whole path, so a `//` that straddles the cut is found too. + const cut = path.indexOf("//"); + if (cut >= 0 && cut < end) end = cut; + let p = path; + if (end < path.length) { + p = path.slice(0, end) || "/"; + out.push(p); } - if (first !== undefined) out.push(first); while (p.length > 1) { p = p.slice(0, p.lastIndexOf("/")) || "/"; out.push(p); diff --git a/packages/core-ts/test/redos-174.test.ts b/packages/core-ts/test/redos-174.test.ts index 4de2474a..da496a44 100644 --- a/packages/core-ts/test/redos-174.test.ts +++ b/packages/core-ts/test/redos-174.test.ts @@ -34,15 +34,18 @@ function strings(alphabet: readonly string[], count: number, maxLength: number, return out; } -/** Runs `f` and fails when it takes longer than `ms` (generous: CI machines are busy). */ -function fast(ms: number, f: () => T): T { +/** + * Runs `f` and fails when it takes longer than 1.5 s, the bound of the repo's ReDoS tests (#168): + * linear code answers in milliseconds, the replaced regexes took seconds on these inputs. + */ +function fast(f: () => T): T { const start = performance.now(); const value = f(); - expect(performance.now() - start).toBeLessThan(ms); + expect(performance.now() - start).toBeLessThan(1500); return value; } -const N = 100_000; +const N = 200_000; describe("redirect paths (#174 item 2)", () => { const oldNormalise = (path: string) => { @@ -60,7 +63,9 @@ describe("redirect paths (#174 item 2)", () => { }; /** What a redirect `from` can be (the contract's pattern). */ const FROM = /^\/(?:[^\s?#/\\]+(?:\/[^\s?#/\\]+)*)?$/; - const validFrom = (a: string) => FROM.test(a) && a.length <= REDIRECT_FROM_MAX_LENGTH; + /** The contract's `maxLength` counts code points (ajv), not UTF-16 units. */ + const validFrom = (a: string) => FROM.test(a) && [...a].length <= REDIRECT_FROM_MAX_LENGTH; + const usable = (list: string[]) => list.filter(validFrom); it("normalisePath and joinTarget answer as before", () => { for (const p of strings(["/", "a", "?", "#", "b"], 3000, 12)) @@ -95,23 +100,45 @@ describe("redirect paths (#174 item 2)", () => { expect(matchRedirect(redirects, "//x")?.redirect.from).toBe("/"); }); + it("counts a from's length in code points, and never cuts inside a `//`", () => { + // 600 astral characters: 601 code points (a valid `from`), 1201 UTF-16 units. + const from = `/${"😀".repeat(600)}`; + const redirect = { from, to: "/new", match: "prefix" } as unknown as Redirect; + expect(matchRedirect([redirect], `${from}/x`)?.location).toBe("/new/x"); + // Long enough for the cut (more than 2 × 1024 units): the walk still starts at `from`. + const tail = `/${"x".repeat(1000)}`; + expect(matchRedirect([redirect], `${from}${tail}`)?.location).toBe(`/new${tail}`); + // A `//` right at the cut: the first ancestor stops before it, never ending in "/". + for (const p of [ + `/${"a".repeat(1022)}//b`, + `/${"a".repeat(2046)}//b`, + `/${"a".repeat(2047)}//b`, + ]) { + const list = ancestorPaths(p); + expect(list[0], `${p.length}`).toBe(p.slice(0, p.indexOf("//"))); + expect(list.every((a) => a === "/" || !a.endsWith("/"))).toBe(true); + expect(usable(list)).toEqual(usable(oldAncestors(p))); + } + }); + it("stays linear on long runs of slashes", () => { - fast(100, () => expect(normalisePath(`/a${"/".repeat(N)}b/`)).toBe(`/a${"/".repeat(N)}b`)); - fast(100, () => expect(joinTarget(`/t${"/".repeat(N)}`, "/r")).toBe("/t/r")); - fast(100, () => expect(ancestorPaths(`/a${"/".repeat(N)}b`)).toEqual(["/a", "/"])); + fast(() => expect(normalisePath(`/a${"/".repeat(N)}b/`)).toBe(`/a${"/".repeat(N)}b`)); + fast(() => expect(joinTarget(`/t${"/".repeat(N)}`, "/r")).toBe("/t/r")); + fast(() => expect(ancestorPaths(`/a${"/".repeat(N)}b`)).toEqual(["/a", "/"])); // Many short segments: only ancestors within a `from`'s length are listed, so the output // stays bounded (it used to be one ancestor per segment, quadratic in total size). const long = "/a".repeat(N / 2); - const ancestors = fast(100, () => ancestorPaths(long)); - expect(ancestors[0]).toBe("/a".repeat(REDIRECT_FROM_MAX_LENGTH / 2)); - expect(ancestors).toEqual(oldAncestors(long.slice(0, 4000)).filter(validFrom)); + const ancestors = fast(() => ancestorPaths(long)); + expect(ancestors[0]).toBe("/a".repeat(REDIRECT_FROM_MAX_LENGTH)); + expect(usable(ancestors)).toEqual(usable(oldAncestors(long.slice(0, 5000)))); + const units = 2 * REDIRECT_FROM_MAX_LENGTH; expect(ancestors.reduce((n, a) => n + a.length, 0)).toBeLessThanOrEqual( - REDIRECT_FROM_MAX_LENGTH * (REDIRECT_FROM_MAX_LENGTH / 2 + 1), + units * (units / 2 + 1), ); - // Long fuzzed paths still give the old answer filtered to what a `from` can be. - for (const raw of strings(["/", "a", "bb"], 200, 900)) { + // Long fuzzed paths (some astral) keep every ancestor a `from` could be. + for (const raw of strings(["/", "a", "bb", "😀"], 300, 1600)) { const p = `/${raw}`; - expect(ancestorPaths(p)).toEqual(oldAncestors(p).filter(validFrom)); + expect(usable(ancestorPaths(p))).toEqual(usable(oldAncestors(p))); } }); }); @@ -136,16 +163,16 @@ describe("SVG CSS (#174 item 4)", () => { }); it("stays linear on unclosed comments, url( runs and long lengths", () => { - fast(100, () => stripCssComments("/* ".repeat(N / 3))); - fast(100, () => cssUrlValues(`url(${" ".repeat(N)}`)); - fast(100, () => cssUrlValues("url(".repeat(N / 4))); - fast(100, () => cssUrlValues(`url('${"a".repeat(N)}`)); - fast(100, () => svgPixels(`${"1".repeat(N)}x`)); - fast(100, () => svgPixels(`1${" ".repeat(N)}x`)); + fast(() => stripCssComments("/* ".repeat((2 * N) / 3))); + fast(() => cssUrlValues(`url(${" ".repeat(N)}`)); + fast(() => cssUrlValues("url(".repeat(N / 4))); + fast(() => cssUrlValues(`url('${"a".repeat(N)}`)); + fast(() => svgPixels(`${"1".repeat(N)}x`)); + fast(() => svgPixels(`1${" ".repeat(N)}x`)); // The old `url(` check took minutes on 20k spaces; a whole document with them is quick now // (attribute values are read from a 64 KiB window, so the document case stays below it). const svg = ``; - expect(fast(500, () => checkSvg(svg)).ok).toBe(true); + expect(fast(() => checkSvg(svg)).ok).toBe(true); }); }); @@ -182,17 +209,21 @@ describe("search text (#174 item 4)", () => { }); it("stays linear on openers without closers", () => { - fast(150, () => stripFences("```a\n".repeat(N / 5))); + fast(() => stripFences("```a\n".repeat(N / 5))); // Line ends without "\n": every opener used to search to the end for one. - fast(150, () => stripFences("```a\r".repeat(N / 5))); - fast(150, () => stripFences("```a\u2028".repeat(N / 5))); - fast(150, () => stripFences(`${"```a\r".repeat(N / 5)}\n`)); + fast(() => stripFences("```a\r".repeat(N / 5))); + fast(() => stripFences("```a\u2028".repeat(N / 5))); + fast(() => stripFences(`${"```a\r".repeat(N / 5)}\n`)); + fast(() => stripFences("```a\u2029".repeat(N / 5))); + fast(() => stripFences("```a\r\n".repeat(N / 6))); // Directive lines with CR endings: `[^\n]*` used to run across every following line. - fast(150, () => searchText({ format: "markdown", value: "::a{}x\r".repeat(N / 7) })); - fast(100, () => linkTexts("![a".repeat(N / 3), true)); - fast(100, () => linkTexts("[a](".repeat(N / 4), false)); - fast(100, () => linkTexts("[a]x".repeat(N / 4), false)); - fast(100, () => stripTags(" searchText({ format: "markdown", value: `${"```a\n[x](<".repeat(N / 10)}` })); + fast(() => searchText({ format: "markdown", value: "::a{}x\r".repeat(N / 7) })); + fast(() => searchText({ format: "markdown", value: "::a{}x\u2028".repeat(N / 7) })); + fast(() => searchText({ format: "markdown", value: "::a{}x\u2029".repeat(N / 7) })); + fast(() => linkTexts("![a".repeat(N / 3), true)); + fast(() => linkTexts("[a](".repeat(N / 4), false)); + fast(() => linkTexts("[a]x".repeat(N / 4), false)); + fast(() => stripTags(" searchText({ format: "markdown", value: `${"```a\n[x](<".repeat(N / 10)}` })); }); }); diff --git a/packages/schedules/test/xml-scan.test.ts b/packages/schedules/test/xml-scan.test.ts index ceb59550..4b45244e 100644 --- a/packages/schedules/test/xml-scan.test.ts +++ b/packages/schedules/test/xml-scan.test.ts @@ -22,14 +22,17 @@ function strings(alphabet: readonly string[], count: number, maxLength: number, return out; } -function fast(ms: number, f: () => T): T { +/** Fails when `f` takes longer than 1.5 s, the bound of the repo's ReDoS tests (#168). */ +function fast(f: () => T): T { const start = performance.now(); const value = f(); - expect(performance.now() - start).toBeLessThan(ms); + expect(performance.now() - start).toBeLessThan(1500); return value; } -const N = 100_000; +// 200k characters, more where the old regex's cost grew slowly: each case took the replaced +// regex seconds. +const N = 200_000; const OLD_CDATA = //g; const OLD_MEDIA = /<(?:enclosure|media:content)\b[^>]*?\burl\s*=\s*(["'])(.*?)\1/g; const oldBlocks = (text: string, n: string) => @@ -68,6 +71,26 @@ describe("feed-check scanners (#174 item 4)", () => { expect(mediaUrlValues('')).toEqual([ "https://x/a.mp3", ]); + // A feed with several tags of both kinds: `type=` before and after `url=`, the word `url` + // in text between tags, and `data-url`. The regex has a word boundary between "-" and "url", + // so it took `data-url` for the url attribute; the scanner keeps that (results unchanged). + const feed = [ + "The url of the day", + '', + "", + "see url = here", + '', + '', + "", + ].join("\n"); + expect(mediaUrlValues(feed)).toEqual([ + "https://x/1.mp3", + "https://x/2.jpg", + "https://x/no", + "https://x/no", + "https://x/4.mp3", + ]); + expect(mediaUrlValues(feed)).toEqual([...feed.matchAll(OLD_MEDIA)].map((m) => m[2])); }); it("urlPath trims trailing slashes as before", () => { @@ -82,20 +105,20 @@ describe("feed-check scanners (#174 item 4)", () => { }); it("stays linear on openers without closers", () => { - fast(100, () => cdataParts(" elementContents("".repeat(N / 6), "item")); - fast(100, () => elementContents(" elementContents(`${" mediaUrlValues(" mediaUrlValues(''.repeat(N / 19))); + fast(() => cdataParts(" elementContents("".repeat((2 * N) / 6), "item")); + fast(() => elementContents(" elementContents(`${" mediaUrlValues(" mediaUrlValues(''.repeat(N / 19))); // Per-tag searches for `>` and `url` used to run to the end of the feed for every tag. - fast(100, () => mediaUrlValues(' mediaUrlValues("".repeat(N / 11))); - fast(100, () => + fast(() => mediaUrlValues(' mediaUrlValues("".repeat((2 * N) / 11))); + fast(() => mediaUrlValues(`${'>`), ); expect(mediaUrlValues(' expect(urlPath(`https://x/a${"/".repeat(N)}b/`)).toBe(`/a${"/".repeat(N)}b`)); + fast(() => expect(urlPath(`https://x/a${"/".repeat(N)}b/`)).toBe(`/a${"/".repeat(N)}b`)); }); }); diff --git a/packages/ui/test/markdown-blocks-redos.test.tsx b/packages/ui/test/markdown-blocks-redos.test.tsx index 4fb919f0..d72ad8c5 100644 --- a/packages/ui/test/markdown-blocks-redos.test.tsx +++ b/packages/ui/test/markdown-blocks-redos.test.tsx @@ -19,10 +19,10 @@ describe("markdown block trims (#174)", () => { }); it("merges a block with a long run of blank lines in linear time", () => { - const tall = `x${"\n".repeat(100_000)}y`; + const tall = `x${"\n".repeat(200_000)}y`; const start = performance.now(); const out = mergeBlocks(splitMarkdown(""), [], ["a", tall, "b"]); - expect(performance.now() - start).toBeLessThan(500); + expect(performance.now() - start).toBeLessThan(1500); expect(out).toBe(`a\n\n${tall}\n\nb\n`); }); });