ReviewOS

stacks/stx

`stripDocumentWrapper`: unanchored `<!DOCTYPE` regex misclassifies fragments as full documents

#1787
Closed glennmichael123 opened this 9 days ago ·
9 days ago

stripDocumentWrapper decides whether a string is a full HTML document or a fragment using two regexes. One is anchored to the start of the string, the other is not. As a result, the literal text <!DOCTYPE appearing anywhere in a fragment — including inside an HTML comment, a <pre> block, or a JS string — causes that fragment to be treated as a full document.

Location

dist/app-shell.js:117-119 (v0.2.113)

export function stripDocumentWrapper(html, options = {}) {
  const trimmed = html.trim();
  if (!/<!DOCTYPE\b/i.test(trimmed) && !/^<html[\s>]/i.test(trimmed))
    return html;
  // ...strips <head>/<body>/document wrapper from here on
  • /^<html[\s>]/i — anchored. Correct.
  • /<!DOCTYPE\b/i — not anchored. Matches anywhere in the string.

Reproduce

A fragment that is unambiguously not a document:

<div class="docs">
  <!-- every page starts with <!DOCTYPE html> -->
  <p>hello</p>
</div>

stripDocumentWrapper treats this as a full document and proceeds into wrapper-stripping, rather than returning it untouched.

This is easy to hit in practice on any page that documents HTML — docs sites, code samples in <pre><code>, a changelog entry mentioning a doctype.

Suggested fix

Anchor the doctype test the same way the <html> test is anchored:

if (!/^<!DOCTYPE\b/i.test(trimmed) && !/^<html[\s>]/i.test(trimmed))
  return html;

A document's doctype is required to be the first thing in the file, so anchoring loses nothing.

Downstream we've seen a symptom where a literal <html> inside an HTML comment in a page fragment causes SPA navigation to fall back to a full page reload. I have not confirmed that's the same code path — dist/router.js contains no <html reference, so the reload decision appears to be made elsewhere. Filing this separately since the anchoring asymmetry above is demonstrable on its own; happy to open a second issue with a repro for the reload behaviour if that's a distinct path.

Environment

  • @stacksjs/stx 0.2.113
  • Found while auditing template/fragment handling ahead of a migration.
9 days ago

hasDocumentShell in packages/stx/src/document-shell.ts has the same unanchored shape, so the misclassification is not limited to stripDocumentWrapper:

export function hasDocumentShell(html: string): boolean {
  return /<!DOCTYPE\b/i.test(html) || /<html[\s>]/i.test(html)
}

ensureDocumentShell gates on it, so a fragment containing the literal text <html anywhere — including inside an HTML comment or a documentation code sample — is treated as already having a shell and skips wrapping entirely.

The existing doc comment is explicit that this is deliberate ("permissive on placement: if the rendered output contains a <!DOCTYPE> or <html> ANYWHERE, the user has supplied a shell"), and the reasoning given is sound — directives happily prepend <script> tags before any layout markup, so anchoring to the leading bytes made the check spuriously fail. But permissive-anywhere trades one false negative for a false positive, and the false positive is silent: the page renders, just without the shell's head.

Worth flagging because it interacts with #1789: the fragment head-tag hoist runs inside generateDocumentShell, so any fragment misclassified this way skips the hoist too and keeps the unstyled-on-SPA-navigation behaviour that #1789 fixes.

A middle ground that keeps the #1765 fix intact would be to test only outside comments and script/template content, rather than anchoring to the leading bytes.

Sign in to comment on this issue.