Parser: Properly resolve nonce: true for javascript_tag (#1452)

In Rails, `nonce: true` on `javascript_include_tag` and `javascript_tag`
resolve to `content_security_policy_nonce` at runtime, and `nonce:
false` omits the attribute. Other helpers like `tag.script` and
`content_tag` pass it through as a literal value.

Previously, `javascript_include_tag` and `javascript_tag` also just
passed along and transformed the `nonce` value as a literal value.

This pulls request adds `resolve_nonce_attribute()` to the parser to
handle this transformation for `javascript_include_tag` and
`javascript_tag`.

So a document like:
```erb
<%= javascript_tag nonce: true do %>
  alert('Hello')
<% end %>
```

Now properly parses as:
```diff
@ DocumentNode (location: (1:0)-(4:0))
└── children: (2 items)
    ├── @ HTMLElementNode (location: (1:0)-(3:9))
    │   ├── open_tag:
    │   │   └── @ ERBOpenTagNode (location: (1:0)-(1:36))
    │   │       ├── tag_opening: "<%=" (location: (1:0)-(1:3))
    │   │       ├── content: " javascript_tag nonce: true do " (location: (1:3)-(1:34))
    │   │       ├── tag_closing: "%>" (location: (1:34)-(1:36))
    │   │       ├── tag_name: "script" (location: (1:4)-(1:18))
    │   │       └── children: (1 item)
    │   │           └── @ HTMLAttributeNode (location: (1:19)-(1:30))
    │   │               ├── name:
    │   │               │   └── @ HTMLAttributeNameNode (location: (1:19)-(1:24))
    │   │               │       └── children: (1 item)
    │   │               │           └── @ LiteralNode (location: (1:19)-(1:24))
    │   │               │               └── content: "nonce"
    │   │               │
    │   │               ├── equals: ": " (location: (1:24)-(1:26))
    │   │               └── value:
    │   │                   └── @ HTMLAttributeValueNode (location: (1:26)-(1:30))
    │   │                       ├── open_quote: ∅
    │   │                       ├── children: (1 item)
-   │   │                       │   └── @ LiteralNode (location: (1:26)-(1:30))
-   │   │                       │       └── content: "true"
+   │   │                       │   └── @ RubyLiteralNode (location: (1:26)-(1:30))
+   │   │                       │       └── content: "content_security_policy_nonce"
    │   │                       │
    │   │                       ├── close_quote: ∅
    │   │                       └── quoted: false
    │   │
    │   ├── tag_name: "script" (location: (1:4)-(1:18))
    │   ├── body: (1 item)
    │   │   └── @ LiteralNode (location: (1:36)-(3:0))
    │   │       └── content: "\n  alert('Hello')\n"
    │   │
    │   ├── close_tag:
    │   │   └── @ ERBEndNode (location: (3:0)-(3:9))
    │   │       ├── tag_opening: "<%" (location: (3:0)-(3:2))
    │   │       ├── content: " end " (location: (3:2)-(3:7))
    │   │       └── tag_closing: "%>" (location: (3:7)-(3:9))
    │   │
    │   ├── is_void: false
    │   └── element_source: "ActionView::Helpers::JavaScriptHelper#javascript_tag"
    │
    └── @ HTMLTextNode (location: (3:9)-(4:0))
        └── content: "\n"
```

So that it can get properly transformed from/to:
```erb
<script nonce="<%= content_security_policy_nonce %>">
  alert('Hello')
</script>
```

Additionally, it updates the `html-require-script-nonce` linter rule to
flag `tag.script` and `content_tag :script` using `nonce: true` or
`nonce: false`, since those produce literal attribute values that will
not match the CSP header. The rule now flags the following:
```erb
<%= tag.script nonce: true do %>
  alert('Hello')
<% end %>
```

with:
```txt
`nonce: true` on `tag.script` outputs a literal `nonce="true"` attribute, which will not match the Content Security Policy header and the browser will block the script. Only `javascript_tag` and `javascript_include_tag` resolve `nonce: true` to the per-request `content_security_policy_nonce`. Use `javascript_tag` with `nonce: true` instead.
```

Follow up on #1384

Author: marcoroth

javascript/packages/linter/src/rules/html-require-script-nonce.ts

@@ -3,7 +3,12 @@ import { BaseRuleVisitor } from "./rule-utils.js"
 import { getTagLocalName, getAttribute, getStaticAttributeValue, hasAttributeValue, findAttributeByName, isERBOpenTagNode } from "@herb-tools/core"
 
 import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js"
-import type { ParseResult, ParserOptions, HTMLElementNode } from "@herb-tools/core"
+import type { ParseResult, ParserOptions, HTMLElementNode, HTMLAttributeNode } from "@herb-tools/core"
+
+const HELPERS_WITH_CSP_NONCE_SUPPORT = [
+  "ActionView::Helpers::AssetTagHelper#javascript_include_tag",
+  "ActionView::Helpers::JavaScriptHelper#javascript_tag",
+]
 
 class RequireScriptNonceVisitor extends BaseRuleVisitor {
   visitHTMLElementNode(node: HTMLElementNode): void {
@@ -24,7 +29,39 @@ class RequireScriptNonceVisitor extends BaseRuleVisitor {
         "Missing a `nonce` attribute on `<script>` tag. Use `request.content_security_policy_nonce`.",
         node.tag_name!.location,
       )
+
+      return
+    }
+
+    this.checkLiteralNonceOnTagHelper(node, nonceAttribute)
+  }
+
+  private checkLiteralNonceOnTagHelper(node: HTMLElementNode, nonceAttribute: HTMLAttributeNode): void {
+    if (!node.element_source) return
+    if (HELPERS_WITH_CSP_NONCE_SUPPORT.includes(node.element_source)) return
+
+    const nonceValue = getStaticAttributeValue(nonceAttribute)
+
+    if (nonceValue === "true" || nonceValue === "false") {
+      this.addOffense(
+        `\`nonce: ${nonceValue}\` on \`${this.helperName(node)}\` outputs a literal \`nonce="${nonceValue}"\` attribute, which will not match the Content Security Policy header and the browser will block the script. Only \`javascript_tag\` and \`javascript_include_tag\` resolve \`nonce: true\` to the per-request \`content_security_policy_nonce\`. Use \`javascript_tag\` with \`nonce: true\` instead.`,
+        nonceAttribute.name!.location,
+        undefined,
+        "error",
+      )
+    }
+  }
+
+  private helperName(node: HTMLElementNode): string {
+    if (node.element_source === "ActionView::Helpers::TagHelper#content_tag") {
+      return "content_tag"
+    }
+
+    if (node.element_source === "ActionView::Helpers::TagHelper#tag") {
+      return "tag.script"
     }
+
+    return node.element_source ?? "unknown"
   }
 
   private isJavaScriptTag(node: HTMLElementNode): boolean {

javascript/packages/linter/test/rules/html-require-script-nonce.test.ts

@@ -100,16 +100,76 @@ describe("html-require-script-nonce", () => {
       `)
     })
 
-    test("passes when tag.script is used with nonce", () => {
-      expectNoOffenses(dedent`
+    test("warns when tag.script is used with nonce: true", () => {
+      expectError(
+        '`nonce: true` on `tag.script` outputs a literal `nonce="true"` attribute, which will not match the Content Security Policy header and the browser will block the script. Only `javascript_tag` and `javascript_include_tag` resolve `nonce: true` to the per-request `content_security_policy_nonce`. Use `javascript_tag` with `nonce: true` instead.'
+      )
+
+      assertOffenses(dedent`
         <%= tag.script nonce: true %>
       `)
     })
   })
 
+  describe("literal nonce warnings for tag helpers", () => {
+    test("warns when content_tag :script uses nonce: true", () => {
+      expectError(
+        '`nonce: true` on `content_tag` outputs a literal `nonce="true"` attribute, which will not match the Content Security Policy header and the browser will block the script. Only `javascript_tag` and `javascript_include_tag` resolve `nonce: true` to the per-request `content_security_policy_nonce`. Use `javascript_tag` with `nonce: true` instead.'
+      )
+
+      assertOffenses(dedent`
+        <%= content_tag(:script, "alert(1)", nonce: true) %>
+      `)
+    })
+
+    test("warns when content_tag :script uses nonce: false", () => {
+      expectError(
+        '`nonce: false` on `content_tag` outputs a literal `nonce="false"` attribute, which will not match the Content Security Policy header and the browser will block the script. Only `javascript_tag` and `javascript_include_tag` resolve `nonce: true` to the per-request `content_security_policy_nonce`. Use `javascript_tag` with `nonce: true` instead.'
+      )
+
+      assertOffenses(dedent`
+        <%= content_tag(:script, "alert(1)", nonce: false) %>
+      `)
+    })
+
+    test("warns when tag.script uses nonce: true", () => {
+      expectError(
+        '`nonce: true` on `tag.script` outputs a literal `nonce="true"` attribute, which will not match the Content Security Policy header and the browser will block the script. Only `javascript_tag` and `javascript_include_tag` resolve `nonce: true` to the per-request `content_security_policy_nonce`. Use `javascript_tag` with `nonce: true` instead.'
+      )
+
+      assertOffenses(dedent`
+        <%= tag.script(nonce: true) { "alert(1)".html_safe } %>
+      `)
+    })
+
+    test("warns when tag.script uses nonce: false", () => {
+      expectError(
+        '`nonce: false` on `tag.script` outputs a literal `nonce="false"` attribute, which will not match the Content Security Policy header and the browser will block the script. Only `javascript_tag` and `javascript_include_tag` resolve `nonce: true` to the per-request `content_security_policy_nonce`. Use `javascript_tag` with `nonce: true` instead.'
+      )
+
+      assertOffenses(dedent`
+        <%= tag.script(nonce: false) { "alert(1)".html_safe } %>
+      `)
+    })
+
+    test("does not warn when javascript_include_tag uses nonce: true", () => {
+      expectNoOffenses(dedent`
+        <%= javascript_include_tag "application", nonce: true %>
+      `)
+    })
+
+    test("does not warn when javascript_tag uses nonce: true", () => {
+      expectNoOffenses(dedent`
+        <%= javascript_tag nonce: true do %>
+          alert('Hello')
+        <% end %>
+      `)
+    })
+  })
+
   test("passes using unrelated content_tag", () => {
     expectNoOffenses(dedent`
-      <%= content_tag :div, "hello" %>
+      <%= content_tag :div, "hello", nonce: true %>
     `)
   })
 })

