Skip to content

Decide: restore html-comment-strip broad sweep with framework-marker allowlist #178

@twschiller

Description

@twschiller

Follow-up to #176.

Before #176, html-comment-strip removed every comment outside <script> / <style> / <noscript>. After #176, it scrubs only comments whose data matches the INJECTION_PATTERNS set (currently 11 patterns) — and blanks them in place rather than detaching. The narrowing is what makes React Suspense markers ($, /$, $?, $!, [, ], …) safe automatically: they don't match any pattern, so they're left alone, and the framework's unmount walk doesn't trip on a missing node.

Tradeoff

Narrowing to pattern matches caps coverage at the recognized shapes. Novel injection phrasings, off-pattern prose, and benign-looking comments carrying instruction-shaped text — all of which used to be removed — now stay visible.

Decision needed

Should we restore broad-sweep behavior with a framework-marker allowlist (skip $ / /$ / [ / ]-prefixed data, plus any other markers we identify), and continue blanking via comment.data = \"\" rather than detaching?

Tradeoffs to weigh:

  • For: restores pre-Fix: scrub instead of detach for framework-rendered DOM #176 coverage for off-pattern injection prose; reduces dependence on INJECTION_PATTERNS keeping pace with attacker phrasing.
  • Against: allowlist enumeration becomes load-bearing — a missed framework marker re-introduces the navigation crash class through a different path. Frameworks evolve their hydration marker syntax over time (React 18 → 19 changed marker shapes); we'd need a story for keeping the allowlist in sync.

Either way, no detach — the carrier stays attached.

Suggested next step

Catalog the marker shapes currently in the wild (React 18/19, Vue 3, Svelte 4/5, Astro islands, htmx, Lit hydration) and a few SSR build stamps / license headers we don't want to touch, then re-evaluate whether the allowlist is small/stable enough to be worth the broad sweep.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestquestionFurther information is requested

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions