From c6b7a723be52a77b6e4ebd6fa15f0511782b8bcc Mon Sep 17 00:00:00 2001 From: Jeff Bartolotta Date: Thu, 17 Sep 2020 08:36:45 -0700 Subject: [PATCH 1/9] Analyze inner classes for sharing violations Fixes https://github.com/pmd/pmd/issues/2774, false positives and false negatives for ApexSharingViolationsRule. Sharing settings are not inherited by inner classes. Sharing settings need to be declared on the class that contains the Database method, DML, SOQL, or SOSL. The change inverts the direction from which nodes are found and analyzed. The previous code visited the ASTUserClass and then searched for descendant nodes that met a certain criteria. It did not visit inner ASTUserClass nodes because it didn't use rule chains or call the super's visit moethod for ASTUserClassi. The new implementation visits all nodes that correspond to Database method, DML, SOQL, or SOSL nodes and then finds the nearest ASTUserClass parent node. This ASTUserClass is examined to determine if it has declared a sharing setting as required. --- .../security/ApexSharingViolationsRule.java | 134 ++++++++---- .../ApexSharingViolationsNestedClassTest.java | 194 ++++++++++++++++++ .../security/xml/ApexSharingViolations.xml | 57 +++++ 3 files changed, 345 insertions(+), 40 deletions(-) create mode 100644 pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsNestedClassTest.java diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsRule.java index 2e7e69ef82..cf6e8323f8 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsRule.java @@ -4,11 +4,20 @@ package net.sourceforge.pmd.lang.apex.rule.security; -import java.util.List; +import java.util.Optional; import java.util.WeakHashMap; +import net.sourceforge.pmd.RuleContext; +import net.sourceforge.pmd.lang.apex.ast.ASTDmlDeleteStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTDmlInsertStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTDmlMergeStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTDmlUndeleteStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTDmlUpdateStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTDmlUpsertStatement; import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; import net.sourceforge.pmd.lang.apex.ast.ASTModifierNode; +import net.sourceforge.pmd.lang.apex.ast.ASTSoqlExpression; +import net.sourceforge.pmd.lang.apex.ast.ASTSoslExpression; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; @@ -21,45 +30,103 @@ import net.sourceforge.pmd.lang.apex.rule.internal.Helper; */ public class ApexSharingViolationsRule extends AbstractApexRule { + /** + * Keep track of previously reported violations to avoid duplicates. + */ private WeakHashMap, Object> localCacheOfReportedNodes = new WeakHashMap<>(); public ApexSharingViolationsRule() { setProperty(CODECLIMATE_CATEGORIES, "Security"); setProperty(CODECLIMATE_REMEDIATION_MULTIPLIER, 100); setProperty(CODECLIMATE_BLOCK_HIGHLIGHTING, false); + addRuleChainVisit(ASTDmlDeleteStatement.class); + addRuleChainVisit(ASTDmlInsertStatement.class); + addRuleChainVisit(ASTDmlMergeStatement.class); + addRuleChainVisit(ASTDmlUndeleteStatement.class); + addRuleChainVisit(ASTDmlUpdateStatement.class); + addRuleChainVisit(ASTDmlUpsertStatement.class); + addRuleChainVisit(ASTMethodCallExpression.class); + addRuleChainVisit(ASTSoqlExpression.class); + addRuleChainVisit(ASTSoslExpression.class); } @Override - public Object visit(ASTUserClass node, Object data) { - - if (Helper.isTestMethodOrClass(node) || Helper.isSystemLevelClass(node)) { - return data; // stops all the rules - } - - if (!Helper.isTestMethodOrClass(node)) { - boolean sharingFound = isSharingPresent(node); - checkForSharingDeclaration(node, data, sharingFound); - checkForDatabaseMethods(node, data, sharingFound); - } - + public void start(RuleContext ctx) { + super.start(ctx); localCacheOfReportedNodes.clear(); + } + + @Override + public Object visit(ASTSoqlExpression node, Object data) { + checkForViolation(node, data); return data; } - /** - * Check if class contains any Database.query / Database.insert [ Database.* - * ] methods - * - * @param node - * @param data - */ - private void checkForDatabaseMethods(ASTUserClass node, Object data, boolean sharingFound) { - List calls = node.findDescendantsOfType(ASTMethodCallExpression.class); - for (ASTMethodCallExpression call : calls) { - if (Helper.isMethodName(call, "Database", Helper.ANY_METHOD)) { - if (!sharingFound) { - reportViolation(node, data); - } + @Override + public Object visit(ASTSoslExpression node, Object data) { + checkForViolation(node, data); + return data; + } + + @Override + public Object visit(ASTDmlUpsertStatement node, Object data) { + checkForViolation(node, data); + return data; + } + + @Override + public Object visit(ASTDmlUpdateStatement node, Object data) { + checkForViolation(node, data); + return data; + } + + @Override + public Object visit(ASTDmlUndeleteStatement node, Object data) { + checkForViolation(node, data); + return data; + } + + @Override + public Object visit(ASTDmlMergeStatement node, Object data) { + checkForViolation(node, data); + return data; + } + + @Override + public Object visit(ASTDmlInsertStatement node, Object data) { + checkForViolation(node, data); + return data; + } + + @Override + public Object visit(ASTDmlDeleteStatement node, Object data) { + checkForViolation(node, data); + return data; + } + + @Override + public Object visit(ASTMethodCallExpression node, Object data) { + if (Helper.isMethodName(node, "Database", Helper.ANY_METHOD)) { + checkForViolation(node, data); + } + + return data; + } + + private void checkForViolation(ApexNode node, Object data) { + // The closest ASTUserClass class in the tree hierarchy is the node that requires the sharing declaration + ASTUserClass sharingDeclarationClass = node.getFirstParentOfType(ASTUserClass.class); + + // This is null in the case of triggers + if (sharingDeclarationClass != null) { + // Apex allows a single level of class nesting. Check to see if sharingDeclarationClass has an outer class + ASTUserClass outerClass = sharingDeclarationClass.getFirstParentOfType(ASTUserClass.class); + // The test annotation needs to be on the outermost class + ASTUserClass testAnnotationClass = Optional.ofNullable(outerClass).orElse(sharingDeclarationClass); + + if (!Helper.isTestMethodOrClass(testAnnotationClass) && !Helper.isSystemLevelClass(sharingDeclarationClass) && !isSharingPresent(sharingDeclarationClass)) { + // The violation is reported on the class, not the node that performs data access + reportViolation(sharingDeclarationClass, data); } } } @@ -77,19 +144,6 @@ public class ApexSharingViolationsRule extends AbstractApexRule { } } - /** - * Check if class has no sharing declared - * - * @param node - * @param data - */ - private void checkForSharingDeclaration(ApexNode node, Object data, boolean sharingFound) { - final boolean foundAnyDMLorSOQL = Helper.foundAnyDML(node) || Helper.foundAnySOQLorSOSL(node); - if (!sharingFound && !Helper.isTestMethodOrClass(node) && foundAnyDMLorSOQL) { - reportViolation(node, data); - } - } - /** * Does class have sharing keyword declared? * diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsNestedClassTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsNestedClassTest.java new file mode 100644 index 0000000000..64ac540a72 --- /dev/null +++ b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSharingViolationsNestedClassTest.java @@ -0,0 +1,194 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.rule.security; + +import static org.junit.Assert.assertEquals; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import net.sourceforge.pmd.Report; +import net.sourceforge.pmd.Rule; +import net.sourceforge.pmd.RuleViolation; +import net.sourceforge.pmd.lang.LanguageRegistry; +import net.sourceforge.pmd.lang.apex.ApexLanguageModule; +import net.sourceforge.pmd.testframework.RuleTst; + +/** + *