javascript/packages/rewriter/test/action-view-tag-helper-to-html.test.ts

@@ -206,6 +206,18 @@ describe("ActionViewTagHelperToHTMLRewriter", () => {
 
       expect(transform(input)).toBe(expected)
     })
+
+    test("tag.script with nonce true passes through as literal", () => {
+      expect(transform('<%= tag.script(nonce: true) { "alert(1)".html_safe } %>')).toBe(
+        '<script nonce="true"><%= "alert(1)".html_safe %></script>'
+      )
+    })
+
+    test("tag.script with nonce false passes through as literal", () => {
+      expect(transform('<%= tag.script(nonce: false) { "alert(1)".html_safe } %>')).toBe(
+        '<script nonce="false"><%= "alert(1)".html_safe %></script>'
+      )
+    })
   })
 
   describe("content_tag helpers", () => {
@@ -232,6 +244,18 @@ describe("ActionViewTagHelperToHTMLRewriter", () => {
         "<div>Block</div>"
       )
     })
+
+    test("content_tag :script with nonce true passes through as literal", () => {
+      expect(transform('<%= content_tag(:script, "alert(1)", nonce: true) %>')).toBe(
+        '<script nonce="true">alert(1)</script>'
+      )
+    })
+
+    test("content_tag :script with nonce false passes through as literal", () => {
+      expect(transform('<%= content_tag(:script, "alert(1)", nonce: false) %>')).toBe(
+        '<script nonce="false">alert(1)</script>'
+      )
+    })
   })
 
   describe("nested helpers", () => {
@@ -430,6 +454,38 @@ describe("ActionViewTagHelperToHTMLRewriter", () => {
         `<script type="application/javascript">alert('Hello')</script>`
       )
     })
+
+    test("javascript_tag with nonce true resolves to content_security_policy_nonce", () => {
+      const input = dedent`
+        <%= javascript_tag nonce: true do %>
+          alert('Hello')
+        <% end %>
+      `
+
+      const expected = dedent`
+        <script nonce="<%= content_security_policy_nonce %>">
+          alert('Hello')
+        </script>
+      `
+
+      expect(transform(input)).toBe(expected)
+    })
+
+    test("javascript_tag with nonce false omits nonce attribute", () => {
+      const input = dedent`
+        <%= javascript_tag nonce: false do %>
+          alert('Hello')
+        <% end %>
+      `
+
+      const expected = dedent`
+        <script>
+          alert('Hello')
+        </script>
+      `
+
+      expect(transform(input)).toBe(expected)
+    })
   })
 
   describe("javascript_include_tag helpers", () => {
@@ -454,15 +510,15 @@ describe("ActionViewTagHelperToHTMLRewriter", () => {
       )
     })
 
-    test("javascript_include_tag with nonce", () => {
+    test("javascript_include_tag with nonce true resolves to content_security_policy_nonce", () => {
       expect(transform(`<%= javascript_include_tag "application", nonce: true %>`)).toBe(
-        `<script src="<%= javascript_path("application") %>" nonce="true"></script>`
+        `<script src="<%= javascript_path("application") %>" nonce="<%= content_security_policy_nonce %>"></script>`
       )
     })
 
-    test("javascript_include_tag with nonce false", () => {
+    test("javascript_include_tag with nonce false omits nonce attribute", () => {
       expect(transform(`<%= javascript_include_tag "application", nonce: false %>`)).toBe(
-        `<script src="<%= javascript_path("application") %>" nonce="false"></script>`
+        `<script src="<%= javascript_path("application") %>"></script>`
       )
     })
 
@@ -504,7 +560,7 @@ describe("ActionViewTagHelperToHTMLRewriter", () => {
 
     test("javascript_include_tag with URL and nonce", () => {
       expect(transform(`<%= javascript_include_tag "http://www.example.com/xmlhr.js", nonce: true %>`)).toBe(
-        `<script src="http://www.example.com/xmlhr.js" nonce="true"></script>`
+        `<script src="http://www.example.com/xmlhr.js" nonce="<%= content_security_policy_nonce %>"></script>`
       )
     })
 

src/analyze/action_view/attribute_extraction_helpers.c

@@ -254,6 +254,58 @@ static AST_HTML_ATTRIBUTE_NODE_T* create_attribute_from_value(
   }
 }
 
+static const char* get_attribute_name_string(AST_HTML_ATTRIBUTE_NODE_T* attribute) {
+  if (!attribute || !attribute->name || !attribute->name->children) { return NULL; }
+  if (hb_array_size(attribute->name->children) == 0) { return NULL; }
+
+  AST_NODE_T* first_child = (AST_NODE_T*) hb_array_get(attribute->name->children, 0);
+  if (!first_child || first_child->type != AST_LITERAL_NODE) { return NULL; }
+
+  AST_LITERAL_NODE_T* literal = (AST_LITERAL_NODE_T*) first_child;
+  return (const char*) literal->content.data;
+}
+
+void resolve_nonce_attribute(hb_array_T* attributes, hb_allocator_T* allocator) {
+  if (!attributes) { return; }
+
+  for (size_t index = 0; index < hb_array_size(attributes); index++) {
+    AST_NODE_T* node = hb_array_get(attributes, index);
+    if (!node || node->type != AST_HTML_ATTRIBUTE_NODE) { continue; }
+
+    AST_HTML_ATTRIBUTE_NODE_T* attribute = (AST_HTML_ATTRIBUTE_NODE_T*) node;
+    const char* name = get_attribute_name_string(attribute);
+    if (!name || strcmp(name, "nonce") != 0) { continue; }
+
+    if (!attribute->value || !attribute->value->children) { continue; }
+    if (hb_array_size(attribute->value->children) == 0) { continue; }
+
+    AST_NODE_T* value_child = (AST_NODE_T*) hb_array_get(attribute->value->children, 0);
+    if (!value_child || value_child->type != AST_LITERAL_NODE) { continue; }
+
+    AST_LITERAL_NODE_T* literal = (AST_LITERAL_NODE_T*) value_child;
+
+    if (hb_string_equals(literal->content, hb_string("true"))) {
+      AST_RUBY_LITERAL_NODE_T* ruby_node = ast_ruby_literal_node_init(
+        hb_string_from_c_string("content_security_policy_nonce"),
+        attribute->value->base.location.start,
+        attribute->value->base.location.end,
+        hb_array_init(0, allocator),
+        allocator
+      );
+
+      hb_array_T* new_children = hb_array_init(1, allocator);
+      hb_array_append(new_children, (AST_NODE_T*) ruby_node);
+      attribute->value->children = new_children;
+      return;
+    }
+
+    if (hb_string_equals(literal->content, hb_string("false"))) {
+      hb_array_remove(attributes, index);
+      return;
+    }
+  }
+}
+
 AST_HTML_ATTRIBUTE_NODE_T* extract_html_attribute_from_assoc(
   pm_assoc_node_t* assoc,
   const uint8_t* source,

src/analyze/action_view/tag_helpers.c

@@ -269,6 +269,11 @@ static AST_NODE_T* transform_tag_helper_with_attributes(
     );
   }
 
+  if (attributes && handler->name
+      && (strcmp(handler->name, "javascript_include_tag") == 0 || strcmp(handler->name, "javascript_tag") == 0)) {
+    resolve_nonce_attribute(attributes, allocator);
+  }
+
   char* helper_content = NULL;
   bool content_is_ruby_expression = false;
 
@@ -604,6 +609,8 @@ static hb_array_T* transform_javascript_include_tag_multi_source(
   );
   if (!shared_attributes) { shared_attributes = hb_array_init(0, allocator); }
 
+  resolve_nonce_attribute(shared_attributes, allocator);
+
   hb_array_T* elements = hb_array_init(source_count * 2, allocator);
 
   for (size_t i = 0; i < source_count; i++) {
@@ -698,6 +705,12 @@ static AST_NODE_T* transform_erb_block_to_tag_helper(
     );
   }
 
+  if (attributes && parse_context->matched_handler && parse_context->matched_handler->name
+      && (strcmp(parse_context->matched_handler->name, "javascript_include_tag") == 0
+          || strcmp(parse_context->matched_handler->name, "javascript_tag") == 0)) {
+    resolve_nonce_attribute(attributes, allocator);
+  }
+
   if (detect_link_to(parse_context->info->call_node, &parse_context->parser)
       && parse_context->info->call_node->arguments && parse_context->info->call_node->arguments->arguments.size >= 1) {
     pm_node_t* first_argument = parse_context->info->call_node->arguments->arguments.nodes[0];

src/include/analyze/action_view/attribute_extraction_helpers.h

@@ -23,6 +23,8 @@ hb_array_T* extract_html_attributes_from_keyword_hash(
   hb_allocator_T* allocator
 );
 
+void resolve_nonce_attribute(hb_array_T* attributes, hb_allocator_T* allocator);
+
 bool has_html_attributes_in_call(pm_call_node_t* call_node);
 
 hb_array_T* extract_html_attributes_from_call_node(

test/analyze/action_view/asset_tag_helper/javascript_include_tag_test.rb

@@ -24,7 +24,7 @@ class JavaScriptIncludeTagTest < Minitest::Spec
       HTML
     end
 
-    test "javascript_include_tag with nonce" do
+    test "javascript_include_tag with nonce true" do
       assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
         <%= javascript_include_tag "application", nonce: true %>
       HTML
@@ -113,5 +113,11 @@ class JavaScriptIncludeTagTest < Minitest::Spec
         <%= javascript_include_tag "application", nonce: "static-#{dynamic}" %>
       HTML
     end
+
+    test "javascript_include_tag with nonce false" do
+      assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
+        <%= javascript_include_tag "application", nonce: false %>
+      HTML
+    end
   end
 end

test/analyze/action_view/javascript_helper/javascript_tag_test.rb

@@ -26,7 +26,7 @@ class JavaScriptTagTest < Minitest::Spec
       HTML
     end
 
-    test "javascript_tag with nonce" do
+    test "javascript_tag with nonce true" do
       assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
         <%= javascript_tag nonce: true do %>
           alert('Hello')
@@ -135,5 +135,13 @@ class JavaScriptTagTest < Minitest::Spec
       assert_parsed_snapshot(template, action_view_helpers: true)
       assert_parsed_snapshot(template)
     end
+
+    test "javascript_tag with nonce false" do
+      assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
+        <%= javascript_tag nonce: false do %>
+          alert('Hello')
+        <% end %>
+      HTML
+    end
   end
 end