Skip to content

fix(security): prototype pollution, HTML text and ReDoS backlog (CodeQL) - #187

Merged
Madraka merged 4 commits into
mainfrom
fix/codeql-backlog-1
Nov 10, 2026
Merged

Madraka merged 4 commits into
mainfrom
fix/codeql-backlog-1

Conversation

@Madraka

@Madraka Madraka commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Closes the production-code CodeQL alerts open on main since the first scan (#160): prototype pollution (42–44), the HTML-text sanitizer (41) and the polynomial/exponential ReDoS backlog (11, 12, 13, 16, 17, 19, 20, 23, 24, 30, 32, 35, 36), plus alert 45, whose real sink was in surfaces/body.ts. The Actions and test/script alerts follow in a second PR.

Prototype pollution (alerts 42, 43, 44)

pointerSet in @cloudhal/core-ts/interop (A2UI updateDataModel, reachable from agent and MCP input) skipped reserved keys silently and wrote through cur[s]. It now lives in surface-interop-data.ts and:

  • refuses __proto__, constructor and prototype at every level with an InteropError (not a silent skip); the check is inline at each step, in front of every write;
  • reads with Object.hasOwn and writes with Object.defineProperty (own data properties only, never a setter or the prototype chain); deletes only own keys;
  • steps into arrays by index only (/list/length is an error).

Other setters found by grepping:

  • Surface binding pointers accepted /__proto__/x, and the Build eval's withSampleInputs (packages/api/src/build-eval.ts) writes through binding paths with at[k] = {} — a real pollution path from model output. checkSurface now reports reserved keys in pointers the page writes through (an input's value/checked, a Scrolly's step, a Button action's path; checkWrittenPointer); read bindings may still name constructor or prototype (they go through the own-only getPointer), so existing pages keep saving. withSampleInputs skips reserved keys and reads own properties only.
  • A2UI fallback data key: an input with a literal value gets its component id as data key; an id prototype/constructor now gets <id>-value instead of failing the conversion (dataKeyFor).
  • MCP merge patches: set_chrome's part merge and update_entry's fields merge assigned out[k] = v for keys from JSON input. A JSON "__proto__" key there swaps the object's prototype (not global, but wrong). Both now refuse reserved keys with ToolError("invalid").
  • JSON Patch (applyPatch / patchPage) already refused reserved keys with an error and used hasOwn; it now shares RESERVED_KEYS from @cloudhal/core-ts/names (new, with isReservedKey).
  • Checked and left: shallow one-level copies keyed by validated names (taxonomy keys, field keys matching the key pattern, directive attrs), and the browser surface runtime (surface-client.ts), which already skips reserved keys and only ever writes the page's own data.

Tests (core-ts/test/interop-pollution.test.ts, mcp-server chrome and fields tests, api/test/design-pass.test.ts): /__proto__/x, /__proto__, /constructor/prototype/x, nested and ~1-encoded forms, deletes; each asserts the error and that ({}).x stays undefined.

htmlText (alert 41)

htmlText stripped tags with /<[^>]*>/ (also a rule-13 pattern) and decoded entities afterwards, so &lt;script&gt; became <script>. Callers and sinks, traced:

Caller Sink Escaped there?
markdownText → archiveHead description <meta name="description" content>, og:/twitter: tags (SeoHead.astro attributes), JSON-LD yes: Astro attribute escaping; jsonLdText escapes <, >, &
markdownText → archiveFeedXml RSS <description> yes: xml()
inlineMarkdownText → correction notes JSON-LD CorrectionComment.text yes: jsonLdText
tableOfContents theme templates (Article.astro {item.text}) yes in the starter; a theme could set:html it
headingSlug heading ids slug is [a-z0-9-]
readingMinutes a word count n/a

