Fix makeTemplateNode bypassing HTML sanitization

[phillipc]

Problem

makeTemplateNode in packages/binding.foreach/src/foreach.ts assigned sourceNode.text directly to parentNode.innerHTML for <script> template elements, bypassing all three HTML sanitization controls:

• options.templateSizeLimit

• options.allowScriptTagsInTemplates

• options.sanitizeHtmlTemplate

• Detected in Add TypeScript code review skill and updated findings plans #350

Fix

• Export validateHTMLInput from @tko/utils (was previously internal to html.ts)
• Route the <script> element's .text through validateHTMLInput() before assigning to innerHTML

This ensures the same size-limit, script-tag, and custom sanitizer checks apply to foreach <script> templates as they do everywhere else via parseHtmlFragment.

Testing

All 158 existing foreach tests pass (happy-dom + chromium).

Summary by CodeRabbit

• Bug Fixes

  • Fixed HTML sanitization being bypassed for script-based foreach templates. Templates now properly enforce size limits, nested script detection, and HTML sanitization checks. Applications that embed script markup in foreach templates must enable allowScriptTagsInTemplates: true.

• Tests

  • Added comprehensive test coverage for foreach script template sanitization, including template size limit validation and nested script markup detection.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c2aa878c-be5f-4992-a069-4cb1aea370ca

📥 Commits

Reviewing files that changed from the base of the PR and between 5657f94 and 54a1887.

📒 Files selected for processing (3)

• .changeset/fix-foreach-script-template-sanitization.md
• packages/binding.foreach/spec/eachBehavior.ts
• packages/binding.foreach/src/foreach.ts

---

📝 Walkthrough

Walkthrough

This PR fixes a security vulnerability in the foreach binding where <script> templates bypassed HTML sanitization. The fix routes script template text through the existing parseHtmlForTemplateNodes function, ensuring consistent application of template size limits, disallowed nested script checks, and sanitization. Three test cases validate the fix.

Changes

Script Template Sanitization Fix

Layer / File(s)	Summary
Core Implementation packages/binding.foreach/src/foreach.ts	makeTemplateNode now parses SCRIPT-element templates via parseHtmlForTemplateNodes instead of direct DOM innerHTML assignment, routing them through existing safeguards. Import statements updated to include parseHtmlForTemplateNodes and remove unused types.
Test Validation packages/binding.foreach/spec/eachBehavior.ts	Three new test cases verify: (1) templateSizeLimit enforcement throws on oversized script templates; (2) nested <script> rejection when allowScriptTagsInTemplates is false; (3) sanitizeHtmlTemplate callback is applied before rendering. Each test saves/restores options and cleans up template elements.
Documentation .changeset/fix-foreach-script-template-sanitization.md	Changeset documents the patch fix, noting that apps intentionally embedding <script> markup in foreach templates must enable allowScriptTagsInTemplates: true.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• knockout/tko#219: Both modify HTML template sanitization code paths; retrieved PR adds sanitizeHtmlTemplate/validateHTMLInput that this PR's fix leverages.
• knockout/tko#232: Both touch template parsing logic; retrieved PR refactors nativeTemplateEngine to rely on parseHtmlFragment, while this PR routes SCRIPT templates through parseHtmlForTemplateNodes.

Suggested reviewers

• brianmhunt

Poem

🐰 Scripts hid in templates, bypassing care,
Till our friends found the sanitizer snare!
Now SCRIPT tags dance through parseHTML's gate,
Security checks arrive—never too late! ✨
Foreach templates safe, the rabbit's way,
Building trust, one patch a day! 🛡️

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main change: fixing makeTemplateNode's HTML sanitization bypass for script templates.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[brianmhunt]

Adversarial review

Fix at foreach.ts:60 lands at the right site. Bypass is real. Verified no other unsanitized HTML sinks in packages/**/src.

Must fix before merge:

1. Regression test — none added. Per AGENTS.md ("missing test that would have caught the bug"). Need: templateSizeLimit throw, allowScriptTagsInTemplates: false throw, sanitizeHtmlTemplate invocation.
2. Changeset — none in diff. Post-chore(release): collapse release flow to single human action #377 release pipeline won't bump versions without one. Both @tko/binding.foreach and @tko/utils change behavior.

Worth deciding:
3. Refactor to parseHtmlFragment instead of exporting validateHTMLInput. parseHtmlFragment + parseHtmlForTemplateNodes already do the validate-then-parse contract; matches how binding.template and utils.component consume HTML strings. Avoids adding validateHTMLInput to @tko/utils public API as a SemVer commitment.
4. Behavior change is breaking-ish. Silent-accept → throw on existing apps with <script> foreach templates containing <script> bodies under allowScriptTagsInTemplates: false. Changeset body should call out migration: set allowScriptTagsInTemplates: true if intentional.

Out of scope (note for the record):

• <template> and <div> branches in makeTemplateNode don't run TKO sanitization — they arrive as already-parsed DOM, not strings. TKO controls are string-only by design. Confirm this matches threat model.

[brianmhunt]

Changeset how-to (sorry, should have included this):

bunx changeset add
# select @tko/binding.foreach AND @tko/utils, pick "patch" (or "minor" if treating the silent→throw shift as breaking-ish), paste description
# commit the generated .changeset/<slug>.md with the fix

Or hand-write .changeset/fix-foreach-sanitize-script-template.md:

---
'@tko/binding.foreach': patch
'@tko/utils': patch
---

Fix `makeTemplateNode` bypassing HTML sanitization for `<script>` foreach templates. `validateHTMLInput` is now exported from `@tko/utils`. Behavior change: previously silent acceptance of oversized templates / embedded `<script>` bodies under `allowScriptTagsInTemplates: false` now throws. If you intentionally embed `<script>` content in a foreach template, set `allowScriptTagsInTemplates: true`.

[phillipc]

@brianmhunt I have implemented the requested changes