From b5235ba722a431fc4e0ac2562f63e27836444b75 Mon Sep 17 00:00:00 2001
From: Sergey <sergey.gorbaty@salesforce.com>
Date: Tue, 21 Feb 2017 15:08:40 -0800
Subject: [PATCH] Improved the rule to catch escaped values

---
 .../vf/rule/security/VfUnescapeElRule.java    | 38 +++++++++++++--
 .../vf/rule/security/xml/VfUnescapeEl.xml     | 48 ++++++++++++++++++-
 2 files changed, 80 insertions(+), 6 deletions(-)

diff --git a/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfUnescapeElRule.java b/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfUnescapeElRule.java
index a5cd22865a..64d8d14b30 100644
--- a/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfUnescapeElRule.java
+++ b/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfUnescapeElRule.java
@@ -10,6 +10,7 @@ import java.util.regex.Pattern;
 import net.sourceforge.pmd.lang.vf.ast.ASTAttribute;
 import net.sourceforge.pmd.lang.vf.ast.ASTElExpression;
 import net.sourceforge.pmd.lang.vf.ast.ASTElement;
+import net.sourceforge.pmd.lang.vf.ast.ASTIdentifier;
 import net.sourceforge.pmd.lang.vf.ast.ASTText;
 import net.sourceforge.pmd.lang.vf.rule.AbstractVfRule;
 
@@ -19,6 +20,9 @@ import net.sourceforge.pmd.lang.vf.rule.AbstractVfRule;
  */
 public class VfUnescapeElRule extends AbstractVfRule {
 
+    private static final String JSINHTMLENCODE = "JSINHTMLENCODE";
+    private static final String URLENCODE = "URLENCODE";
+    private static final String HTMLENCODE = "HTMLENCODE";
     private static final String APEX_PARAM = "apex:param";
     private static final String VALUE = "value";
     private static final String ITEM_VALUE = "itemValue";
@@ -30,7 +34,6 @@ public class VfUnescapeElRule extends AbstractVfRule {
     private static final String APEX_SELECT_OPTION = "apex:selectoption ";
     private static final String FALSE = "false";
 
-    
     @Override
     public Object visit(ASTElement node, Object data) {
         if (doesTagSupportEscaping(node)) {
@@ -54,8 +57,9 @@ public class VfUnescapeElRule extends AbstractVfRule {
                         break;
                     case VALUE:
                     case ITEM_VALUE:
+
                         final ASTElExpression elInVal = attr.getFirstDescendantOfType(ASTElExpression.class);
-                        if (elInVal != null) {
+                        if (doesElContainAnyUnescapedIdentifiers(elInVal)) {
                             isEL = true;
                         }
 
@@ -88,7 +92,33 @@ public class VfUnescapeElRule extends AbstractVfRule {
         return super.visit(node, data);
     }
 
-    private boolean doesTagSupportEscaping(ASTElement node) {
+    private boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression) {
+        if (elExpression == null) {
+            return false;
+        }
+
+        boolean isEscaped = false;
+        List<ASTIdentifier> ids = elExpression.findDescendantsOfType(ASTIdentifier.class);
+        boolean foundAny = !ids.isEmpty();
+
+        for (final ASTIdentifier id : ids) {
+            if (id.getImage() != null) {
+                switch (id.getImage()) {
+                case HTMLENCODE:
+                case URLENCODE:
+                case JSINHTMLENCODE:
+                    isEscaped = true;
+                    break;
+                default:
+                    break;
+                }
+            }
+        }
+
+        return (foundAny && !isEscaped);
+    }
+
+    private boolean doesTagSupportEscaping(final ASTElement node) {
         if (node.getName() == null) {
             return false;
         }
@@ -105,7 +135,7 @@ public class VfUnescapeElRule extends AbstractVfRule {
 
     }
 
-    private boolean hasAnyEL(ASTElement node) {
+    private boolean hasAnyEL(final ASTElement node) {
         final List<ASTElement> innerElements = node.findChildrenOfType(ASTElement.class);
         for (ASTElement element : innerElements) {
             if (element.getName().equalsIgnoreCase(APEX_PARAM)) {
diff --git a/pmd-visualforce/src/test/resources/net/sourceforge/pmd/lang/vf/rule/security/xml/VfUnescapeEl.xml b/pmd-visualforce/src/test/resources/net/sourceforge/pmd/lang/vf/rule/security/xml/VfUnescapeEl.xml
index 6282d84a97..9d9771df85 100644
--- a/pmd-visualforce/src/test/resources/net/sourceforge/pmd/lang/vf/rule/security/xml/VfUnescapeEl.xml
+++ b/pmd-visualforce/src/test/resources/net/sourceforge/pmd/lang/vf/rule/security/xml/VfUnescapeEl.xml
@@ -3,7 +3,7 @@
 
 	<test-code>
 		<description><![CDATA[
-No XSS 
+Default escaped EL - no XSS 
      ]]></description>
 		<expected-problems>0</expected-problems>
 		<code><![CDATA[
@@ -26,9 +26,10 @@ No XSS
 ]]></code>
 		<source-type>vf</source-type>
 	</test-code>
+
 	<test-code>
 		<description><![CDATA[
-XSS via EL and no escaping
+XSS via EL identifier and no escaping
      ]]></description>
 		<expected-problems>1</expected-problems>
 		<code><![CDATA[
@@ -36,6 +37,34 @@ XSS via EL and no escaping
 	<apex:outputText value='XSS might be {! here }' escape="false" />
 </apex:page>
     
+]]></code>
+		<source-type>vf</source-type>
+	</test-code>
+
+	<test-code>
+		<description><![CDATA[
+XSS via EL literal and no escaping
+     ]]></description>
+		<expected-problems>0</expected-problems>
+		<code><![CDATA[
+<apex:page>
+	<apex:outputText value='XSS might be {! 'here' }' escape="false" />
+</apex:page>
+    
+]]></code>
+		<source-type>vf</source-type>
+	</test-code>
+
+<test-code>
+		<description><![CDATA[
+XSS via EL boolean and no escaping
+     ]]></description>
+		<expected-problems>0</expected-problems>
+		<code><![CDATA[
+<apex:page>
+	<apex:outputText value='XSS might be {! false }' escape="false" />
+</apex:page>
+    
 ]]></code>
 		<source-type>vf</source-type>
 	</test-code>
@@ -89,4 +118,19 @@ No XSS via EL via param binding
 ]]></code>
 		<source-type>vf</source-type>
 	</test-code>
+	
+	<test-code>
+		<description><![CDATA[
+No XSS with escaped EL 
+     ]]></description>
+		<expected-problems>0</expected-problems>
+		<code><![CDATA[
+<apex:page>
+	<apex:outputText value=" {!HTMLENCODE(myTextField)}" escape="false"/>
+</apex:page>    
+]]></code>
+		<source-type>vf</source-type>
+	</test-code>
+	
+	
 </test-data>