phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #3211 from jonathanwiesel:no-assume-apex-getter-vf

Browse Source 
[apex] ApexCRUDViolationRule: Do not assume method is VF getter to avoid
CRUD checks #3211
This commit is contained in:
Andreas Dangel 2021-04-23 09:25:00 +02:00
commit 1985f72d2a
4 changed files with 32 additions and 67 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
docs/pages/release_notes.md
View File

@ -27,6 +27,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
@ -39,11 +46,12 @@ This is a {{ site.pmd.release_type }} release.
* java-errorprone
* {% 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
@ -59,6 +67,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 %}

50
pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java
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);
@ -514,7 +512,6 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
collectCRUDMethodLevelChecks(prevCall);
}
boolean isGetter = false;
String returnType = null;
final ASTMethod wrappingMethod = node.getFirstParentOfType(ASTMethod.class);
@ -527,7 +524,6 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
}
if (wrappingMethod != null) {
isGetter = isMethodAGetter(wrappingMethod);
returnType = getReturnType(wrappingMethod);
}
@ -538,13 +534,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);
}
}
@ -557,15 +551,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);
}
}
}
}
@ -574,13 +565,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);
}
}
}
@ -602,13 +591,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;
}
}

5
pmd-apex/src/main/resources/category/apex/security.xml
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>

33
pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml
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>