From 8ecae8c2b34d8d8447c002523a14c3de6acd7175 Mon Sep 17 00:00:00 2001 From: "romain.birling" Date: Tue, 23 Dec 2025 11:29:47 +0100 Subject: [PATCH 1/8] Draft of SONARXML-221 solution --- .../android/AndroidPermissionsCheck.java | 23 ++++++++++++++++--- .../xml/checks/security/android/Utils.java | 1 + .../android/AndroidPermissionsCheckTest.java | 5 ++++ .../ToolsNodeRemove/AndroidManifest.xml | 11 +++++++++ 4 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemove/AndroidManifest.xml diff --git a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java index 007e489b8..6611c7291 100644 --- a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java +++ b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java @@ -18,19 +18,22 @@ import java.util.Arrays; import java.util.HashSet; +import java.util.Optional; import java.util.Set; 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 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") + private final XPathExpression xPathExpression = XPathBuilder.forExpression("/manifest/uses-permission") .withNamespace("n1", ANDROID_MANIFEST_XMLNS) .build(); @@ -111,12 +114,26 @@ 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())))); + .filter(node -> !hasToolsNodeRemove(node)) + .filter(node -> DANGEROUS_PERMISSIONS.contains(findPermissionValue(node))) + .forEach(node -> reportIssue(findPermissionNode(node), String.format(MESSAGE, simpleName(findPermissionValue(node))))); } private static String simpleName(String fullyQualifiedName) { return fullyQualifiedName.substring(fullyQualifiedName.lastIndexOf('.') + 1); } + private static Node findPermissionNode(Node node) { + return node.getAttributes().getNamedItemNS(ANDROID_MANIFEST_XMLNS, "name"); + } + + private static String findPermissionValue(Node node) { + return findPermissionNode(node).getNodeValue(); + } + + private static boolean hasToolsNodeRemove(Node node) { + Node toolsNodeAttribute = node.getAttributes().getNamedItemNS(ANDROID_MANIFEST_TOOLS, "node"); + return Optional.ofNullable(toolsNodeAttribute).map(n -> "remove".equals(n.getNodeValue())).orElse(false); + } + } diff --git a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/Utils.java b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/Utils.java index 1c97ad0a5..a8d7bfe6d 100644 --- a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/Utils.java +++ b/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) { diff --git a/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java b/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java index 5e87677ba..181d89a84 100644 --- a/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java +++ b/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java @@ -32,4 +32,9 @@ void not_manifest() { SonarXmlCheckVerifier.verifyNoIssue(Paths.get("AnyFileName.xml").toString(), new AndroidPermissionsCheck()); } + @Test + void tools_node_remove() { + SonarXmlCheckVerifier.verifyIssues(Paths.get("ToolsNodeRemove/AndroidManifest.xml").toString(), new AndroidPermissionsCheck()); + } + } diff --git a/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemove/AndroidManifest.xml b/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemove/AndroidManifest.xml new file mode 100644 index 000000000..ca5b083e9 --- /dev/null +++ b/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemove/AndroidManifest.xml @@ -0,0 +1,11 @@ + + + + + + + + + + + From 99bb0fb49ee1d5382706e907c6c21789121ebe93 Mon Sep 17 00:00:00 2001 From: "romain.birling" Date: Mon, 5 Jan 2026 14:22:20 +0100 Subject: [PATCH 2/8] Refactoring : apply most change requests --- .../android/AndroidPermissionsCheck.java | 26 +++++++++---------- .../android/AndroidPermissionsCheckTest.java | 2 +- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java index 6611c7291..6737e04ca 100644 --- a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java +++ b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java @@ -18,7 +18,6 @@ import java.util.Arrays; import java.util.HashSet; -import java.util.Optional; import java.util.Set; import javax.xml.xpath.XPathExpression; import org.sonar.check.Rule; @@ -33,8 +32,8 @@ 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") - .withNamespace("n1", ANDROID_MANIFEST_XMLNS) + private final XPathExpression xPathExpression = XPathBuilder + .forExpression("/manifest/uses-permission") .build(); private static final Set DANGEROUS_PERMISSIONS = new HashSet<>(Arrays.asList( @@ -114,26 +113,27 @@ public class AndroidPermissionsCheck extends AbstractAndroidManifestCheck { @Override protected final void scanAndroidManifest(XmlFile file) { evaluateAsList(xPathExpression, file.getDocument()).stream() - .filter(node -> !hasToolsNodeRemove(node)) - .filter(node -> DANGEROUS_PERMISSIONS.contains(findPermissionValue(node))) - .forEach(node -> reportIssue(findPermissionNode(node), String.format(MESSAGE, simpleName(findPermissionValue(node))))); + .forEach(this::checkAndReportPermissionIssue); + } + + private void checkAndReportPermissionIssue(Node node) { + Node permissionsAttribute = findPermissionAttribute(node); + String permissionValue = permissionsAttribute.getNodeValue(); + if (!hasToolsNodeRemove(node) && 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 findPermissionNode(Node node) { + private static Node findPermissionAttribute(Node node) { return node.getAttributes().getNamedItemNS(ANDROID_MANIFEST_XMLNS, "name"); } - private static String findPermissionValue(Node node) { - return findPermissionNode(node).getNodeValue(); - } - private static boolean hasToolsNodeRemove(Node node) { Node toolsNodeAttribute = node.getAttributes().getNamedItemNS(ANDROID_MANIFEST_TOOLS, "node"); - return Optional.ofNullable(toolsNodeAttribute).map(n -> "remove".equals(n.getNodeValue())).orElse(false); + return toolsNodeAttribute != null && "remove".equals(toolsNodeAttribute.getNodeValue()); } - } diff --git a/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java b/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java index 181d89a84..c11b82631 100644 --- a/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java +++ b/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java @@ -33,7 +33,7 @@ void not_manifest() { } @Test - void tools_node_remove() { + void should_not_raise_on_tools_node_remove() { SonarXmlCheckVerifier.verifyIssues(Paths.get("ToolsNodeRemove/AndroidManifest.xml").toString(), new AndroidPermissionsCheck()); } From 069f0e069d350b94e6c4ab74f29ebefa92170b85 Mon Sep 17 00:00:00 2001 From: "romain.birling" Date: Mon, 5 Jan 2026 15:33:12 +0100 Subject: [PATCH 3/8] Add tools:node="removeAll" support - no caching --- .../android/AndroidPermissionsCheck.java | 29 +++++++++++++++++-- .../android/AndroidPermissionsCheckTest.java | 9 ++++++ .../AndroidManifest.xml | 11 +++++++ .../AndroidManifest.xml | 13 +++++++++ 4 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllCompliant/AndroidManifest.xml create mode 100644 sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllNonCompliant/AndroidManifest.xml diff --git a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java index 6737e04ca..2adf6ed5d 100644 --- a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java +++ b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java @@ -24,6 +24,7 @@ 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; @@ -112,14 +113,16 @@ public class AndroidPermissionsCheck extends AbstractAndroidManifestCheck { @Override protected final void scanAndroidManifest(XmlFile file) { - evaluateAsList(xPathExpression, file.getDocument()).stream() + evaluateAsList(xPathExpression, file.getDocument()) .forEach(this::checkAndReportPermissionIssue); } private void checkAndReportPermissionIssue(Node node) { Node permissionsAttribute = findPermissionAttribute(node); String permissionValue = permissionsAttribute.getNodeValue(); - if (!hasToolsNodeRemove(node) && DANGEROUS_PERMISSIONS.contains(permissionValue)) { + if (!hasToolsNodeRemove(node) + && !hasParentToolsNodeRemoveAll(node) + && DANGEROUS_PERMISSIONS.contains(permissionValue)) { reportIssue(permissionsAttribute, String.format(MESSAGE, simpleName(permissionValue))); } } @@ -136,4 +139,26 @@ private static boolean hasToolsNodeRemove(Node node) { Node toolsNodeAttribute = node.getAttributes().getNamedItemNS(ANDROID_MANIFEST_TOOLS, "node"); return toolsNodeAttribute != null && "remove".equals(toolsNodeAttribute.getNodeValue()); } + + private static boolean hasParentToolsNodeRemoveAll(Node node) { + Node parent = node.getParentNode(); + if (parent == null) { + return false; + } + // todo : use caching here to avoid rechecking same parent nodes for each child (n^2 complexity reduced to n) + // check for all children if any is of type "use-permission" and contains tools:node="removeAll" + NodeList nodeList = parent.getChildNodes(); + int nodeListLength = nodeList.getLength(); + for (int i = 0; i < nodeListLength; i++) { + Node childNode = nodeList.item(i); + if (!"uses-permission".equals(childNode.getNodeName())) { + continue; + } + Node toolsNodeAttribute = childNode.getAttributes().getNamedItemNS(ANDROID_MANIFEST_TOOLS, "node"); + if (toolsNodeAttribute != null && "removeAll".equals(toolsNodeAttribute.getNodeValue())) { + return true; + } + } + return false; + } } diff --git a/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java b/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java index c11b82631..35e8e7e59 100644 --- a/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java +++ b/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java @@ -37,4 +37,13 @@ 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_compliant() { + SonarXmlCheckVerifier.verifyNoIssue(Paths.get("ToolsNodeRemoveAllCompliant/AndroidManifest.xml").toString(), new AndroidPermissionsCheck()); + } + + @Test + void should_not_raise_on_tools_node_remove_all_non_compliant() { + SonarXmlCheckVerifier.verifyIssues(Paths.get("ToolsNodeRemoveAllNonCompliant/AndroidManifest.xml").toString(), new AndroidPermissionsCheck()); + } } diff --git a/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllCompliant/AndroidManifest.xml b/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllCompliant/AndroidManifest.xml new file mode 100644 index 000000000..54e377fd1 --- /dev/null +++ b/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllCompliant/AndroidManifest.xml @@ -0,0 +1,11 @@ + + + + + + + + + + diff --git a/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllNonCompliant/AndroidManifest.xml b/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllNonCompliant/AndroidManifest.xml new file mode 100644 index 000000000..f3e14cfbf --- /dev/null +++ b/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllNonCompliant/AndroidManifest.xml @@ -0,0 +1,13 @@ + + + + + + + + + + + + From 1ee0cc59131826bf854eb69d791e616e8fa5ca39 Mon Sep 17 00:00:00 2001 From: "romain.birling" Date: Mon, 5 Jan 2026 15:42:44 +0100 Subject: [PATCH 4/8] Caching for tools:node="removeAll" --- .../security/android/AndroidPermissionsCheck.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java index 2adf6ed5d..adcea752e 100644 --- a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java +++ b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java @@ -17,7 +17,9 @@ package org.sonar.plugins.xml.checks.security.android; import java.util.Arrays; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Set; import javax.xml.xpath.XPathExpression; import org.sonar.check.Rule; @@ -37,6 +39,10 @@ public class AndroidPermissionsCheck extends AbstractAndroidManifestCheck { .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 final Map removeAllPropertyCache = new HashMap<>(); + private static final Set DANGEROUS_PERMISSIONS = new HashSet<>(Arrays.asList( // the below list is dangerous // see https://developer.android.com/reference/android/Manifest.permission @@ -140,13 +146,14 @@ private static boolean hasToolsNodeRemove(Node node) { return toolsNodeAttribute != null && "remove".equals(toolsNodeAttribute.getNodeValue()); } - private static boolean hasParentToolsNodeRemoveAll(Node node) { + private boolean hasParentToolsNodeRemoveAll(Node node) { Node parent = node.getParentNode(); if (parent == null) { return false; } - // todo : use caching here to avoid rechecking same parent nodes for each child (n^2 complexity reduced to n) - // check for all children if any is of type "use-permission" and contains tools:node="removeAll" + if (removeAllPropertyCache.containsKey(parent)) { + return removeAllPropertyCache.get(parent); + } NodeList nodeList = parent.getChildNodes(); int nodeListLength = nodeList.getLength(); for (int i = 0; i < nodeListLength; i++) { @@ -156,9 +163,11 @@ private static boolean hasParentToolsNodeRemoveAll(Node node) { } Node toolsNodeAttribute = childNode.getAttributes().getNamedItemNS(ANDROID_MANIFEST_TOOLS, "node"); if (toolsNodeAttribute != null && "removeAll".equals(toolsNodeAttribute.getNodeValue())) { + removeAllPropertyCache.put(parent, true); return true; } } + removeAllPropertyCache.put(parent, false); return false; } } From 191e4bdccb6793bb23a29d635e83542264542912 Mon Sep 17 00:00:00 2001 From: "romain.birling" Date: Mon, 5 Jan 2026 15:48:22 +0100 Subject: [PATCH 5/8] Refactoring : rename tests --- .../checks/security/android/AndroidPermissionsCheckTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java b/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java index 35e8e7e59..13d206551 100644 --- a/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java +++ b/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java @@ -38,12 +38,12 @@ void should_not_raise_on_tools_node_remove() { } @Test - void should_not_raise_on_tools_node_remove_all_compliant() { + void should_not_raise_on_tools_node_remove_all() { SonarXmlCheckVerifier.verifyNoIssue(Paths.get("ToolsNodeRemoveAllCompliant/AndroidManifest.xml").toString(), new AndroidPermissionsCheck()); } @Test - void should_not_raise_on_tools_node_remove_all_non_compliant() { + void should_raise_on_tools_node_remove_all_with_wrong_tag() { SonarXmlCheckVerifier.verifyIssues(Paths.get("ToolsNodeRemoveAllNonCompliant/AndroidManifest.xml").toString(), new AndroidPermissionsCheck()); } } From 887b423aee18c9653f954abb3cb23656ebbf29a9 Mon Sep 17 00:00:00 2001 From: "romain.birling" Date: Tue, 6 Jan 2026 13:07:47 +0100 Subject: [PATCH 6/8] Refactoring, rename tests, remove cache, remove repetitions --- .../android/AndroidPermissionsCheck.java | 50 +++++++------------ .../android/AndroidPermissionsCheckTest.java | 4 +- .../AndroidManifest.xml | 0 .../AndroidManifest.xml | 0 4 files changed, 21 insertions(+), 33 deletions(-) rename sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/{ToolsNodeRemoveAllCompliant => ToolsNodeRemoveAll}/AndroidManifest.xml (100%) rename sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/{ToolsNodeRemoveAllNonCompliant => ToolsNodeRemoveAllWrongTag}/AndroidManifest.xml (100%) diff --git a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java index adcea752e..ab2cfa4e4 100644 --- a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java +++ b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java @@ -17,10 +17,10 @@ package org.sonar.plugins.xml.checks.security.android; import java.util.Arrays; -import java.util.HashMap; import java.util.HashSet; -import java.util.Map; 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; @@ -41,7 +41,6 @@ public class AndroidPermissionsCheck extends AbstractAndroidManifestCheck { // 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 final Map removeAllPropertyCache = new HashMap<>(); private static final Set DANGEROUS_PERMISSIONS = new HashSet<>(Arrays.asList( // the below list is dangerous @@ -119,15 +118,16 @@ public class AndroidPermissionsCheck extends AbstractAndroidManifestCheck { @Override protected final void scanAndroidManifest(XmlFile file) { - evaluateAsList(xPathExpression, file.getDocument()) - .forEach(this::checkAndReportPermissionIssue); + 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 (!hasToolsNodeRemove(node) - && !hasParentToolsNodeRemoveAll(node) + if (!hasToolsNodeValue(node, "remove") && DANGEROUS_PERMISSIONS.contains(permissionValue)) { reportIssue(permissionsAttribute, String.format(MESSAGE, simpleName(permissionValue))); } @@ -141,33 +141,21 @@ private static Node findPermissionAttribute(Node node) { return node.getAttributes().getNamedItemNS(ANDROID_MANIFEST_XMLNS, "name"); } - private static boolean hasToolsNodeRemove(Node node) { + private static boolean hasToolsNodeValue(Node node, String value) { Node toolsNodeAttribute = node.getAttributes().getNamedItemNS(ANDROID_MANIFEST_TOOLS, "node"); - return toolsNodeAttribute != null && "remove".equals(toolsNodeAttribute.getNodeValue()); + return toolsNodeAttribute != null && value.equals(toolsNodeAttribute.getNodeValue()); } - private boolean hasParentToolsNodeRemoveAll(Node node) { - Node parent = node.getParentNode(); - if (parent == null) { - return false; - } - if (removeAllPropertyCache.containsKey(parent)) { - return removeAllPropertyCache.get(parent); + private boolean noToolsNodeRemoveAll(@Nullable Node node) { + if (node == null) { + return true; } - NodeList nodeList = parent.getChildNodes(); - int nodeListLength = nodeList.getLength(); - for (int i = 0; i < nodeListLength; i++) { - Node childNode = nodeList.item(i); - if (!"uses-permission".equals(childNode.getNodeName())) { - continue; - } - Node toolsNodeAttribute = childNode.getAttributes().getNamedItemNS(ANDROID_MANIFEST_TOOLS, "node"); - if (toolsNodeAttribute != null && "removeAll".equals(toolsNodeAttribute.getNodeValue())) { - removeAllPropertyCache.put(parent, true); - return true; - } - } - removeAllPropertyCache.put(parent, false); - return false; + + 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")); } } diff --git a/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java b/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java index 13d206551..79f29e32c 100644 --- a/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java +++ b/sonar-xml-plugin/src/test/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheckTest.java @@ -39,11 +39,11 @@ void should_not_raise_on_tools_node_remove() { @Test void should_not_raise_on_tools_node_remove_all() { - SonarXmlCheckVerifier.verifyNoIssue(Paths.get("ToolsNodeRemoveAllCompliant/AndroidManifest.xml").toString(), new AndroidPermissionsCheck()); + 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("ToolsNodeRemoveAllNonCompliant/AndroidManifest.xml").toString(), new AndroidPermissionsCheck()); + SonarXmlCheckVerifier.verifyIssues(Paths.get("ToolsNodeRemoveAllWrongTag/AndroidManifest.xml").toString(), new AndroidPermissionsCheck()); } } diff --git a/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllCompliant/AndroidManifest.xml b/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAll/AndroidManifest.xml similarity index 100% rename from sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllCompliant/AndroidManifest.xml rename to sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAll/AndroidManifest.xml diff --git a/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllNonCompliant/AndroidManifest.xml b/sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllWrongTag/AndroidManifest.xml similarity index 100% rename from sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllNonCompliant/AndroidManifest.xml rename to sonar-xml-plugin/src/test/resources/checks/AndroidPermissionsCheck/ToolsNodeRemoveAllWrongTag/AndroidManifest.xml From a842ee936d57430d4f96ac8e237091b315e917f0 Mon Sep 17 00:00:00 2001 From: "romain.birling" Date: Wed, 7 Jan 2026 14:33:12 +0100 Subject: [PATCH 7/8] Refactoring : noToolsNodeRemoveAll signature change - remove @nullable decorator on node argument - invert return type, change name accordingly - takes uses-permission nodes list instead of manifest node as argument --- .../android/AndroidPermissionsCheck.java | 28 ++++++------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java index ab2cfa4e4..fb1b61892 100644 --- a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java +++ b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java @@ -18,15 +18,13 @@ import java.util.Arrays; import java.util.HashSet; +import java.util.List; 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; @@ -39,9 +37,6 @@ public class AndroidPermissionsCheck extends AbstractAndroidManifestCheck { .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 DANGEROUS_PERMISSIONS = new HashSet<>(Arrays.asList( // the below list is dangerous // see https://developer.android.com/reference/android/Manifest.permission @@ -118,9 +113,9 @@ public class AndroidPermissionsCheck extends AbstractAndroidManifestCheck { @Override protected final void scanAndroidManifest(XmlFile file) { - if (noToolsNodeRemoveAll(file.getDocument().getFirstChild())) { - evaluateAsList(xPathExpression, file.getDocument()) - .forEach(this::checkAndReportPermissionIssue); + List usesPermissionNodes = evaluateAsList(xPathExpression, file.getDocument()); + if (!hasToolsNodeRemoveAll(usesPermissionNodes)) { + usesPermissionNodes.forEach(this::checkAndReportPermissionIssue); } } @@ -146,16 +141,9 @@ private static boolean hasToolsNodeValue(Node node, String value) { 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")); + private boolean hasToolsNodeRemoveAll(List permissionNodes) { + return permissionNodes + .stream() + .anyMatch(node -> hasToolsNodeValue(node, "removeAll")); } } From df47080de06b1e203e130b47f9a18ab1fdfbd45b Mon Sep 17 00:00:00 2001 From: "romain.birling" Date: Wed, 7 Jan 2026 15:50:10 +0100 Subject: [PATCH 8/8] Make function hasToolsNodeRemoveAll static --- .../xml/checks/security/android/AndroidPermissionsCheck.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java index fb1b61892..16772121a 100644 --- a/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java +++ b/sonar-xml-plugin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java @@ -141,7 +141,7 @@ private static boolean hasToolsNodeValue(Node node, String value) { return toolsNodeAttribute != null && value.equals(toolsNodeAttribute.getNodeValue()); } - private boolean hasToolsNodeRemoveAll(List permissionNodes) { + private static boolean hasToolsNodeRemoveAll(List permissionNodes) { return permissionNodes .stream() .anyMatch(node -> hasToolsNodeValue(node, "removeAll"));