Sharing settings are not inherited by inner classes. Sharing settings need to be declared on the class that + * contains the Database method, DML, SOQL, or SOSL.

+ * + *

This test runs against Apex code that has an Outer class and and Inner class. Different Apex code is generated + * based on the boolean permutations. Any classes that includes data access cod, but doesn't declare a sharing setting + * should trigger a violation.

+ */ +@RunWith(Parameterized.class) +public class ApexSharingViolationsNestedClassTest extends RuleTst { + /** + * Type of operation that may require a sharing declaration. + */ + private enum Operation { + NONE(null), + DML_DELETE("Contact c = new Contact(); delete c;"), + DML_INSERT("Contact c = new Contact(); insert c;"), + DML_MERGE("Contact c1 = new Contact(); Contact c2 = new Contact(); merge c1 c2;"), + DML_UNDELETE("Contact c = new Contact(); undelete c;"), + DML_UPDATE("Contact c = new Contact(); update c;"), + DML_UPSERT("Contact c = new Contact(); upsert c;"), + METHOD_DATABASE("Database.query('Select Id from Contact LIMIT 100');"), + SOQL("[SELECT Name FROM Contact];"), + SOSL("[FIND 'Foo' IN ALL FIELDS RETURNING Account(Name)];"); + + final boolean requiresSharingDeclaration; + final String codeSnippet; + + Operation(String codeSnippet) { + this.requiresSharingDeclaration = codeSnippet != null; + this.codeSnippet = codeSnippet; + } + } + + /** + * The permutations used for class generation. See {@link #generateClass(boolean, Operation, boolean, Operation)} + */ + private final boolean outerSharingDeclared; + private final Operation outerOperation; + private final boolean innerSharingDeclared; + private final Operation innerOperation; + + /** + * Track the expected violations based on the permutations. + */ + private final int expectedViolations; + private final List expectedLineNumbers; + + public ApexSharingViolationsNestedClassTest(boolean outerSharingDeclared, Operation outerOperation, + boolean innerSharingDeclared, Operation innerOperation, + int expectedViolations, List expectedLineNumbers) { + this.outerSharingDeclared = outerSharingDeclared; + this.outerOperation = outerOperation; + this.innerSharingDeclared = innerSharingDeclared; + this.innerOperation = innerOperation; + this.expectedViolations = expectedViolations; + this.expectedLineNumbers = expectedLineNumbers; + } + + @Test + public void testSharingPermutation() { + String apexClass = generateClass(outerSharingDeclared, outerOperation, innerSharingDeclared, innerOperation); + Report rpt = new Report(); + runTestFromString(apexClass, new ApexSharingViolationsRule(), rpt, + LanguageRegistry.getLanguage(ApexLanguageModule.NAME).getDefaultVersion()); + List violations = rpt.getViolations(); + assertEquals("Unexpected Violation Size\n" + apexClass, expectedViolations, violations.size()); + List lineNumbers = violations.stream().map(v -> v.getBeginLine()).collect(Collectors.toList()); + assertEquals("Unexpected Line Numbers\n" + apexClass, expectedLineNumbers, lineNumbers); + } + + @Override + protected List getRules() { + Rule rule = findRule("category/apex/security.xml", "ApexSharingViolations"); + return Collections.singletonList(rule); + } + + /** + * Parameter provider that covers are all permutations + */ + @Parameterized.Parameters + public static Collection data() { + List data = new ArrayList<>(); + + boolean[] boolPermutations = {false, true}; + + for (boolean outerSharingDeclared : boolPermutations) { + for (Operation outerOperation : Operation.values()) { + for (boolean innerSharingDeclared : boolPermutations) { + for (Operation innerOperation : Operation.values()) { + int expectedViolations = 0; + List expectedLineNumbers = new ArrayList<>(); + if (outerOperation.requiresSharingDeclaration && !outerSharingDeclared) { + // The outer class contains SOQL but doesn't declare sharing + expectedViolations++; + expectedLineNumbers.add(1); + } + + if (innerOperation.requiresSharingDeclaration && !innerSharingDeclared) { + // The inner class contains SOQL but doesn't declare sharing + expectedViolations++; + // The location of the inner class declaration depends upon the content of the outer class + expectedLineNumbers.add(outerOperation.requiresSharingDeclaration ? 3 : 2); + } + data.add(new Object[]{outerSharingDeclared, outerOperation, innerSharingDeclared, innerOperation, + expectedViolations, expectedLineNumbers}); + } + } + } + } + + return data; + } + + /** + *

Generate an Apex class with various Sharing/Database/DML/SOQL/SOSL permutations. An example of the class + * returned when invoked with generateClass(true, SOQL, true, SOQL).

+ * + * 
+     * public with sharing class Outer {
+     *    public void outerSOQL() {[SELECT Name FROM Contact];}
+     *    public with sharing class Inner {
+     *       public void innerSOQL() {[SELECT Name FROM Contact];}
+     *    }
+     * }
+     *
+ * + * @param outerSharing Add 'with sharing' to Outer class definition + * @param outerOperation Add a method to Outer class that performs the given operation + * @param innerSharing Add 'with sharing' to Inner class definition + * @param innerOperation Add a method to Inner class that performs the given operation + * @return String that represents Apex code + */ + private static String generateClass(boolean outerSharing, Operation outerOperation, boolean innerSharing, + Operation innerOperation) { + StringBuilder sb = new StringBuilder(); + + sb.append("public "); + if (outerSharing) { + sb.append("with sharing "); + } + sb.append("class Outer {\n"); + switch (outerOperation) { + case NONE: + // Do nothing + break; + default: + sb.append(String.format("\t\tpublic void outer%s(){ %s }\n", outerOperation.name(), outerOperation.codeSnippet)); + break; + } + sb.append("\tpublic "); + if (innerSharing) { + sb.append("with sharing "); + } + sb.append("class Inner {\n"); + switch (innerOperation) { + case NONE: + // DO Nothing + break; + default: + sb.append(String.format("\t\tpublic void inner%s(){ %s }\n", innerOperation.name(), innerOperation.codeSnippet)); + break; + } + sb.append("\t}\n"); // Closes class Inner + sb.append("}\n"); // Closes class Outer + + return sb.toString(); + } +} diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml index 5ff9aebef9..5fb7f381e7 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml @@ -99,6 +99,63 @@ public inherited sharing class MyClass { public List getAllTheSecrets() { return [SELECT Name FROM Contact]; } +} + ]]> + + + + Apex test classes do not need a sharing declaration + 0 + getAllTheSecrets() { + return [SELECT Name FROM Contact]; + } +} + ]]> + + + + + Apex inner test classes do not need a sharing declaration + 0 + getAllTheOuterSecrets() { + return [SELECT Name FROM Contact]; + } + public class Inner { + public List getAllTheInnerSecrets() { + return [SELECT Name FROM Contact]; + } + } +} + ]]> + + + + Nested method calls are detected + 1 + > recordsMap = new Map>(); + + public List getRecords(String query) { + if (!recordsMap.containsKey(query)) { + recordsMap.put(query, Database.query(query)); + } + return recordsMap.get(query); + } +} + ]]> + + + + Trigger does not require sharing + 0 + From d421927547b28a20051f852ac36f97289bd1221c Mon Sep 17 00:00:00 2001 From: Dan Rollo Date: Thu, 17 Sep 2020 12:04:14 -0400 Subject: [PATCH 2/9] Add badge for reproducible build --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index fa1852e804..f9e7e67a02 100644 --- a/README.md +++ b/README.md @@ -3,6 +3,7 @@ [![Join the chat at https://gitter.im/pmd/pmd](https://badges.gitter.im/pmd/pmd.svg)](https://gitter.im/pmd/pmd?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge) [![Build Status](https://travis-ci.org/pmd/pmd.svg?branch=master)](https://travis-ci.org/pmd/pmd) [![Maven Central](https://maven-badges.herokuapp.com/maven-central/net.sourceforge.pmd/pmd/badge.svg)](https://maven-badges.herokuapp.com/maven-central/net.sourceforge.pmd/pmd) +[![Reproducible Builds](https://img.shields.io/badge/Reproducible_Builds-ok-green?labelColor=blue)](https://github.com/jvm-repo-rebuild/reproducible-central#net.sourceforge.pmd:pmd) [![Coverage Status](https://coveralls.io/repos/github/pmd/pmd/badge.svg)](https://coveralls.io/github/pmd/pmd) [![Codacy Badge](https://api.codacy.com/project/badge/Grade/a674ee8642ed44c6ba7633626ee95967)](https://www.codacy.com/app/pmd/pmd?utm_source=github.com&utm_medium=referral&utm_content=pmd/pmd&utm_campaign=Badge_Grade) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-v2.0%20adopted-ff69b4.svg)](code_of_conduct.md) From c21146019ad5637445ed351ed4eccb1c35a0a736 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 17 Sep 2020 19:00:11 +0200 Subject: [PATCH 3/9] [doc] Update release notes, refs #2789 --- docs/pages/release_notes.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 2fa8a74515..377372c1a5 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -68,6 +68,7 @@ AbstractTokenizer and the custom tokenizers of Fortran, Perl and Ruby are deprec * [#2735](https://github.com/pmd/pmd/pull/2735): \[ci] Add github actions for a fast view of pr succeed/not - [XenoAmess](https://github.com/XenoAmess) * [#2747](https://github.com/pmd/pmd/pull/2747): \[java] Don't trigger FinalFieldCouldBeStatic when field is annotated with lombok @Builder.Default - [Ollie Abbey](https://github.com/ollieabbey) * [#2773](https://github.com/pmd/pmd/pull/2773): \[java] issue-2738: Adding null check to avoid npe when switch case is default - [Nimit Patel](https://github.com/nimit-patel) +* [#2789](https://github.com/pmd/pmd/pull/2789): Add badge for reproducible build - [Dan Rollo](https://github.com/bhamail) {% endtocmaker %} From 20930119137620c622b742dda74f24c55f7ab70d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Fri, 18 Sep 2020 03:58:05 +0200 Subject: [PATCH 4/9] Fix array & annot subtyping --- .../pmd/lang/java/types/TypeTestUtil.java | 17 ++-- .../pmd/lang/java/types/TypeTestUtilTest.java | 94 +++++++++++++++++-- 2 files changed, 97 insertions(+), 14 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java index a2d187c631..13f16b7200 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java @@ -4,10 +4,9 @@ package net.sourceforge.pmd.lang.java.types; +import java.lang.reflect.Modifier; import java.util.List; -import org.objectweb.asm.Opcodes; - import net.sourceforge.pmd.lang.java.ast.ASTAnnotationTypeDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; @@ -65,8 +64,11 @@ public final class TypeTestUtil { return true; } - return canBeExtended(clazz) ? isA(clazz.getName(), node) - : isExactlyA(clazz, node); + if (hasNoSubtypes(clazz)) { + return isExactlyA(clazz, node); + } + String canoName = clazz.getCanonicalName(); + return canoName != null && isA(canoName, node); } @@ -198,9 +200,12 @@ public final class TypeTestUtil { } } - private static boolean canBeExtended(Class clazz) { + + private static boolean hasNoSubtypes(Class clazz) { // Neither final nor an annotation. Enums & records have ACC_FINAL - return (clazz.getModifiers() & (Opcodes.ACC_ANNOTATION | Opcodes.ACC_FINAL)) == 0; + // Note: arrays have ACC_FINAL, but have subtypes by covariance + // Note: annotations may be implemented by classes + return Modifier.isFinal(clazz.getModifiers()) && !clazz.isArray(); } // those fallbacks can be removed with the newer symbol resolution, diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/TypeTestUtilTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/TypeTestUtilTest.java index d672afc702..b1c7c660c0 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/TypeTestUtilTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/TypeTestUtilTest.java @@ -4,6 +4,7 @@ package net.sourceforge.pmd.lang.java.types; +import java.io.ObjectStreamField; import java.io.Serializable; import java.lang.annotation.Annotation; import java.util.concurrent.Callable; @@ -20,6 +21,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTEnumDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMarkerAnnotation; import net.sourceforge.pmd.lang.java.ast.ASTName; +import net.sourceforge.pmd.lang.java.ast.ASTType; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.symboltable.BaseNonParserTest; import net.sourceforge.pmd.lang.java.types.testdata.SomeClassWithAnon; @@ -56,12 +58,60 @@ public class TypeTestUtilTest extends BaseNonParserTest { Assert.assertNull(klass.getType()); Assert.assertTrue(TypeTestUtil.isA("org.FooBar", klass)); - assertIsA(klass, Iterable.class); - assertIsA(klass, Enum.class); - assertIsA(klass, Serializable.class); - assertIsA(klass, Object.class); + assertIsStrictSubtype(klass, Iterable.class); + assertIsStrictSubtype(klass, Enum.class); + assertIsStrictSubtype(klass, Serializable.class); + assertIsStrictSubtype(klass, Object.class); } + + @Test + public void testIsAnArrayClass() { + + ASTType arrayT = + java.parse("import java.io.ObjectStreamField; " + + "class Foo { private static final ObjectStreamField[] serialPersistentFields; }") + .getFirstDescendantOfType(ASTType.class); + + + assertIsExactlyA(arrayT, ObjectStreamField[].class); + assertIsStrictSubtype(arrayT, Object[].class); + assertIsStrictSubtype(arrayT, Serializable.class); + assertIsNot(arrayT, Serializable[].class); + assertIsStrictSubtype(arrayT, Object.class); + } + + @Test + public void testIsAnAnnotationClass() { + + ASTType arrayT = + java.parse("class Foo { org.junit.Test field; }") + .getFirstDescendantOfType(ASTType.class); + + + assertIsExactlyA(arrayT, Test.class); + assertIsStrictSubtype(arrayT, Annotation.class); + assertIsStrictSubtype(arrayT, Object.class); + } + + @Test + public void testIsAPrimitiveArrayClass() { + + ASTType arrayT = + java.parse("import java.io.ObjectStreamField; " + + "class Foo { private static final int[] serialPersistentFields; }") + .getFirstDescendantOfType(ASTType.class); + + + assertIsExactlyA(arrayT, int[].class); + assertIsNot(arrayT, long[].class); + assertIsNot(arrayT, Object[].class); + + assertIsStrictSubtype(arrayT, Serializable.class); + assertIsStrictSubtype(arrayT, Object.class); + } + + @Test public void testIsAFallbackAnnotation() { @@ -161,10 +211,38 @@ public class TypeTestUtilTest extends BaseNonParserTest { } private void assertIsA(TypeNode node, Class type) { - Assert.assertTrue("TypeTestUtil::isA with class arg: " + type.getCanonicalName(), - TypeTestUtil.isA(type, node)); - Assert.assertTrue("TypeTestUtil::isA with string arg: " + type.getCanonicalName(), - TypeTestUtil.isA(type.getCanonicalName(), node)); + assertIsA(node, type, false, true); } + private void assertIsExactlyA(TypeNode node, Class type) { + assertIsA(node, type, true, true); + assertIsA(node, type, false, true); + } + + private void assertIsNot(TypeNode node, Class type) { + assertIsA(node, type, true, false); + assertIsA(node, type, false, false); + } + + private void assertIsNotExactly(TypeNode node, Class type) { + assertIsA(node, type, true, false); + } + + private void assertIsStrictSubtype(TypeNode node, Class type) { + assertIsNotExactly(node, type); + assertIsA(node, type); + } + + private void assertIsA(TypeNode node, Class type, boolean exactly, boolean expectTrue) { + Assert.assertEquals("TypeTestUtil::isA with class arg: " + type.getCanonicalName(), + expectTrue, + exactly ? TypeTestUtil.isExactlyA(type, node) + : TypeTestUtil.isA(type, node)); + Assert.assertEquals("TypeTestUtil::isA with string arg: " + type.getCanonicalName(), + expectTrue, + exactly ? TypeTestUtil.isExactlyA(type.getCanonicalName(), node) + : TypeTestUtil.isA(type.getCanonicalName(), node)); + } + + } From 8c97300af8b3d10c010ed0e8c0c6857e05e94a1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Fri, 18 Sep 2020 04:04:29 +0200 Subject: [PATCH 5/9] Fix doc Parameters were in the wrong order. --- .../pmd/lang/java/types/TypeTestUtil.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java index 13f16b7200..b5a8e559bf 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java @@ -41,10 +41,10 @@ public final class TypeTestUtil { * if the type of the node is parameterized. Examples: * * 
{@code
-     * isA(()>, List.class)      = true
-     * isA(()>, ArrayList.class) = true
-     * isA(, int[].class)                  = true
-     * isA(, Object[].class)            = true
+     * isA(List.class, ()>)      = true
+     * isA(ArrayList.class, ()>) = true
+     * isA(int[].class, )                  = true
+     * isA(Object[].class, )            = true
      * isA(_, null) = false
      * isA(null, _) = NullPointerException
      * }
@@ -78,10 +78,10 @@ public final class TypeTestUtil { * if the type of the node is parameterized. Examples: * * 
{@code
-     * isA(()>, "java.util.List")      = true
-     * isA(()>, "java.util.ArrayList") = true
-     * isA(, "int[]")                            = true
-     * isA(, "java.lang.Object[]")            = true
+     * isA("java.util.List", ()>)      = true
+     * isA("java.util.ArrayList", ()>) = true
+     * isA("int[]", )                            = true
+     * isA("java.lang.Object[]", )            = true
      * isA(_, null) = false
      * isA(null, _) = NullPointerException
      * }
@@ -158,10 +158,10 @@ public final class TypeTestUtil { * if the type of the node is parameterized. * * 
{@code
-     * isExactlyA(()>, List.class)      = false
-     * isExactlyA(()>, ArrayList.class) = true
-     * isExactlyA(, int[].class)                  = true
-     * isExactlyA(, Object[].class)            = false
+     * isExactlyA(List.class, ()>)      = false
+     * isExactlyA(ArrayList.class, ()>) = true
+     * isExactlyA(int[].class, )                  = true
+     * isExactlyA(Object[].class, )            = false
      * isExactlyA(_, null) = false
      * isExactlyA(null, _) = NullPointerException
      * }
From 59f1fe251e36ee586d1e91796aab054a98a15872 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 18 Sep 2020 15:53:33 +0200 Subject: [PATCH 6/9] Update ant from 1.10.1 to 1.10.8 Fixes CVE-2020-1945 (Moderate severity) --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 186208bfcb..2b01158a5a 100644 --- a/pom.xml +++ b/pom.xml @@ -95,7 +95,7 @@ 8.30 3.1.1 3.13.0 - 1.10.1 + 1.10.8 3.2.0 4.7 From 5429cebf380233f7569a41feb968ba7fcd30108c Mon Sep 17 00:00:00 2001 From: Jeff Bartolotta Date: Mon, 21 Sep 2020 20:11:07 -0700 Subject: [PATCH 7/9] Add issue reproducers as standard rule tests Added false negative and false positive test cases that were logged with the original issue. --- .../security/xml/ApexSharingViolations.xml | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml index 5fb7f381e7..7cd3d83052 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSharingViolations.xml @@ -156,6 +156,30 @@ public class Dao { + + + + Issue #2774(false positive). Sharing correctly declared on inner class. + 0 + getAllInnerSoqlSecrets() { return [SELECT Name FROM Contact]; } + } +} + ]]> + + + + Issue #2774(false negative). Sharing incorrectly declared on outer class. + 1 + getAllInnerSoqlSecrets() { return [SELECT Name FROM Contact]; } + } } ]]> From eb0f7e4f991c23e5b0a9973bfd977e12c323384b Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 24 Sep 2020 11:54:35 +0200 Subject: [PATCH 8/9] [doc] Update release notes, refs #2791, fixes #2774 --- docs/pages/release_notes.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 5d8124da48..de087aaf08 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -29,6 +29,9 @@ AbstractTokenizer and the custom tokenizers of Fortran, Perl and Ruby are deprec * [#2758](https://github.com/pmd/pmd/pull/2758): \[cpd] Improve AnyTokenizer * [#2760](https://github.com/pmd/pmd/issues/2760): \[cpd] AnyTokenizer doesn't count columns correctly +* apex-security + * [#2774](https://github.com/pmd/pmd/issues/2774): \[apex] ApexSharingViolations does not correlate sharing settings with class that contains data access + * pmd-java * [#2708](https://github.com/pmd/pmd/issues/2708): \[java] False positive FinalFieldCouldBeStatic when using lombok Builder.Default * [#2738](https://github.com/pmd/pmd/issues/2738): \[java] Custom rule with @ExhaustiveEnumSwitch throws NPE @@ -67,6 +70,7 @@ AbstractTokenizer and the custom tokenizers of Fortran, Perl and Ruby are deprec * [#2735](https://github.com/pmd/pmd/pull/2735): \[ci] Add github actions for a fast view of pr succeed/not - [XenoAmess](https://github.com/XenoAmess) * [#2747](https://github.com/pmd/pmd/pull/2747): \[java] Don't trigger FinalFieldCouldBeStatic when field is annotated with lombok @Builder.Default - [Ollie Abbey](https://github.com/ollieabbey) * [#2773](https://github.com/pmd/pmd/pull/2773): \[java] issue-2738: Adding null check to avoid npe when switch case is default - [Nimit Patel](https://github.com/nimit-patel) +* [#2791](https://github.com/pmd/pmd/pull/2791): \[apex] Analyze inner classes for sharing violations - [Jeff Bartolotta](https://github.com/jbartolotta-sfdc) {% endtocmaker %} From f1e9a23b0123e1ad04da8f8cc98b95f549ac5872 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 24 Sep 2020 12:08:10 +0200 Subject: [PATCH 9/9] [doc] Update release notes - group fixes by category --- docs/pages/release_notes.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 36356aee48..0b4353d908 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -32,13 +32,15 @@ AbstractTokenizer and the custom tokenizers of Fortran, Perl and Ruby are deprec * apex-security * [#2774](https://github.com/pmd/pmd/issues/2774): \[apex] ApexSharingViolations does not correlate sharing settings with class that contains data access -* pmd-java - * [#2708](https://github.com/pmd/pmd/issues/2708): \[java] False positive FinalFieldCouldBeStatic when using lombok Builder.Default +* java * [#2738](https://github.com/pmd/pmd/issues/2738): \[java] Custom rule with @ExhaustiveEnumSwitch throws NPE * [#2756](https://github.com/pmd/pmd/issues/2756): \[java] TypeTestUtil fails with NPE for anonymous class - * [#2759](https://github.com/pmd/pmd/issues/2759): \[java] False positive in UnusedAssignment * [#2767](https://github.com/pmd/pmd/issues/2767): \[java] IndexOutOfBoundsException when parsing an initializer BlockStatement * [#2783](https://github.com/pmd/pmd/issues/2783): \[java] Error while parsing with lambda of custom interface +* java-bestpractices + * [#2759](https://github.com/pmd/pmd/issues/2759): \[java] False positive in UnusedAssignment +* java-design + * [#2708](https://github.com/pmd/pmd/issues/2708): \[java] False positive FinalFieldCouldBeStatic when using lombok Builder.Default ### API Changes