Enhance sast-rule javascript/xss/rule-xss_rule-mustache-escape.yml
Problem
- escapeMarkup is not a valid method in mustache, instead check for global overwrites: ref: https://github.com/janl/mustache.js?tab=readme-ov-file#variables
Mustache.escape = function(text) {return text;}
Solution
Follow the enhance rule checklist.
Designs
- Show closed items
Activity
-
Newest first Oldest first
-
Show all activity Show comments only Show history only
- Isaac Dawson marked this issue as related to #434267 (closed)
marked this issue as related to #434267 (closed)
- Isaac Dawson changed milestone to %Backlog
changed milestone to %Backlog
- Isaac Dawson added Category:SAST devopssecure featureenhancement groupstatic analysis sectionsec typefeature + 1 deleted label
added Category:SAST devopssecure featureenhancement groupstatic analysis sectionsec typefeature + 1 deleted label
- Isaac Dawson removed the relation with #434267 (closed)
removed the relation with #434267 (closed)
- Isaac Dawson added to epic &10971 (closed)
added to epic &10971 (closed)
- Isaac Dawson changed title from Enhance sast-rule javascript/dos/rule-non_literal_regexp.yml to Enhance sast-rule javascript/xss/rule-xss_rule-mustache-escape.yml
changed title from Enhance sast-rule javascript/dos/rule-non_literal_regexp.yml to Enhance sast-rule javascript/xss/rule-xss_rule-mustache-escape.yml
- Isaac Dawson changed the description
Compare with previous version changed the description
- Isaac Dawson mentioned in issue #414898 (closed)
mentioned in issue #414898 (closed)
- Reporter
@dbolkensteyn, I'm picking this up.
1 - Dinesh Bolkensteyn assigned to @jayanakafonseka
assigned to @jayanakafonseka
- Jayanaka Fonseka mentioned in merge request gitlab-org/security-products/tests/sast-rules-apps/javascript-web-apps!3 (merged)
mentioned in merge request gitlab-org/security-products/tests/sast-rules-apps/javascript-web-apps!3 (merged)
- Reporter
Hey @idawson, I've tested this existing rule on Semgrep playground and using the local semgrep CLI as well, but the test file appears to be invalid. Upon further inspection, the test file appears to be an eslint rule test for an eslint rule for the same vulnerability. The following tests also have the same characteristics.
- sast-rules\javascript\buf\test-buffer-noassert.js
- sast-rules\javascript\exec\test-child-process.js
- sast-rules\javascript\random\test-pseudo-random-bytes.js
- sast-rules\javascript\require\test-non-literal-require.js
- sast-rules\javascript\timing\test-possible-timing-attacks.js
- Author Developer
Hey @jayanakafonseka i'm unable to access that playground link. Could you paste the errors?
Collapse replies - Reporter
@idawson, Could you please try this link? There's no error, but it's not identifying anything as vulnerable. Tried some other JS rules as well. Here's another semgrep playground example for the rule possible-timing-attacks.
- Author Developer
The code you are trying to match against is an eslint rule, not an example of vulnerable code incorrectly using mustache.
- Reporter
Are there any other test files that I could use to test these rules?
- Author Developer
Hmm, maybe try searching code search? https://github.com/search?type=code&auto_enroll=true&q=%2FMustache.escape%2F+language%3AJavaScript+
(Note i changed this url to point to the correct method to check for
Edited by Isaac Dawson - Author Developer
Just make sure any test files you find / copy are acceptable for use: https://handbook.gitlab.com/handbook/legal/product/#using-open-source-software and https://blueoakcouncil.org/list?
- Author Developer
You are probably best off building a real world app that actually demonstrates the flaw.
- Reporter
I've raised an MR for the JavaScript web apps containing real world scenarios, could you please take a look?
- Author Developer
OK i see now, yes the MR you linked is the correct thing to check for. The eslint rule got me confused, as that method of disabling escaping in mustache is deprecated. We should only be checking for
Mustache.escape = function ...
which you are doing in the real world app MR. Please copy that test for the sast-rule test as well and we should be good. - Reporter
Will do that, thank you for the feedback.
- Isaac Dawson changed milestone to %16.9
changed milestone to %16.9
- Jayson Salazar Rodriguez added workflowin dev label
added workflowin dev label
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#16170 (closed)
mentioned in issue gitlab-org/quality/triage-reports#16170 (closed)
- Adam Cohen mentioned in commit adamcohen/sast-rules-automation@a2fc4eca
mentioned in commit adamcohen/sast-rules-automation@a2fc4eca
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#16274 (closed)
mentioned in issue gitlab-org/quality/triage-reports#16274 (closed)
- 🤖 GitLab Bot 🤖 changed milestone to %16.10
changed milestone to %16.10
- 🤖 GitLab Bot 🤖 added missed:16.9 label
added missed:16.9 label
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#16356 (closed)
mentioned in issue gitlab-org/quality/triage-reports#16356 (closed)
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#16462 (closed)
mentioned in issue gitlab-org/quality/triage-reports#16462 (closed)
- Adam Cohen mentioned in issue #440373
mentioned in issue #440373
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#16632 (closed)
mentioned in issue gitlab-org/quality/triage-reports#16632 (closed)
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#16732 (closed)
mentioned in issue gitlab-org/quality/triage-reports#16732 (closed)
- Contributor
@jayanakafonseka-ext What should the priority of this rule be? Please set the label for this.
- Jayanaka Fonseka added SAST::RulesetP2 label
added SAST::RulesetP2 label
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#16815 (closed)
mentioned in issue gitlab-org/quality/triage-reports#16815 (closed)
- 🤖 GitLab Bot 🤖 changed milestone to %16.11
changed milestone to %16.11
- 🤖 GitLab Bot 🤖 added missed:16.10 label
added missed:16.10 label
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#16921 (closed)
mentioned in issue gitlab-org/quality/triage-reports#16921 (closed)
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#17000 (closed)
mentioned in issue gitlab-org/quality/triage-reports#17000 (closed)
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#17193 (closed)
mentioned in issue gitlab-org/quality/triage-reports#17193 (closed)
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#17273 (closed)
mentioned in issue gitlab-org/quality/triage-reports#17273 (closed)
- 🤖 GitLab Bot 🤖 changed milestone to %17.0
changed milestone to %17.0
- 🤖 GitLab Bot 🤖 added missed:16.11 label
added missed:16.11 label
- Bhavya Kaushal removed workflowin dev label
removed workflowin dev label
- Bhavya Kaushal added workflowcomplete label
added workflowcomplete label
- Contributor
- Jayson Salazar Rodriguez mentioned in merge request gitlab-org/security-products/sast-rules!303 (merged)
mentioned in merge request gitlab-org/security-products/sast-rules!303 (merged)
- Contributor
Closing as per @craigmsmith 's comment in gitlab-org/security-products/sast-rules!303 (comment 1815804896)
1 - Jayson Salazar Rodriguez closed
closed