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

rombirli · 2025-12-23T10:38:15Z

No description provided.

hashicorp-vault-sonar-prod · 2025-12-23T10:38:29Z

SONARXML-221

sonarqube-next · 2025-12-23T10:53:12Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

github-actions · 2025-12-31T03:05:59Z

This PR is stale because it has been open 7 days with no activity. If there is no activity in the next 7 days it will be closed automatically

dorian-burihabwa-sonarsource

This solution looks pretty solid. I think there are a couple of things we could do to get to the extra mile:

Cover removeAll like we cover remove
Rename the new test and new utility methods in the check

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

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

dorian-burihabwa-sonarsource

The approach looks good but there are a couple of things we need to fix before we can merge.

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

dorian-burihabwa-sonarsource · 2026-01-05T16:59:30Z

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


+  // 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<Node, Boolean> removeAllPropertyCache = new HashMap<>();


AFAIK, we create one instance of check per (module) analysis. This means that this cache is likely to grow with every new file we explore. In addition to the growth in terms of memory, we are likely to void any potential for garbage collection as the nodes are objects within potentially large graphs.
I think we need to do 1 of 2 things:

We decide to keep track of all nodes across the project and replace the key type of the map with a cache and memory-friendlier type

We decide to keep track of all nodes within the file only and reset the map on every call to scanAndroidManifest / scanFile

As this cache is not static, shouldn't the gc collect it once scanning the module is done ?

However I can still optimize it : I propose the cache to be just an Optional :

None : didnt search removeAll in children yet

Some(false) : No uses-permission node contains removeAll

Some(true) : A uses-permission node contains removeAll

As this cache is not static, shouldn't the gc collect it once scanning the module is done ?

It should, but keeping things in memory when they no longer serve any purpose (ie: we moved to another file and can no longer encounter the same parent) we might as well clear the cache as we enter a new file.

dorian-burihabwa-sonarsource · 2026-01-05T17:31:25Z

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

+    if (!hasToolsNodeRemove(node)
+      && !hasParentToolsNodeRemoveAll(node)


Disclaimer: not sure about everything below but I feel like we could avoid doubling the work here.

Since the introduction of removeAll means that we have to look at all <uses-permission> nodes in the parent, and the parent can only be the manifest as far as I understand, we might as well change the approach here.

Could we consider an other approach where we:

Build a list of <uses-permission> nodes as an initial candidate list

Group the candidates by android:name attribute

For each group, loop through the candidates
a. if a candidate has tools:node="removeAll" as an attribute, remove the group from the candidates, continue to the next group
b. if a candidate has tools:node="remove" as an attribute, remove from the list of candidates and continue to the next candidate

Report on the remaining candidates

If I understand correctly your approach, a tools:node="removeAll" removes all emenent with tag:uses-permission AND same name, but I understand the documentation differently, for what i understood, the name is not checked, and elements with same tag in same parent are removed

Indeed, the logic in the documentation seems to be that all tags of the same type will be dropped from the merged manifest, and we might able to simplify the logic I proposed by skipping the grouping at step 2 and exiting altogether at step 3a if we encounter removeAll.

rombirli · 2026-01-06T12:50:24Z

I finally managed to completely get rid of the cache, and applied the few refactoring recommendations

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

dorian-burihabwa-sonarsource

Thank you for the changes, we are almost there and should be getting there soon.

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

- remove @nullable decorator on node argument - invert return type, change name accordingly - takes uses-permission nodes list instead of manifest node as argument

dorian-burihabwa-sonarsource

Great job on this one! The checks looks a lot more approachable with this version.

dorian-burihabwa-sonarsource · 2026-01-07T14:19:07Z

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

-      .map(nodeList::item)
-      .filter(childNode -> "uses-permission".equals(childNode.getNodeName()))
-      .noneMatch(childNode -> hasToolsNodeValue(childNode, "removeAll"));
+  private boolean hasToolsNodeRemoveAll(List<Node> permissionNodes) {


Let's mark this method as static since it does not depend on the check's state

sonarqube-next · 2026-01-07T14:59:14Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

Draft of SONARXML-221 solution

8ecae8c

rombirli requested a review from a team December 23, 2025 13:28

github-actions bot added the stale label Dec 31, 2025

GabrielFleischer self-requested a review January 5, 2026 12:49

dorian-burihabwa-sonarsource requested changes Jan 5, 2026

View reviewed changes

GabrielFleischer removed their request for review January 5, 2026 12:50

tomasz-tylenda-sonarsource reviewed Jan 5, 2026

View reviewed changes

rombirli added 4 commits January 5, 2026 14:22

Refactoring : apply most change requests

99bb0fb

Add tools:node="removeAll" support - no caching

069f0e0

Caching for tools:node="removeAll"

1ee0cc5

Refactoring : rename tests

191e4bd

dorian-burihabwa-sonarsource requested changes Jan 5, 2026

View reviewed changes

github-actions bot removed the stale label Jan 6, 2026

Refactoring, rename tests, remove cache, remove repetitions

887b423

rombirli requested review from dorian-burihabwa-sonarsource and tomasz-tylenda-sonarsource January 6, 2026 13:06

tomasz-tylenda-sonarsource reviewed Jan 6, 2026

View reviewed changes

...gin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java Outdated Show resolved Hide resolved

...gin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java Outdated Show resolved Hide resolved

dorian-burihabwa-sonarsource requested changes Jan 7, 2026

View reviewed changes

...gin/src/main/java/org/sonar/plugins/xml/checks/security/android/AndroidPermissionsCheck.java Outdated Show resolved Hide resolved

Refactoring : noToolsNodeRemoveAll signature change

a842ee9

- remove @nullable decorator on node argument - invert return type, change name accordingly - takes uses-permission nodes list instead of manifest node as argument

rombirli requested review from dorian-burihabwa-sonarsource and tomasz-tylenda-sonarsource January 7, 2026 13:56

dorian-burihabwa-sonarsource approved these changes Jan 7, 2026

View reviewed changes

Make function hasToolsNodeRemoveAll static

df47080

rombirli merged commit e29d923 into master Jan 7, 2026
8 checks passed

rombirli deleted the rombirli/SONARXML-221-dont-raise-on-node-remove branch January 7, 2026 15:45

		if (!hasToolsNodeRemove(node)
		&& !hasParentToolsNodeRemoveAll(node)

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

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

Uh oh!

Conversation

rombirli commented Dec 23, 2025

Uh oh!

hashicorp-vault-sonar-prod bot commented Dec 23, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqube-next bot commented Dec 23, 2025

Quality Gate passed

Uh oh!

github-actions bot commented Dec 31, 2025

Uh oh!

dorian-burihabwa-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dorian-burihabwa-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dorian-burihabwa-sonarsource Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

rombirli Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

dorian-burihabwa-sonarsource Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

dorian-burihabwa-sonarsource Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

rombirli Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

dorian-burihabwa-sonarsource Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

rombirli commented Jan 6, 2026

Uh oh!

Uh oh!

Uh oh!

dorian-burihabwa-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dorian-burihabwa-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

dorian-burihabwa-sonarsource Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqube-next bot commented Jan 7, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hashicorp-vault-sonar-prod bot commented Dec 23, 2025 •

edited by atlassian bot

Loading