From 9c85a788849bc65de245c75991b782c6d22ff712 Mon Sep 17 00:00:00 2001 From: Sergey Date: Wed, 4 Jan 2017 13:33:06 -0800 Subject: [PATCH 1/8] Improving open redirect detection for strings prefixed with / --- .../rule/security/ApexOpenRedirectRule.java | 24 ++++++++++++-- .../rule/security/xml/ApexOpenRedirect.xml | 31 +++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexOpenRedirectRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexOpenRedirectRule.java index 2db134d25d..63ec1d3c36 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexOpenRedirectRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexOpenRedirectRule.java @@ -64,15 +64,33 @@ public class ApexOpenRedirectRule extends AbstractApexRule { } private void findSafeLiterals(AbstractApexNode node) { + ASTBinaryExpression binaryExp = node.getFirstChildOfType(ASTBinaryExpression.class); + if (binaryExp != null) { + findSafeLiterals(binaryExp); + } + ASTLiteralExpression literal = node.getFirstChildOfType(ASTLiteralExpression.class); if (literal != null) { - ASTVariableExpression variable = node.getFirstChildOfType(ASTVariableExpression.class); - if (variable != null) { - listOfStringLiteralVariables.add(Helper.getFQVariableName(variable)); + int index = literal.jjtGetChildIndex(); + if (index == 0) { + if (node instanceof ASTVariableDeclaration) { + addVariable(node); + } else { + ASTVariableDeclaration parent = node.getFirstParentOfType(ASTVariableDeclaration.class); + addVariable(parent); + + } } } } + private void addVariable(AbstractApexNode node) { + ASTVariableExpression variable = node.getFirstChildOfType(ASTVariableExpression.class); + if (variable != null) { + listOfStringLiteralVariables.add(Helper.getFQVariableName(variable)); + } + } + /** * Traverses all new declarations to find PageReferences * diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexOpenRedirect.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexOpenRedirect.xml index 74cca0be67..8278b975cd 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexOpenRedirect.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexOpenRedirect.xml @@ -136,8 +136,39 @@ public class Foo { static PageReference redirect() { return pr; } +} + ]]> + + + + Unsafe pageReference object + 1 + + + Safe pageReference object + 0 + + + + From 0be5a1554cb7beb7f06cd7d7f1b1abdc71e2d795 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Sat, 7 Jan 2017 22:01:28 -0300 Subject: [PATCH 2/8] Update changelog --- src/site/markdown/overview/changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 52bc0fa655..1c46eada7a 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -223,4 +223,5 @@ to avoid XSS attacks. * [#169](https://github.com/pmd/pmd/pull/169): \[apex] Improving detection for DML with inline new object * [#172](https://github.com/pmd/pmd/pull/172): \[apex] Bug fix, detects both Apex fields and class members * [#175](https://github.com/pmd/pmd/pull/175): \[apex] ApexXSSFromURLParam: Adding missing casting methods +* [#176](https://github.com/pmd/pmd/pull/176): \[apex] Bug fix for FP: open redirect for strings prefixed with / is safe From 03a5aa2519a14bae091b8373a0f34c4ae0deb085 Mon Sep 17 00:00:00 2001 From: Sergey Date: Thu, 5 Jan 2017 14:50:43 -0800 Subject: [PATCH 3/8] Legacy test class declaration --- .../pmd/lang/apex/rule/security/Helper.java | 6 ++++++ .../apex/rule/security/xml/ApexCRUDViolation.xml | 12 ++++++++++++ 2 files changed, 18 insertions(+) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/Helper.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/Helper.java index 95ba9372d0..518ef4ba81 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/Helper.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/Helper.java @@ -49,6 +49,12 @@ public final class Helper { return true; } } + + final String className = node.getNode().getDefiningType().getApexName(); + if (className.endsWith("Test")) { + return true; + } + return false; } diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml index 1776c3210b..972d9fbad6 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml @@ -465,6 +465,18 @@ public class Foo { } } } +} + ]]> + + + + No issues found in test classes + 0 + From 6f6d37b2a1ed7246e6a7aac2868e61d512d0cdaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Sat, 7 Jan 2017 22:22:07 -0300 Subject: [PATCH 4/8] Update changelog --- src/site/markdown/overview/changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 1c46eada7a..db574ad84d 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -224,4 +224,5 @@ to avoid XSS attacks. * [#172](https://github.com/pmd/pmd/pull/172): \[apex] Bug fix, detects both Apex fields and class members * [#175](https://github.com/pmd/pmd/pull/175): \[apex] ApexXSSFromURLParam: Adding missing casting methods * [#176](https://github.com/pmd/pmd/pull/176): \[apex] Bug fix for FP: open redirect for strings prefixed with / is safe +* [#179](https://github.com/pmd/pmd/pull/179): \[apex] Legacy test class declaration support From 174ffa9461b20ac99577489cb888ef18700f0b59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Thu, 12 Jan 2017 22:03:15 -0300 Subject: [PATCH 5/8] Update changelog --- src/site/markdown/overview/changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index eb6adc0444..3e9cd1c390 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -227,4 +227,5 @@ to avoid XSS attacks. * [#175](https://github.com/pmd/pmd/pull/175): \[apex] ApexXSSFromURLParam: Adding missing casting methods * [#176](https://github.com/pmd/pmd/pull/176): \[apex] Bug fix for FP: open redirect for strings prefixed with / is safe * [#179](https://github.com/pmd/pmd/pull/179): \[apex] Legacy test class declaration support +* [#181](https://github.com/pmd/pmd/pull/181): \[apex] Control flow based CRUD rule checking From 4055a79e4c36fdd68f567810407b5780db0e991f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Thu, 12 Jan 2017 22:35:09 -0300 Subject: [PATCH 6/8] Update changelog --- src/site/markdown/overview/changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 3e9cd1c390..830a043722 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -228,4 +228,5 @@ to avoid XSS attacks. * [#176](https://github.com/pmd/pmd/pull/176): \[apex] Bug fix for FP: open redirect for strings prefixed with / is safe * [#179](https://github.com/pmd/pmd/pull/179): \[apex] Legacy test class declaration support * [#181](https://github.com/pmd/pmd/pull/181): \[apex] Control flow based CRUD rule checking +* [#184](https://github.com/pmd/pmd/pull/184): \[apex] Improving open redirect detection for static fields & assignment operations From 80b2792db5eb3962d356a691f55602d71e42d5a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Thu, 12 Jan 2017 17:41:29 -0300 Subject: [PATCH 7/8] Fix #183 - Count whole expressions as 1 line - This is consistent with how we count "lines" in other languages. - Notice we are not actually counting lines, so multiline expressions are counted as 1. - Having multiple variable declarations together is still counting 1 per variable. --- .../complexity/AbstractNcssCountRule.java | 221 +++++++++--------- .../rule/complexity/xml/NcssMethodCount.xml | 23 ++ 2 files changed, 131 insertions(+), 113 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/complexity/AbstractNcssCountRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/complexity/AbstractNcssCountRule.java index 5899c9ae47..f152c488b2 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/complexity/AbstractNcssCountRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/complexity/AbstractNcssCountRule.java @@ -5,7 +5,6 @@ package net.sourceforge.pmd.lang.apex.rule.complexity; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.apex.ast.ASTBreakStatement; -import net.sourceforge.pmd.lang.apex.ast.ASTTryCatchFinallyBlockStatement; import net.sourceforge.pmd.lang.apex.ast.ASTContinueStatement; import net.sourceforge.pmd.lang.apex.ast.ASTDoLoopStatement; import net.sourceforge.pmd.lang.apex.ast.ASTForEachStatement; @@ -17,6 +16,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTReturnStatement; import net.sourceforge.pmd.lang.apex.ast.ASTStatement; import net.sourceforge.pmd.lang.apex.ast.ASTThrowStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTTryCatchFinallyBlockStatement; import net.sourceforge.pmd.lang.apex.ast.ASTWhileLoopStatement; import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractStatisticalApexRule; @@ -31,139 +31,134 @@ import net.sourceforge.pmd.util.NumericConstants; */ public abstract class AbstractNcssCountRule extends AbstractStatisticalApexRule { - private Class nodeClass; + private Class nodeClass; - /** - * Count the nodes of the given type using NCSS rules. - * - * @param nodeClass - * class of node to count - */ - protected AbstractNcssCountRule(Class nodeClass) { - this.nodeClass = nodeClass; + /** + * Count the nodes of the given type using NCSS rules. + * + * @param nodeClass + * class of node to count + */ + protected AbstractNcssCountRule(Class nodeClass) { + this.nodeClass = nodeClass; - setProperty(MINIMUM_DESCRIPTOR, 1000d); - setProperty(CODECLIMATE_CATEGORIES, new String[]{ "Complexity" }); - setProperty(CODECLIMATE_REMEDIATION_MULTIPLIER, 100); - setProperty(CODECLIMATE_BLOCK_HIGHLIGHTING, false); - } + setProperty(MINIMUM_DESCRIPTOR, 1000d); + setProperty(CODECLIMATE_CATEGORIES, new String[] { "Complexity" }); + setProperty(CODECLIMATE_REMEDIATION_MULTIPLIER, 100); + setProperty(CODECLIMATE_BLOCK_HIGHLIGHTING, false); + } - @Override - public Object visit(ApexNode node, Object data) { - int numNodes = 0; + @Override + public Object visit(ApexNode node, Object data) { + int numNodes = 0; - for (int i = 0; i < node.jjtGetNumChildren(); i++) { - ApexNode n = (ApexNode) node.jjtGetChild(i); - Integer treeSize = (Integer) n.jjtAccept(this, data); - numNodes += treeSize.intValue(); - } + for (int i = 0; i < node.jjtGetNumChildren(); i++) { + ApexNode n = (ApexNode) node.jjtGetChild(i); + Integer treeSize = (Integer) n.jjtAccept(this, data); + numNodes += treeSize.intValue(); + } - if (this.nodeClass.isInstance(node)) { - // Add 1 to account for base node - numNodes++; - DataPoint point = new DataPoint(); - point.setNode(node); - point.setScore(1.0 * numNodes); - point.setMessage(getMessage()); - addDataPoint(point); - } + if (this.nodeClass.isInstance(node)) { + // Add 1 to account for base node + numNodes++; + DataPoint point = new DataPoint(); + point.setNode(node); + point.setScore(1.0 * numNodes); + point.setMessage(getMessage()); + addDataPoint(point); + } - return Integer.valueOf(numNodes); - } + return Integer.valueOf(numNodes); + } - /** - * Count the number of children of the given node. Adds one to count the - * node itself. - * - * @param node - * node having children counted - * @param data - * node data - * @return count of the number of children of the node, plus one - */ - protected Integer countNodeChildren(Node node, Object data) { - Integer nodeCount; - int lineCount = 0; - for (int i = 0; i < node.jjtGetNumChildren(); i++) { - nodeCount = (Integer) ((ApexNode) node.jjtGetChild(i)).jjtAccept(this, data); - lineCount += nodeCount.intValue(); - } - return ++lineCount; - } + /** + * Count the number of children of the given node. Adds one to count the + * node itself. + * + * @param node + * node having children counted + * @param data + * node data + * @return count of the number of children of the node, plus one + */ + protected Integer countNodeChildren(Node node, Object data) { + Integer nodeCount; + int lineCount = 0; + for (int i = 0; i < node.jjtGetNumChildren(); i++) { + nodeCount = (Integer) ((ApexNode) node.jjtGetChild(i)).jjtAccept(this, data); + lineCount += nodeCount.intValue(); + } + return ++lineCount; + } - @Override - public Object visit(ASTForLoopStatement node, Object data) { - return countNodeChildren(node, data); - } + @Override + public Object visit(ASTForLoopStatement node, Object data) { + return countNodeChildren(node, data); + } - @Override - public Object visit(ASTForEachStatement node, Object data) { - return countNodeChildren(node, data); - } + @Override + public Object visit(ASTForEachStatement node, Object data) { + return countNodeChildren(node, data); + } - @Override - public Object visit(ASTDoLoopStatement node, Object data) { - return countNodeChildren(node, data); - } + @Override + public Object visit(ASTDoLoopStatement node, Object data) { + return countNodeChildren(node, data); + } - @Override - public Object visit(ASTIfBlockStatement node, Object data) { + @Override + public Object visit(ASTIfBlockStatement node, Object data) { - Integer lineCount = countNodeChildren(node, data); - return lineCount; - } + Integer lineCount = countNodeChildren(node, data); + return lineCount; + } - @Override - public Object visit(ASTIfElseBlockStatement node, Object data) { + @Override + public Object visit(ASTIfElseBlockStatement node, Object data) { - Integer lineCount = countNodeChildren(node, data); - lineCount++; + Integer lineCount = countNodeChildren(node, data); + lineCount++; - return lineCount; - } + return lineCount; + } - @Override - public Object visit(ASTWhileLoopStatement node, Object data) { - return countNodeChildren(node, data); - } + @Override + public Object visit(ASTWhileLoopStatement node, Object data) { + return countNodeChildren(node, data); + } - @Override - public Object visit(ASTBreakStatement node, Object data) { - return NumericConstants.ONE; - } + @Override + public Object visit(ASTBreakStatement node, Object data) { + return NumericConstants.ONE; + } - @Override - public Object visit(ASTTryCatchFinallyBlockStatement node, Object data) { - return countNodeChildren(node, data); - } + @Override + public Object visit(ASTTryCatchFinallyBlockStatement node, Object data) { + return countNodeChildren(node, data); + } - @Override - public Object visit(ASTContinueStatement node, Object data) { - return NumericConstants.ONE; - } + @Override + public Object visit(ASTContinueStatement node, Object data) { + return NumericConstants.ONE; + } - @Override - public Object visit(ASTReturnStatement node, Object data) { - return countNodeChildren(node, data); - } + @Override + public Object visit(ASTReturnStatement node, Object data) { + return countNodeChildren(node, data); + } - @Override - public Object visit(ASTThrowStatement node, Object data) { - return countNodeChildren(node, data); - } + @Override + public Object visit(ASTThrowStatement node, Object data) { + return countNodeChildren(node, data); + } - @Override - public Object visit(ASTStatement node, Object data) { - return NumericConstants.ONE; - } + @Override + public Object visit(ASTStatement node, Object data) { + return NumericConstants.ONE; + } - @Override - public Object visit(ASTVariableDeclaration node, Object data) { - return countNodeChildren(node, data); - } - - @Override - public Object visit(ASTMethodCallExpression node, Object data) { - return countNodeChildren(node, data); - } + @Override + public Object visit(ASTMethodCallExpression node, Object data) { + return NumericConstants.ONE; + } } diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/complexity/xml/NcssMethodCount.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/complexity/xml/NcssMethodCount.xml index 6879b0d3c7..ccc0fd10d2 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/complexity/xml/NcssMethodCount.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/complexity/xml/NcssMethodCount.xml @@ -103,5 +103,28 @@ public class Foo { } ]]> + + Github issue #183 - lines are counted properly + 10 + 0 + +@isTest +private class AcceptanceTests_Test { + @isTest + private static void test() { + // Setup + Opportunity o1 = new Opportunity() + .add(new Contact().foo(1) .bar(1).year(2012) .bar(1).price(5) .vol(100)) + .add(new Contact().foo(1) .bar(2).year(2013) .bar(1).price(5) .vol(110)) + .add(new Contact().foo(1) .bar(3).year(2014) .bar(1).price(5) .vol(120)) + .add(new Contact().foo(1) .bar(4).year(2015) .bar(1).price(5) .vol(130)) + .persist(); + + // Verify + System.assert(attribute()); + } +} + + From 2fdfa26e4cb12e441628f3ed2b57de1e0ad042a2 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sun, 15 Jan 2017 19:24:02 +0100 Subject: [PATCH 8/8] Update changelog --- src/site/markdown/overview/changelog.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 830a043722..dfa65760e5 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -180,6 +180,8 @@ to avoid XSS attacks. * [#1542](https://sourceforge.net/p/pmd/bugs/1542/): \[java] CPD throws an NPE when parsing enums with -ignore-identifiers * apex-apexunit * [#1543](https://sourceforge.net/p/pmd/bugs/1543/): \[apex] ApexUnitTestClassShouldHaveAsserts assumes APEX is case sensitive +* apex-complexity + * [#183](https://github.com/pmd/pmd/issues/183): \[apex] NCSS Method length is incorrect when using method chaining * Java * [#1545](https://sourceforge.net/p/pmd/bugs/1545/): \[java] Symbol Table fails to resolve inner classes * java-design