From 9baf99f19dd55e298f759f4afc56df99f4101cbc Mon Sep 17 00:00:00 2001 From: Sergey Gorbaty Date: Mon, 9 Jan 2017 14:37:02 -0800 Subject: [PATCH 1/5] CF based CRUD rule checking --- .../rule/security/ApexCRUDViolationRule.java | 190 ++++++++++++------ .../rule/security/xml/ApexCRUDViolation.xml | 74 +++++++ 2 files changed, 206 insertions(+), 58 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java index 6bfa0c5b01..059bd441f7 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java @@ -3,17 +3,22 @@ */ package net.sourceforge.pmd.lang.apex.rule.security; -import java.util.List; -import java.util.regex.Matcher; -import java.util.regex.Pattern; -import java.util.stream.Collectors; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ListMultimap; import apex.jorje.data.ast.Identifier; import net.sourceforge.pmd.lang.apex.ast.ASTAssignmentExpression; +import net.sourceforge.pmd.lang.apex.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.apex.ast.ASTDmlDeleteStatement; import net.sourceforge.pmd.lang.apex.ast.ASTDmlInsertStatement; import net.sourceforge.pmd.lang.apex.ast.ASTDmlMergeStatement; @@ -46,6 +51,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule { private final HashMap varToTypeMapping = new HashMap<>(); private final ListMultimap typeToDMLOperationMapping = ArrayListMultimap.create(); private final HashMap checkedTypeToDMLOperationViaESAPI = new HashMap<>(); + private final WeakHashMap classMethods = new WeakHashMap<>(); private static final String IS_CREATEABLE = "isCreateable"; private static final String IS_DELETABLE = "isDeletable"; @@ -74,50 +80,22 @@ public class ApexCRUDViolationRule extends AbstractApexRule { setProperty(CODECLIMATE_BLOCK_HIGHLIGHTING, false); } + @Override + public Object visit(ASTUserClass node, Object data) { + for (ASTMethod n : node.findDescendantsOfType(ASTMethod.class)) { + StringBuilder sb = new StringBuilder().append(n.getNode().getDefiningType().getApexName()).append(":") + .append(n.getNode().getMethodInfo().getCanonicalName()).append(":") + .append(n.getNode().getMethodInfo().getParameterTypes().size()); + classMethods.put(sb.toString(), n); + } + + node.childrenAccept(this, data); + return data; + } + @Override public Object visit(ASTMethodCallExpression node, Object data) { - final String method = node.getNode().getMethodName(); - final ASTReferenceExpression ref = node.getFirstChildOfType(ASTReferenceExpression.class); - if (ref == null) { - return data; - } - - List a = ref.getNode().getJadtIdentifiers(); - if (!a.isEmpty()) { - extractObjectAndFields(a, method, node.getNode().getDefiningType().getApexName()); - } else { - // see if ESAPI - if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_VIEW)) { - extractObjectTypeFromESAPI(node, IS_ACCESSIBLE); - } - - if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_CREATE)) { - extractObjectTypeFromESAPI(node, IS_CREATEABLE); - } - - if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_UPDATE)) { - extractObjectTypeFromESAPI(node, IS_UPDATEABLE); - } - - if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_DELETE)) { - extractObjectTypeFromESAPI(node, IS_DELETABLE); - } - - // see if getDescribe() - final ASTDottedExpression dottedExpr = ref.getFirstChildOfType(ASTDottedExpression.class); - if (dottedExpr != null) { - final ASTMethodCallExpression nestedMethodCall = dottedExpr - .getFirstChildOfType(ASTMethodCallExpression.class); - if (nestedMethodCall != null) { - if (isLastMethodName(nestedMethodCall, S_OBJECT_TYPE, GET_DESCRIBE)) { - String resolvedType = getType(nestedMethodCall); - typeToDMLOperationMapping.put(resolvedType, method); - } - } - } - - } - + performMethodLevelChecks(node); return data; } @@ -170,9 +148,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule { } String type = node.getNode().getLocalInfo().getType().getApexName(); - StringBuilder sb = new StringBuilder().append(node.getNode().getDefiningType().getApexName()).append(":") - .append(node.getNode().getLocalInfo().getName()); - addVariableToMapping(sb.toString(), type); + addVariableToMapping(Helper.getFQVariableName(node), type); return data; @@ -208,20 +184,58 @@ public class ApexCRUDViolationRule extends AbstractApexRule { public Object visit(final ASTProperty node, Object data) { ASTField field = node.getFirstChildOfType(ASTField.class); if (field != null) { - String fieldName = field.getNode().getFieldInfo().getName(); String fieldType = field.getNode().getFieldInfo().getType().getApexName(); - - StringBuilder sb = new StringBuilder().append(field.getNode().getDefiningType().getApexName()).append(":") - .append(fieldName); - - addVariableToMapping(sb.toString(), fieldType); - + addVariableToMapping(Helper.getFQVariableName(field), fieldType); } return data; } + private void performMethodLevelChecks(final ASTMethodCallExpression node) { + final String method = node.getNode().getMethodName(); + final ASTReferenceExpression ref = node.getFirstChildOfType(ASTReferenceExpression.class); + if (ref == null) { + return; + } + + List a = ref.getNode().getJadtIdentifiers(); + if (!a.isEmpty()) { + extractObjectAndFields(a, method, node.getNode().getDefiningType().getApexName()); + } else { + // see if ESAPI + if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_VIEW)) { + extractObjectTypeFromESAPI(node, IS_ACCESSIBLE); + } + + if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_CREATE)) { + extractObjectTypeFromESAPI(node, IS_CREATEABLE); + } + + if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_UPDATE)) { + extractObjectTypeFromESAPI(node, IS_UPDATEABLE); + } + + if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED_TO_DELETE)) { + extractObjectTypeFromESAPI(node, IS_DELETABLE); + } + + // see if getDescribe() + final ASTDottedExpression dottedExpr = ref.getFirstChildOfType(ASTDottedExpression.class); + if (dottedExpr != null) { + final ASTMethodCallExpression nestedMethodCall = dottedExpr + .getFirstChildOfType(ASTMethodCallExpression.class); + if (nestedMethodCall != null) { + if (isLastMethodName(nestedMethodCall, S_OBJECT_TYPE, GET_DESCRIBE)) { + String resolvedType = getType(nestedMethodCall); + typeToDMLOperationMapping.put(resolvedType, method); + } + } + } + + } + } + private boolean isLastMethodName(final ASTMethodCallExpression methodNode, final String className, final String methodName) { final ASTReferenceExpression reference = methodNode.getFirstChildOfType(ASTReferenceExpression.class); @@ -256,6 +270,11 @@ public class ApexCRUDViolationRule extends AbstractApexRule { } private void checkForCRUD(final AbstractApexNode node, final Object data, final String crudMethod) { + final HashSet prevCalls = getPreviousCalls(node); + for (ASTMethodCallExpression prevCall : prevCalls) { + performMethodLevelChecks(prevCall); + } + final ASTMethod wrappingMethod = node.getFirstParentOfType(ASTMethod.class); final ASTUserClass wrappingClass = node.getFirstParentOfType(ASTUserClass.class); @@ -274,14 +293,69 @@ public class ApexCRUDViolationRule extends AbstractApexRule { if (variable != null) { final String type = varToTypeMapping.get(Helper.getFQVariableName(variable)); if (type != null) { - StringBuilder typeCheck = new StringBuilder().append(node.getNode().getDefiningType().getApexName()).append(":") - .append(type); + StringBuilder typeCheck = new StringBuilder().append(node.getNode().getDefiningType().getApexName()) + .append(":").append(type); validateCRUDCheckPresent(node, data, crudMethod, typeCheck.toString()); } } } + private HashSet getPreviousCalls(final AbstractApexNode self) { + final HashSet innerMethodCalls = new HashSet<>(); + + final ASTBlockStatement blockStatement = self.getFirstParentOfType(ASTBlockStatement.class); + if (blockStatement != null) { + + List nodes = blockStatement.findDescendantsOfType(ASTMethodCallExpression.class); + mapCallToMethodDecl(self, innerMethodCalls, nodes); + + } + + final List specialMethods = findSpecialMethods(self); + for (ASTMethod method : specialMethods) { + innerMethodCalls.addAll(method.findDescendantsOfType(ASTMethodCallExpression.class)); + } + + // some methods might be within this class + mapCallToMethodDecl(self, innerMethodCalls, new ArrayList(innerMethodCalls)); + + return innerMethodCalls; + } + + private void mapCallToMethodDecl(final AbstractApexNode self, + final HashSet innerMethodCalls, final List nodes) { + for (ASTMethodCallExpression node : nodes) { + if (node == self) { + break; + } + + final ASTMethod methodBody = resolveMethodCalls(node); + if (methodBody != null) { + innerMethodCalls.addAll(methodBody.findDescendantsOfType(ASTMethodCallExpression.class)); + } + + } + } + + private List findSpecialMethods(final AbstractApexNode node) { + final ArrayList ret = new ArrayList<>(); + final Set constructors = classMethods.keySet().stream() + .filter(p -> (p.contains("") || p.contains(""))).collect(Collectors.toSet()); + + for (String c : constructors) { + ret.add(classMethods.get(c)); + } + + return ret; + } + + private ASTMethod resolveMethodCalls(final ASTMethodCallExpression node) { + StringBuilder sb = new StringBuilder().append(node.getNode().getDefiningType().getApexName()).append(":") + .append(node.getNode().getMethodName()).append(":").append(node.getNode().getInputParameters().size()); + return classMethods.get(sb.toString()); + } + private boolean isProperESAPICheckForDML(final String typeToCheck, final String dmlOperation) { final boolean hasMapping = checkedTypeToDMLOperationViaESAPI.containsKey(typeToCheck.toString()); if (hasMapping) { 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..0d3835c8e4 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 @@ -468,4 +468,78 @@ public class Foo { } ]]> + + + No issues found in test classes + 0 + + + + + Control flow based CRUD,FLS check + 0 + + + + Control flow based CRUD,FLS check recursive + 0 + + + + Control flow constructor based CRUD,FLS check + 0 + + From 7a3ab3d7a18649ad60d8517044e64cbb9bcadccd Mon Sep 17 00:00:00 2001 From: Sergey Gorbaty Date: Tue, 10 Jan 2017 09:46:10 -0800 Subject: [PATCH 2/5] Accessibility checks improved with CF --- .../rule/security/ApexCRUDViolationRule.java | 17 ++++-- .../rule/security/xml/ApexCRUDViolation.xml | 60 +++++++++++++++++++ 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java index 059bd441f7..b9591d66fd 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java @@ -95,7 +95,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule { @Override public Object visit(ASTMethodCallExpression node, Object data) { - performMethodLevelChecks(node); + collectCRUDMethodLevelChecks(node); return data; } @@ -192,7 +192,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule { } - private void performMethodLevelChecks(final ASTMethodCallExpression node) { + private void collectCRUDMethodLevelChecks(final ASTMethodCallExpression node) { final String method = node.getNode().getMethodName(); final ASTReferenceExpression ref = node.getFirstChildOfType(ASTReferenceExpression.class); if (ref == null) { @@ -270,9 +270,9 @@ public class ApexCRUDViolationRule extends AbstractApexRule { } private void checkForCRUD(final AbstractApexNode node, final Object data, final String crudMethod) { - final HashSet prevCalls = getPreviousCalls(node); + final HashSet prevCalls = getPreviousMethodCalls(node); for (ASTMethodCallExpression prevCall : prevCalls) { - performMethodLevelChecks(prevCall); + collectCRUDMethodLevelChecks(prevCall); } final ASTMethod wrappingMethod = node.getFirstParentOfType(ASTMethod.class); @@ -301,7 +301,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule { } } - private HashSet getPreviousCalls(final AbstractApexNode self) { + private HashSet getPreviousMethodCalls(final AbstractApexNode self) { final HashSet innerMethodCalls = new HashSet<>(); final ASTBlockStatement blockStatement = self.getFirstParentOfType(ASTBlockStatement.class); @@ -414,7 +414,12 @@ public class ApexCRUDViolationRule extends AbstractApexRule { } } - private void checkForAccessibility(final AbstractApexNode node, Object data) { + private void checkForAccessibility(final ASTSoqlExpression node, Object data) { + final HashSet prevCalls = getPreviousMethodCalls(node); + for (ASTMethodCallExpression prevCall : prevCalls) { + collectCRUDMethodLevelChecks(prevCall); + } + boolean isGetter = false; String returnType = null; 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 0d3835c8e4..aadadb5398 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 @@ -542,4 +542,64 @@ public class Foo { } ]]> + + + Control flow accessibility CRUD check + 0 + + + + + Control flow substitute CRUD check + 0 + + + + + Forgot to call the CRUD check + 1 + + From 9d384f404c62386c27f0fbf67513776a35bf6045 Mon Sep 17 00:00:00 2001 From: Sergey Date: Thu, 12 Jan 2017 12:11:01 -0800 Subject: [PATCH 3/5] Bug fixes --- .../rule/security/ApexCRUDViolationRule.java | 52 ++++++--- .../rule/security/xml/ApexCRUDViolation.xml | 18 +++ .../net/sourceforge/pmd/ant/Formatter.java | 110 ++++++++++++++++-- src/site/markdown/overview/changelog.md | 1 + src/site/markdown/usage/ant-task.md | 6 +- 5 files changed, 165 insertions(+), 22 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java index b9591d66fd..71e255ca67 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java @@ -26,6 +26,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTDmlUpdateStatement; import net.sourceforge.pmd.lang.apex.ast.ASTDmlUpsertStatement; import net.sourceforge.pmd.lang.apex.ast.ASTDottedExpression; import net.sourceforge.pmd.lang.apex.ast.ASTField; +import net.sourceforge.pmd.lang.apex.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTMethod; import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; import net.sourceforge.pmd.lang.apex.ast.ASTNewNameValueObjectExpression; @@ -38,6 +39,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; import net.sourceforge.pmd.lang.apex.ast.AbstractApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; +import net.sourceforge.pmd.lang.ast.Node; /** * Finding missed CRUD checks for SOQL and DML operations. @@ -142,13 +144,24 @@ public class ApexCRUDViolationRule extends AbstractApexRule { @Override public Object visit(final ASTVariableDeclaration node, Object data) { + String type = node.getNode().getLocalInfo().getType().getApexName(); + addVariableToMapping(Helper.getFQVariableName(node), type); + final ASTSoqlExpression soql = node.getFirstChildOfType(ASTSoqlExpression.class); if (soql != null) { checkForAccessibility(soql, data); } - String type = node.getNode().getLocalInfo().getType().getApexName(); - addVariableToMapping(Helper.getFQVariableName(node), type); + return data; + + } + + @Override + public Object visit(final ASTFieldDeclaration node, Object data) { + final ASTSoqlExpression soql = node.getFirstChildOfType(ASTSoqlExpression.class); + if (soql != null) { + checkForAccessibility(soql, data); + } return data; @@ -303,23 +316,34 @@ public class ApexCRUDViolationRule extends AbstractApexRule { private HashSet getPreviousMethodCalls(final AbstractApexNode self) { final HashSet innerMethodCalls = new HashSet<>(); + final ASTMethod outerMethod = self.getFirstParentOfType(ASTMethod.class); + if (outerMethod != null) { + final ASTBlockStatement blockStatement = outerMethod.getFirstChildOfType(ASTBlockStatement.class); + if (blockStatement != null) { + int numberOfStatements = blockStatement.jjtGetNumChildren(); + for (int i = 0; i < numberOfStatements; i++) { + Node n = blockStatement.jjtGetChild(i); + AbstractApexNode match = n.getFirstDescendantOfType(self.getClass()); + if (match == self) { + break; + } + ASTMethodCallExpression methodCall = n.getFirstDescendantOfType(ASTMethodCallExpression.class); + if (methodCall != null) { + mapCallToMethodDecl(self, innerMethodCalls, Arrays.asList(methodCall)); + } + } - final ASTBlockStatement blockStatement = self.getFirstParentOfType(ASTBlockStatement.class); - if (blockStatement != null) { + } - List nodes = blockStatement.findDescendantsOfType(ASTMethodCallExpression.class); - mapCallToMethodDecl(self, innerMethodCalls, nodes); + final List specialMethods = findSpecialMethods(self); + for (ASTMethod method : specialMethods) { + innerMethodCalls.addAll(method.findDescendantsOfType(ASTMethodCallExpression.class)); + } + // some methods might be within this class + mapCallToMethodDecl(self, innerMethodCalls, new ArrayList(innerMethodCalls)); } - final List specialMethods = findSpecialMethods(self); - for (ASTMethod method : specialMethods) { - innerMethodCalls.addAll(method.findDescendantsOfType(ASTMethodCallExpression.class)); - } - - // some methods might be within this class - mapCallToMethodDecl(self, innerMethodCalls, new ArrayList(innerMethodCalls)); - return innerMethodCalls; } 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 aadadb5398..b54a555f06 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 @@ -600,6 +600,24 @@ public class Foo { } } } +]]> + + + Control flow substitute CRUD check should fail when check follows SOQL + 1 + profiles = [SELECT Id FROM Profile WHERE Name = 'System Administrator']; + checkPerms(); + } + + private void checkPerms() { + if (!Profile.sObjectType.getDescribe().isCreateable()) { + throw new NoAccessException(); + } + } +} ]]> diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/ant/Formatter.java b/pmd-core/src/main/java/net/sourceforge/pmd/ant/Formatter.java index 0cf95fa414..277ad57292 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/ant/Formatter.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/ant/Formatter.java @@ -4,15 +4,23 @@ package net.sourceforge.pmd.ant; import java.io.BufferedWriter; +import java.io.Console; import java.io.File; -import java.io.FileWriter; +import java.io.FileOutputStream; import java.io.IOException; +import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.Writer; +import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; import java.util.Properties; +import org.apache.commons.io.IOUtils; import org.apache.tools.ant.BuildException; import org.apache.tools.ant.types.Parameter; @@ -56,12 +64,43 @@ public class Formatter { } public void start(String baseDir) { + + Properties properties = createProperties(); + + Charset charset; + { + String s = (String) properties.get("encoding"); + if (null == s) { + + if (toConsole) { + s = getConsoleEncoding(); + if (null == s) { + s = System.getProperty("file.encoding"); + } + } + + if (null == s) { + charset = StandardCharsets.UTF_8; + } else { + charset = Charset.forName(s); + } + + // Configures the encoding for the renderer. + final Parameter parameter = new Parameter(); + parameter.setName("encoding"); + parameter.setValue(charset.name()); + parameters.add(parameter); + } else { + charset = Charset.forName(s); + } + } + try { if (toConsole) { - writer = new BufferedWriter(new OutputStreamWriter(System.out)); + writer = new BufferedWriter(new OutputStreamWriter(System.out, charset)); } if (toFile != null) { - writer = getToFileWriter(baseDir); + writer = getToFileWriter(baseDir, toFile, charset); } renderer = createRenderer(); renderer.setWriter(writer); @@ -131,10 +170,67 @@ public class Formatter { return properties; } - private Writer getToFileWriter(String baseDir) throws IOException { - if (!toFile.isAbsolute()) { - return new BufferedWriter(new FileWriter(new File(baseDir + System.getProperty("file.separator") + toFile.getPath()))); + private static Writer getToFileWriter(String baseDir, File toFile, Charset charset) throws IOException { + final File file; + if (toFile.isAbsolute()) { + file = toFile; + } else { + file = new File(baseDir + System.getProperty("file.separator") + toFile.getPath()); } - return new BufferedWriter(new FileWriter(toFile)); + + OutputStream output = null; + Writer writer = null; + boolean isOnError = true; + try { + output = new FileOutputStream(file); + writer = new OutputStreamWriter(output, charset); + writer = new BufferedWriter(writer); + isOnError = false; + } finally { + if (isOnError) { + IOUtils.closeQuietly(output); + IOUtils.closeQuietly(writer); + } + } + return writer; + } + + private static String getConsoleEncoding() { + Console console = System.console(); + // in case of pipe or redirect, no interactive console. + if (console != null) { + try { + Field f = Console.class.getDeclaredField("cs"); + f.setAccessible(true); + Object res = f.get(console); + if (res instanceof Charset) { + return ((Charset) res).name(); + } + } catch (NoSuchFieldException e) { + // fall-through + } catch (IllegalAccessException e) { + // fall-through + } + return getNativeConsoleEncoding(); + } + return null; + } + + private static String getNativeConsoleEncoding() { + try { + Method m = Console.class.getDeclaredMethod("encoding"); + m.setAccessible(true); + Object res = m.invoke(null); + if (res instanceof String) { + return (String) res; + } + } catch (NoSuchMethodException e) { + // fall-through + } catch (InvocationTargetException e) { + // fall-through + } catch (IllegalAccessException e) { + // fall-through + } + return null; } } diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index cabd7a9b20..2372c59db8 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -220,6 +220,7 @@ to avoid XSS attacks. * [#165](https://github.com/pmd/pmd/pull/165): \[apex] Improving open redirect rule to avoid test classes/methods * [#167](https://github.com/pmd/pmd/pull/167): \[apex] GC and thread safety changes * [#169](https://github.com/pmd/pmd/pull/169): \[apex] Improving detection for DML with inline new object +* [#170](https://github.com/pmd/pmd/pull/170): \[core] Ant Task Formatter encoding issue with XMLRenderer * [#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 diff --git a/src/site/markdown/usage/ant-task.md b/src/site/markdown/usage/ant-task.md index a7553b7b65..5eacce2a56 100644 --- a/src/site/markdown/usage/ant-task.md +++ b/src/site/markdown/usage/ant-task.md @@ -98,6 +98,8 @@ Runs a set of static code analysis rules on some Java source code files and gene

The formatter element can contain nested param elements to configure the formatter in detail, e.g.

+
encoding
+
Specifies the encoding to be used in the generated report (only honored when used with `toFile`). When rendering `toConsole` PMD will automatically detect the terminal's encoding and use it, unless the output is being redirected / piped, in which case `file.encoding` is used. See example below.
linkPrefix
Used for linking to online HTMLized source (like this). See example below.
linePrefix
@@ -162,7 +164,9 @@ Several folks (most recently, Wouter Zelle) have written XSLT scripts which you can use to transform the XML report into nifty HTML. To do this, make sure you use the XML formatter in the PMD task invocation, i.e.: - + + + Then, after the end of the PMD task, do this: From 9db4b2503d0b70ad99513c8ce7ce689036bdf26f Mon Sep 17 00:00:00 2001 From: Sergey Date: Thu, 12 Jan 2017 14:31:20 -0800 Subject: [PATCH 4/5] Bug fixes part 2 --- .../rule/security/ApexCRUDViolationRule.java | 48 ++++++++++++------- .../rule/security/xml/ApexCRUDViolation.xml | 29 ++++++++++- 2 files changed, 57 insertions(+), 20 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java index 71e255ca67..384ab71eb7 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java @@ -27,6 +27,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTDmlUpsertStatement; import net.sourceforge.pmd.lang.apex.ast.ASTDottedExpression; import net.sourceforge.pmd.lang.apex.ast.ASTField; import net.sourceforge.pmd.lang.apex.ast.ASTFieldDeclaration; +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.ASTNewNameValueObjectExpression; @@ -319,24 +320,10 @@ public class ApexCRUDViolationRule extends AbstractApexRule { final ASTMethod outerMethod = self.getFirstParentOfType(ASTMethod.class); if (outerMethod != null) { final ASTBlockStatement blockStatement = outerMethod.getFirstChildOfType(ASTBlockStatement.class); - if (blockStatement != null) { - int numberOfStatements = blockStatement.jjtGetNumChildren(); - for (int i = 0; i < numberOfStatements; i++) { - Node n = blockStatement.jjtGetChild(i); - AbstractApexNode match = n.getFirstDescendantOfType(self.getClass()); - if (match == self) { - break; - } - ASTMethodCallExpression methodCall = n.getFirstDescendantOfType(ASTMethodCallExpression.class); - if (methodCall != null) { - mapCallToMethodDecl(self, innerMethodCalls, Arrays.asList(methodCall)); - } - } + recursivelyEvaluateCRUDMethodCalls(self, innerMethodCalls, blockStatement); - } - - final List specialMethods = findSpecialMethods(self); - for (ASTMethod method : specialMethods) { + final List constructorMethods = findConstructorlMethods(self); + for (ASTMethod method : constructorMethods) { innerMethodCalls.addAll(method.findDescendantsOfType(ASTMethodCallExpression.class)); } @@ -347,6 +334,31 @@ public class ApexCRUDViolationRule extends AbstractApexRule { return innerMethodCalls; } + private void recursivelyEvaluateCRUDMethodCalls(final AbstractApexNode self, + final HashSet innerMethodCalls, final ASTBlockStatement blockStatement) { + if (blockStatement != null) { + int numberOfStatements = blockStatement.jjtGetNumChildren(); + for (int i = 0; i < numberOfStatements; i++) { + Node n = blockStatement.jjtGetChild(i); + + if (n instanceof ASTIfElseBlockStatement) { + ASTBlockStatement innerBlock = n.getFirstDescendantOfType(ASTBlockStatement.class); + recursivelyEvaluateCRUDMethodCalls(self, innerMethodCalls, innerBlock); + } + + AbstractApexNode match = n.getFirstDescendantOfType(self.getClass()); + if (match == self) { + break; + } + ASTMethodCallExpression methodCall = n.getFirstDescendantOfType(ASTMethodCallExpression.class); + if (methodCall != null) { + mapCallToMethodDecl(self, innerMethodCalls, Arrays.asList(methodCall)); + } + } + + } + } + private void mapCallToMethodDecl(final AbstractApexNode self, final HashSet innerMethodCalls, final List nodes) { for (ASTMethodCallExpression node : nodes) { @@ -362,7 +374,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule { } } - private List findSpecialMethods(final AbstractApexNode node) { + private List findConstructorlMethods(final AbstractApexNode node) { final ArrayList ret = new ArrayList<>(); final Set constructors = classMethods.keySet().stream() .filter(p -> (p.contains("") || p.contains(""))).collect(Collectors.toSet()); 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 b54a555f06..1c942b2ff6 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 @@ -1,7 +1,7 @@ - + Proper CRUD,FLS via upsert 0 @@ -619,5 +619,30 @@ public class Foo { } } ]]> - + + + + Control flow with nested statementsL + 0 + profiles = [SELECT Id FROM Profile WHERE Name = 'System Administrator']; + } + } + } + + private void checkPerms() { + if (!Profile.sObjectType.getDescribe().isCreateable()) { + throw new NoAccessException(); + } + } +} + +]]> + + From 608c9139be0f4cb905683dae507ab399707f9108 Mon Sep 17 00:00:00 2001 From: Sergey Date: Thu, 12 Jan 2017 14:37:43 -0800 Subject: [PATCH 5/5] Bug fix #3 for else statements --- .../pmd/lang/apex/rule/security/ApexCRUDViolationRule.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java index 384ab71eb7..fddb8300c7 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java @@ -342,8 +342,10 @@ public class ApexCRUDViolationRule extends AbstractApexRule { Node n = blockStatement.jjtGetChild(i); if (n instanceof ASTIfElseBlockStatement) { - ASTBlockStatement innerBlock = n.getFirstDescendantOfType(ASTBlockStatement.class); - recursivelyEvaluateCRUDMethodCalls(self, innerMethodCalls, innerBlock); + List innerBlocks = n.findDescendantsOfType(ASTBlockStatement.class); + for (ASTBlockStatement innerBlock : innerBlocks) { + recursivelyEvaluateCRUDMethodCalls(self, innerMethodCalls, innerBlock); + } } AbstractApexNode match = n.getFirstDescendantOfType(self.getClass());