diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6813.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6813.json index b3cb1e5287d..21c0b4871c4 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6813.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6813.json @@ -1,6 +1,6 @@ { "ruleKey": "S6813", "hasTruePositives": true, - "falseNegatives": 70, + "falseNegatives": 71, "falsePositives": 0 } diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S8983.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S8983.json new file mode 100644 index 00000000000..534924762d8 --- /dev/null +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S8983.json @@ -0,0 +1,6 @@ +{ + "ruleKey": "S8983", + "hasTruePositives": false, + "falseNegatives": 6, + "falsePositives": 0 +} diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/StatelessBeanInstanceFieldCheckJakartaSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/StatelessBeanInstanceFieldCheckJakartaSample.java new file mode 100644 index 00000000000..95dadf7c5c5 --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/StatelessBeanInstanceFieldCheckJakartaSample.java @@ -0,0 +1,59 @@ +package checks; + +import jakarta.annotation.Resource; +import jakarta.ejb.EJB; +import jakarta.ejb.Singleton; +import jakarta.ejb.Stateful; +import jakarta.ejb.Stateless; +import jakarta.inject.Inject; +import jakarta.persistence.EntityManager; +import jakarta.persistence.EntityManagerFactory; +import jakarta.persistence.PersistenceContext; +import jakarta.persistence.PersistenceUnit; +import jakarta.xml.ws.WebServiceRef; + +@Stateless +class StatelessWithMutableFieldsJakarta { + private int requestCount; // Noncompliant {{Remove this mutable instance field or replace it with a local variable, a "static final" constant, or an injected resource.}} + // ^^^^^^^^^^^^ + private String lastClientId; // Noncompliant + private Object cachedResult = null; // Noncompliant + public int publicField; // Noncompliant + protected int protectedField; // Noncompliant +} + +@Stateless +class StatelessWithSafeFieldsJakarta { + private static final int MAX_RETRIES = 3; // Compliant - static final constant + private static int sharedCounter; // Compliant - static field + private final String name = "service"; // Compliant - final field + + @EJB + private StatelessWithSafeFieldsJakarta ejbRef; // Compliant - injected EJB + + @Inject + private Object cdiBean; // Compliant - CDI injection + + @PersistenceContext + private EntityManager em; // Compliant - injected persistence context + + @PersistenceUnit + private EntityManagerFactory emf; // Compliant - injected persistence unit + + @Resource + private javax.sql.DataSource dataSource; // Compliant - injected resource + + @WebServiceRef + private Object wsRef; // Compliant - injected web service reference +} + +@Stateful +class StatefulWithMutableFieldsJakarta { + private int requestCount; // Compliant - @Stateful beans are designed to hold state + private String lastClientId; +} + +@Singleton +class SingletonWithMutableFieldsJakarta { + private int requestCount; // Compliant - not a @Stateless bean +} diff --git a/java-checks-test-sources/default/src/main/java/checks/StatelessBeanInstanceFieldCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/StatelessBeanInstanceFieldCheckSample.java new file mode 100644 index 00000000000..902bd0b6f32 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/StatelessBeanInstanceFieldCheckSample.java @@ -0,0 +1,63 @@ +package checks; + +import javax.annotation.Resource; +import javax.ejb.EJB; +import javax.ejb.Singleton; +import javax.ejb.Stateful; +import javax.ejb.Stateless; +import javax.inject.Inject; +import javax.persistence.EntityManager; +import javax.persistence.EntityManagerFactory; +import javax.persistence.PersistenceContext; +import javax.persistence.PersistenceUnit; +import javax.xml.ws.WebServiceRef; + +@Stateless +class StatelessWithMutableFields { + private int requestCount; // Noncompliant {{Remove this mutable instance field or replace it with a local variable, a "static final" constant, or an injected resource.}} + // ^^^^^^^^^^^^ + private String lastClientId; // Noncompliant + private Object cachedResult = null; // Noncompliant + public int publicField; // Noncompliant + protected int protectedField; // Noncompliant +} + +@Stateless +class StatelessWithSafeFields { + private static final int MAX_RETRIES = 3; // Compliant - static final constant + private static int sharedCounter; // Compliant - static field + private final String name = "service"; // Compliant - final field + + @EJB + private StatelessWithSafeFields ejbRef; // Compliant - injected EJB + + @Inject + private Object cdiBean; // Compliant - CDI injection + + @PersistenceContext + private EntityManager em; // Compliant - injected persistence context + + @PersistenceUnit + private EntityManagerFactory emf; // Compliant - injected persistence unit + + @Resource + private javax.sql.DataSource dataSource; // Compliant - injected resource + + @WebServiceRef + private Object wsRef; // Compliant - injected web service reference +} + +@Stateful +class StatefulWithMutableFields { + private int requestCount; // Compliant - @Stateful beans are designed to hold state + private String lastClientId; +} + +@Singleton +class SingletonWithMutableFields { + private int requestCount; // Compliant - not a @Stateless bean +} + +class PlainClassWithMutableFields { + private int requestCount; // Compliant - not an EJB at all +} diff --git a/java-checks-test-sources/default/src/main/java/jakarta/ejb/EJB.java b/java-checks-test-sources/default/src/main/java/jakarta/ejb/EJB.java new file mode 100644 index 00000000000..fd4e4e6b83a --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/jakarta/ejb/EJB.java @@ -0,0 +1,14 @@ +package jakarta.ejb; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +import static java.lang.annotation.ElementType.FIELD; +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.ElementType.TYPE; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +@Retention(RUNTIME) +@Target({TYPE, METHOD, FIELD}) +public @interface EJB { +} diff --git a/java-checks-test-sources/default/src/main/java/jakarta/xml/ws/WebServiceRef.java b/java-checks-test-sources/default/src/main/java/jakarta/xml/ws/WebServiceRef.java new file mode 100644 index 00000000000..35e7fe30bc2 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/jakarta/xml/ws/WebServiceRef.java @@ -0,0 +1,14 @@ +package jakarta.xml.ws; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +import static java.lang.annotation.ElementType.FIELD; +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.ElementType.TYPE; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +@Retention(RUNTIME) +@Target({TYPE, METHOD, FIELD}) +public @interface WebServiceRef { +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/StatelessBeanInstanceFieldCheck.java b/java-checks/src/main/java/org/sonar/java/checks/StatelessBeanInstanceFieldCheck.java new file mode 100644 index 00000000000..661bdc19262 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/StatelessBeanInstanceFieldCheck.java @@ -0,0 +1,85 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import java.util.List; +import org.sonar.check.Rule; +import org.sonar.java.model.ModifiersUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.SymbolMetadata; +import org.sonar.plugins.java.api.tree.Modifier; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.VariableTree; + +@Rule(key = "S8983") +public class StatelessBeanInstanceFieldCheck extends IssuableSubscriptionVisitor { + + private static final List STATELESS_ANNOTATIONS = List.of( + "javax.ejb.Stateless", + "jakarta.ejb.Stateless" + ); + + private static final List SAFE_FIELD_ANNOTATIONS = List.of( + "javax.ejb.EJB", + "jakarta.ejb.EJB", + "javax.inject.Inject", + "jakarta.inject.Inject", + "javax.persistence.PersistenceContext", + "jakarta.persistence.PersistenceContext", + "javax.persistence.PersistenceUnit", + "jakarta.persistence.PersistenceUnit", + "javax.annotation.Resource", + "jakarta.annotation.Resource", + "javax.xml.ws.WebServiceRef", + "jakarta.xml.ws.WebServiceRef" + ); + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.VARIABLE); + } + + @Override + public void visitNode(Tree tree) { + var variable = (VariableTree) tree; + if (isInstanceFieldOfStatelessBean(variable) && !isSafe(variable)) { + reportIssue(variable.simpleName(), + "Remove this mutable instance field or replace it with a local variable, a \"static final\" constant, or an injected resource."); + } + } + + private static boolean isInstanceFieldOfStatelessBean(VariableTree variable) { + if (!variable.parent().is(Tree.Kind.CLASS)) { + return false; + } + Symbol owner = variable.symbol().owner(); + if (!owner.isTypeSymbol()) { + return false; + } + SymbolMetadata ownerMetadata = owner.metadata(); + return STATELESS_ANNOTATIONS.stream().anyMatch(ownerMetadata::isAnnotatedWith); + } + + private static boolean isSafe(VariableTree variable) { + if (ModifiersUtils.hasAnyOf(variable.modifiers(), Modifier.STATIC, Modifier.FINAL)) { + return true; + } + SymbolMetadata varMetadata = variable.symbol().metadata(); + return SAFE_FIELD_ANNOTATIONS.stream().anyMatch(varMetadata::isAnnotatedWith); + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/StatelessBeanInstanceFieldCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/StatelessBeanInstanceFieldCheckTest.java new file mode 100644 index 00000000000..9b288e3e2da --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/StatelessBeanInstanceFieldCheckTest.java @@ -0,0 +1,51 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; + +class StatelessBeanInstanceFieldCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/StatelessBeanInstanceFieldCheckSample.java")) + .withCheck(new StatelessBeanInstanceFieldCheck()) + .verifyIssues(); + } + + @Test + void test_jakarta() { + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath("checks/StatelessBeanInstanceFieldCheckJakartaSample.java")) + .withCheck(new StatelessBeanInstanceFieldCheck()) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/StatelessBeanInstanceFieldCheckSample.java")) + .withCheck(new StatelessBeanInstanceFieldCheck()) + .withoutSemantic() + .verifyNoIssues(); + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8983.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8983.html new file mode 100644 index 00000000000..fac6f21bcd8 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8983.html @@ -0,0 +1,97 @@ +

A component annotated or configured as stateless in an enterprise component framework declares a mutable instance field that is not managed by the +framework’s dependency injection mechanism.

+

Why is this an issue?

+

Stateless server components are pooled and reused by the application container. When a client invokes a business method, the container assigns an +available instance from the pool to handle that request. After the method completes, the same instance returns to the pool and may be assigned to a +completely different client on the next invocation.

+

If you store data in an instance variable during one client’s invocation, that data remains in memory and will be visible to the next client who +receives that component instance. This creates several problems:

+
    +
  • Data leakage: Client A’s data becomes visible to Client B
    • +
  • Inconsistent behavior: Methods may see unexpected values from previous invocations
    • +
  • Race conditions: Multiple threads may access the same field concurrently
    • +
+

The container does not reset or clear instance variables between invocations. It only does so when creating a new instance or during specific +lifecycle callbacks.

+

Safe instance fields

+

Container-injected resources are safe because they are either thread-safe or context-aware:

+
    +
  • Injected component references
    • +
  • Dependency injection container beans
    • +
  • Injected persistence contexts (container-managed, context-aware)
    • +
  • Injected persistence manager factories
    • +
  • JNDI resources, data sources, and similar container-managed resources
    • +
  • Web service references
    • +
+

These resources are managed by the container to work correctly in a pooled environment.

+

In Java EE/Jakarta EE, these safe resources are typically injected using annotations: @EJB for component references, +@Inject for CDI beans, @PersistenceContext for entity managers, @PersistenceUnit for entity manager factories, +@Resource for JNDI resources and data sources, and @WebServiceRef for web service references.

+

What is the potential impact?

+

When mutable state leaks between clients:

+
    +
  • Security risk: Sensitive data from one user (passwords, personal information, session data) may be exposed to another user
    • +
  • Data corruption: Business logic may operate on stale or incorrect data, leading to wrong calculations, duplicate processing, or + inconsistent database updates
    • +
  • Unpredictable behavior: The application behaves differently depending on which shared component instance is used, making bugs + difficult to reproduce and diagnose
    • +
+

How to fix it

+

The right fix depends on the intended purpose of each instance field.

+

Code examples

+

Noncompliant code example

+
+@Stateless
+public class OrderService {
+    private int lastOrderId;      // Noncompliant
+    private int maxRetries = 3;   // Noncompliant
+    private int requestCount;     // Noncompliant
+
+    public void processOrder(Order order) {
+        lastOrderId = order.getId();
+        requestCount++;
+        // lastOrderId and requestCount may leak to the next client
+    }
+}
+
+

Compliant solution

+
+@Stateless
+public class OrderService {
+    private static final int MAX_RETRIES = 3;  // Compliant; safe as a static final field
+
+    @EJB
+    private OrderStats orderStats;  // Compliant; shared state delegated to a helper singleton EJB
+
+    public void processOrder(Order order) {
+        int lastOrderId = order.getId();  // Compliant; moved to a method-local variable
+        orderStats.incrementRequestCount();
+    }
+}
+
+@Singleton
+public class OrderStats {
+    private int requestCount;
+
+    @Lock(LockType.WRITE)
+    public void incrementRequestCount() {
+        requestCount++;
+    }
+
+    @Lock(LockType.READ)
+    public int getRequestCount() {
+        return requestCount;
+    }
+}
+
+

If a value must persist across multiple method calls for the same client, consider switching to a @Stateful session bean instead, +which provides a dedicated instance per client.

+

Resources

+

Documentation

+ + diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8983.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8983.json new file mode 100644 index 00000000000..0b267bc7dee --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8983.json @@ -0,0 +1,26 @@ +{ + "title": "Stateless session beans should not store mutable state in instance variables", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "15min" + }, + "tags": [ + "ejb", + "jakarta", + "concurrency" + ], + "defaultSeverity": "Critical", + "ruleSpecification": "RSPEC-8983", + "sqKey": "S8983", + "scope": "Main", + "quickfix": "unknown", + "code": { + "impacts": { + "RELIABILITY": "HIGH", + "MAINTAINABILITY": "MEDIUM" + }, + "attribute": "MODULAR" + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S8983 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S8983 new file mode 100644 index 00000000000..e69de29bb2d