SONARXML-221 : S5604 Should not raise on items containing tools:node="remove"

sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java

@@ -19,21 +19,29 @@
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Set;
+import java.util.stream.IntStream;
+import javax.annotation.Nullable;
 import javax.xml.xpath.XPathExpression;
 import org.sonar.check.Rule;
 import org.sonarsource.analyzer.commons.xml.XPathBuilder;
 import org.sonarsource.analyzer.commons.xml.XmlFile;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
 
+import static org.sonar.plugins.xml.checks.security.android.Utils.ANDROID_MANIFEST_TOOLS;
 import static org.sonar.plugins.xml.checks.security.android.Utils.ANDROID_MANIFEST_XMLNS;
 
 @Rule(key = "S5604")
 public class AndroidPermissionsCheck extends AbstractAndroidManifestCheck {
 
   private static final String MESSAGE = "Make sure the use of \"%s\" permission is necessary.";
-  private final XPathExpression xPathExpression = XPathBuilder.forExpression("/manifest/uses-permission/@n1:name")
-    .withNamespace("n1", ANDROID_MANIFEST_XMLNS)
+  private final XPathExpression xPathExpression = XPathBuilder
+    .forExpression("/manifest/uses-permission")
     .build();
 
+  // finding if any child contains tools:node="removeAll" can be costly if there are many uses-permission nodes
+  // so we cache the result for each parent node
+
   private static final Set<String> DANGEROUS_PERMISSIONS = new HashSet<>(Arrays.asList(
     // the below list is dangerous
     // see https://developer.android.com/reference/android/Manifest.permission
@@ -110,13 +118,44 @@ public class AndroidPermissionsCheck extends AbstractAndroidManifestCheck {
 
   @Override
   protected final void scanAndroidManifest(XmlFile file) {
-    evaluateAsList(xPathExpression, file.getDocument()).stream()
-      .filter(node -> DANGEROUS_PERMISSIONS.contains(node.getNodeValue()))
-      .forEach(node -> reportIssue(node, String.format(MESSAGE, simpleName(node.getNodeValue()))));
+    if (noToolsNodeRemoveAll(file.getDocument().getFirstChild())) {
+      evaluateAsList(xPathExpression, file.getDocument())
+        .forEach(this::checkAndReportPermissionIssue);
+    }
+  }
+
+  private void checkAndReportPermissionIssue(Node node) {
+    Node permissionsAttribute = findPermissionAttribute(node);
+    String permissionValue = permissionsAttribute.getNodeValue();
+    if (!hasToolsNodeValue(node, "remove")
+      && DANGEROUS_PERMISSIONS.contains(permissionValue)) {
+      reportIssue(permissionsAttribute, String.format(MESSAGE, simpleName(permissionValue)));
+    }
   }
 
   private static String simpleName(String fullyQualifiedName) {
     return fullyQualifiedName.substring(fullyQualifiedName.lastIndexOf('.') + 1);
   }
 
+  private static Node findPermissionAttribute(Node node) {
+    return node.getAttributes().getNamedItemNS(ANDROID_MANIFEST_XMLNS, "name");
+  }
+
+  private static boolean hasToolsNodeValue(Node node, String value) {
+    Node toolsNodeAttribute = node.getAttributes().getNamedItemNS(ANDROID_MANIFEST_TOOLS, "node");
+    return toolsNodeAttribute != null && value.equals(toolsNodeAttribute.getNodeValue());
+  }
+
+  private boolean noToolsNodeRemoveAll(@Nullable Node node) {
+    if (node == null) {
+      return true;
+    }
+
+    NodeList nodeList = node.getChildNodes();
+    return IntStream.range(0, nodeList.getLength())
+      .boxed()
+      .map(nodeList::item)
+      .filter(childNode -> "uses-permission".equals(childNode.getNodeName()))
+      .noneMatch(childNode -> hasToolsNodeValue(childNode, "removeAll"));
+  }
 }

sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/Utils.java

@@ -24,6 +24,7 @@ private Utils() {
   }
 
   public static final String ANDROID_MANIFEST_XMLNS = "http://schemas.android.com/apk/res/android";
+  public static final String ANDROID_MANIFEST_TOOLS = "http://schemas.android.com/tools";
   public static final String ANDROID_MANIFEST_FILENAME = "AndroidManifest.xml";
 
   public static boolean isAndroidManifestFile(XmlFile file) {

sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java

@@ -32,4 +32,18 @@ void not_manifest() {
     SonarXmlCheckVerifier.verifyNoIssue(Paths.get("AnyFileName.xml").toString(), new AndroidPermissionsCheck());
   }
 
+  @Test
+  void should_not_raise_on_tools_node_remove() {
+    SonarXmlCheckVerifier.verifyIssues(Paths.get("ToolsNodeRemove/AndroidManifest.xml").toString(), new AndroidPermissionsCheck());
+  }
+
+  @Test
+  void should_not_raise_on_tools_node_remove_all() {
+    SonarXmlCheckVerifier.verifyNoIssue(Paths.get("ToolsNodeRemoveAll/AndroidManifest.xml").toString(), new AndroidPermissionsCheck());
+  }
+
+  @Test
+  void should_raise_on_tools_node_remove_all_with_wrong_tag() {
+    SonarXmlCheckVerifier.verifyIssues(Paths.get("ToolsNodeRemoveAllWrongTag/AndroidManifest.xml").toString(), new AndroidPermissionsCheck());
+  }
 }

sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemove/AndroidManifest.xml

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="utf-8"?>
+<manifest xmlns:android="http://schemas.android.com/apk/res/android" xmlns:tools="http://schemas.android.com/tools" package="com.example.myapp">
+
+  <!-- Compliant -->
+  <uses-permission android:name="android.permission.ACCEPT_HANDOVER" tools:node="remove"/>
+
+  <!-- Noncompliant@+1 {{Make sure the use of "ACCESS_BACKGROUND_LOCATION" permission is necessary.}} -->
+  <uses-permission android:name="android.permission.ACCESS_BACKGROUND_LOCATION" />
+  <!--             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -->
+
+</manifest>

sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAll/AndroidManifest.xml

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="utf-8"?>
+<manifest xmlns:android="http://schemas.android.com/apk/res/android" xmlns:tools="http://schemas.android.com/tools"
+          package="com.example.myapp">
+
+  <!-- Compliant -->
+  <uses-permission android:name="android.permission.ACCEPT_HANDOVER" tools:node="removeAll"/>
+  <!-- Compliant -->
+  <uses-permission android:name="android.permission.ACCESS_BACKGROUND_LOCATION"/>
+
+</manifest>
+

sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllWrongTag/AndroidManifest.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="utf-8"?>
+<manifest xmlns:android="http://schemas.android.com/apk/res/android" xmlns:tools="http://schemas.android.com/tools"
+          package="com.example.myapp">
+
+  <!-- Compliant -->
+  <something-random android:name="android.permission.ACCEPT_HANDOVER" tools:node="removeAll"/>
+
+  <!-- Noncompliant@+1 {{Make sure the use of "ACCESS_BACKGROUND_LOCATION" permission is necessary.}} -->
+  <uses-permission android:name="android.permission.ACCESS_BACKGROUND_LOCATION"/>
+  <!--             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -->
+
+</manifest>
+