Decision (after review): htmlText returns plain text, verbatim (Array&lt;string&gt; reads Array<string>; the first revision's U+FF1C rewrite changed visible text and is gone). Safety is each sink's, and test/html-text-sinks.test.ts proves it sink by sink: &lt;script&gt;alert(1)&lt;/script&gt; &lt;!-- c --&gt; &lt;img onerror&gt; &quot;&gt;&lt;svg onload&gt; goes through each, and the output is parsed with parse5, which must find no element or comment beyond the sink's own:

Sink Escaping Test
<meta name=description>, og:*, twitter:* Astro addAttribute (&, ") each tag parses to one <meta>; content round-trips
JSON-LD (archive, correction notes) jsonLdText (< > & as \u003c…: no </script>, no <!--) one <script> element; JSON round-trips
Archive RSS <description> xml() escaped text; no <script/<img/<svg/<!--
TOC labels ({item.text}) Astro escapeHTML one <a>
Search snippets (not an htmlText caller; innerHTML/set:html) snippetHtml only <mark>

llms.txt does not call htmlText: it is Markdown served as text/plain. No sink needed a fix. The tag strip is still the linear scanner, and entities are decoded once (&amp;#60; is the text &#60;; the old chain decoded it twice). If CodeQL still reports htmlText after merge, that one alert is dismissed with this table as the reason.

ReDoS (rule 13)

Each rewrite is a linear scanner with an equivalence test against the old regex (hand-picked cases plus 20k seeded random strings over the characters that matter, or every short string over a small alphabet) and 100k-character adversarial inputs under 1.5 s.

Alert Site Was Now
24, 35, 36 core-ts/src/media/sniff.ts isSvgText (uploads) exponential on <!-- + --><!--×n (22 repeats ≈ 1.7 s) scanner; a comment ends at its first --> (the regex could stretch one past it: only ever answers "not svg")
32 ui/src/blocks/editor/paste.ts link check quadratic on [×n, [!](×n hasInlineLink: each ]( once, target scans shared
30, 45 core-ts/src/surfaces/body.ts fence opener; closer RegExp built from body text quadratic on tabs + lone \r; CodeQL traced the line into new RegExp (alert 45's "used as a regular expression") fenceOpen, closesFence scanners, no dynamic RegExp
23 core-ts/src/fields/conversions.ts markdownToPlain links/images quadratic on ![×n, ![](×n unlink scanner (fields/markdown-scan.ts)
(same file) escapeMarkdown ^(\s*)…/gm quadratic on blank lines (400 ms at 20k) — not flagged, same file escapeAtLineStarts
20 core-ts/src/domains.ts normalizeHostname [/?#].*$ quadratic with a line break after cutPathPart
19 cloudflare/src/mail.ts MAIL_ADDRESS quadratic on .x runs isMailAddress (new in @cloudhal/api); MAIL_ADDRESS kept, deprecated. The CLI's copy of the regex (config.ts) gets the same linear check locally, so reading cloudhal.json does not load the API
16, 41 astro-renderer/src/markdown.ts tag strip quadratic on <×n stripTags
17 markdown.ts leading H1 cubic on a heading line of spaces (hung at 20k) leadingHeading, exactly the old regex's match (incl. #-only and blank lines after the H1); seeded random comparison over #, space, \n, \r, x, \t, \u2028, plus 3.5M strings offline: 0 differences
13 astro-renderer/src/kit.ts table of contents quadratic on unclosed <h2 id>×n renderedHeadings, closer searched once per depth
12 astro-renderer/src/embed-client.ts ::embed check quadratic on blank/quote lines hasEmbedLine
11 api/src/tokens.ts studioOrigin /\/+$/ quadratic trimEndChar

Checks

  • Review round: astro-renderer 51 files / 369 tests, core-ts 63 / 1127 (alone; chained after another suite the Miniflare D1/DO halves dropped connections again), size:check, design:lint, lint.
  • First round, pnpm exec vitest run per package, one at a time, all green: core-ts 63 files / 1122 tests, astro-renderer 50 / 363, mcp-server 24 / 176, ui 18 / 216, api 45 / 617, cloudflare 14 / 57, cli 63 / 446. (A first core-ts run alongside other suites dropped Miniflare D1 connections in the D1 halves of the store suites; the re-run on its own passed.)
  • pnpm lint (warnings only, as on main), pnpm size:check (surface-interop.ts shrank 1233 → 1192; baseline tightened), pnpm design:lint, tsc --noEmit in all seven packages.

Patch changeset for core-ts, astro-renderer, api, mcp-server, cloudflare and cli (@cloudhal/ui is private).

Follow-up (not in this PR)

core-ts/src/structured-data/values.ts bodyText (HTML bodies → JSON-LD articleBody) has the same strip-then-decode shape and a lazy <script>…</script> scan; CodeQL did not flag it and its sink (jsonLdText) escapes, but it should use the same scanners.

🤖 Generated with Claude Code

Close the production-code CodeQL alerts open on main since the first
scan: 11, 12, 13, 16, 17, 19, 20, 23, 24, 30, 32, 35, 36, 41, 42, 43, 44
and 45.

- Prototype pollution: A2UI data paths refuse __proto__, constructor and
  prototype at every level with an InteropError, write own properties
  only (defineProperty) and step into arrays by index only. Surface
  binding pointers with those keys are a checkSurface issue, the Build
  eval's sample inputs skip them, and set_chrome / update_entry's fields
  merge refuse them. RESERVED_KEYS and isReservedKey in core-ts/names.
- htmlText never returns markup: a linear tag strip, entities decoded
  once, and a `<` that would open markup becomes U+FF1C. Every sink
  escapes too (Astro attributes, jsonLdText, the feeds' xml()).
- ReDoS (AGENTS.md rule 13): linear scanners for the svg sniff
  (exponential on uploads), fence opener and closer (no RegExp built
  from body text), field markdown conversions, hostname normalization,
  leading H1, table of contents, ::embed check, sender address
  (isMailAddress in @cloudhal/api; MAIL_ADDRESS deprecated), Studio's
  origin trim and the editor's paste link check. Each has an
  equivalence test against the old regex (cases plus seeded random
  strings) and 100k adversarial inputs.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Madraka <[email protected]>
Madraka and others added 2 commits November 10, 2026 12:01
- htmlText returns the decoded text verbatim again (no U+FF1C rewrite:
  "Array<string>" and "x<y" read as written). Safety is each sink's,
  now tested one by one with &lt;script&gt;, &lt;!-- and a quote
  breakout, parsed with parse5: meta description, og: and twitter:
  attributes, JSON-LD (archive and correction notes, jsonLdText), the
  archive's RSS description, table-of-contents labels and search
  snippets. Every sink already escaped; none needed a fix.
- The reserved-key check in surface pointers applies only where the
  page writes (input value/checked, Scrolly step, Button action path),
  so read bindings through a data key named constructor or prototype
  stay valid.
- An A2UI input whose component id is a reserved key gets the data key
  `<id>-value` instead of failing the conversion.
- leadingHeading matches the old regex exactly again (#-only and blank
  lines after the H1, `#` followed by a line break, 
 and lone \r
  as tail whitespace), checked by a seeded random comparison over
  #, space, \n, \r, x, \t and 
.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Madraka <[email protected]>
Move the A2UI fallback data-key rule into surface-interop-data.ts
(dataKeyFor), so fromA2ui and surface-interop.ts do not grow.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Madraka <[email protected]>
…id cases

- html-text-sinks: compare against JSON.stringify(DECODED) instead of a
  quote-only replace (CodeQL alert 52), and check Studio's archive
  preview: its description meta (esc()) holds the decoded text as an
  attribute value, and the page parses to no script, img, svg or
  comment.
- interop-pollution: an A2UI input whose component id is __proto__
  converts, folded to the plain id and data key "proto".

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Madraka <[email protected]>
@Madraka
Madraka merged commit f6b2f1d into main Nov 10, 2026
10 of 12 checks passed
@Madraka
Madraka deleted the fix/codeql-backlog-1 branch November 10, 2026 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants