SONARJAVA-7154: Implemented rule S9428 MessageDigest instances should not be stored as fields

[romainbrenguier]

Detects MessageDigest instances retained in type fields, which can share mutable digest state across invocations or threads.

Private fields are not reported when each usage is an assignment, a call that does not depend on the digest state (clone(), getAlgorithm(), getDigestLength(), getProvider(), toString()), or sits in a synchronized block or method. This covers the prototype-for-cloning pattern (guava MessageDigestHashFunction.prototype).

Ruling: 2 expected issues (guava MessageDigestHashFunction.java:121, eclipse-jetty Credential.java:190, which is guarded by a custom AutoLock rather than synchronized).

Agent workflow

PR created using uv run new_rule_implementation.py S9428 -j SONARJAVA-7154 -a codex -g AGENTS.md .claude/skills/new-rule/SKILL.md

Tool link: https://github.com/SonarSource/languages-experimental-tooling/tree/romain/my-tickets/personal/romain-brenguier

PR created using uv run create_with_claude.py /tmp/action_plan_romain/new-rule-s9428-sonarjava-7154.txt

---

Summary by Gitar

• Safe usage exemption:
  • Private fields are skipped when every usage is an assignment, a stateless call such as clone(), or inside a synchronized context
• Tests:
  • Added MessageDigestInstanceFieldCheckSample and verifier tests, including a run without semantics
• Rule metadata:
  • Added S9428 description, metadata, Sonar way profile entry, and ruling expectations for eclipse-jetty and guava

_{This will update automatically on new commits.}

🤖 Generated with Claude Code

PR created using uv run create_with_claude.py /tmp/action_plan_romain/new-rule-s9428-sonarjava-7154.txt

PR created using uv run create_with_claude.py /tmp/action_plan_romain/new-rule-s9428-sonarjava-7154.txt

PR created using uv run create_with_claude.py /tmp/action_plan_romain/new-rule-s9428-sonarjava-7154.txt

Iterated on the PR with uv run ci_loop.py for 4 iterations.
✔️ The PR is now ready for review.

Addressed review comments in pr_report_6321.md using uv run address_reviews.py pr_report_6321.md

PR updated using uv run update_with_claude.py --prompt "Implement the action plan described in the document All the non-compliant examples in java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java should have caret markers to show where the issue is raised. Some are missing currently.." -a "All the non-compliant examples in java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java should have caret markers to show where the issue is raised. Some are missing currently." -g "claude"

[hashicorp-vault-sonar-prod]

SONARJAVA-7154

[github-actions]

⚖️ Ruling update ready for review: #6326

[github-actions]

Ruling Diff Summary

Detected changes in 2 rule files: 0 issues removed, 2 issues added.

S9428 (java) on eclipse-jetty - 0 issues removed, 1 issues added - new ruling file

Added jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java (line 190)

(source file not found at this revision: jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java)

S9428 (java) on guava - 0 issues removed, 1 issues added - new ruling file

