diff --git a/CHANGELOG.md b/CHANGELOG.md index 17cda04..c9fd451 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,73 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [10.3.1] - 2026-09-10 + +Fixes a polynomial ReDoS reachable from attacker-controlled input in the HTML +sanitizer, the template renderer and the router. No API change. + +### Security +- **`HtmlSanitizer` could be stalled for minutes by one request (high).** Every + tag pattern ended in `[^>]*`. On input containing many `<` and no `>`, the + regex engine rescanned to the end of input from each `<` and backtracked, + making the work quadratic in input size. `MAX_INPUT_SIZE` did not help — 1MB + is *inside* the cap, and quadratic growth at that size extrapolated to roughly + 19 minutes of blocked event loop. Node is single-threaded, so a single request + stalled the whole process; on Lambda it is a timeout you are billed for. + + Measured before the fix: 20KB took 421ms, 40KB 1.6s, 80KB 6.6s — a clean 4x + per doubling. After: 500KB completes in under 60ms. + + The tag patterns now match unquoted characters and fully quoted attribute + values separately. The alternatives are disjoint on their first character, so + the match is deterministic and linear. A naive `[^<>]*` would also have been + linear but wrong — a quoted attribute value may legally contain `<`, and + ending the tag there let attribute content escape into the document as + structure. + + CodeQL flagged two of these sites; the same defect was present at three more + that it did not report. All five are fixed. + +- **`Router` path normalization was quadratic (moderate).** `path.replace(/\/+$/, '')` + has an anchored `+`, so the engine retried from every index in a run of + slashes, scanning to the end each time. Request paths are attacker-controlled, + though bounded by HTTP header limits, so the practical cost was ~100-260ms of + CPU per request rather than minutes. Trailing slashes are now stripped with an + index scan. + +- **`HtmlRenderer` `{{#if}}` condition matching was quadratic (low).** `\s+([^}]+)` + — both parts match a space, so the engine tried every split point and rescanned + to the end for each. Exploiting it requires an attacker-controlled *template*; + values passed as data go through the slot table and are never re-parsed as + template syntax, so ordinary use is not affected. `HtmlRenderer` has no input + cap, so it is fixed regardless: the condition must now start with a non-space, + making the two parts disjoint. + +### Changed +- Sanitizing malformed input where a stray `<` immediately precedes a tag now + removes the whole construct instead of leaving part of it behind. + `HtmlSanitizer.clean('<>')` returns `''` where it previously returned + `'>'`. This is the only observable output difference — verified by diffing + every sanitizer and renderer output against 10.3.0 across a payload corpus — + and it errs toward removing more, not less. + +### Added +- ReDoS regression tests for all three modules, asserting linear-time completion + on pathological input. Against the unfixed code these take 4-55 seconds each + and fail; against the fix they complete in milliseconds, so the bounds are not + timing-sensitive. +- Sanitizer bypass tests covering multi-character sanitization: nested tag + reassembly (`ipt>`), triple nesting, comment splitting, entity and + hex encoding, tab-broken `javascript:` URLs, and `on*` handlers smuggled + through attribute values. + + These pin a mitigation CodeQL cannot see. It reports + `js/incomplete-multi-character-sanitization` on the individual `.replace()` + calls, but the sanitizer repeats the strip until output is stable, escapes any + residual `<`, and re-emits only allowlisted tags from a builder. Those two + alerts are dismissed as false positives; these tests are what keeps that + dismissal honest, by failing if the mitigation is ever removed. + ## [10.3.0] - 2026-09-10 Type-definition fixes found while integrating the package into an AWS Lambda + diff --git a/package-lock.json b/package-lock.json index a241850..1748893 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@designofadecade/server", - "version": "10.3.0", + "version": "10.3.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@designofadecade/server", - "version": "10.3.0", + "version": "10.3.1", "license": "MIT", "dependencies": { "ws": "^8.21.3" diff --git a/package.json b/package.json index 7239dda..430021c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@designofadecade/server", - "version": "10.3.0", + "version": "10.3.1", "description": "Design of a Decade Server - A modern TypeScript server with WebSocket support", "type": "module", "main": "./dist/index.js", diff --git a/src/router/Router.security.test.ts b/src/router/Router.security.test.ts new file mode 100644 index 0000000..7293cb6 --- /dev/null +++ b/src/router/Router.security.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect } from 'vitest'; +import Router from './Router.ts'; +import Routes from './Routes.ts'; + +/** + * Route paths are attacker-controlled. Trailing slashes were stripped with + * `replace(/\/+$/, '')`, whose anchored `+` makes the engine retry from every + * index in a run of slashes, scanning to the end each time — quadratic. A + * request of many slashes turned into O(n^2) work on the shared event loop. + */ +describe('Router path normalization ReDoS resistance', () => { + const BUDGET_MS = 2000; + + const build = () => + new Router({ + initRoutes: [ + class extends Routes { + constructor(router: Router) { + super(router); + this.addRoute('/ok', ['GET'], async () => ({ status: 200, body: 'ok' })); + } + }, + ], + }); + + it('normalizes a long run of slashes in linear time', async () => { + const router = build(); + const started = performance.now(); + await router.lambdaEvent({ + requestContext: { http: { method: 'GET', path: '/'.repeat(200_000) + 'x' } }, + headers: {}, + }); + expect(performance.now() - started).toBeLessThan(BUDGET_MS); + }); + + it('still routes and still strips trailing slashes', async () => { + const router = build(); + const hit = await router.lambdaEvent({ + requestContext: { http: { method: 'GET', path: '/ok///' } }, + headers: {}, + }); + expect(hit.statusCode).toBe(200); + + const root = await router.lambdaEvent({ + requestContext: { http: { method: 'GET', path: '/' } }, + headers: {}, + }); + expect(root.statusCode).toBe(404); + }); +}); diff --git a/src/router/Router.ts b/src/router/Router.ts index e554746..7237552 100644 --- a/src/router/Router.ts +++ b/src/router/Router.ts @@ -180,8 +180,7 @@ export default class Router { // Normalize: collapse multiple slashes, remove trailing slash (except root) let normalizedPath = route.path.replace(/\/+/g, '/'); - normalizedPath = - normalizedPath.length > 1 ? normalizedPath.replace(/\/+$/, '') : normalizedPath; + normalizedPath = Router.#stripTrailingSlashes(normalizedPath); for (const method of route.methods) { const pathMethodKey = `${normalizedPath}::${method}`; @@ -214,7 +213,7 @@ export default class Router { method: string ): RouteRegistration | { handler: (request: RouterRequest) => Promise } | null { // Normalize path: remove trailing slash (except for root "/") - const normalizedPath = path.length > 1 ? path.replace(/\/+$/, '') : path; + const normalizedPath = Router.#stripTrailingSlashes(path); if (this.#routes.static.has(`${normalizedPath}::${method}`)) return this.#routes.static.get(`${normalizedPath}::${method}`)!; @@ -236,6 +235,20 @@ export default class Router { return route || null; } + /** + * Strip trailing slashes, keeping a lone "/" intact. + * + * `replace(/\/+$/, '')` looks equivalent but is quadratic: the anchored `+` + * makes the engine retry from every index in a run of slashes, each time + * scanning to the end before failing. A path is attacker-controlled, so a + * request of many slashes turned into O(n^2) work on the event loop. + */ + static #stripTrailingSlashes(path: string): string { + let end = path.length; + while (end > 1 && path.charCodeAt(end - 1) === 47 /* '/' */) end--; + return end === path.length ? path : path.slice(0, end); + } + #pruneCache(): void { if (this.#routes.cache.size > Router.#MAX_CACHE_SIZE) { const firstKey = this.#routes.cache.keys().next().value; diff --git a/src/sanitizer/HtmlSanitizer.security.test.ts b/src/sanitizer/HtmlSanitizer.security.test.ts new file mode 100644 index 0000000..f2632f7 --- /dev/null +++ b/src/sanitizer/HtmlSanitizer.security.test.ts @@ -0,0 +1,104 @@ +import { describe, it, expect } from 'vitest'; +import HtmlSanitizer from './HtmlSanitizer.ts'; + +/** + * Sanitizer input is attacker-controlled by definition, so the cost of + * sanitizing has to stay linear in input size. + * + * The tag patterns used to end in `[^>]*`. On input with many '<' and no '>' + * the engine rescanned to the end of input from every '<' and backtracked, + * making the work quadratic: 80KB took ~6.6s and 1MB — inside MAX_INPUT_SIZE — + * extrapolated to roughly 19 minutes of blocked event loop for one request. + * + * The bounds below are deliberately loose. Post-fix these inputs take tens of + * milliseconds, so a 2s ceiling is ~100x headroom against CI jitter while still + * failing decisively if the quadratic behaviour returns — against the unfixed + * code these same cases take 4-55 seconds each. + * + * These assert an absolute budget rather than a growth ratio between two input + * sizes. At millisecond scale a ratio is dominated by JIT warmup, which made it + * flaky; the absolute ceiling has orders of magnitude more headroom. + */ +describe('HtmlSanitizer ReDoS resistance', () => { + const BUDGET_MS = 2000; + const N = 200_000; + + const timed = (fn: () => unknown): number => { + const started = performance.now(); + fn(); + return performance.now() - started; + }; + + it('sanitizes a long run of "<" in linear time', () => { + expect(timed(() => HtmlSanitizer.clean('<'.repeat(N)))).toBeLessThan(BUDGET_MS); + }); + + it('sanitizes repeated dangerous-tag prefixes in linear time', () => { + const payload = ' HtmlSanitizer.clean(payload))).toBeLessThan(BUDGET_MS); + }); + + it('sanitizes unterminated tags in linear time with an allowlist', () => { + const payload = 'x', + 'x', + 'x', + 'x', + 'hi', + '