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

[datadog-sonarsource]

❌ Errors

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 2 Pipeline jobs failed

Build | Ruling QA (warp-custom-ubuntu-24-04, without-sonarqube-project, LATEST_RELEASE)

View more details · View in GitHub Actions

Build | Ruling QA (warp-custom-windows-2022-l, without-sonarqube-project, LATEST_RELEASE)

View more details · View in GitHub Actions

Useful? React with 👍 / 👎

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: ae92bb2 | Docs | View more details | Give us feedback!}

[github-actions]

⚖️ Ruling update ready for review: #6326

[github-actions]

Ruling Diff Summary

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

S9428 (java) on eclipse-jetty - 1 issues removed, 1 issues added

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)

Removed 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 - 1 issues removed, 1 issues added

Removed 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;

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 👍 / 👎}

[gitar-bot]

CI failed: Ruling QA failed on two fronts: the new S9428 rule's output likely diverges from the Eclipse Jetty and Guava expected ruling snapshots (change-related, medium confidence), and a Jetty expected entry for Credential.java was not analyzed while the Guava ruling test errored with its cause missing from the logs (relation unclear).

Overview

Two unique error templates across 12 analyzed logs from the Ruling QA jobs (Linux and Windows). Cache, runner, and setup steps completed without error. The real failures are in the ruling integration tests, which compare actual analyzer output against expected snapshots.

Failures

Ruling snapshot mismatch for S9428 (confidence: medium)

• Type: test
• Affected jobs: 114792039453, 115351243209, 115363577018
• Related to change: yes
• Root cause: The excerpt does not show the exact diff, but the Ruling QA jobs run against the expected ruling results, and this PR adds the S9428 rule. The follow-up PR Update ruling results for PR #6321 #6342 updates the Eclipse Jetty and Guava S9428 expected files, which suggests actual findings differ from the committed expectations.
• Suggested fix: Compare the generated actual S9428 results (under its/ruling/target/actual) with the expected snapshots for eclipse-jetty and guava, confirm the intended findings, and update the expected files accordingly. Rerun Ruling QA to verify.

Jetty expected file not analyzed and Guava ruling test error (confidence: medium)

• Type: test
• Affected jobs: 115363576979
• Related to change: unclear
• Root cause: The Jetty sonar-maven-plugin fails with Files listed in Expected directory were not analyzed for jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java, meaning an expected entry references a file the analysis did not process. Separately, JavaRulingTest.guava reports 2 errors in Surefire, but its terminal diagnostic is missing from the excerpt. This PR touches expected ruling files for Jetty and Guava, so it may be involved, but the logs do not confirm that.
• Suggested fix: Inspect the full Jetty and Guava Surefire reports in its/ruling/target/surefire-reports. Remove or correct stale expected entries for Credential.java, or fix analysis configuration if that source should be analyzed. Diagnose the Guava error from its full stack trace.

Summary

• Change-related failures: 1 finding (S9428 ruling snapshot mismatch against Jetty and Guava expectations), attributed to this PR.
• Infrastructure/flaky failures: None identified. Cache misses and runner hooks are benign.
• Recommended action: Review the generated actual S9428 ruling output and update the Jetty and Guava expected files to match intended results. Separately, investigate the Jetty missing-file expectation and the Guava error using the full Surefire reports, then rerun Ruling QA.

Code Review 👍 Approved with suggestions 4 closed / 5 findings

🟡 Medium risk · Adding S9428 changes Java analysis results by flagging retained MessageDigest fields; imperfect usage exemptions could miss unsafe shared digest state or flag safe fields.

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 Knowledge ✅ 1 Repos 3 inspected

Functional validation No results

Rules No rules evaluated

Knowledge ✅ 1 expectation met

MessageDigest stateless-call exemption requires consistent usage expectation met

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

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

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

[github-actions]

⚖️ Ruling update ready for review: #6342