Added src/com/google/common/hash/MessageDigestHashFunction.java (line 121)

       116 | 
       117 |   /**
       118 |    * Hasher that updates a message digest.
       119 |    */
       120 |   private static final class MessageDigestHasher extends AbstractByteHasher {
>>>    121 |     private final MessageDigest digest;
       122 |     private final int bytes;
       123 |     private boolean done;
       124 | 
       125 |     private MessageDigestHasher(MessageDigest digest, int bytes) {
       126 |       this.digest = digest;

[rombirli]

/pvf

[hashicorp-vault-sonar-prod]

📊 Performance Validation complete
🆕 25 new · 🗑️ 0 lost issues
View dashboard

[rombirli]

This rule looks good!
REQUEST CHANGE: issue location in unit tests
NIT:

• Use matchers instead of raw strings
• I would like to understand further the heuristic choices before merging, some of them are too heavy to me and might not be necessary, we could greatly simplify rule implementation and reproducer.

[rombirli]

We should add location markers ^^^^^^^^ in the tests. Currently the issue reports only on the field name:

  private final MessageDigest configuredDigest; // Noncompliant {{Make this MessageDigest a local variable.}}
  //                          ^^^^^^^^^^^^^^^^

it would make more sense to me to report on the whole field declaration.

  private final MessageDigest configuredDigest; // Noncompliant {{Make this MessageDigest a local variable.}}
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[rombirli]

I'm not entirely sure the heuristic every usage is synchronized is great.
For computing a few digest occasionally, instantiating a new instance every time is more efficient. For higher loads it's recommended to use a ThreadLocal<MessageDigest>. Tracking usages also makes the rule more complexe.

[rombirli]

This hasSemantic could be removed:

• It is not needed at all, if semantic is not available, the MESSAGE_DIGEST will never match -> it is useless logic/code.
• It makes the rule stateful even if it only stores a boolean and there is no risk of memory leak. In general it's much better to have only constant fields in rules.

[rombirli]

could we use a constant matcher :

    private static final MethodMatchers.create()
      .ofSubTypes(MESSAGE_DIGEST);

instead of

   type.isSubtypeOf(MESSAGE_DIGEST)

The part !type.isUnknown() is useless as well as an unknown type will not match the MESSAGE_DIGEST FQN

[romainbrenguier]

I removed !type.isUnknown(), since an unknown type never matches. I kept type.isSubtypeOf(MESSAGE_DIGEST) because MethodMatchers matches method invocations and references, not the type of a variable. ofSubTypes only sets the owner type criterion of a method matcher, so there is no method call to match here.

[rombirli]

This looks to cover only aliasing the MessageDigest objects. What about if a MessageDigest escapes a function either by giving it to another function or returning it?

public void f(){
   g(myMessageDigest); // escapes the function
   return myMessageDigest; // escapes the function
}

[romainbrenguier]

Escaping is already reported. Only usages that are assignment targets or receivers of state-independent calls (clone(), getAlgorithm(), ...) are exempt, so passing the field to g(md), returning it, or aliasing it all lead to an issue. I added EscapingPrototypeHolder to the sample to cover these cases.

[rombirli]

Could we also try running ruling/pvf without the && !isOnlyUsedSafely(...) part. I'm curious how much these heuristics filter out issues. If they have no impact I would be for keeping this rule simple and removing everything after line 79

[romainbrenguier]

I checked the ruling sources. There are only 3 MessageDigest fields. The synchronized heuristic filtered none of them (Jetty Credential.MD5.__md uses an AutoLock and was already reported). So I removed the whole monitor tracking. The prototype heuristic filters 1 issue: Guava MessageDigestHashFunction.prototype, which is only used with clone()/getAlgorithm()/getDigestLength(). That is a real false positive and a common pattern, so I kept this small check.

[github-actions]

⚖️ Ruling update ready for review: #6342

[gitar-bot]

💡 Quality: PR description still promises a synchronized exemption the rule dropped

This commit removes all monitor handling from isOnlyUsedAsPrototype. Private fields used only inside synchronized blocks or methods are now reported, and the sample's SynchronizedHolder.lockedDigest/methodDigest are now marked Noncompliant. The PR description still says private fields are not reported when each usage "sits in a synchronized block or method". It also explains the jetty ruling hit as "guarded by a custom AutoLock rather than synchronized", which no longer matters, since any synchronized-only field is now reported too. The ruling expectations were recorded while the synchronized exemption was in place (commit e73dd8c) and this commit doesn't touch them, so the description and the ruling rationale no longer describe what the rule does. Fix: update the PR description, and re-run the ruling to confirm that no synchronized-only fields in the ruling projects now produce new issues.

_{Was this helpful? React with 👍 / 👎}

[github-actions]

⚖️ Ruling update ready for review: #6342

[gitar-bot]

Code Review 👍 Approved with suggestions 4 closed / 5 findings

🟡 Medium risk · Adding S9428 to the Sonar way profile could surface new field findings and cause existing project quality gates to fail.

Adds rule S9428 to flag MessageDigest instances stored in type fields, which can share mutable digest state across invocations or threads, with a sample test file and ruling expectations for guava and eclipse-jetty. Approved with a suggestion: the PR description still says private fields used only inside synchronized blocks or methods are not reported, but the current rule reports them, so the description and the jetty ruling rationale should be updated and the ruling re-run to confirm no new issues appear.

💡 Quality: PR description still promises a synchronized exemption the rule dropped

📄 java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java:57-65 📄 java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java:118-132

This commit removes all monitor handling from isOnlyUsedAsPrototype. Private fields used only inside synchronized blocks or methods are now reported, and the sample's SynchronizedHolder.lockedDigest/methodDigest are now marked Noncompliant. The PR description still says private fields are not reported when each usage "sits in a synchronized block or method". It also explains the jetty ruling hit as "guarded by a custom AutoLock rather than synchronized", which no longer matters, since any synchronized-only field is now reported too. The ruling expectations were recorded while the synchronized exemption was in place (commit e73dd8c) and this commit doesn't touch them, so the description and the ruling rationale no longer describe what the rule does. Fix: update the PR description, and re-run the ruling to confirm that no synchronized-only fields in the ruling projects now produce new issues.

✅ 4 closed

✅ Bug: Synchronized exemption ignores which monitor is held

📄 java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java:79-81 📄 java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java:105-119
isInSynchronizedContext treats a usage as safe if it sits inside any synchronized block or method, no matter which lock is held. So private static MessageDigest md; used only from instance synchronized methods is treated as compliant, even though each instance locks its own this and two instances can mutate the shared static digest at the same time. The same goes for a field used under synchronized(LOCK) in one method and in a synchronized instance method (or a nested class's synchronized method) in another: the usages don't exclude each other but all count as safe. This is the concurrent shared-state case the rule is meant to catch. Fix: require every synchronized usage of a field to lock the same monitor. A simple version: a static field must be locked by a static synchronized method or by synchronized on one shared static expression. If the monitors can't be proven equal, report the field.

✅ Bug: clone()/toString() exemption hides races with synchronized updates

📄 java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java:42-46 📄 java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java:74-88 📄 java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java:97-103
The Javadoc says clone() "does not depend on the digest state", but MessageDigest.clone() copies the in-progress state and toString() reads it. These calls are only safe when nothing ever mutates the field. Because isSafeUsage checks each usage independently, a field that is update()d inside synchronized(LOCK) and clone()d outside the lock passes every per-usage check and is not reported. The unlocked clone can then copy a half-updated state. Fix: only allow the stateless-call exemption when every non-assignment usage of the field is a stateless call, and do not mix it with the synchronized exemption. Alternatively, also require the stateless calls to be synchronized when other usages mutate the field.

Closed: Bug: Test sample won't compile: CustomDigest has no matching super ctor

📄 java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java:50-51
abstract static class CustomDigest extends MessageDigest {} gets an implicit no-arg constructor that calls super(). java.security.MessageDigest has only one constructor, protected MessageDigest(String algorithm), so javac fails with "constructor MessageDigest in class MessageDigest cannot be applied to given types". The java-checks-test-sources/default module is a real Maven module compiled with maven-compiler-plugin (release 26), so this sample breaks the build. The existing MyMessageDigest in checks/security/UnpredictableSaltCheck.java avoids the problem by declaring protected MyMessageDigest(String algorithm) { super(algorithm); }. The fix is to add an explicit constructor that calls super(algorithm).

Closed: Edge Case: Instance-monitor exemption ignores which instance owns the field

📄 java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java:97-100 📄 java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java:127-139 📄 java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java:178-181
monitorOf only looks at the code around the usage, and Monitor.canGuard only compares the lock's owning class with the field's owning class. Neither checks that the field is read through this. So synchronized void merge(Holder other) { other.digest.update(p); } or synchronized (lockA) { other.digest.update(p); } count as guarded: they get the same Monitor(owner, owner) / Monitor(lockA, owner) as the real this-guarded usages. But the lock held belongs to this, not to other, so other.digest can still be updated concurrently. The rule then silently misses the field. For monitors with a non-null instanceOwner, only accept usages whose field expression is an unqualified identifier or this.field. For example, look at the fieldExpression(usage) receiver and treat any other qualifier as unguarded.

🤖 Prompt for agents

Code Review: Adds rule S9428 to flag `MessageDigest` instances stored in type fields, which can share mutable digest state across invocations or threads, with a sample test file and ruling expectations for guava and eclipse-jetty. Approved with a suggestion: the PR description still says private fields used only inside `synchronized` blocks or methods are not reported, but the current rule reports them, so the description and the jetty ruling rationale should be updated and the ruling re-run to confirm no new issues appear.

1. 💡 Quality: PR description still promises a synchronized exemption the rule dropped
   Files: java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java:57-65, java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java:118-132

   This commit removes all monitor handling from `isOnlyUsedAsPrototype`. Private fields used only inside `synchronized` blocks or methods are now reported, and the sample's `SynchronizedHolder.lockedDigest`/`methodDigest` are now marked Noncompliant. The PR description still says private fields are not reported when each usage "sits in a `synchronized` block or method". It also explains the jetty ruling hit as "guarded by a custom `AutoLock` rather than `synchronized`", which no longer matters, since any synchronized-only field is now reported too. The ruling expectations were recorded while the synchronized exemption was in place (commit e73dd8c) and this commit doesn't touch them, so the description and the ruling rationale no longer describe what the rule does. Fix: update the PR description, and re-run the ruling to confirm that no synchronized-only fields in the ruling projects now produce new issues.

Verification Repos 3 inspected

Functional validation No results

Rules No rules evaluated

Cross-repo 3 repositories inspected 0 open impacts

SonarSource/sonarlint-intellij Inspected
SonarSource/sonarlint-eclipse Inspected
SonarSource/sonarlint-vscode Inspected

Auto-approval Not enabled

Set up

_{For more verification details, comment gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

[rombirli]

/pvf

[rombirli]

LGTM, thanks for the changes. Please wait for second PVF run to validate that new change didn't impact the rule performance before merging

[hashicorp-vault-sonar-prod]

📊 Performance Validation complete
🆕 25 new · 🗑️ 0 lost issues
View dashboard