2

The CheckMarx security scanner says that the 4th line:

$j('#h{!category}').on('click', function(event) {

in the following script, causes an XSS vulnerability. Can someone explain the best way to fix it? or if it's a false positive, how can I rewrite the script so it does not happen?

<script type="text/javascript" charset="UTF-8">
        $j = jQuery.noConflict();
        $j(document).ready(function(){
        $j('#h{!category}').on('click', function(event) {        
             $j('#s{!category}').toggle();
             $j('#h{!category}').css("background-color","{!entity.CPHeader__c}");
             $j('#lmenu').css("width","195px")
             $j('#mainbodyout').css("left","220px")
        });
            $j('#lmenu').on('mouseout', function(event) {        
                $j('#s{!category}').hide();
            });
            $j('#s{!category}').on('mouseover', function(event) {        
                 $j('#s{!category}').show();
            });
        });
    </script>

Thanks!

2 Answers 2

5

I believe its because you are using an unescaped output in your JS

$j('#h{!category}').css("background-color","{!entity.CPHeader__c}");

. Instead use the JSENCODE formula

$j('#h{!category}').css("background-color","{!JSENCODE(entity.CPHeader__c)}");

for further details see
https://developer.salesforce.com/page/Secure_Coding_Cross_Site_Scripting#S-Control_Template_and_Formula_Tags

1

The Checkmarx scanner is flagging "naked" (e.g. unencoded) merge-fields in a javascript context, so the following will quiet the scanner:

<script type="text/javascript" charset="UTF-8">
    $j = jQuery.noConflict();
    $j(document).ready(function(){
    $j('#h{!JSENCODE(category)}').on('click', function(event) {        
         $j('#s{!JSENCODE(category)}').toggle();
         $j('#h{!JSENCODE(category)}').css("background-color","{!JSENCODE(entity.CPHeader__c)}");
         $j('#lmenu').css("width","195px")
         $j('#mainbodyout').css("left","220px")
    });
        $j('#lmenu').on('mouseout', function(event) {        
            $j('#s{!JSENCODE(category)}').hide();
        });
        $j('#s{!JSENCODE(category)}').on('mouseover', function(event) {        
             $j('#s{!JSENCODE(category)}').show();
        });
    });
</script>

You may want to assign the merge-fields to variables first or use an anonymous function:

<script>
var category = "{!JSENCODE(category)}";
[...]
$J('#h' + category).on(...)
[...]

or

</script>
(function(c, h) {   [...]     }  
 )('{!JSENCODE(category)}', '{!JSENCODE(entity.CPHeafder__c)}')
</script>

As to whether this is a false positive, it depends on what the values of the merge-fields can be.

However, as a best practice, you should protect these fields with JSENCODE anyways, otherwise you are creating an unnecessary coupling between your controller and javascript code. For example now assume that category is an enum. In the future, you might make the code more dynamic and pull a value from the db. Suddenly you have introduced a stored XSS into your page without changing any of your page code.

This will also make your code easier to audit because you won't need to track down the possible values of 'category' when determining whether this page is vulnerable or not.

2
  • You'd likely want to use JSINHTMLENCODE over JSENCODE in your example - there's a few extra things it catches. May 28, 2015 at 2:52
  • No, that would lead to double encoding and the code would break, because the merge-field values do not pass through an html renderer in a script context. I.e. if you had a div with id 'a&b', JSINHTMLENCODING this value would result in 'a&amp;b', so jquery wouldn't find the div. May 29, 2015 at 0:17

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .