.
+
+PMD holds an initial implementation version of SARIF rendering. This means SARIF allows for more complexity and the
+current implementation can be extended.
+
+[Example](report-examples/pmd-report.sarif.json)
+
## codeclimate
Renderer for Code Climate JSON format.
diff --git a/docs/pages/pmd/userdocs/tools/maven.md b/docs/pages/pmd/userdocs/tools/maven.md
index d9a13cc468..d7998bdfd4 100644
--- a/docs/pages/pmd/userdocs/tools/maven.md
+++ b/docs/pages/pmd/userdocs/tools/maven.md
@@ -26,17 +26,17 @@ report additionally in `` elements. Here's an e
section:
```xml
-
-
-
-
- org.apache.maven.plugins
- maven-pmd-plugin
- {{ page.mpmd_version }}
-
-
-
-
+
+
+
+
+ org.apache.maven.plugins
+ maven-pmd-plugin
+ {{ page.mpmd_version }}
+
+
+
+
```
When defining the version in the pluginManagment section, then it doesn't need to be specified in the normal plugins
@@ -123,22 +123,22 @@ To specify a ruleset, simply edit the previous configuration:
``` xml
-
-
-
- org.apache.maven.plugins
- maven-pmd-plugin
- {{ page.mpmd_version }}
-
-
- /rulesets/java/quickstart.xml
- d:\rulesets\my-ruleset.xml
- http://localhost/design.xml
-
-
-
-
-
+
+
+
+ org.apache.maven.plugins
+ maven-pmd-plugin
+ {{ page.mpmd_version }}
+
+
+ /rulesets/java/quickstart.xml
+ d:\rulesets\my-ruleset.xml
+ http://localhost/design.xml
+
+
+
+
+
```
The value of the 'ruleset' element can either be a relative address, an absolute address or even an url.
@@ -156,17 +156,17 @@ When using the Maven PMD plugin 3.8 or later along with PMD 5.6.0 or later, you
speed up PMD's execution while retaining the quality of the analysis. You can additionally customize where the cache is stored::
```xml
-
- org.apache.maven.plugins
- maven-pmd-plugin
- {{ page.mpmd_version }}
-
-
- true
-
- ${project.build.directory}/pmd/pmd.cache
-
-
+
+ org.apache.maven.plugins
+ maven-pmd-plugin
+ {{ page.mpmd_version }}
+
+
+ true
+
+ ${project.build.directory}/pmd/pmd.cache
+
+
```
#### Other configurations
@@ -175,17 +175,17 @@ The Maven PMD plugin allows you to configure CPD, targetJDK, and the use of XRef
the report to html source files, and the file encoding:
```xml
-
- org.apache.maven.plugins
- maven-pmd-plugin
- {{ page.mpmd_version }}
-
- true
- ISO-8859-1
- 30
- 1.4
-
-
+
+ org.apache.maven.plugins
+ maven-pmd-plugin
+ {{ page.mpmd_version }}
+
+ true
+ ISO-8859-1
+ 30
+ 1.4
+
+
```
#### Upgrading the PMD version at runtime
diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md
index b8f8783555..0db79e7282 100644
--- a/docs/pages/release_notes.md
+++ b/docs/pages/release_notes.md
@@ -14,11 +14,74 @@ This is a {{ site.pmd.release_type }} release.
### New and noteworthy
+#### Java 16 Support
+
+This release of PMD brings support for Java 16. PMD supports [JEP 394: Pattern Matching for instanceof](https://openjdk.java.net/jeps/394) and [JEP 395: Records](https://openjdk.java.net/jeps/395). Both have been promoted
+to be a standard language feature of Java 16.
+
+PMD also supports [JEP 397: Sealed Classes (Second Preview)](https://openjdk.java.net/jeps/397) as a preview
+language feature. In order to analyze a project with PMD that uses these language features, you'll need to enable
+it via the environment variable `PMD_JAVA_OPTS` and select the new language version `16-preview`:
+
+ export PMD_JAVA_OPTS=--enable-preview
+ ./run.sh pmd -language java -version 16-preview ...
+
+Note: Support for Java 14 preview language features have been removed. The version "14-preview" is no longer available.
+
+#### Modified Rules
+
+* The Apex rule {% rule "apex/documentation/ApexDoc" %} has two new properties: `reportPrivate` and
+ `reportProtected`. Previously the rule only considered public and global classes, methods, and
+ properties. With these properties, you can verify the existence of ApexDoc comments for private
+ and protected methods as well. By default, these properties are disabled to preserve backwards
+ compatible behavior.
+
### Fixed Issues
+* apex-documentation
+ * [#3075](https://github.com/pmd/pmd/issues/3075): \[apex] ApexDoc should support private access modifier
+* java
+ * [#3101](https://github.com/pmd/pmd/issues/3101): \[java] NullPointerException when running PMD under JRE 11
+* java-bestpractices
+ * [#3132](https://github.com/pmd/pmd/issues/3132): \[java] UnusedImports with static imports on subclasses
+* java-errorprone
+ * [#2716](https://github.com/pmd/pmd/issues/2716): \[java] CompareObjectsWithEqualsRule: False positive with Enums
+ * [#3089](https://github.com/pmd/pmd/issues/3089): \[java] CloseResource rule throws exception on spaces in property types
+ * [#3133](https://github.com/pmd/pmd/issues/3133): \[java] InvalidLogMessageFormat FP with StringFormattedMessage and ParameterizedMessage
+* plsql
+ * [#3106](https://github.com/pmd/pmd/issues/3106): \[plsql] ParseException while parsing EXECUTE IMMEDIATE 'drop database link ' \|\| linkname;
+
### API Changes
+#### Experimental APIs
+
+* The experimental class `ASTTypeTestPattern` has been renamed to {% jdoc java::lang.java.ast.ASTTypePattern %}
+ in order to align the naming to the JLS.
+* The experimental class `ASTRecordConstructorDeclaration` has been renamed to {% jdoc java::lang.java.ast.ASTCompactConstructorDeclaration %}
+ in order to align the naming to the JLS.
+* The AST types and APIs around Pattern Matching and Records are not experimental anymore:
+ * {% jdoc !!java::lang.java.ast.ASTVariableDeclaratorId#isPatternBinding() %}
+ * {% jdoc java::lang.java.ast.ASTPattern %}
+ * {% jdoc java::lang.java.ast.ASTTypePattern %}
+ * {% jdoc java::lang.java.ast.ASTRecordDeclaration %}
+ * {% jdoc java::lang.java.ast.ASTRecordComponentList %}
+ * {% jdoc java::lang.java.ast.ASTRecordComponent %}
+ * {% jdoc java::lang.java.ast.ASTRecordBody %}
+ * {% jdoc java::lang.java.ast.ASTCompactConstructorDeclaration %}
+
+#### Internal API
+
+Those APIs are not intended to be used by clients, and will be hidden or removed with PMD 7.0.0.
+You can identify them with the `@InternalApi` annotation. You'll also get a deprecation warning.
+
+* The protected or public member of the Java rule {% jdoc java::lang.java.rule.bestpractices.AvoidUsingHardCodedIPRule %}
+ are deprecated and considered to be internal API. They will be removed with PMD 7.
+
### External Contributions
+* [#3098](https://github.com/pmd/pmd/pull/3098): \[apex] ApexDoc optionally report private and protected - [Jonathan Wiesel](https://github.com/jonathanwiesel)
+* [#3107](https://github.com/pmd/pmd/pull/3107): \[plsql] Fix ParseException for EXECUTE IMMEDIATE str1\|\|str2; - [hvbtup](https://github.com/hvbtup)
+* [#3125](https://github.com/pmd/pmd/pull/3125): \[doc] Fix sample code indentation in documentation - [Artur Dryomov](https://github.com/arturdryomov)
+
{% endtocmaker %}
diff --git a/docs/pages/release_notes_old.md b/docs/pages/release_notes_old.md
index cc2bb4a7f7..d06ea6ccf0 100644
--- a/docs/pages/release_notes_old.md
+++ b/docs/pages/release_notes_old.md
@@ -5,6 +5,123 @@ permalink: pmd_release_notes_old.html
Previous versions of PMD can be downloaded here: https://github.com/pmd/pmd/releases
+## 30-January-2021 - 6.31.0
+
+The PMD team is pleased to announce PMD 6.31.0.
+
+This is a minor release.
+
+### Table Of Contents
+
+* [New and noteworthy](#new-and-noteworthy)
+ * [SARIF Format](#sarif-format)
+ * [CPD](#cpd)
+ * [New Rules](#new-rules)
+ * [Deprecated rules](#deprecated-rules)
+* [Fixed Issues](#fixed-issues)
+* [API Changes](#api-changes)
+ * [Deprecated API](#deprecated-api)
+ * [Experimental APIs](#experimental-apis)
+* [External Contributions](#external-contributions)
+* [Stats](#stats)
+
+### New and noteworthy
+
+#### SARIF Format
+
+PMD now supports the [Static Analysis Results Interchange Format (SARIF)](https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=sarif)
+as an additional report format. Just use the [command line parameter](pmd_userdocs_cli_reference.html#format) `-format sarif` to select it.
+SARIF is an OASIS standard format for static analysis tools.
+PMD creates SARIF JSON files in [SARIF version 2.1.0](https://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html).
+An example report can be found in the documentation in [Report formats for PMD](pmd_userdocs_report_formats.html#sarif).
+
+#### CPD
+
+* The C++ module now supports the new option [`--ignore-literal-sequences`](https://pmd.github.io/latest/pmd_userdocs_cpd.html#-ignore-literal-sequences),
+ which can be used to avoid detection of some uninteresting clones. This options has been
+ introduced with PMD 6.30.0 for C# and is now available for C++ as well. See [#2963](https://github.com/pmd/pmd/pull/2963).
+
+#### New Rules
+
+* The new Apex rule [`OverrideBothEqualsAndHashcode`](https://pmd.github.io/pmd-6.31.0/pmd_rules_apex_errorprone.html#overridebothequalsandhashcode) brings the well known Java rule
+ to Apex. In Apex the same principle applies: `equals` and `hashCode` should always be overridden
+ together to ensure collection classes such as Maps and Sets work as expected.
+
+* The new Visualforce rule [`VfHtmlStyleTagXss`](https://pmd.github.io/pmd-6.31.0/pmd_rules_vf_security.html#vfhtmlstyletagxss) checks for potential XSS problems
+ when using ` content.
+ */
+ private void verifyEncoding(
+ ASTElExpression node,
+ ASTContent contentNode,
+ ASTElement elementNode,
+ Object data) {
+ final String previousText = getPreviousText(contentNode, node);
+ final boolean isWithinSafeResource = ElEscapeDetector.startsWithSafeResource(node);
+
+ // if El is inside a tag
+ // and is not surrounded by a safe resource, check for violations
+ if (isStyleTag(elementNode) && !isWithinSafeResource) {
+
+ // check if we are within a URL expression
+ if (isWithinUrlMethod(previousText)) {
+ verifyEncodingWithinUrl(node, data);
+ } else {
+ verifyEncodingWithoutUrl(node, data);
+ }
+ }
+ }
+
+ private boolean isStyleTag(ASTElement elementNode) {
+ // are we dealing with HTML tag?
+ return STYLE_TAG.equalsIgnoreCase(elementNode.getLocalName());
+ }
+
+ private void verifyEncodingWithinUrl(ASTElExpression elExpressionNode, Object data) {
+
+ // only allow URLENCODING or JSINHTMLENCODING
+ if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(
+ elExpressionNode,
+ URLENCODE_JSINHTMLENCODE)) {
+ addViolationWithMessage(
+ data,
+ elExpressionNode,
+ "Dynamic EL content within URL in style tag should be URLENCODED or JSINHTMLENCODED as appropriate");
+ }
+
+ }
+
+ private void verifyEncodingWithoutUrl(ASTElExpression elExpressionNode, Object data) {
+ if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(
+ elExpressionNode,
+ ANY_ENCODE)) {
+ addViolationWithMessage(
+ data,
+ elExpressionNode,
+ "Dynamic EL content in style tag should be appropriately encoded");
+ }
+ }
+
+ private boolean isApexPrefixed(ASTElement node) {
+ return node.isHasNamespacePrefix()
+ && APEX_PREFIX.equalsIgnoreCase(node.getNamespacePrefix());
+ }
+
+ /**
+ * Get text content within style tag that leads up to the ElExpression.
+ * For example, in this snippet:
+ *
+ *
+ * <style>
+ * div {
+ * background: url('{!HTMLENCODE(XSSHere)}');
+ * }
+ * </style>
+ *
+ *
+ * {@code getPreviousText(...)} would return "\n div {\n background: url(".
+ *
+ */
+ private String getPreviousText(ASTContent content, ASTElExpression elExpressionNode) {
+ final int indexInParent = elExpressionNode.getIndexInParent();
+ final VfNode previous = indexInParent > 0 ? content.getChild(indexInParent - 1) : null;
+ return previous instanceof ASTText ? previous.getImage() : "";
+ }
+
+ // visible for unit testing
+ static boolean isWithinUrlMethod(String previousText) {
+ // match for a pattern that
+ // 1. contains "url" (case insensitive),
+ // 2. followed by any number of whitespaces,
+ // 3. a starting bracket "("
+ // 4. and anything else but an ending bracket ")"
+ // For example:
+ // Matches: "div { background: url('", "div { background: Url ( blah"
+ // Does not match: "div { background: url('myUrl')", "div { background: myStyle('"
+
+ return URL_METHOD_PATTERN.matcher(previousText).find();
+ }
+
+}
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 77527770e5..fa4a4b4491 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
@@ -12,22 +12,17 @@ import java.util.Set;
import java.util.regex.Pattern;
import net.sourceforge.pmd.lang.ast.Node;
-import net.sourceforge.pmd.lang.vf.DataType;
-import net.sourceforge.pmd.lang.vf.ast.ASTArguments;
import net.sourceforge.pmd.lang.vf.ast.ASTAttribute;
import net.sourceforge.pmd.lang.vf.ast.ASTContent;
-import net.sourceforge.pmd.lang.vf.ast.ASTDotExpression;
import net.sourceforge.pmd.lang.vf.ast.ASTElExpression;
import net.sourceforge.pmd.lang.vf.ast.ASTElement;
import net.sourceforge.pmd.lang.vf.ast.ASTExpression;
import net.sourceforge.pmd.lang.vf.ast.ASTHtmlScript;
-import net.sourceforge.pmd.lang.vf.ast.ASTIdentifier;
import net.sourceforge.pmd.lang.vf.ast.ASTLiteral;
-import net.sourceforge.pmd.lang.vf.ast.ASTNegationExpression;
import net.sourceforge.pmd.lang.vf.ast.ASTText;
-import net.sourceforge.pmd.lang.vf.ast.AbstractVFNode;
-import net.sourceforge.pmd.lang.vf.ast.VfTypedNode;
import net.sourceforge.pmd.lang.vf.rule.AbstractVfRule;
+import net.sourceforge.pmd.lang.vf.rule.security.internal.ElEscapeDetector;
+
/**
* @author sergey.gorbaty February 2017
@@ -51,83 +46,44 @@ public class VfUnescapeElRule extends AbstractVfRule {
private static final String FALSE = "false";
private static final Pattern ON_EVENT = Pattern.compile("^on(\\w)+$");
private static final Pattern PLACEHOLDERS = Pattern.compile("\\{(\\w|,|\\.|'|:|\\s)*\\}");
+ private static final EnumSet JSENCODE_JSINHTMLENCODE = EnumSet.of(ElEscapeDetector.Escaping.JSENCODE, ElEscapeDetector.Escaping.JSINHTMLENCODE);
+ private static final EnumSet ANY_ENCODE = EnumSet.of(ElEscapeDetector.Escaping.ANY);
@Override
public Object visit(ASTHtmlScript node, Object data) {
checkIfCorrectlyEscaped(node, data);
+
return super.visit(node, data);
}
private void checkIfCorrectlyEscaped(ASTHtmlScript node, Object data) {
- ASTText prevText = null;
-
// churn thru every child just once instead of twice
for (int i = 0; i < node.getNumChildren(); i++) {
Node n = node.getChild(i);
- if (n instanceof ASTText) {
- prevText = (ASTText) n;
- continue;
- }
-
if (n instanceof ASTElExpression) {
- processElInScriptContext((ASTElExpression) n, prevText, data);
+ processElInScriptContext((ASTElExpression) n, data);
}
}
}
- private void processElInScriptContext(ASTElExpression elExpression, ASTText prevText, Object data) {
- boolean quoted = false;
- boolean jsonParse = false;
-
- if (prevText != null) {
- jsonParse = isJsonParse(prevText);
- if (isUnbalanced(prevText.getImage(), '\'') || isUnbalanced(prevText.getImage(), '\"')) {
- quoted = true;
- }
- }
- if (quoted) {
- // check escaping too
- if (!(jsonParse || startsWithSafeResource(elExpression) || containsSafeFields(elExpression))) {
- if (doesElContainAnyUnescapedIdentifiers(elExpression,
- EnumSet.of(Escaping.JSENCODE, Escaping.JSINHTMLENCODE))) {
- addViolation(data, elExpression);
- }
- }
- } else {
- if (!(startsWithSafeResource(elExpression) || containsSafeFields(elExpression))) {
- final boolean hasUnscaped = doesElContainAnyUnescapedIdentifiers(elExpression,
- EnumSet.of(Escaping.JSENCODE, Escaping.JSINHTMLENCODE));
- if (!(jsonParse && !hasUnscaped)) {
- addViolation(data, elExpression);
- }
- }
+ private void processElInScriptContext(ASTElExpression elExpression, Object data) {
+ if (!properlyEscaped(elExpression)) {
+ addViolation(data, elExpression);
}
}
- private boolean isJsonParse(ASTText prevText) {
- final String text = prevText.getImage().endsWith("'")
- ? prevText.getImage().substring(0, prevText.getImage().length() - 1) : prevText.getImage();
+ private boolean properlyEscaped(ASTElExpression el) {
+ // Find the first Expression-type child of this top-level node.
+ ASTExpression expression = el.getFirstChildOfType(ASTExpression.class);
- return text.endsWith("JSON.parse(") || text.endsWith("jQuery.parseJSON(") || text.endsWith("$.parseJSON(");
- }
-
- private boolean isUnbalanced(String image, char pattern) {
- char[] array = image.toCharArray();
-
- boolean foundPattern = false;
-
- for (int i = array.length - 1; i > 0; i--) {
- if (array[i] == pattern) {
- foundPattern = true;
- }
-
- if (array[i] == ';') {
- return foundPattern;
- }
+ // If no such node was found, then there's nothing to escape, so we're fine.
+ if (expression == null) {
+ return true;
}
- return foundPattern;
+ // Otherwise, we should pass the expression node into our recursive checker.
+ return ElEscapeDetector.expressionRecursivelyValid(expression, JSENCODE_JSINHTMLENCODE);
}
@Override
@@ -181,11 +137,11 @@ public class VfUnescapeElRule extends AbstractVfRule {
break;
}
- if (startsWithSafeResource(el)) {
+ if (ElEscapeDetector.startsWithSafeResource(el)) {
break;
}
- if (doesElContainAnyUnescapedIdentifiers(el, Escaping.URLENCODE)) {
+ if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el, ElEscapeDetector.Escaping.URLENCODE)) {
isEL = true;
toReport.add(el);
}
@@ -216,12 +172,11 @@ public class VfUnescapeElRule extends AbstractVfRule {
if (ON_EVENT.matcher(name).matches()) {
final List elsInVal = attr.findDescendantsOfType(ASTElExpression.class);
for (ASTElExpression el : elsInVal) {
- if (startsWithSafeResource(el)) {
+ if (ElEscapeDetector.startsWithSafeResource(el)) {
continue;
}
- if (doesElContainAnyUnescapedIdentifiers(el,
- EnumSet.of(Escaping.ANY))) {
+ if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el, ANY_ENCODE)) {
isEL = true;
toReport.add(el);
}
@@ -238,78 +193,6 @@ public class VfUnescapeElRule extends AbstractVfRule {
}
- private boolean startsWithSafeResource(final ASTElExpression el) {
- final ASTExpression expression = el.getFirstChildOfType(ASTExpression.class);
- if (expression != null) {
- final ASTNegationExpression negation = expression.getFirstChildOfType(ASTNegationExpression.class);
- if (negation != null) {
- return true;
- }
-
- final ASTIdentifier id = expression.getFirstChildOfType(ASTIdentifier.class);
- if (id != null) {
- String lowerCaseId = id.getImage().toLowerCase(Locale.ROOT);
- List args = expression.findChildrenOfType(ASTArguments.class);
- if (!args.isEmpty()) {
- switch (lowerCaseId) {
- case "urlfor":
- case "casesafeid":
- case "begins":
- case "contains":
- case "len":
- case "getrecordids":
- case "linkto":
- case "sqrt":
- case "round":
- case "mod":
- case "log":
- case "ln":
- case "exp":
- case "abs":
- case "floor":
- case "ceiling":
- case "nullvalue":
- case "isnumber":
- case "isnull":
- case "isnew":
- case "isblank":
- case "isclone":
- case "year":
- case "month":
- case "day":
- case "datetimevalue":
- case "datevalue":
- case "date":
- case "now":
- case "today":
- return true;
-
- default:
- }
- } else {
- // has no arguments
- switch (lowerCaseId) {
- case "$action":
- case "$page":
- case "$site":
- case "$resource":
- case "$label":
- case "$objecttype":
- case "$component":
- case "$remoteaction":
- case "$messagechannel":
- return true;
-
- default:
- }
- }
- }
-
- }
-
- return false;
- }
-
private boolean startsWithSlashLiteral(final ASTElExpression elExpression) {
final ASTExpression expression = elExpression.getFirstChildOfType(ASTExpression.class);
if (expression != null) {
@@ -352,11 +235,12 @@ public class VfUnescapeElRule extends AbstractVfRule {
final List elsInVal = attr.findDescendantsOfType(ASTElExpression.class);
for (ASTElExpression el : elsInVal) {
- if (startsWithSafeResource(el)) {
+ if (ElEscapeDetector.startsWithSafeResource(el)) {
continue;
}
- if (doesElContainAnyUnescapedIdentifiers(el, Escaping.HTMLENCODE)) {
+ if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el,
+ ElEscapeDetector.Escaping.HTMLENCODE)) {
isEL = true;
toReport.add(el);
}
@@ -390,117 +274,6 @@ public class VfUnescapeElRule extends AbstractVfRule {
}
}
- private boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression, Escaping escape) {
- return doesElContainAnyUnescapedIdentifiers(elExpression, EnumSet.of(escape));
-
- }
-
- private boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression,
- EnumSet escapes) {
- if (elExpression == null) {
- return false;
- }
-
- final Set nonEscapedIds = new HashSet<>();
-
- final List exprs = elExpression.findChildrenOfType(ASTExpression.class);
- for (final ASTExpression expr : exprs) {
-
- if (innerContainsSafeFields(expr)) {
- continue;
- }
-
- if (expressionContainsSafeDataNodes(expr)) {
- continue;
- }
-
- final List ids = expr.findChildrenOfType(ASTIdentifier.class);
- for (final ASTIdentifier id : ids) {
- boolean isEscaped = false;
-
- for (Escaping e : escapes) {
-
- if (id.getImage().equalsIgnoreCase(e.toString())) {
- isEscaped = true;
- break;
- }
-
- if (e.equals(Escaping.ANY)) {
- for (Escaping esc : Escaping.values()) {
- if (id.getImage().equalsIgnoreCase(esc.toString())) {
- isEscaped = true;
- break;
- }
- }
- }
-
- }
-
- if (!isEscaped) {
- nonEscapedIds.add(id);
- }
- }
-
- }
-
- return !nonEscapedIds.isEmpty();
- }
-
- /**
- * Return true if the type of all data nodes can be determined and none of them require escaping
- */
- private boolean expressionContainsSafeDataNodes(ASTExpression expression) {
- try {
- for (VfTypedNode node : expression.getDataNodes().keySet()) {
- DataType dataType = node.getDataType();
- if (dataType == null || dataType.requiresEscaping) {
- return false;
- }
- }
-
- return true;
- } catch (ASTExpression.DataNodeStateException e) {
- return false;
- }
- }
-
- private boolean containsSafeFields(final AbstractVFNode expression) {
- final ASTExpression ex = expression.getFirstChildOfType(ASTExpression.class);
-
- return ex != null && innerContainsSafeFields(ex);
-
- }
-
- private boolean innerContainsSafeFields(final AbstractVFNode expression) {
- for (int i = 0; i < expression.getNumChildren(); i++) {
- Node child = expression.getChild(i);
-
- if (child instanceof ASTIdentifier) {
- switch (child.getImage().toLowerCase(Locale.ROOT)) {
- case "id":
- case "size":
- case "caseNumber":
- return true;
- default:
- }
- }
-
- if (child instanceof ASTArguments) {
- if (containsSafeFields((ASTArguments) child)) {
- return true;
- }
- }
-
- if (child instanceof ASTDotExpression) {
- if (innerContainsSafeFields((ASTDotExpression) child)) {
- return true;
- }
- }
-
- }
-
- return false;
- }
private boolean doesTagSupportEscaping(final ASTElement node) {
if (node.getName() == null) {
@@ -530,11 +303,12 @@ public class VfUnescapeElRule extends AbstractVfRule {
for (ASTAttribute attrib : innerAttributes) {
final List elsInVal = attrib.findDescendantsOfType(ASTElExpression.class);
for (final ASTElExpression el : elsInVal) {
- if (startsWithSafeResource(el)) {
+ if (ElEscapeDetector.startsWithSafeResource(el)) {
continue;
}
- if (doesElContainAnyUnescapedIdentifiers(el, Escaping.HTMLENCODE)) {
+ if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el,
+ ElEscapeDetector.Escaping.HTMLENCODE)) {
toReturn.add(el);
}
@@ -546,24 +320,4 @@ public class VfUnescapeElRule extends AbstractVfRule {
return toReturn;
}
-
- enum Escaping {
- HTMLENCODE("HTMLENCODE"),
- URLENCODE("URLENCODE"),
- JSINHTMLENCODE("JSINHTMLENCODE"),
- JSENCODE("JSENCODE"),
- ANY("ANY");
-
- private final String text;
-
- Escaping(final String text) {
- this.text = text;
- }
-
- @Override
- public String toString() {
- return text;
- }
- }
-
}
diff --git a/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/internal/ElEscapeDetector.java b/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/internal/ElEscapeDetector.java
new file mode 100644
index 0000000000..1cd88abccd
--- /dev/null
+++ b/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/internal/ElEscapeDetector.java
@@ -0,0 +1,404 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.vf.rule.security.internal;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+
+import net.sourceforge.pmd.lang.vf.DataType;
+import net.sourceforge.pmd.lang.vf.ast.ASTArguments;
+import net.sourceforge.pmd.lang.vf.ast.ASTDotExpression;
+import net.sourceforge.pmd.lang.vf.ast.ASTElExpression;
+import net.sourceforge.pmd.lang.vf.ast.ASTExpression;
+import net.sourceforge.pmd.lang.vf.ast.ASTIdentifier;
+import net.sourceforge.pmd.lang.vf.ast.ASTNegationExpression;
+import net.sourceforge.pmd.lang.vf.ast.VfNode;
+import net.sourceforge.pmd.lang.vf.ast.VfTypedNode;
+
+/**
+ * Helps detect visualforce encoding in EL Expressions
+ * (porting over code previously living in VfUnescapeElRule for reusability)
+ */
+
+public final class ElEscapeDetector {
+
+ private static final Set SAFE_PROPERTIES = new HashSet<>(Arrays.asList("id", "size", "caseNumber"));
+ private static final Set SAFE_BUILTIN_FUNCTIONS = new HashSet<>(Arrays.asList(
+ // These DateTime functions accept or return dates, and therefore don't need escaping.
+ "addmonths", "date", "datevalue", "datetimevalue", "day", "hour", "millisecond", "minute", "month", "now",
+ "second", "timenow", "timevalue", "today", "weekday", "year",
+ // These Logical functions accept or return booleans, and therefore don't need escaping.
+ "and", "isblank", "isclone", "isnew", "isnull", "isnumber", "not", "or",
+ // These Math functions return numbers, and therefore don't require escaping.
+ "abs", "ceiling", "exp", "floor", "ln", "log", "max", "mceiling", "mfloor", "min", "mod", "round", "sqrt",
+ // These Text functions are safe, either because of what they accept or what they return.
+ "begins", "br", "casesafeid", "contains", "find", "getsessionid", "ispickval", "len",
+ // These Advanced functions are safe because of what they accept or what they return.
+ "currencyrate", "getrecordids", "ischanged", "junctionidlist", "linkto", "regex", "urlfor"
+ ));
+ private static final Set FUNCTIONS_WITH_XSSABLE_ARG0 = new HashSet<>(Arrays.asList(
+ // For these methods, the first argument is a string that must be escaped.
+ "left", "lower", "lpad", "mid", "right", "rpad", "upper"
+ ));
+ private static final Set FUNCTIONS_WITH_XSSABLE_ARG2 = new HashSet<>(Arrays.asList(
+ // For these methods, the third argument is a string that must be escaped.
+ "lpad", "rpad"
+ ));
+ private static final Set SAFE_GLOBAL_VARS = new HashSet<>(Arrays.asList("$action", "$page", "$site",
+ "$resource", "$label", "$objecttype", "$component", "$remoteaction", "$messagechannel"));
+
+ private ElEscapeDetector() {
+ // utility class
+ }
+
+ private static VfNode getNextSibling(VfNode node) {
+ VfNode parent = node.getParent();
+ if (parent != null && node.getIndexInParent() < (parent.getNumChildren() - 1)) {
+ return parent.getChild(node.getIndexInParent() + 1);
+ }
+ return null;
+ }
+
+ /**
+ * Given an ASTExpression node, determines whether that expression and any expressions under it are properly escaped.
+ * @param expression - Represents a VF expression
+ * @param escapes - The escape operations that are acceptable in this context
+ * @return - True if the expression is properly escaped, otherwise false.
+ */
+ public static boolean expressionRecursivelyValid(final ASTExpression expression, final EnumSet escapes) {
+ // We'll want to iterate over all of this expression's children.
+ int childCount = expression.getNumChildren();
+ String prevId = "";
+ List relevantChildren = new ArrayList<>();
+ for (int i = 0; i < childCount; i++) {
+ VfNode child = expression.getChild(i);
+
+ if (child instanceof ASTIdentifier) {
+ // How we deal with an Identifier depends on what the next node after it is.
+ VfNode nextNode = getNextSibling(child);
+ if (nextNode instanceof ASTArguments || nextNode instanceof ASTDotExpression) {
+ // If the next node is Arguments or Dot expression, that means this Identifier represents the name
+ // of a function, or some kind of object. In that case, we might be okay. So we'll store the name
+ // and keep going.
+ prevId = child.getImage();
+ continue;
+ }
+ // If there's no next node, or the next node isn't one of the desired types, then this Identifier is a raw
+ // variable.
+ if (typedNodeIsSafe((ASTIdentifier) child)) {
+ // If the raw variable is of an inherently safe type, we can keep going.
+ continue;
+ }
+ // If the raw variable isn't inherently safe, then it's dangerous, and must be escaped.
+ return false;
+ } else if (child instanceof ASTArguments) {
+ // An Arguments node means we're looking at some kind of function call.
+ // If it's one of our designated escape functions, we're in the clear and we can keep going.
+ // Also, some built-in functions are inherently safe, which would mean we're good to continue.
+ if (functionIsEscape(prevId, escapes) || functionInherentlySafe(prevId)) {
+ continue;
+ }
+
+ // Otherwise, identify the argument expressions that must be checked, and add them to the list.
+ relevantChildren.addAll(getXssableArguments(prevId, (ASTArguments) child));
+ } else if (child instanceof ASTDotExpression) {
+ // Dot expressions mean we're doing accessing properties of variables.
+ // If the variable is one of the definitely-safe global variables, then we're in the clear.
+ if (isSafeGlobal(prevId)) {
+ continue;
+ }
+ // If the node after this one is also a Dot expression, then this is a chained access, and we can't make
+ // any final judgements.
+ if (getNextSibling(child) instanceof ASTDotExpression) {
+ continue;
+ }
+ // If none of those things are true, then we need to determine whether the field being accessed is
+ // definitely safe.
+ ASTIdentifier propId = child.getFirstChildOfType(ASTIdentifier.class);
+ // If there's an identifier for a field/property, we need to check whether that property is inherently safe,
+ // either because it corresponds to a safe field or because its data type is known to be safe.
+ if (propId != null && !isSafeProperty(propId.getImage()) && !typedNodeIsSafe(propId)) {
+ // If the node isn't definitely safe, it ought to be escaped. Return false.
+ return false;
+ }
+ } else if (child instanceof ASTExpression) {
+ // Expressions should always be added to the list.
+ relevantChildren.add((ASTExpression) child);
+ }
+ }
+ // Just because there's nothing immediately wrong with this node doesn't mean its children are guaranteed to be
+ // fine. Iterate over all of the children and make a recursive call. If any of those calls return false, we need
+ // to relay that back up the chain.
+ for (ASTExpression e : relevantChildren) {
+ if (!expressionRecursivelyValid(e, escapes)) {
+ return false;
+ }
+ }
+ // If we didn't find a reason to return false, we're good. Return true.
+ return true;
+ }
+
+ /**
+ * Indicates whether the provided function name corresponds to any of the provided escape functions.
+ * @param functionName - The name of a VF function
+ * @param escapes - A set of acceptable escape functions (e.g., JSENCODE, HTMLENCODE, etc)
+ * @return - True if the function is a viable escape.
+ */
+ private static boolean functionIsEscape(String functionName, EnumSet escapes) {
+ // If one of the escapes we were passed is ANY, then we should replace the provided set with one that contains
+ // all possible escapes.
+ EnumSet handledEscapes = escapes.contains(Escaping.ANY) ? EnumSet.allOf(Escaping.class) : escapes;
+ for (Escaping e : handledEscapes) {
+ if (functionName.equalsIgnoreCase(e.toString())) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Certain built-in functions are inherently safe and don't require any escaping. This method determines whether the
+ * function name provided corresponds to one of those methods.
+ * @param functionName - The name of a VF function
+ * @return - True if the function is an inherently safe built-in function
+ */
+ private static boolean functionInherentlySafe(String functionName) {
+ String lowerCaseName = functionName.toLowerCase(Locale.ROOT);
+ return SAFE_BUILTIN_FUNCTIONS.contains(lowerCaseName);
+ }
+
+ /**
+ * Given a function name and a node containing its arguments, returns the ASTExpression nodes corresponding to arguments
+ * that require escaping. Frequently, this will be all arguments, but not always.
+ * @param functionName - The name of a function being called
+ * @param arguments - Contains the ASTExpression nodes representing the function's arguments
+ * @return - ASTExpression list containing all arguments that are vulnerable to XSS.
+ */
+ private static List getXssableArguments(String functionName, ASTArguments arguments) {
+ List exprs = new ArrayList<>();
+ int argCount = arguments.getNumChildren();
+ if (argCount != 0) {
+ String lowerCaseName = functionName.toLowerCase(Locale.ROOT);
+ List indicesToCheck = new ArrayList<>();
+
+ // See if the function name corresponds to one of the built-in functions that don't require us to examine
+ // every argument.
+ if ("case".equals(lowerCaseName)) {
+ // CASE accepts (exx, val1, res1, val2, res2, ...else_res). We want resX and else_res.
+ for (int i = 2; i < argCount; i += 2) {
+ indicesToCheck.add(i);
+ }
+ indicesToCheck.add(argCount - 1);
+ } else if ("if".equals(lowerCaseName)) {
+ // IF accepts (test, if_true, if_false). We care about if_true and if_false.
+ indicesToCheck.add(1);
+ indicesToCheck.add(2);
+ } else {
+ boolean checkAllArgs = true;
+ // If this is a function with an XSSable first arg, add 0 to the array.
+ if (FUNCTIONS_WITH_XSSABLE_ARG0.contains(lowerCaseName)) {
+ checkAllArgs = false;
+ indicesToCheck.add(0);
+ }
+ // If this is a function that has at least 3 args, and the third arg is XSSable, add 2 to the array.
+ if (argCount > 2 && FUNCTIONS_WITH_XSSABLE_ARG2.contains(lowerCaseName)) {
+ checkAllArgs = false;
+ indicesToCheck.add(2);
+ }
+ // If the function has no known pattern for argument checking, all arguments must be checked.
+ if (checkAllArgs) {
+ for (int i = 0; i < argCount; i++) {
+ indicesToCheck.add(i);
+ }
+ }
+ }
+
+ // Add each of the targeted arguments to the return array if they represent an Expression node. (They always
+ // should, but better safe than sorry.)
+ for (int i : indicesToCheck) {
+ VfNode ithArg = arguments.getChild(i);
+ if (ithArg instanceof ASTExpression) {
+ exprs.add((ASTExpression) ithArg);
+ }
+ }
+ }
+ return exprs;
+ }
+
+ /**
+ * VF has global variables prefixed with a '$'. Some of those are inherently safe to access, and this method determines
+ * whether the provided ID corresponds to one of those globals.
+ * @param id - Identifier of some variable.
+ * @return - True if the global is inherently safe.
+ */
+ private static boolean isSafeGlobal(String id) {
+ String lowerCaseId = id.toLowerCase(Locale.ROOT);
+ return SAFE_GLOBAL_VARS.contains(lowerCaseId);
+ }
+
+ /**
+ * Determines whether the property being referenced is inherently safe, or if it requires XSS escaping.
+ * @param propertyName - The name of a field or property being referenced.
+ * @return - True if that field/property is inherently safe
+ */
+ private static boolean isSafeProperty(String propertyName) {
+ String lowerCaseName = propertyName.toLowerCase(Locale.ROOT);
+ return SAFE_PROPERTIES.contains(lowerCaseName);
+ }
+
+ private static boolean innerContainsSafeFields(final VfNode expression) {
+ for (VfNode child : expression.children()) {
+
+ if (child instanceof ASTIdentifier && isSafeProperty(child.getImage())) {
+ return true;
+ }
+
+ if (child instanceof ASTArguments) {
+ if (containsSafeFields((ASTArguments) child)) {
+ return true;
+ }
+ }
+
+ if (child instanceof ASTDotExpression) {
+ if (innerContainsSafeFields((ASTDotExpression) child)) {
+ return true;
+ }
+ }
+
+ }
+
+ return false;
+ }
+
+ public static boolean containsSafeFields(final VfNode expression) {
+ final ASTExpression ex = expression.getFirstChildOfType(ASTExpression.class);
+
+ return ex != null && innerContainsSafeFields(ex);
+ }
+
+ public static boolean startsWithSafeResource(final ASTElExpression el) {
+ final ASTExpression expression = el.getFirstChildOfType(ASTExpression.class);
+ if (expression != null) {
+ final ASTNegationExpression negation = expression.getFirstChildOfType(ASTNegationExpression.class);
+ if (negation != null) {
+ return true;
+ }
+
+ final ASTIdentifier id = expression.getFirstChildOfType(ASTIdentifier.class);
+ if (id != null) {
+ List args = expression.findChildrenOfType(ASTArguments.class);
+
+ if (!args.isEmpty()) {
+ return functionInherentlySafe(id.getImage());
+ } else {
+ return isSafeGlobal(id.getImage());
+ }
+ }
+ }
+ return false;
+ }
+
+ public static boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression, Escaping escape) {
+ return doesElContainAnyUnescapedIdentifiers(elExpression, EnumSet.of(escape));
+ }
+
+ public static boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression,
+ EnumSet escapes) {
+ if (elExpression == null) {
+ return false;
+ }
+
+ final Set nonEscapedIds = new HashSet<>();
+
+ final List exprs = elExpression.findChildrenOfType(ASTExpression.class);
+ for (final ASTExpression expr : exprs) {
+
+ if (innerContainsSafeFields(expr)) {
+ continue;
+ }
+
+ if (expressionContainsSafeDataNodes(expr)) {
+ continue;
+ }
+
+ final List ids = expr.findChildrenOfType(ASTIdentifier.class);
+ for (final ASTIdentifier id : ids) {
+ boolean isEscaped = false;
+
+ for (Escaping e : escapes) {
+
+ if (id.getImage().equalsIgnoreCase(e.toString())) {
+ isEscaped = true;
+ break;
+ }
+
+ if (e.equals(Escaping.ANY)) {
+ for (Escaping esc : Escaping.values()) {
+ if (id.getImage().equalsIgnoreCase(esc.toString())) {
+ isEscaped = true;
+ break;
+ }
+ }
+ }
+
+ }
+
+ if (!isEscaped) {
+ nonEscapedIds.add(id);
+ }
+ }
+
+ }
+
+ return !nonEscapedIds.isEmpty();
+ }
+
+ /**
+ * Return true if the type of all data nodes can be determined and none of them require escaping
+ * @param expression
+ */
+ private static boolean expressionContainsSafeDataNodes(ASTExpression expression) {
+ try {
+ for (VfTypedNode node : expression.getDataNodes().keySet()) {
+ if (!typedNodeIsSafe(node)) {
+ return false;
+ }
+ }
+
+ return true;
+ } catch (ASTExpression.DataNodeStateException e) {
+ return false;
+ }
+ }
+
+ private static boolean typedNodeIsSafe(VfTypedNode node) {
+ DataType dataType = node.getDataType();
+ return dataType != null && !dataType.requiresEscaping;
+ }
+
+ public enum Escaping {
+ HTMLENCODE("HTMLENCODE"),
+ URLENCODE("URLENCODE"),
+ JSINHTMLENCODE("JSINHTMLENCODE"),
+ JSENCODE("JSENCODE"),
+ ANY("ANY");
+
+ private final String text;
+
+ Escaping(final String text) {
+ this.text = text;
+ }
+
+ @Override
+ public String toString() {
+ return text;
+ }
+ }
+}
diff --git a/pmd-visualforce/src/main/resources/category/vf/security.xml b/pmd-visualforce/src/main/resources/category/vf/security.xml
index fcb7c49a37..2f4408e929 100644
--- a/pmd-visualforce/src/main/resources/category/vf/security.xml
+++ b/pmd-visualforce/src/main/resources/category/vf/security.xml
@@ -26,6 +26,41 @@ Avoid calling VF action upon page load as the action becomes vulnerable to CSRF.
+
+
+Checks for the correct encoding in `<style/>` tags in Visualforce pages.
+
+The rule is based on Salesforce Security's recommendation to prevent XSS in Visualforce as mentioned
+on [Secure Coding Cross Site Scripting](https://developer.salesforce.com/docs/atlas.en-us.secure_coding_guide.meta/secure_coding_guide/secure_coding_cross_site_scripting.htm).
+
+In order to avoid cross site scripting, the relevant encoding must be used in HTML tags. The rule expects
+`URLENCODING` or `JSINHTMLENCODING` for URL-based style values and any kind of encoding
+(e.g. `HTMLENCODING`) for non-url style values.
+
+See also {% rule "VfUnescapeEl" %} to check escaping in other places on Visualforce pages.
+
+ 3
+
+
+
+
+]]>
+
+
+
+
+
+
+ sObject field (from controller) assigned to url variable in style tag (html encoded)
+ 1
+
+
+
+ ]]>
+
+
+
+ sObject field (from controller) assigned to url variable in style tag (JS encoded)
+ 1
+
+
+
+ ]]>
+
+
+
+ sObject field (from controller) assigned to url variable in style tag (JSINHTML encoded)
+ 0
+
+
+
+ ]]>
+
+
+
+ sObject field (from controller) assigned to url variable in style tag (URL encoded)
+ 0
+
+
+
+ ]]>
+
+
+
+ sObject field (from controller) assigned to url variable in style tag
+ 1
+
+
+
+ ]]>
+
+
+
+ sObject field (from controller) assigned to variable in style tag
+ 1
+
+
+
+ ]]>
+
+
+
+ sObject field (from controller) assigned to variable in style tag with some encoding
+ 0
+
+
+
+ ]]>
+
+
+
+ EL assigned to url variable in style tag within a safe resource
+ 0
+
+
+
+ ]]>
+
+
+
\ No newline at end of file
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 aec8e5e7d8..b09f596b3a 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
@@ -437,22 +437,126 @@
- NOT method evaluates to safe boolean
+ IF() with literals in returns is safe
0
]]>
- JSON.parse method evaluates quoted EL to safe JSON
+ IF() with escaped XSS in returns is safe
0
+
+
+ ]]>
+
+
+
+ IF() with unescaped XSS in returns is unsafe
+ 2
+
+
+
+ ]]>
+
+
+
+ CASE() with XSS in conditions is safe
+ 0
+
+
+
+ ]]>
+
+
+
+ CASE() with escaped XSS in results is safe
+ 0
+
+
+
+ ]]>
+
+
+
+ CASE() with unescaped XSS in results is unsafe
+ 1
+
+
+
+ ]]>
+
+
+
+ LPAD() is not inherently safe
+ 3
+
+
+
+ ]]>
+
+
+
+ Escaped LPAD() call is fine
+ 0
+
+
+
+ ]]>
+
+
+
+ NOT, ISBLANK, BEGINS evaluate to safe booleans
+ 0
+
+
+
+ ]]>
+
+
+
+ JSON.parse method evaluates quoted EL to unsafe XSS
+ 3
+