hiifong/pmd
1
0 Fork 0
forked from phoedos/pmd
Code Pull Requests Activity

Merge branch 'master' into pmd/7.0.x

Browse Source
This commit is contained in:
Andreas Dangel committed 2021-04-23 22:17:09 +02:00
2 parents 6c9eee83b4 1985f72d2a
commit 742e1e5a47
4 files changed
+32 -67
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

docs/pages/release_notes.md
+10 -1
View File
@@ -32,6 +32,13 @@ This is a {{ site.pmd.release_type }} release.
This rule is also part of the Quickstart Ruleset (`rulesets/java/quickstart.xml`) for Java.
#### Modified rules
* The Apex rule {% rule "apex/security/ApexCRUDViolation" %} does not ignore getters anymore and also flags
SOQL/SOSL/DML operations without access permission checks in getters. This will produce false positives now for
VF getter methods, but we can't reliably detect, whether a getter is a VF getter or not. In such cases,
the violation should be [suppressed](pmd_userdocs_suppressing_warnings.html).
#### Deprecated rules
* java-bestpractices
@@ -44,11 +51,12 @@ This is a {{ site.pmd.release_type }} release.
* java-errorprone
* {% deleted_rule java/errorprone/ImportFromSamePackage %}: use the rule {% rule "java/codestyle/UnnecessaryImport" %} instead
### Fixed Issues
* apex-performance
* [#3198](https://github.com/pmd/pmd/pull/3198): \[apex] OperationWithLimitsInLoopRule: Support more limit consuming static method invocations
* apex-security
* [#3210](https://github.com/pmd/pmd/issues/3210): \[apex] ApexCRUDViolationRule false-negative on non-VF getter
* java-bestpractices
* [#3190](https://github.com/pmd/pmd/issues/3190): \[java] Use StandardCharsets instead of Charset.forName
* [#3224](https://github.com/pmd/pmd/issues/3224): \[java] UnusedAssignment crashes with nested records
@@ -64,6 +72,7 @@ This is a {{ site.pmd.release_type }} release.
* [#3193](https://github.com/pmd/pmd/pull/3193): \[java] New rule: UseStandardCharsets - [Andrea Aime](https://github.com/aaime)
* [#3198](https://github.com/pmd/pmd/pull/3198): \[apex] OperationWithLimitsInLoopRule: Support more limit consuming static method invocations - [Jonathan Wiesel](https://github.com/jonathanwiesel)
* [#3211](https://github.com/pmd/pmd/pull/3211): \[apex] ApexCRUDViolationRule: Do not assume method is VF getter to avoid CRUD checks - [Jonathan Wiesel](https://github.com/jonathanwiesel)
{% endtocmaker %}
pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java
+15 -35
View File
@@ -32,7 +32,6 @@ import net.sourceforge.pmd.lang.apex.ast.ASTIfElseBlockStatement;
import net.sourceforge.pmd.lang.apex.ast.ASTMethod;
import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression;
import net.sourceforge.pmd.lang.apex.ast.ASTNewKeyValueObjectExpression;
import net.sourceforge.pmd.lang.apex.ast.ASTParameter;
import net.sourceforge.pmd.lang.apex.ast.ASTProperty;
import net.sourceforge.pmd.lang.apex.ast.ASTReferenceExpression;
import net.sourceforge.pmd.lang.apex.ast.ASTReturnStatement;
@@ -56,7 +55,6 @@ import com.google.common.collect.ListMultimap;
*
*/
public class ApexCRUDViolationRule extends AbstractApexRule {
private static final Pattern VOID_OR_STRING_PATTERN = Pattern.compile("^(string|void)$", Pattern.CASE_INSENSITIVE);
private static final Pattern SELECT_FROM_PATTERN = Pattern.compile("[\\S|\\s]+?FROM[\\s]+?(\\w+)",
Pattern.CASE_INSENSITIVE);
@@ -508,7 +506,6 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
collectCRUDMethodLevelChecks(prevCall);
}
boolean isGetter = false;
String returnType = null;
final ASTMethod wrappingMethod = node.getFirstParentOfType(ASTMethod.class);
@@ -521,7 +518,6 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
}
if (wrappingMethod != null) {
isGetter = isMethodAGetter(wrappingMethod);
returnType = getReturnType(wrappingMethod);
}
@@ -532,13 +528,11 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
StringBuilder typeCheck = new StringBuilder().append(variableDecl.getDefiningType())
.append(":").append(type);
if (!isGetter) {
if (typesFromSOQL.isEmpty()) {
validateCRUDCheckPresent(node, data, ANY, typeCheck.toString());
} else {
for (String typeFromSOQL : typesFromSOQL) {
validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
}
if (typesFromSOQL.isEmpty()) {
validateCRUDCheckPresent(node, data, ANY, typeCheck.toString());
} else {
for (String typeFromSOQL : typesFromSOQL) {
validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
}
}
@@ -551,15 +545,12 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
String variableWithClass = Helper.getFQVariableName(variable);
if (varToTypeMapping.containsKey(variableWithClass)) {
String type = varToTypeMapping.get(variableWithClass);
if (!isGetter) {
if (typesFromSOQL.isEmpty()) {
validateCRUDCheckPresent(node, data, ANY, type);
} else {
for (String typeFromSOQL : typesFromSOQL) {
validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
}
if (typesFromSOQL.isEmpty()) {
validateCRUDCheckPresent(node, data, ANY, type);
} else {
for (String typeFromSOQL : typesFromSOQL) {
validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
}
}
}
}
@@ -568,13 +559,11 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
final ASTReturnStatement returnStatement = node.getFirstParentOfType(ASTReturnStatement.class);
if (returnStatement != null) {
if (!isGetter) {
if (typesFromSOQL.isEmpty()) {
validateCRUDCheckPresent(node, data, ANY, returnType);
} else {
for (String typeFromSOQL : typesFromSOQL) {
validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
}
if (typesFromSOQL.isEmpty()) {
validateCRUDCheckPresent(node, data, ANY, returnType);
} else {
for (String typeFromSOQL : typesFromSOQL) {
validateCRUDCheckPresent(node, data, ANY, typeFromSOQL);
}
}
}
@@ -596,13 +585,4 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
return new StringBuilder().append(method.getDefiningType()).append(":")
.append(method.getReturnType()).toString();
}
private boolean isMethodAGetter(final ASTMethod method) {
final boolean startsWithGet = method.getCanonicalName().startsWith("get");
final boolean voidOrString = VOID_OR_STRING_PATTERN
.matcher(method.getReturnType()).matches();
final boolean noParams = method.findChildrenOfType(ASTParameter.class).isEmpty();
return startsWithGet && noParams && !voidOrString;
}
}
pmd-apex/src/main/resources/category/apex/security.xml
+5
View File
@@ -42,6 +42,11 @@ public without sharing class Foo {
The rule validates you are checking for access permissions before a SOQL/SOSL/DML operation.
Since Apex runs in system mode not having proper permissions checks results in escalation of
privilege and may produce runtime errors. This check forces you to handle such scenarios.
Note: This rule will produce false positives for VF getter methods. In VF getters the access permission
check happens automatically and is not needed explicitly. However, the rule can't reliably determine
whether a getter is a VF getter or not and reports a violation in any case. In such cases, the violation
should be [suppressed](pmd_userdocs_suppressing_warnings.html).
</description>
<priority>3</priority>
<example>
pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml
+2 -31
View File
@@ -135,8 +135,8 @@ public class Foo {
</test-code>
<test-code>
<description>VF built-in CRUD checks via getter</description>
<expected-problems>0</expected-problems>
<description>VF built-in CRUD checks via getter, but cannot determine if is really VF</description>
<expected-problems>2</expected-problems>
<code><![CDATA[
public class Foo {
public Contact getFoo() {
@@ -153,35 +153,6 @@ public class Foo {
]]></code>
</test-code>
<test-code>
<description>Bypassing VF built-in CRUD checks via getter method
</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Foo {
public String getUserName() {
String tempID = 'someID';
Contact c = [SELECT Name FROM Contact WHERE Id=:tempID];
return c.Name;
}
}
]]></code>
</test-code>
<test-code>
<description>No VF provided CRUD checks via getter method
</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Foo {
public Contact justGiveMeFoo() {
String tempID = 'someID';
return [SELECT Name FROM Contact WHERE Id=:tempID];
}
}
]]></code>
</test-code>
<test-code>
<description>Proper CRUD checks</description>
<expected-problems>0</expected-problems>