fix: escape grave character in template literals #2867
Conversation
🦋 Changeset detectedLatest commit: ec4ef53 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
rturnq
force-pushed
the
fix-invalid-raw
branch
from
346b39b to
ddd7959
Compare
WalkthroughEscapes backtick (`) characters in template literal raw parts by updating Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (20)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (9)
🧰 Additional context used🧬 Code graph analysis (6)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/template.js (1)
packages/runtime-tags/src/translator/util/body-to-text-literal.ts (1)
packages/runtime-tags/src/translator/core/html-style.ts (3)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/tags/child.js (2)
packages/runtime-tags/src/translator/core/html-script.ts (3)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/template.js (1)
🪛 markdownlint-cli2 (0.18.1)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr.expected.md32-32: Fenced code blocks should have a language specified (MD040, fenced-code-language) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/runtime-tags/src/translator/util/normalize-string-expression.ts
Dismissed
Show dismissed
Hide dismissed
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2867 +/- ##
==========================================
- Coverage 88.07% 88.07% -0.01%
==========================================
Files 368 369 +1
Lines 45891 45889 -2
Branches 3670 3665 -5
==========================================
- Hits 40420 40418 -2
Misses 5439 5439
Partials 32 32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/template.marko (1)
1-5: Consider adding test cases for backslash escaping.The current fixture tests backticks effectively, but doesn't cover edge cases involving backslashes. Consider adding test cases for scenarios like:
- Backslash before backtick: ```
- Multiple consecutive backslashes:
\\- Backslash at various positions
This would ensure comprehensive coverage of the escaping logic.
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr.expected.md (1)
29-29: Optional: Consider adding language specification to the fenced code block.The Mutations section uses a fenced code block without a language identifier. While not critical for test fixtures, adding a language identifier (e.g., ```text) would satisfy the markdownlint rule and improve clarity.
-``` +```text INSERT html
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.changeset/big-horses-sniff.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/.name-cache.json(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/csr-sanitized.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/csr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/tags/child.js(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/template.hydrate.js(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/template.js(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/tags/child.js(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/template.js(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/resume-sanitized.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/resume.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr-sanitized.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/tags/child.marko(1 hunks)packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/template.marko(1 hunks)packages/runtime-tags/src/translator/util/normalize-string-expression.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/tags/child.js (1)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/template.js (5)
$template(1-1)$template(1-1)$walks(2-2)$walks(2-2)$setup(6-9)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/template.js (1)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/tags/child.js (6)
$template(1-1)$template(1-1)$walks(2-2)$walks(2-2)$setup(3-3)$setup(3-3)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/template.js (1)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/tags/child.js (1)
$scope0_id(3-3)
🪛 GitHub Check: CodeQL
packages/runtime-tags/src/translator/util/normalize-string-expression.ts
[failure] 47-47: Incomplete string escaping or encoding
This does not escape backslash characters in the input.
🪛 markdownlint-cli2 (0.18.1)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr.expected.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test: node@18
- GitHub Check: test: node@22
- GitHub Check: test: node@20
🔇 Additional comments (10)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/tags/child.marko (1)
1-1: LGTM!This test fixture appropriately tests the escaping of backticks, single quotes, and double quotes in template content.
.changeset/big-horses-sniff.md (1)
1-5: LGTM!The changeset entry is properly formatted and accurately describes the change as a patch-level update for @marko/runtime-tags.
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/resume-sanitized.expected.md (1)
1-9: LGTM! Correct expected output for sanitized resume mode.The snapshot correctly shows backticks rendered as literal characters in the final HTML output, which is the expected behavior after the escaping fix is applied during template compilation.
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/resume.expected.md (1)
1-21: LGTM! Correct expected output for resume mode.The snapshot correctly demonstrates:
- Backticks rendered as literal characters in the HTML output
- Proper runtime initialization script for hydration
- Correct structure with HTML comments for hydration markers
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/csr.expected.md (1)
1-14: LGTM! Correct expected output for CSR mode.The snapshot correctly shows:
- Backticks rendered as literal characters in the HTML
- Proper DOM mutation tracking for client-side rendering
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr.expected.md (1)
1-41: LGTM! Correct expected output for SSR mode.The snapshot correctly demonstrates server-side rendering with:
- Backticks rendered as literal characters in the final HTML
- Comprehensive mutation tracking showing all DOM operations
- Proper WALKER_RUNTIME initialization
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/template.js (1)
6-6: Excellent! Backtick properly escaped in template literal.This line correctly demonstrates the fix: the backtick is escaped as ``` within the template literal on line 6:
_._html(`<div>${_._escape(count)}${_._el_resume($scope0_id, "#text/0")}\` `);Without this escaping, the backtick would prematurely close the template literal, causing a syntax error. The escaped backtick will render as a literal
`character in the final HTML output.packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/template.js (1)
1-1: Excellent! Backtick properly escaped in template literal.The template string correctly escapes the backtick as ```:
export const $template = `<div><!>\` ${_child_template}</div>`;This escaping is essential to prevent the backtick from prematurely closing the template literal, which would cause a syntax error. The escaped backtick will be rendered as a literal
`character in the DOM.packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/tags/child.js (1)
4-4: LGTM! Correct handling of backtick in regular string.Line 4 correctly uses a regular string (not a template literal) for the HTML content:
_._html("<span>child`\"'</span>");The backtick doesn't need escaping in regular strings—only in template literals where it would otherwise close the literal. This demonstrates that the escaping fix is correctly applied only where necessary.
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/tags/child.js (1)
1-1: LGTM! Correct handling of backtick in regular string.The template correctly uses a regular string (not a template literal):
export const $template = "<span>child`\"'</span>";The backtick doesn't require escaping in regular strings—escaping is only necessary in template literals where the backtick would otherwise close the literal. This demonstrates that the fix correctly targets only template literals.
|
|
||
| return t.templateLiteral( | ||
| strs.map((raw) => t.templateElement({ raw })), | ||
| strs.map((raw) => t.templateElement({ raw: raw.replace(/`/g, "\\`") })), |
There was a problem hiding this comment.
Incomplete escaping: backslashes must be escaped before backticks.
The current implementation only escapes backticks but not backslashes. In JavaScript template literals, both characters require escaping in the raw value. This can lead to syntax errors or incorrect string values when the input contains backslashes.
For example:
- Input
foo\barshould produce rawfoo\\bar, but currently producesfoo\bar(invalid) - Input
foo\`` should produce rawfoo\`, but currently producesfoo\`` (incorrect)
Apply this diff to escape backslashes before backticks:
- strs.map((raw) => t.templateElement({ raw: raw.replace(/`/g, "\\`") })),
+ strs.map((raw) => t.templateElement({ raw: raw.replace(/\\/g, "\\\\").replace(/`/g, "\\`") })),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| strs.map((raw) => t.templateElement({ raw: raw.replace(/`/g, "\\`") })), | |
| strs.map((raw) => | |
| t.templateElement({ | |
| raw: raw.replace(/\\/g, "\\\\").replace(/`/g, "\\`"), | |
| }) | |
| ), |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 47-47: Incomplete string escaping or encoding
This does not escape backslash characters in the input.
🤖 Prompt for AI Agents
In packages/runtime-tags/src/translator/util/normalize-string-expression.ts
around line 47, the code only escapes backticks which leaves backslashes
unescaped and can produce invalid template raw values; update the transformation
to first escape backslashes and then escape backticks (e.g., apply
raw.replace(/\\/g, "\\\\").replace(/`/g, "\\`")) so backslashes are doubled
before backticks are escaped.
ddd7959 to
e3139dd
Compare
packages/runtime-tags/src/translator/util/body-to-text-literal.ts
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/runtime-tags/src/translator/util/normalize-string-expression.ts (1)
47-47: Critical: Backslashes must be escaped before backticks.This issue has already been identified by previous reviewers and CodeQL. The current implementation only escapes backticks but not backslashes, which can produce invalid template literal raw values.
Apply this diff to fix the escaping order:
- strs.map((raw) => t.templateElement({ raw: raw.replace(/`/g, "\\`") })), + strs.map((raw) => t.templateElement({ raw: raw.replace(/\\/g, "\\\\").replace(/`/g, "\\`") })),
🧹 Nitpick comments (2)
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/tags/child.marko (1)
1-4: Add test cases for backslash and "${" in text.To fully cover template-literal escaping, include samples with a backslash "" and a literal "${" inside text/script/style bodies.
I can add a minimal variant to this fixture if you want.
packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr.expected.md (1)
31-46: Label fenced code block to satisfy markdownlint (MD040).Specify a language for the Mutations code fence (e.g., "text").
Apply:
-``` +```text INSERT html ...</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ddd7959614284d5065a60ac5e89d420b402ddbed and e3139dd15f79973b8280b227e4db70d6333fa57c. </details> <details> <summary>📒 Files selected for processing (20)</summary> * `.changeset/big-horses-sniff.md` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/.name-cache.json` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/csr-sanitized.expected.md` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/csr.expected.md` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/tags/child.js` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/template.hydrate.js` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/template.js` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/tags/child.js` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/template.js` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/resume-sanitized.expected.md` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/resume.expected.md` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr-sanitized.expected.md` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr.expected.md` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/tags/child.marko` (1 hunks) * `packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/template.marko` (1 hunks) * `packages/runtime-tags/src/translator/core/html-comment.ts` (3 hunks) * `packages/runtime-tags/src/translator/core/html-script.ts` (3 hunks) * `packages/runtime-tags/src/translator/core/html-style.ts` (3 hunks) * `packages/runtime-tags/src/translator/util/body-to-text-literal.ts` (1 hunks) * `packages/runtime-tags/src/translator/util/normalize-string-expression.ts` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (3)</summary> * .changeset/big-horses-sniff.md * packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/csr-sanitized.expected.md * packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/resume.expected.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (7)</summary> * packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/template.hydrate.js * packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/template.js * packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/tags/child.js * packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr-sanitized.expected.md * packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/.name-cache.json * packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/template.marko * packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/csr.expected.md </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (6)</summary> <details> <summary>packages/runtime-tags/src/translator/core/html-script.ts (3)</summary><blockquote> <details> <summary>packages/runtime-tags/src/translator/util/body-to-text-literal.ts (1)</summary> * `bodyToTextLiteral` (3-25) </details> <details> <summary>packages/runtime-tags/src/translator/util/signals.ts (1)</summary> * `addStatement` (670-696) </details> <details> <summary>packages/runtime-tags/src/translator/util/sections.ts (1)</summary> * `getSection` (151-159) </details> </blockquote></details> <details> <summary>packages/runtime-tags/src/translator/core/html-comment.ts (1)</summary><blockquote> <details> <summary>packages/runtime-tags/src/translator/util/body-to-text-literal.ts (1)</summary> * `bodyToTextLiteral` (3-25) </details> </blockquote></details> <details> <summary>packages/runtime-tags/src/translator/core/html-style.ts (3)</summary><blockquote> <details> <summary>packages/runtime-tags/src/translator/util/body-to-text-literal.ts (1)</summary> * `bodyToTextLiteral` (3-25) </details> <details> <summary>packages/runtime-tags/src/translator/util/signals.ts (1)</summary> * `addStatement` (670-696) </details> <details> <summary>packages/runtime-tags/src/translator/util/sections.ts (1)</summary> * `getSection` (151-159) </details> </blockquote></details> <details> <summary>packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/tags/child.js (2)</summary><blockquote> <details> <summary>packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/dom.expected/tags/child.js (1)</summary> * `value` (4-4) </details> <details> <summary>packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/template.js (1)</summary> * `$scope0_id` (4-4) </details> </blockquote></details> <details> <summary>packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/template.js (1)</summary><blockquote> <details> <summary>packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/tags/child.js (1)</summary> * `$scope0_id` (4-4) </details> </blockquote></details> <details> <summary>packages/runtime-tags/src/translator/util/body-to-text-literal.ts (1)</summary><blockquote> <details> <summary>packages/runtime-tags/src/translator/index.ts (2)</summary> * `MarkoTagBody` (88-90) * `MarkoPlaceholder` (100-102) </details> </blockquote></details> </details><details> <summary>🪛 GitHub Check: CodeQL</summary> <details> <summary>packages/runtime-tags/src/translator/util/body-to-text-literal.ts</summary> [failure] 30-30: Incomplete string escaping or encoding This does not escape backslash characters in the input. </details> <details> <summary>packages/runtime-tags/src/translator/util/normalize-string-expression.ts</summary> [failure] 47-47: Incomplete string escaping or encoding This does not escape backslash characters in the input. </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/ssr.expected.md</summary> 32-32: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)</summary> * GitHub Check: test: node@20 * GitHub Check: test: node@18 * GitHub Check: test: node@22 </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/template.js (1)</summary><blockquote> `6-6`: **LGTM — backtick correctly escaped in template literal.** The emitted `\`` in the quasi prevents breaking the template literal. </blockquote></details> <details> <summary>packages/runtime-tags/src/__tests__/fixtures/escape-html-strings/__snapshots__/html.expected/tags/child.js (1)</summary><blockquote> `1-2`: **Ignore import ordering in snapshots** Fixtures under `__tests__/fixtures/.../__snapshots__` are compared as plain text, not executed as ESM; import placement here is irrelevant. > Likely an incorrect or invalid review comment. </blockquote></details> <details> <summary>packages/runtime-tags/src/translator/core/html-style.ts (1)</summary><blockquote> `15-15`: **LGTM: Refactoring to centralize text literal construction.** The introduction of `bodyToTextLiteral` utility centralizes the logic for converting tag bodies into text literals with proper escaping, improving maintainability. </blockquote></details> <details> <summary>packages/runtime-tags/src/translator/core/html-comment.ts (1)</summary><blockquote> `11-11`: **LGTM: Refactoring to centralize text literal construction.** The introduction of `bodyToTextLiteral` utility centralizes the logic for converting tag bodies into text literals, improving code consistency across HTML-related translators. </blockquote></details> <details> <summary>packages/runtime-tags/src/translator/core/html-script.ts (1)</summary><blockquote> `15-15`: **LGTM: Correct refactoring with proper StringLiteral handling.** The refactoring to use `bodyToTextLiteral` is implemented correctly, including proper access to the `.value` property when writing StringLiteral nodes (line 412). This is the correct pattern that should be followed by the other HTML translator files. Also applies to: 409-430 </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| if (templateExpressions.length === 0) { | ||
| write`${currentQuasi}`; | ||
| if (t.isStringLiteral(textLiteral)) { | ||
| write`${textLiteral}`; |
There was a problem hiding this comment.
Access .value property for StringLiteral nodes.
When textLiteral is a StringLiteral, you should access its .value property to write the actual string content. This matches the correct pattern used in html-script.ts at line 412.
Apply this diff:
- write`${textLiteral}`;
+ write`${textLiteral.value}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| write`${textLiteral}`; | |
| write`${textLiteral.value}`; |
🤖 Prompt for AI Agents
In packages/runtime-tags/src/translator/core/html-comment.ts around line 187,
the code writes textLiteral directly but when textLiteral is a StringLiteral you
must use its .value property; modify the write call to detect if textLiteral is
a StringLiteral (or has a .value) and pass textLiteral.value to write in that
case, otherwise keep writing textLiteral as before so the actual string content
is emitted (mirror the pattern used in html-script.ts).
| if (!referencePlaceholder) { | ||
| write`${currentQuasi}`; | ||
| if (t.isStringLiteral(textLiteral)) { | ||
| write`${textLiteral}`; |
There was a problem hiding this comment.
Access .value property for StringLiteral nodes.
When textLiteral is a StringLiteral, you should access its .value property to write the actual string content. This matches the pattern used correctly in html-script.ts at line 412.
Apply this diff:
- write`${textLiteral}`;
+ write`${textLiteral.value}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| write`${textLiteral}`; | |
| write`${textLiteral.value}`; |
🤖 Prompt for AI Agents
In packages/runtime-tags/src/translator/core/html-style.ts around line 412, the
code writes textLiteral directly which fails for StringLiteral AST nodes; update
the write call to check if textLiteral is a StringLiteral and, if so, use its
.value property (otherwise use the node as before), mirroring the pattern used
in html-script.ts at line 412 so the actual string content is emitted.
| function templateElement(value: string, tail: boolean) { | ||
| return t.templateElement( | ||
| { | ||
| raw: value.replace(/`/g, "\\`"), | ||
| cooked: value, | ||
| }, | ||
| tail, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Incomplete escaping for template literal quasis (risk of invalid/broken output).
When building TemplateElement.raw, only backticks are escaped. You must also escape backslashes and the sequence "${" in quasis; otherwise text like "${x}" will be parsed as an expression, and backslashes may alter escapes.
Apply:
function templateElement(value: string, tail: boolean) {
return t.templateElement(
{
- raw: value.replace(/`/g, "\\`"),
+ raw: value
+ .replace(/\\/g, "\\\\") // escape backslash first
+ .replace(/`/g, "\\`") // escape backtick
+ .replace(/\$\{/g, "\\${"), // escape "${"
cooked: value,
},
tail,
);
}Also consider reusing a shared escape helper (eg, from normalize-string-expression) to avoid divergence.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 30-30: Incomplete string escaping or encoding
This does not escape backslash characters in the input.
🤖 Prompt for AI Agents
In packages/runtime-tags/src/translator/util/body-to-text-literal.ts around
lines 27 to 35, the TemplateElement.raw currently only escapes backticks which
can yield invalid quasis when backslashes or the sequence "${" are present;
update the escaping to also replace backslashes with "\\\\" and replace "${"
with "\\${" (in that order to avoid double-escaping) before passing to raw, and
if a shared escape helper (eg. normalize-string-expression) exists, import and
reuse it instead of duplicating logic to keep escaping consistent.
rturnq
force-pushed
the
fix-invalid-raw
branch
from
e3139dd to
ec4ef53
Compare
1 participant
Description
Escape grave (`) in template literals.
Checklist: