Linter: Implement html-require-script-nonce rule#1384
Linter: Implement html-require-script-nonce rule#1384marcoroth merged 1 commit intomarcoroth:mainfrom
html-require-script-nonce rule#1384Conversation
89a0241 to
aacd635
Compare
marcoroth
left a comment
There was a problem hiding this comment.
Hey @markokajzer, thanks for working on this rule!
I think we want to wait for #1374, so that we can write the linter rule by just operating on HTMLElementNodes.
And then, we can probably also rename the rule to be html-* prefixed.
|
Just merged #1374 🙌🏼 |
|
Thank you for your tireless work @marcoroth I'll rebase, and give it another shot! 👍 |
aacd635 to
5709c8d
Compare
|
|
||
| get defaultConfig(): FullRuleConfig { | ||
| return { | ||
| enabled: true, |
There was a problem hiding this comment.
just noticed it's not enabled by default in erb_lint. do we want the same here?
There was a problem hiding this comment.
I think its fine to keep this one enabled for now 🙌🏼
| get defaultConfig(): FullRuleConfig { | ||
| return { | ||
| enabled: true, | ||
| severity: "error" |
There was a problem hiding this comment.
what's the correct severity?
There was a problem hiding this comment.
We still haven't really balanced all the severities yet. So I think we can keep this one as error for now, even though it might be a bit harsh/aggressive.
|
@marcoroth I think I got it now! Given that #1374 did not include parsing of Let me know if any other changes are necessary, I wasn't sure about severity and whether we want to enable by default! 🙏 |
5709c8d to
93aa945
Compare
erb-require-script-nonce rulehtml-require-script-nonce rule
marcoroth
left a comment
There was a problem hiding this comment.
Awesome, thank you @markokajzer! I think this looks great!
Given the way the rule is structured, it should also be able to catch javascript_pack_tag once we add support for it in the parser. So I think it's fine to leave that out!
Thank you! 🙏🏼
|
|
||
| get defaultConfig(): FullRuleConfig { | ||
| return { | ||
| enabled: true, |
There was a problem hiding this comment.
I think its fine to keep this one enabled for now 🙌🏼
| get defaultConfig(): FullRuleConfig { | ||
| return { | ||
| enabled: true, | ||
| severity: "error" |
There was a problem hiding this comment.
We still haven't really balanced all the severities yet. So I think we can keep this one as error for now, even though it might be a bit harsh/aggressive.
|
Implemented a small follow up on #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
closes #543
handles three things
<script>tagsjavascript_tag,javascript_include_tag,andjavascript_pack_tagtag.scriptuses #1374 to handle both
<script>andjavascript_taghelpers viaBaseRuleVisitor#visitHTMLElementNode.Note
Does not handle
nilvalues, e.g.tag.script nonce: nilbased on the specs of the inspiration. If wanted, we can add more specs and handlers.