Linter: Warn about j() in erb-no-unsafe-script-interpolation (#1407)

This pull request addresses the comment raised by @frederikspang in
#1330 (comment) by
adding a specific message to the `erb-no-unsafe-script-interpolation`
linter rule to warn about the `j()` and `escape_javascript()` usage
inside `<script>` tags.

It also updates the linter rule to base the checks off of Prism nodes
instead of the previous regex-based approach.

Author: marcoroth

javascript/packages/linter/docs/rules/erb-no-unsafe-script-interpolation.md

@@ -4,11 +4,13 @@
 
 ## Description
 
-ERB interpolation in `<script>` tags must call `.to_json` to safely serialize Ruby data into JavaScript. Without `.to_json`, user-controlled values can break out of string literals and execute arbitrary JavaScript.
+ERB interpolation in `<script>` tags must use `.to_json` to safely serialize Ruby data into JavaScript. Without `.to_json`, user-controlled values can break out of string literals and execute arbitrary JavaScript.
+
+This rule also detects usage of `j()` and `escape_javascript()` inside `<script>` tags and recommends `.to_json` instead, because `j()` is only safe when the output is placed inside quoted string literals, a subtle requirement that is easy to get wrong.
 
 ## Rationale
 
-The main goal of this rule is to assert that Ruby data translates into JavaScript data, but never becomes JavaScript code. ERB output inside `<script>` tags is interpolated directly into the JavaScript context. Without proper serialization via `.to_json`, an attacker can inject arbitrary JavaScript by manipulating the interpolated value.
+The main goal of this rule is to assert that Ruby data translates into JavaScript data, but never becomes JavaScript code. ERB output inside `<script>` tags is interpolated directly into the JavaScript context. Without proper serialization, an attacker can inject arbitrary JavaScript by manipulating the interpolated value.
 
 For example, consider:
 
@@ -18,14 +20,65 @@ For example, consider:
 </script>
 ```
 
-If `user.name` contains `"; alert(1); "`, the resulting JavaScript would execute arbitrary code. Using `.to_json` properly escapes the value and wraps it in quotes:
+If `user.name` contains `"; alert(1); "`, it renders as:
+
+```html
+<script>
+  var name = ""; alert(1); "";
+</script>
+```
+
+This is a Cross-Site Scripting (XSS) vulnerability, as the attacker breaks out of the string literal and executes arbitrary JavaScript.
+
+Using `.to_json` properly escapes the value and wraps it in quotes:
 
 ```erb
 <script>
   var name = <%= user.name.to_json %>;
 </script>
 ```
 
+With the same malicious input `"; alert(1); "`, `.to_json` safely renders:
+
+```html
+<script>
+  var name = "\"; alert(1); \"";
+</script>
+```
+
+The value stays contained as a string, and no code is executed.
+
+### Why not `j()` or `escape_javascript()`?
+
+`j()` escapes characters special inside JavaScript string literals (quotes, newlines, etc.), but it does **not** produce a quoted value. This means it's only safe when wrapped in quotes.
+
+This works, but is fragile. In this example safety depends on the surrounding quotes:
+```erb
+<script>
+  var name = '<%= j user.name %>';
+</script>
+```
+
+Without quotes, `j()` provides no protection and is **UNSAFE**, so code can still be injected:
+
+```erb
+<script>
+  var name = <%= j user.name %>;
+</script>
+```
+
+If `user.name` is `alert(1)`, `j()` passes it through unchanged (no special characters to escape), rendering:
+
+```html
+<script>
+  var name = alert(1);
+</script>
+```
+
+This results in a Cross-Site Scripting (XSS) vulnerability, as the attacker-controlled value is interpreted as JavaScript code rather than a string/data.
+
+`.to_json` is safe in any position because it always produces a valid, quoted JavaScript value.
+
 ## Examples
 
 ### ✅ Good
@@ -68,6 +121,20 @@ If `user.name` contains `"; alert(1); "`, the resulting JavaScript would execute
 </script>
 ```
 
+### ⚠️ Prefer `.to_json` over `j()` / `escape_javascript()`
+
+```diff
+- const url = '<%= j @my_path %>';
++ const url = <%= @my_path.to_json %>;
+```
+
+```diff
+- const name = '<%= escape_javascript(user.name) %>';
++ const name = <%= user.name.to_json %>;
+```
+
 ## References
 
 - [Shopify/better-html — ScriptInterpolation](https://github.com/Shopify/better-html/blob/main/lib/better_html/test_helper/safe_erb/script_interpolation.rb)
+- [`escape_javascript` / `j`](https://api.rubyonrails.org/classes/ActionView/Helpers/JavaScriptHelper.html#method-i-escape_javascript)
+- [`json_escape`](https://api.rubyonrails.org/classes/ERB/Util.html#method-c-json_escape)

javascript/packages/linter/src/rules/erb-no-unsafe-script-interpolation.ts

@@ -1,5 +1,7 @@
 import { ParserRule } from "../types.js"
 import { BaseRuleVisitor } from "./rule-utils.js"
+import { PrismVisitor } from "@herb-tools/core"
+
 import {
   getTagLocalName,
   getAttribute,
@@ -10,9 +12,34 @@ import {
 } from "@herb-tools/core"
 
 import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js"
-import type { ParseResult, HTMLElementNode, Node } from "@herb-tools/core"
+import type { ParseResult, HTMLElementNode, Node, ERBContentNode, ParserOptions, PrismNode } from "@herb-tools/core"
+
+const SAFE_METHOD_NAMES = new Set([
+  "to_json",
+  "json_escape",
+])
+
+const ESCAPE_JAVASCRIPT_METHOD_NAMES = new Set([
+  "j",
+  "escape_javascript",
+])
+
+class SafeCallDetector extends PrismVisitor {
+  public hasSafeCall = false
+  public hasEscapeJavascriptCall = false
+
+  visitCallNode(node: PrismNode): void {
+    if (SAFE_METHOD_NAMES.has(node.name)) {
+      this.hasSafeCall = true
+    }
+
+    if (ESCAPE_JAVASCRIPT_METHOD_NAMES.has(node.name)) {
+      this.hasEscapeJavascriptCall = true
+    }
 
-const SAFE_PATTERN = /\.to_json\b/
+    this.visitChildNodes(node)
+  }
+}
 
 class ERBNoUnsafeScriptInterpolationVisitor extends BaseRuleVisitor {
   visitHTMLElementNode(node: HTMLElementNode): void {
@@ -35,7 +62,6 @@ class ERBNoUnsafeScriptInterpolationVisitor extends BaseRuleVisitor {
     const typeValue = typeAttribute ? getStaticAttributeValue(typeAttribute) : null
 
     if (typeValue === "text/html") return
-
     if (!node.body || node.body.length === 0) return
 
     this.checkNodesForUnsafeOutput(node.body)
@@ -46,9 +72,21 @@ class ERBNoUnsafeScriptInterpolationVisitor extends BaseRuleVisitor {
       if (!isERBNode(child)) continue
       if (!isERBOutputNode(child)) continue
 
-      const content = child.content?.value?.trim() || ""
+      const erbContent = child as ERBContentNode
+      const prismNode = erbContent.prismNode
+      const detector = new SafeCallDetector()
+
+      if (prismNode) detector.visit(prismNode)
+      if (detector.hasSafeCall) continue
+
+      if (detector.hasEscapeJavascriptCall) {
+        this.addOffense(
+          "Avoid `j()` / `escape_javascript()` in `<script>` tags. It is only safe inside quoted string literals. Use `.to_json` instead, which is safe in any position.",
+          child.location,
+        )
 
-      if (SAFE_PATTERN.test(content)) continue
+        continue
+      }
 
       this.addOffense(
         "Unsafe ERB output in `<script>` tag. Use `.to_json` to safely serialize values into JavaScript.",
@@ -68,9 +106,17 @@ export class ERBNoUnsafeScriptInterpolationRule extends ParserRule {
     }
   }
 
+  get parserOptions(): Partial<ParserOptions> {
+    return {
+      prism_nodes: true,
+    }
+  }
+
   check(result: ParseResult, context?: Partial<LintContext>): UnboundLintOffense[] {
     const visitor = new ERBNoUnsafeScriptInterpolationVisitor(this.ruleName, context)
+
     visitor.visit(result.value)
+
     return visitor.offenses
   }
 }

javascript/packages/linter/test/rules/erb-no-unsafe-script-interpolation.test.ts

@@ -8,6 +8,7 @@ import { createLinterTest } from "../helpers/linter-test-helper.js"
 const { expectNoOffenses, expectError, assertOffenses } = createLinterTest(ERBNoUnsafeScriptInterpolationRule)
 
 const MESSAGE = "Unsafe ERB output in `<script>` tag. Use `.to_json` to safely serialize values into JavaScript."
+const ESCAPE_JS_MESSAGE = "Avoid `j()` / `escape_javascript()` in `<script>` tags. It is only safe inside quoted string literals. Use `.to_json` instead, which is safe in any position."
 
 describe("ERBNoUnsafeScriptInterpolationRule", () => {
   describe("unsafe", () => {
@@ -50,6 +51,40 @@ describe("ERBNoUnsafeScriptInterpolationRule", () => {
     })
   })
 
+  describe("escape_javascript", () => {
+    test("j() in script tag is flagged with specific message", () => {
+      expectError(ESCAPE_JS_MESSAGE)
+
+      assertOffenses(dedent`
+        <script>const url = '<%= j @poll_path %>';</script>
+      `)
+    })
+
+    test("j() with parentheses in script tag is flagged with specific message", () => {
+      expectError(ESCAPE_JS_MESSAGE)
+
+      assertOffenses(dedent`
+        <script>const url = '<%= j(@poll_path) %>';</script>
+      `)
+    })
+
+    test("escape_javascript() in script tag is flagged with specific message", () => {
+      expectError(ESCAPE_JS_MESSAGE)
+
+      assertOffenses(dedent`
+        <script>const url = '<%= escape_javascript(@poll_path) %>';</script>
+      `)
+    })
+
+    test("escape_javascript without parentheses in script tag is flagged with specific message", () => {
+      expectError(ESCAPE_JS_MESSAGE)
+
+      assertOffenses(dedent`
+        <script>const url = '<%= escape_javascript @poll_path %>';</script>
+      `)
+    })
+  })
+
   describe("safe", () => {
     test("ERB output in script tag with .to_json is allowed", () => {
       expectNoOffenses(dedent`
@@ -85,6 +120,12 @@ describe("ERBNoUnsafeScriptInterpolationRule", () => {
       `)
     })
 
+    test("json_escape() in script tag is allowed", () => {
+      expectNoOffenses(dedent`
+        <script>const data = '<%= json_escape(@data) %>';</script>
+      `)
+    })
+
     test("script tag with non-executable content type is ignored", () => {
       expectNoOffenses(dedent`
         <script type="text/html">