fix: html script, style, and comment escaping#3159
Merged
DylanPiercey merged 1 commit intomainfrom Apr 17, 2026
Merged
Conversation
🦋 Changeset detectedLatest commit: 57a555e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3159 +/- ##
=======================================
Coverage 89.57% 89.58%
=======================================
Files 370 371 +1
Lines 47086 47134 +48
Branches 4270 4273 +3
=======================================
+ Hits 42177 42224 +47
- Misses 4857 4858 +1
Partials 52 52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThis PR adds patch updates for 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/runtime-tags/src/__tests__/fixtures/html-style-injection/template.marko (1)
1-2: Consider adding a mixed-case</StYlE>variant to harden the regression.Uppercase is covered well; a mixed-case case adds confidence against future regressions in case-insensitive matching.
Based on learnings, tests in this area should be fixture-driven; adding one more fixture case fits that pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-tags/src/__tests__/fixtures/html-style-injection/template.marko` around lines 1 - 2, Add a new fixture case that mirrors the existing test but uses a mixed-case closing tag variant (e.g., "</StYlE>") to ensure case-insensitive matching is robust: update the injection value (the let/injection variable) to include "</StYlE>" and add a corresponding css rule in the <html-style> block (e.g., .evil { content: '${injection}'; }) so the test covers mixed-case closing-tag injection alongside the existing uppercase variant.packages/runtime-tags/src/__tests__/fixtures/html-script-injection/template.marko (1)
1-2: Optional: add mixed-case</ScRiPt>fixture coverage too.This improves long-term confidence that the matcher remains truly case-insensitive.
Based on learnings, fixture-based tests are the expected style for this package area.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-tags/src/__tests__/fixtures/html-script-injection/template.marko` around lines 1 - 2, Add a new fixture (or extend the existing template.marko) that exercises a mixed-case script terminator so the matcher is proven case-insensitive: create a variant where the injected string is "</ScRiPt>" (same symbol name injection used in the diff) and render it inside the <html-script> block (the same html-script usage in template.marko) to ensure the test harness/fixture asserts correct behavior with mixed-case closing tags.packages/runtime-class/test/render/fixtures/escape-comment/test.js (1)
1-3: Add a direct leading->fixture input to mirror the reported bug.
"-->"is good coverage, but a second case like">Uh oh"would directly guard the exact scenario called out in the PR description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-class/test/render/fixtures/escape-comment/test.js` around lines 1 - 3, Add a second fixture entry that includes a string starting with a direct leading '>' to cover the reported edge case; update the exported test data (exports.templateData) or add a new export (e.g., exports.templateDataLeadingGt) to include a value like ">Uh oh" alongside the existing "-->" value so the test harness exercises the exact scenario described in the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/blue-dragons-see.md:
- Around line 6-14: Update the release note text to (1) add the <html-comment>
tag to the list of tags whose dynamic interpolation escaping was tightened so
the sentence includes "<script>, <style>, <html-script>, <html-style> and
<html-comment>", and (2) fix the wording on the sentence containing "wether or
not the closing tag is escaped" to read "whether or not" and change "user
defined" to "user-defined"; keep the existing example and meaning otherwise.
In @.changeset/frank-memes-knock.md:
- Around line 6-13: Update the changeset note to mention both the
`<html-comment>` fix and the script/style close-tag escaping patches: explicitly
state that `<html-comment>` now uses a special escape replacing `>` instead of
the normal XML `<`-based escaping, and add that script and style tag closing
sequences are also fixed to prevent incorrect escaping of their `</...>` close
tags; reference the `<html-comment>` tag and the script/style close-tag behavior
in the single-sentence summary so release notes reflect the full security fix
scope.
In
`@packages/runtime-class/src/runtime/html/helpers/escape-comment-placeholder.js`:
- Around line 15-16: The escapeCommentHelper currently stringifies every value
causing false/null/undefined to render as "false"/"null"/"undefined"; change
escapeCommentHelper to match other HTML escape helpers by returning "" for null,
undefined, and false while preserving 0: i.e., if value === 0 pass "0" to
escape, if value === null || value === undefined || value === false return "",
otherwise stringify and pass to escape; update the exported function
escapeCommentHelper accordingly.
---
Nitpick comments:
In `@packages/runtime-class/test/render/fixtures/escape-comment/test.js`:
- Around line 1-3: Add a second fixture entry that includes a string starting
with a direct leading '>' to cover the reported edge case; update the exported
test data (exports.templateData) or add a new export (e.g.,
exports.templateDataLeadingGt) to include a value like ">Uh oh" alongside the
existing "-->" value so the test harness exercises the exact scenario described
in the PR.
In
`@packages/runtime-tags/src/__tests__/fixtures/html-script-injection/template.marko`:
- Around line 1-2: Add a new fixture (or extend the existing template.marko)
that exercises a mixed-case script terminator so the matcher is proven
case-insensitive: create a variant where the injected string is "</ScRiPt>"
(same symbol name injection used in the diff) and render it inside the
<html-script> block (the same html-script usage in template.marko) to ensure the
test harness/fixture asserts correct behavior with mixed-case closing tags.
In
`@packages/runtime-tags/src/__tests__/fixtures/html-style-injection/template.marko`:
- Around line 1-2: Add a new fixture case that mirrors the existing test but
uses a mixed-case closing tag variant (e.g., "</StYlE>") to ensure
case-insensitive matching is robust: update the injection value (the
let/injection variable) to include "</StYlE>" and add a corresponding css rule
in the <html-style> block (e.g., .evil { content: '${injection}'; }) so the test
covers mixed-case closing-tag injection alongside the existing uppercase
variant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d9f5ab3-6f92-42b0-9ea7-3f2baf66397c
⛔ Files ignored due to path filters (31)
packages/runtime-tags/src/__tests__/fixtures/html-comment-counter/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/__snapshots__/.name-cache.jsonis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/__snapshots__/ssr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-script-injection/__snapshots__/.name-cache.jsonis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-script-injection/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-script-injection/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-script-injection/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-script-injection/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-script-injection/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-script-injection/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-script-injection/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-script-injection/__snapshots__/ssr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-script-injection/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-style-injection/__snapshots__/.name-cache.jsonis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-style-injection/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-style-injection/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-style-injection/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-style-injection/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-style-injection/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-style-injection/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-style-injection/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-style-injection/__snapshots__/ssr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/html-style-injection/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (29)
.changeset/blue-dragons-see.md.changeset/frank-memes-knock.mdpackages/runtime-class/src/runtime/html/helpers/escape-comment-placeholder.jspackages/runtime-class/src/runtime/html/helpers/escape-script-placeholder.jspackages/runtime-class/src/runtime/html/helpers/escape-style-placeholder.jspackages/runtime-class/src/translator/taglib/core/translate-html-comment.jspackages/runtime-class/test/render/fixtures/escape-comment/expected.htmlpackages/runtime-class/test/render/fixtures/escape-comment/template.markopackages/runtime-class/test/render/fixtures/escape-comment/test.jspackages/runtime-class/test/render/fixtures/escape-comment/vdom-expected.htmlpackages/runtime-class/test/render/fixtures/escape-script-case/expected.htmlpackages/runtime-class/test/render/fixtures/escape-script-case/template.markopackages/runtime-class/test/render/fixtures/escape-script-case/test.jspackages/runtime-class/test/render/fixtures/escape-script-case/vdom-expected.htmlpackages/runtime-class/test/render/fixtures/escape-style-case/expected.htmlpackages/runtime-class/test/render/fixtures/escape-style-case/template.markopackages/runtime-class/test/render/fixtures/escape-style-case/test.jspackages/runtime-class/test/render/fixtures/escape-style-case/vdom-expected.htmlpackages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/template.markopackages/runtime-tags/src/__tests__/fixtures/html-comment-placeholder/test.tspackages/runtime-tags/src/__tests__/fixtures/html-script-injection/template.markopackages/runtime-tags/src/__tests__/fixtures/html-script-injection/test.tspackages/runtime-tags/src/__tests__/fixtures/html-style-injection/template.markopackages/runtime-tags/src/__tests__/fixtures/html-style-injection/test.tspackages/runtime-tags/src/__tests__/html-content.test.tspackages/runtime-tags/src/html.tspackages/runtime-tags/src/html/content.tspackages/runtime-tags/src/translator/core/html-comment.tspackages/runtime-tags/src/translator/util/runtime.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix escaping issue for dynamic text interpolation inside
<script>,<style>,<html-script>and<html-style>tags.The issue was that the escaping logic for those tags used a CASE SENSITIVE search for the closing tag which could be bypassed like so:
Note that
scriptandstylethere should never render unsanitized user defined values, regardless of wether or not the closing tag is escaped, since these are conceptually just "eval".Also fixes escaping for
<html-comment>tag.Previously this tag relied on normal xml escaping which looks for
<.This PR updates to have a special escape for
<html-comment>tags that replaces>instead.