Foo[] arr = new Foo[2]; arr[0] = new Foo(0); arr[1] = new Foo(0);
Java
Java - 3
Learn about Java Anti-Patterns and How they help you write better code, and avoid common pitfalls.
Math.abs and negation should not be used on numbers that could be MIN_VALUE
Math.abs and negation should not be used on numbers that could be MIN_VALUE
This rule involves the use of Math.abs and negation on numbers that could be MIN_VALUE. It is a problem because it can lead to incorrect results and unexpected behavior in the program.
When Math.abs and negation are used on numbers that could be MIN_VALUE, the result can be incorrect due to integer overflow. Common methods that can return a MIN_VALUE and raise an issue when used together with Math.abs are:
Random.nextInt() and Random.nextLong()
hashCode()
compareTo()
Alternatively, the absExact() method throws an ArithmeticException for MIN_VALUE.
@CheckForNull or @Nullable should not be used on primitive types
@CheckForNull or @Nullable should not be used on primitive types
By definition, primitive types are not Objects and so they can’t be `null. Adding @CheckForNull or @Nullable on primitive types adds confusion and is useless.
This rule raises an issue when @CheckForNull or @Nullable` is set on a method returning a primitive type: byte, short, int, long, float, double, boolean, char.
equals method overrides should accept Object parameters
equals method overrides should accept Object parameters
In Java, the Object.equals() method is used for object comparison, and it is typically overridden in classes to provide a custom equality check based on your criteria for equality.
The default implementation of equals() provided by the Object class compares the memory references of the two objects, that means it checks if the objects are actually the same instance in memory.
The “equals” as a method name should be used exclusively to override Object.equals(Object) to prevent confusion.
It is important to note that when you override equals(), you should also override the hashCode() method to maintain the contract between equals() and hashCode().
Value-based objects should not be serialized
Value-based objects should not be serialized
According to the documentation,
For example (credit to Brian Goetz), imagine Foo is a value-based class:
Serialization promises that on deserialization of arr, elements 0 and 1 will not be aliased. Similarly, in:
Foo[] arr = new Foo[2]; arr[0] = new Foo(0); arr[1] = arr[0];
Serialization promises that on deserialization of `arr, elements 0 and 1 will be aliased.
While these promises are coincidentally fulfilled in current implementations of Java, that is not guaranteed in the future, particularly when true value types are introduced in the language.
This rule raises an issue when a Serializable class defines a non-transient, non-static field field whose type is a known serializable value-based class. Known serializable value-based classes are: all the classes in the java.time package except Clock; the date classes for alternate calendars: HijrahDate, JapaneseDate, MinguoDate, ThaiBuddhistDate`.
Primitives should not be boxed just for String conversion
Primitives should not be boxed just for String conversion
”Boxing” is the process of putting a primitive value into a primitive-wrapper object. When that’s done purely to use the wrapper class’ toString method, it’s a waste of memory and cycles because those methods are static, and can therefore be used without a class instance. Similarly, using the static method valueOf in the primitive-wrapper classes with a non-String argument should be avoided.
isInstance should not be used with Class arguments
isInstance should not be used with Class arguments
The `Class.isInstance method is the dynamic equivalent of the instanceof operator. According to the JavaDoc, isInstance
Thus, calling isInstance with a class argument is likely a mistake, since any random Class will only be “an instance of the represented class” when the left-hand side of the call is Class.class itself. To test for a class/sub-class relationship, use isAssignableFrom` instead.
toString should not be called on Strings
toString should not be called on Strings
The String class has a toString method because Object itself does. I.e. it couldn’t not have the method. But having the method doesn’t mean it should be used. In fact doing so is worse than pointless since it returns the String itself.
Singletons should have private constructors
Singletons should have private constructors
Singletons that aren’t actually singletons become problems instead. To make sure a singleton isn’t instantiable, make sure all constructors are `private. If the singleton doesn’t have any constructors then add a private no-args constructor to override the default constructor.
This rule raises an issue when a class that holds a public static final instance of itself has non-private` constructors or no constructor.
equals methods should be symmetric and work for subclasses
equals methods should be symmetric and work for subclasses
A key facet of the `equals contract is that if a.equals(b) then b.equals(a), i.e. that the relationship is symmetric.
Using instanceof breaks the contract when there are subclasses, because while the child is an instanceof the parent, the parent is not an instanceof the child. For instance, assume that Raspberry extends Fruit and adds some fields (requiring a new implementation of equals):
Fruit fruit = new Fruit(); Raspberry raspberry = new Raspberry();if (raspberry instanceof Fruit) { … } // true if (fruit instanceof Raspberry) { … } // false
If similar instanceof checks were used in the classes’ equals methods, the symmetry principle would be broken:
raspberry.equals(fruit); // false fruit.equals(raspberry); //true
Additionally, non final classes shouldn’t use a hardcoded class name in the equals method because doing so breaks the method for subclasses. Instead, make the comparison dynamic.
Further, comparing to an unrelated class type breaks the contract for that unrelated type, because while thisClass.equals(unrelatedClass) can return true, unrelatedClass.equals(thisClass)` will not.
Map computeIfAbsent() and computeIfPresent() should not be used to add null values.
Map computeIfAbsent() and computeIfPresent() should not be used to add null values.
Map computeIfAbsent and computeIfPresent methods are convenient to avoid the cumbersome process to check if a key exists or not, followed by the addition of the entry. However, when the function used to compute the value returns `null, the entry key->null will not be added to the Map. Furthermore, in the case of computeIfPresent, if the key is present the entry will be removed. These methods should therefore not be used to conditionally add an entry with a null value. The traditional way should be used instead.
This rule raises an issue when computeIfAbsent or computeIfPresent` is used with a lambda always returning null.
Anonymous inner classes containing only one method should become lambdas
Anonymous inner classes containing only one method should become lambdas
Before Java 8, the only way to partially support closures in Java was by using anonymous inner classes. Java 8 introduced lambdas, which are significantly more readable and should be used instead.
This rule is automatically disabled when the project’s sonar.java.source is lower than 8, as lambda expressions were introduced in Java 8.
Region should be set explicitly when creating a new AwsClient
Region should be set explicitly when creating a new AwsClient
If the region is not specified when creating a new AwsClient with an AwsClientBuilder, the AWS SDK will execute some logic to identify the endpoint automatically.
While it will probably identify the correct one, this extra logic will slow down startup time, already known to be a hotspot.
You should therefore always define the logic to set the region yourself. This is typically done by retrieving the region from the Lambda provided AWS_REGION environment variable.
This will make the code more explicit and spare initialization time.
This rule reports an issue when the region is not set when creating an AwsClient.
Classes that dont define hashCode() should not be used in hashes
Classes that dont define hashCode() should not be used in hashes
Because Object implements hashCode, any Java class can be put into a hash structure. However, classes that define equals(Object) but not hashCode() aren’t truly hash-able because instances that are equivalent according to the equals method can return different hashes.
The non-serializable super class of a Serializable class should have a no-argument constructor
The non-serializable super class of a Serializable class should have a no-argument constructor
Java serialization is the conversion from objects to byte streams for storage or transmission. And later, java deserialization is the reverse conversion, it reconstructs objects from byte streams.
To make a java class serializable, this class should implement the java.io.Serializable interface directly or through its inheritance.
String operations should not rely on the default system locale
String operations should not rely on the default system locale
Failure to specify a locale when calling the methods `toLowerCase(), toUpperCase() or format() on String objects means the system default encoding will be used, possibly creating problems with international characters or number representations. For instance with the Turkish language, when converting the small letter ‘i’ to upper case, the result is capital letter ‘I’ with a dot over it.
Case conversion without a locale may work fine in its “home” environment, but break in ways that are extremely difficult to diagnose for customers who use different encodings. Such bugs can be nearly, if not completely, impossible to reproduce when it’s time to fix them. For locale-sensitive strings, the correct locale should always be used, but Locale.ROOT` can be used for case-insensitive ones.
Maps with keys that are enum values should use the EnumMap implementation
Maps with keys that are enum values should use the EnumMap implementation
If all the keys in a Map are values from a single enum, it is recommended to use an EnumMap as the specific implementation. An EnumMap, which has the advantage of knowing all possible keys in advance, is more efficient compared to other implementations, as it can use a simple array as its underlying data structure.
Optional methods should not be invoked on TYPE_FORWARD_ONLY result sets
Optional methods should not be invoked on TYPE_FORWARD_ONLY result sets
The implementation of certain `ResultSet methods is optional for result sets of type TYPE_FORWARD_ONLY. Even if your current JDBC driver does implement those methods, there’s no guarantee you won’t change drivers in the future.
This rule looks for invocations of the following methods on TYPE_FORWARD_ONLY ResultSets:
isBeforeFirst
isAfterLast
isFirst
getRow`
Return of boolean expressions should not be wrapped into an if-then-else statement
Return of boolean expressions should not be wrapped into an if-then-else statement
Return of boolean literal statements wrapped into `if-then-else ones should be simplified.
Similarly, method invocations wrapped into if-then-else` differing only from boolean literals should be simplified into a single invocation.
@Override should be used on overriding and implementing methods
@Override should be used on overriding and implementing methods
While not mandatory, using the @Override annotation on compliant methods improves readability by making it explicit that methods are overriden.
A compliant method either overrides a parent method or implements an interface or abstract method.
Ints and longs should not be shifted by zero or more than their number of bits-1
Ints and longs should not be shifted by zero or more than their number of bits-1
Since an int is a 32-bit variable, shifting by more than +/-31 is confusing at best and an error at worst. When the runtime shifts 32-bit integers, it uses the lowest 5 bits of the shift count operand. In other words, shifting an int by 32 is the same as shifting it by 0, and shifting it by 33 is the same as shifting it by 1.
Similarly, when shifting 64-bit integers, the runtime uses the lowest 6 bits of the shift count operand and shifting long by 64 is the same as shifting it by 0, and shifting it by 65 is the same as shifting it by 1.
Virtual threads should be used for tasks that include heavy blocking operations
Virtual threads should be used for tasks that include heavy blocking operations
Whenever a virtual thread is started, the JVM will mount it on an OS thread. As soon as the virtual thread runs into a blocking operation like an HTTP request or a filesystem read/write operation, the JVM will detect this and unmount the virtual thread. This allows another virtual thread to take over the OS thread and continue its execution.
This is why virtual threads should be preferred to platform threads for tasks that involve blocking operations. By default, a Java thread is a platform thread. To use a virtual thread it must be started either with Thread.startVirtualThread(Runnable) or Thread.ofVirtual().start(Runnable).
This rule raises an issue when a platform thread is created with a task that includes heavy blocking operations.
String literals should not be concatenated
String literals should not be concatenated
There is no reason to concatenate literal strings. Doing so is an exercise is reducing code readability. Instead, the strings should be combined. Similarly, literal strings should not be appended to a StringBuffer or StringBuilder sequentially, but combined into one call.
Non-singleton Spring beans should not be injected into singleton beans
Non-singleton Spring beans should not be injected into singleton beans
In Spring, singleton beans and their dependencies are initialized when the application context is created.
If a Singleton bean depends on a bean with a shorter-lived scope (like Request or Session beans), it retains the same instance of that bean, even when new instances are created for each Request or Session. This mismatch can cause unexpected behavior and bugs, as the Singleton bean doesn’t interact correctly with the new instances of the shorter-lived bean.
This rule raises an issue when non-singleton beans are injected into a singleton bean.
Reverse iteration should utilize reversed view
Reverse iteration should utilize reversed view
Java 21 introduces the new Sequenced Collections API, which is applicable to all collections with a defined sequence on their elements, such as LinkedList, TreeSet, and others (see JEP 431). For projects using Java 21 and onwards, this API should be utilized instead of workaround implementations that were necessary before Java 21.
This rule reports when a collection is iterated in reverse through explicit implementation or workarounds, instead of using the reversed view of the collection.
Checked exceptions should not be thrown
Checked exceptions should not be thrown
The purpose of checked exceptions is to ensure that errors will be dealt with, either by propagating them or by handling them, but some believe that checked exceptions negatively impact the readability of source code, by spreading this error handling/propagation logic everywhere.
This rule verifies that no method throws a new checked exception.
Externalizable classes should have no-arguments constructors
Externalizable classes should have no-arguments constructors
A class that implements java.io.Externalizable is a class that provides a way to customize the serialization and deserialization, allowing greater control over how the object’s state is written or read.
The first step of the deserialization process is to call the class’ no-argument constructor before the readExternal(ObjectInput in) method.
An implicit default no-argument constructor exists on a class when no constructor is explicitly defined within the class. But this implicit constructor does not exist when any constructor is explicitly defined, and in this case, we should always ensure that one of the constructors has no-argument.
It is an issue if the implicit or explicit no-argument constructor is missing or not public, because the deserialization will fail and throw an InvalidClassException: no valid constructor..
Deprecated methods should not be overridden
Deprecated methods should not be overridden
Deprecated method should be avoided, rather than overridden. Deprecation is a warning that the method has been superseded, and will eventually be removed. The deprecation period allows you to make a smooth transition away from the aging, soon-to-be-retired technology.
Pattern Matching for instanceof operator should be used instead of simple instanceof + cast
Pattern Matching for instanceof operator should be used instead of simple instanceof + cast
In Java 16, the feature “Pattern matching for instanceof” is finalized and can be used in production. Previously developers needed to do 3 operations in order to do this: check the variable type, cast it and assign the casted value to the new variable. This approach is quite verbose and can be replaced with pattern matching for `instanceof, doing these 3 actions (check, cast and assign) in one expression.
This rule raises an issue when an instanceof` check followed by a cast and an assignment could be replaced by pattern matching.
Public constants and fields initialized at declaration should be static final rather than merely final
Public constants and fields initialized at declaration should be static final rather than merely final
Making a `public constant just final as opposed to static final leads to duplicating its value for every instance of the class, uselessly increasing the amount of memory required to execute the application.
Further, when a non-public, final field isn’t also static, it implies that different instances can have different values. However, initializing a non-static final field in its declaration forces every instance to have the same value. So such fields should either be made static` or initialized in the constructor.
Strings should not be concatenated using + in a loop
Strings should not be concatenated using + in a loop
Strings are immutable objects, so concatenation doesn’t simply add the new String to the end of the existing string. Instead, in each loop iteration, the first String is converted to an intermediate object type, the second string is appended, and then the intermediate object is converted back to a String. Further, performance of these intermediate operations degrades as the String gets longer. Therefore, the use of StringBuilder is preferred.
JEE applications should not getClassLoader
JEE applications should not getClassLoader
Using the standard getClassLoader() may not return the right class loader in a JEE context. Instead, go through the currentThread.
Raw types should not be used
Raw types should not be used
A generic type is a generic class or interface that is parameterized over types. For example, java.util.List has one type parameter: the type of its elements.
When generic types are used raw (without type parameters), the compiler is not able to do generic type checking. For this reason, it is sometimes necessary to cast objects and defer type-checking to runtime.
wait(...) should be used instead of Thread.sleep(...) when a lock is held
wait(...) should be used instead of Thread.sleep(...) when a lock is held
In a multithreaded environment, a thread may need to wait for a particular condition to become true. One way of pausing execution in Java is Thread.sleep(…).
If a thread that holds a lock calls Thread.sleep(…), no other thread can acquire said lock. This can lead to performance and scalability issues, in the worst case leading to deadlocks.
Random objects should be reused
Random objects should be reused
Creating a new Random object each time a random value is needed is inefficient and may produce numbers that are not random, depending on the JDK. For better efficiency and randomness, create a single Random, store it, and reuse it.
The Random() constructor tries to set the seed with a distinct value every time. However, there is no guarantee that the seed will be randomly or uniformly distributed. Some JDK will use the current time as seed, making the generated numbers not random.
This rule finds cases where a new Random is created each time a method is invoked.
Migrate your tests from JUnit4 to the new JUnit5 annotations
Migrate your tests from JUnit4 to the new JUnit5 annotations
As mentioned in JUnit5 documentation, it is possible to integrate JUnit4 with JUnit5:
However, maintaining both systems is a temporary solution. This rule flags all the annotations from JUnit4 which would need to be migrated to JUnit5, hence helping migration of a project.
Here is the list of JUnit4 annotations tracked by the rule, with their corresponding annotations in JUnit5:
| JUnit4 | JUnit5 |
|---|---|
`org.junit.Test | org.junit.jupiter.api.Test |
org.junit.Before | org.junit.jupiter.api.BeforeEach |
org.junit.After | org.junit.jupiter.api.AfterEach |
org.junit.BeforeClass | org.junit.jupiter.api.BeforeAll |
org.junit.AfterClass | org.junit.jupiter.api.AfterAll |
org.junit.Ignore | org.junit.jupiter.api.Disabled |
Note that the following annotations might requires some rework of the tests to have JUnit5 equivalent behavior. A simple replacement of the annotation won’t work immediately:
| JUnit4 | JUnit5 |
|---|---|
org.junit.experimental.categories.Category | org.junit.jupiter.api.Tag |
org.junit.Rule | org.junit.jupiter.api.extension.ExtendWith |
org.junit.ClassRule | org.junit.jupiter.api.extension.RegisterExtension |
org.junit.runner.RunWith | org.junit.jupiter.api.extension.ExtendWith` |
Constructor injection should be used instead of field injection
Constructor injection should be used instead of field injection
Field injection seems like a tidy way to get your classes what they need to do their jobs, but it’s really a `NullPointerException waiting to happen unless all your class constructors are private. That’s because any class instances that are constructed by callers, rather than instantiated by a Dependency Injection framework compliant with the JSR-330 (Spring, Guice, …), won’t have the ability to perform the field injection.
Instead @Inject should be moved to the constructor and the fields required as constructor parameters.
This rule raises an issue when classes with non-private` constructors (including the default constructor) use field injection.
Whitespace for text block indent should be consistent
Whitespace for text block indent should be consistent
Either use only spaces or only tabs for the indentation of a text block. Mixing white space will lead to a result with irregular indentation.
Abstract classes without fields should be converted to interfaces
Abstract classes without fields should be converted to interfaces
With Java 8’s “default method” feature, any abstract class without direct or inherited field should be converted into an interface. However, this change may not be appropriate in libraries or other applications where the class is intended to be used as an API.
Note that this rule is automatically disabled when the project’s sonar.java.source is lower than 8.
Parsing should be used to convert Strings to primitives
Parsing should be used to convert Strings to primitives
Rather than creating a boxed primitive from a String to extract the primitive value, use the relevant parse method instead. Using parse makes the code more efficient and the intent of the developer clearer.
Reflection should not be used to increase accessibility of records fields
Reflection should not be used to increase accessibility of records fields
In general, altering or bypassing the accessibility of classes, methods, or fields violates the encapsulation principle and could lead to runtime errors. For records the case is even trickier: you cannot change the visibility of records’s fields and trying to update the existing value will lead to IllegalAccessException at runtime.
This rule raises an issue when reflection is used to change the visibility of a record’s field, and when it is used to directly update a record’s field value.
Subclasses that add fields to classes that override equals should also override equals
Subclasses that add fields to classes that override equals should also override equals
When a class overrides Object.equals, this indicates that the class not just considers object identity as equal (the default implementation of Object.equals) but implements another logic for what is considered equal in the context of this class. Usually (but not necessarily), the semantics of equals in this case is that two objects are equal when their state is equal field by field.
Because of this, adding new fields to a subclass of a class that overrides Object.equals but not updating the implementation of equals in the subclass is most likely an error.
Methods should not be named tostring, hashcode or equal
Methods should not be named tostring, hashcode or equal
Due to the similar name with the methods Object.toString, Object.hashCode and Object.equals, there is a significant likelihood that a developer intended to override one of these methods but made a spelling error.
Even if no such error exists and the naming was done on purpose, these method names can be misleading. Readers might not notice the difference, or if they do, they may falsely assume that the developer made a mistake.
Field dependency injection should be avoided
Field dependency injection should be avoided
Dependency injection frameworks such as Spring support dependency injection by using annotations such as @Inject and @Autowired. These annotations can be used to inject beans via constructor, setter, and field injection.
Generally speaking, field injection is discouraged. It allows the creation of objects in an invalid state and makes testing more difficult. The dependencies are not explicit when instantiating a class that uses field injection.
In addition, field injection is not compatible with final fields. Keeping dependencies immutable where possible makes the code easier to understand, easing development and maintenance.
Finally, because values are injected into fields after the object has been constructed, they cannot be used to initialize other non-injected fields inline.
This rule raises an issue when the @Autowired or @Inject annotations are used on a field.
JUnit5 test classes and methods should have default package visibility
JUnit5 test classes and methods should have default package visibility
JUnit5 is more tolerant regarding the visibility of test classes and methods than JUnit4, which required everything to be public. Test classes and methods can have any visibility except private. It is however recommended to use the default package visibility to improve readability.
— JUnit5 User Guide
Methods with Spring proxy should not be called via this
Methods with Spring proxy should not be called via this
A method annotated with Spring’s @Async or @Transactional annotations will not work as expected if invoked directly from within its class.
This is because Spring generates a proxy class with wrapper code to manage the method’s asynchronicity (@Async) or to handle the transaction (@Transactional). However, when called using this, the proxy instance is bypassed, and the method is invoked directly without the required wrapper code.
Loops should not be infinite
Loops should not be infinite
An infinite loop will never end while the program runs, meaning you have to kill the program to get out of the loop. Every loop should have an end condition, whether by meeting the loop’s termination condition or via a break statement.
@Qualifier should not be used on @Bean methods
@Qualifier should not be used on @Bean methods
In Spring Framework, the @Qualifier annotation is typically used to disambiguate between multiple beans of the same type when auto-wiring dependencies. It is not necessary to use @Qualifier when defining a bean using the @Bean annotation because the bean’s name can be explicitly specified using the name attribute or derived from the method name. Using @Qualifier on @Bean methods can lead to confusion and redundancy. Beans should be named appropriately using either the name attribute of the @Bean annotation or the method name itself.
Types used as keys in Maps should implement Comparable
Types used as keys in Maps should implement Comparable
Maps use hashes of the keys to select a bucket to store data in. Objects that hash to the same value will be added to the same bucket.
When the hashing function has a poor distribution, buckets can grow to very large sizes. This may negatively affect lookup performance, as, by default, matching a key within a bucket has linear complexity.
In addition, as the default hashCode function can be selected at runtime, performance expectations cannot be maintained.
Implementing Comparable mitigates the performance issue for objects that hash to the same value.
IllegalMonitorStateException should not be caught
IllegalMonitorStateException should not be caught
The IllegalMonitorStateException is an exception that occurs when a thread tries to perform an operation on an object’s monitor that it does not own. This exception is typically thrown when a method like wait(), notify(), or notifyAll() is called outside a synchronized block or method.
IllegalMonitorStateException is specifically designed to be an unchecked exception to point out a programming mistake. This exception serves as a reminder for developers to rectify their code by correctly acquiring and releasing locks using synchronized blocks or methods. It also emphasizes the importance of calling monitor-related methods on the appropriate objects to ensure proper synchronization.
Catching and handling this exception can mask underlying synchronization issues and lead to unpredictable behavior.
Tests should use fixed data instead of randomized data
Tests should use fixed data instead of randomized data
Tests should always:
Make sure that production code behaves as expected, including edge cases.
Be easy to debug, i.e. understandable and reproducible.
Using random values in tests will not necessarily check edge cases, and it will make test logs a lot harder to read. It is better to use easily readable hardcoded values. If this makes your code bigger you can use helper functions.
There is one valid use case for random data in tests: when testing every value would make tests impractically slow. In this case the best you can do is use random to test every value on the long run. You should however make sure that random values are logged so that you can reproduce failures. Some libraries exist to make all this easier. You can for example use property-based testing libraries such as jqwik.
This rule raises an issue when new Random() or UUID.randomUUID() are called in test code.
Double-checked locking should not be used
Double-checked locking should not be used
Double-checked locking is the practice of checking a lazy-initialized object’s state both before and after a synchronized block is entered to determine whether to initialize the object. In early JVM versions, synchronizing entire methods was not performant, which sometimes caused this practice to be used in its place.
Apart from float and int types, this practice does not work reliably in a platform-independent manner without additional synchronization of mutable instances. Using double-checked locking for the lazy initialization of any other type of primitive or mutable object risks a second thread using an uninitialized or partially initialized member while the first thread is still creating it. The results can be unexpected, potentially even causing the application to crash.
Class.isAssignableFrom should not be used to check object type
Class.isAssignableFrom should not be used to check object type
To check the type of an object there are at least two options:
The simplest and shortest one with help of the `instanceof operator
The cumbersome and error-prone one with help of the Class.isAssignableFrom(…)` method
Cycles during static initialization should be removed
Cycles during static initialization should be removed
When a cycle exists between classes during their static initialization, the results can be unpredictable because they depend on which class was initialized first.
Transient fields should be restored
Transient fields should be restored
`transient fields are ignored by Java’s automatic serizalization mechanisms, which means that when a object is deserialized, those fields will be set to their default values.
transient fields that are referenced in multiple places in a class seem to play a significant role in that class. Allowing such significant fields to be left in a default state after deserialization may not be the best course, so they should probably be set in either a readObject() or readResolve()` method.
Getters and setters should be synchronized in pairs
Getters and setters should be synchronized in pairs
A synchronized method is a method marked with the synchronized keyword, meaning it can only be accessed by one thread at a time. If multiple threads try to access the synchronized method simultaneously, they will be blocked until the method is available.
Synchronized methods prevent race conditions and data inconsistencies in multi-threaded environments. Ensuring that only one thread can access a method at a time, prevents multiple threads from modifying the same data simultaneously, and causing conflicts.
When one part of a getter/setter pair is synchronized the other should be too. Failure to synchronize both sides may result in inconsistent behavior at runtime as callers access an inconsistent method state.
This rule raises an issue when either the method or the contents of one method in a getter/setter pair are synchronized, but the other is not.
Class.forName() should not load JDBC 4.0+ drivers
Class.forName() should not load JDBC 4.0+ drivers
In the past, it was required to load a JDBC driver before creating a `java.sql.Connection. Nowadays, when using JDBC 4.0 drivers, this is no longer required and Class.forName() can be safely removed because JDBC 4.0 (JDK 6) drivers available in the classpath are automatically loaded.
This rule raises an issue when Class.forName() is used with one of the following values:
com.mysql.jdbc.Driver
oracle.jdbc.driver.OracleDriver
com.ibm.db2.jdbc.app.DB2Driver
com.ibm.db2.jdbc.net.DB2Driver
com.sybase.jdbc.SybDriver
com.sybase.jdbc2.jdbc.SybDriver
com.teradata.jdbc.TeraDriver
com.microsoft.sqlserver.jdbc.SQLServerDriver
org.postgresql.Driver
sun.jdbc.odbc.JdbcOdbcDriver
org.hsqldb.jdbc.JDBCDriver
org.h2.Driver
org.firebirdsql.jdbc.FBDriver
net.sourceforge.jtds.jdbc.Driver
com.ibm.db2.jcc.DB2Driver`
Springs ModelAndViewAssert assertions should be used instead of other assertions
Springs ModelAndViewAssert assertions should be used instead of other assertions
The Spring framework comes with dedicated classes to help writing better and simpler unit tests. In particular, when testing applications built on top of Spring MVC, it is recommended to use Spring’s `ModelAndViewAssert assertions class, instead of manually testing MVC’s properties.
This rule raises an issue when Spring’s ModelAndViewAssert` assertions should be used instead of manual testing.
Web applications should not have a main method
Web applications should not have a main method
There is no reason to have a `main method in a web application. It may have been useful for debugging during application development, but such a method should never make it into production. Having a main method in a web application opens a door to the application logic that an attacker may never be able to reach (but watch out if one does!), but it is a sloppy practice and indicates that other problems may be present.
This rule raises an issue when a main` method is found in a servlet or an EJB.
FlushMode.AUTO should be used
FlushMode.AUTO should be used
By default, Hibernate session flushing is set to FlushMode.AUTO, and is called from Transaction.commit, Session.flush and before some queries are executed. Setting it to FlushMode.COMMIT, FlushMode.NEVER, or FlushMode.MANUAL could mean that parts of your application get stale data, so you should be sure of what you’re doing before you use any of these modes.
This rule raises an issue when flush mode is explicitly set to any of these modes.
Thread.ofVirtual().start(Runnable task) method chain should not be used
Thread.ofVirtual().start(Runnable task) method chain should not be used
While this can be useful, whenever we want to instantiate and start an unnamed virtual thread, there is a more convenient static method to do so: Thread.startVirtualThread(Runnable task)
This rule raises an issue every time the form Thread.ofVirtual().start(task); is found.
Deprecated annotations should include explanations
Deprecated annotations should include explanations
Since Java 9, `@Deprecated has two additional arguments to the annotation:
since allows you to describe when the deprecation took place
forRemoval, indicates whether the deprecated element will be removed at some future date
In order to ease the maintainers work, it is recommended to always add one or both of these arguments.
This rule reports an issue when @Deprecated` is used without any argument.
Using Struts 1 ActionForm is security-sensitive
Using Struts 1 ActionForm is security-sensitive
rm is security-sensitive. For example, their use has led in the past to the following vulnerability:
All classes extending org.apache.struts.action.Action are potentially remotely reachable. The ActionForm object provided as a parameter of the execute method is automatically instantiated and populated with the HTTP parameters. One should review the use of these parameters to be sure they are used safely.
Brackets for an array return type should not appear at the end of the method signature
Brackets for an array return type should not appear at the end of the method signature
The array brackets ([]) for methods that return arrays may appear either immediately after the array type or after the list of parameters. Both styles will compile, but placing array brackets at the end of the method signature is deprecated, and retained in the language specification only for backward compatibility.
Additionally, placing the array brackets at the end is far less readable than keeping the brackets with the return type. Therefore, this style should be found only in legacy code, never in new code.
Iterator.hasNext() should not call Iterator.next()
Iterator.hasNext() should not call Iterator.next()
The hasNext method of an Iterator should only report on the state of the iterator, not change it. Making a change to an iterator in its hasNext method violates all expectations of what the method will do, and almost guarantees bad results when the iterator class is used.
Hard-coded passwords are security-sensitive
Hard-coded passwords are security-sensitive
xtract strings from an application source code or binary, passwords should not be hard-coded. This is particularly true for applications that are distributed or that are open-source.
In the past, it has led to the following vulnerabilities:
Passwords should be stored outside of the code in a configuration file, a database, or a password management service.
This rule flags instances of hard-coded passwords used in database and LDAP connections. It looks for hard-coded passwords in connection strings, and for variable names that match any of the patterns from the provided list.
Double Brace Initialization should not be used
Double Brace Initialization should not be used
Because Double Brace Initialization (DBI) creates an anonymous class with a reference to the instance of the owning object, its use can lead to memory leaks if the anonymous inner class is returned and held by other objects. Even when there’s no leak, DBI is so obscure that it’s bound to confuse most maintainers.
For collections, use Arrays.asList instead, or explicitly add each item directly to the collection.
Equals method should be overridden in records containing array fields
Equals method should be overridden in records containing array fields
In records, the default behavior of the `equals() method is to check the equality by field values. This works well for primitive fields or fields, whose type overrides equals(), but this behavior doesn’t work as expected for array fields.
By default, array fields are compared by their reference, and overriding equals() is highly appreciated to achieve the deep equality check. The same strategy applies to hashCode() and toString() methods.
This rule reports an issue if a record class has an array field and is not overriding equals(), hashCode() or toString()` methods.
Hibernates default connection pool should not be used
Hibernates default connection pool should not be used
According to Hibernate’s documentation:
This rule raises an issue when a hibernate.connection.pool_size value is found in a hibernate.cfg.xml or hibernate.properties file.
switch statements and expressions should not be nested
switch statements and expressions should not be nested
Nested `switch structures are difficult to understand because you can easily confuse the cases of an inner switch as belonging to an outer statement or expression. Therefore nested switch statements and expressions should be avoided.
Specifically, you should structure your code to avoid the need for nested switch statements or expressions, but if you cannot, then consider moving the inner switch` to another method.
Lambdas should not invoke other lambdas synchronously
Lambdas should not invoke other lambdas synchronously
Invoking other Lambdas synchronously from a Lambda is a scalability anti-pattern. Lambdas have a maximum execution time before they timeout (15 minutes as of May 2021). Having to wait for another Lambda to finish its execution could lead to a timeout.
A better solution is to generate events that can be consumed asynchronously by other Lambdas.
Only static class initializers should be used
Only static class initializers should be used
Non-static initializers, also known as instance initializers, are blocks of code within a class that are executed when an instance of the class is created. They are executed when an object of the class is created just before the constructor is called. Non-static initializers are useful when you want to perform some common initialization logic for all objects of a class. They allow you to initialize instance variables in a concise and centralized manner, without having to repeat the same initialization code in each constructor.
While non-static initializers may have some limited use cases, they are rarely used and can be confusing for most developers because they only run when new class instances are created.
for loop increment clauses should modify the loops counters
for loop increment clauses should modify the loops counters
The counter of a for loop should be updated in the loop’s increment clause. The purpose of a for loop is to iterate over a range using a counter variable. It should not be used for other purposes, and alternative loops should be used in those cases.
If the counter is not updated, the loop will be infinite with a constant counter variable. If this is intentional, use a while or do while loop instead of a for loop.
If the counter variable is updated within the loop’s body, try to move it to the increment clause. If this is impossible due to certain conditions, replace the for loop with a while or do while loop.
Try-with-resources should be used
Try-with-resources should be used
Many resources in Java need be closed after they have been used. If they are not, the garbage collector cannot reclaim the resources’ memory, and they are still considered to be in use by the operating system. Such resources are considered to be leaked, which can lead to performance issues.
Java 7 introduced the try-with-resources statement, which guarantees that the resource in question will be closed.
JUnit4 @Ignored and JUnit5 @Disabled annotations should be used to disable tests and should provide a rationale
JUnit4 @Ignored and JUnit5 @Disabled annotations should be used to disable tests and should provide a rationale
When a test fails due, for example, to infrastructure issues, you might want to ignore it temporarily. But without some kind of notation about why the test is being ignored, it may never be reactivated. Such tests are difficult to address without comprehensive knowledge of the project, and end up polluting their projects.
This rule raises an issue for each ignored test that does not have any comment about why it is being skipped.
For Junit4, this rule targets the @Ignore annotation.
For Junit5, this rule targets the @Disabled annotation.
Cases where assumeTrue(false) or assumeFalse(true) are used to skip tests are targeted as well.
JUnit5 test classes and methods should not be silently ignored
JUnit5 test classes and methods should not be silently ignored
JUnit5 is more tolerant regarding the visibilities of Test classes and methods than JUnit4, which required everything to be public. JUnit5 supports default package, public and protected visibility, even if it is recommended to use the default package visibility, which improves the readability of code.
But JUnit5 ignores without any warning:
private classes and private methods
static methods
methods returning a value without being a TestFactory
Tests should include assertions
Tests should include assertions
A test case without assertions ensures only that no exceptions are thrown. Beyond basic runnability, it ensures nothing about the behavior of the code under test.
This rule raises an exception when no assertions from any of the following known frameworks are found in a test:
AssertJ
Awaitility
EasyMock
Eclipse Vert.x
Fest 1.x and 2.x
Hamcrest
JMock
JMockit
JUnit
Mockito
Rest-assured 2.x, 3.x and 4.x
RxJava 1.x and 2.x
Selenide
Spring’s `org.springframework.test.web.servlet.ResultActions.andExpect() and org.springframework.test.web.servlet.ResultActions.andExpectAll()
Truth Framework
WireMock
Furthermore, as new or custom assertion frameworks may be used, the rule can be parametrized to define specific methods that will also be considered as assertions. No issue will be raised when such methods are found in test cases. The parameter value should have the following format <FullyQualifiedClassName>#<MethodName>, where MethodName can end with the wildcard character. For constructors, the pattern should be <FullyQualifiedClassName>#<init>.
Example: com.company.CompareToTester#compare*,com.company.CustomAssert#customAssertMethod,com.company.CheckVerifier#<init>`.
Serializable inner classes of non-serializable outer classes should be static
Serializable inner classes of non-serializable outer classes should be static
Non-static inner classes contain a reference to an instance of the outer class. Hence, serializing a non-static inner class will result in an attempt at serializing the outer class as well. If the outer class is not serializable, serialization will fail, resulting in a runtime error.
Making the inner class static (i.e., “nested”) avoids this problem, as no reference to an instance of the outer class is required. Serializing the inner class can be done independently of the outer class. Hence, inner classes implementing Serializable should be static if the outer class does not implement Serializable.
Be aware of the semantic differences between an inner class and a nested one:
an inner class can only be instantiated within the context of an instance of the outer class.
a nested (static) class can be instantiated independently of the outer class.
Inappropriate Collection calls should not be made
Inappropriate Collection calls should not be made
The java.util.Collection type and its subtypes provide methods to access and modify collections such as Collection.remove(Object o) and Collection.contains(Object o). Some of these methods accept arguments of type java.lang.Object and will compare said argument with objects already in the collection.
If the actual type of the argument is unrelated to the type of object contained in the collection, these methods will always return false, null, or -1. This behavior is most likely unintended and can be indicative of a design issue.
This rule raises an issue when the type of the argument provided to one of the following methods is unrelated to the type used for the collection declaration:
Collection.remove(Object o)
Collection.removeAll(Collection<?>)
Collection.contains(Object o)
List.indexOf(Object o)
List.lastIndexOf(Object o)
Map.containsKey(Object key)
Map.containsValue(Object value)
Map.get(Object key)
Map.getOrDefault(Object key, V defaultValue)
Map.remove(Object key)
Map.remove(Object key, Object value)
compareTo should not be overloaded
compareTo should not be overloaded
When implementing the `Comparable<T>.compareTo method, the parameter’s type has to match the type used in the Comparable declaration. When a different type is used this creates an overload instead of an override, which is unlikely to be the intent.
This rule raises an issue when the parameter of the compareTo method of a class implementing Comparable<T> is not same as the one used in the Comparable` declaration.
Parameters and return values of methods in @Remote interfaces should be Serializable
Parameters and return values of methods in @Remote interfaces should be Serializable
The @Remote annotation indicates that an interface may be called from a remote client. Therefore the parameters and return types of methods in the interface must be Serializable.
Jump statements should not occur in finally blocks
Jump statements should not occur in finally blocks
Using `return, break, throw, and so on from a finally block suppresses the propagation of any unhandled Throwable which was thrown in the try or catch block.
This rule raises an issue when a jump statement (break, continue, return, throw, and goto) would force control flow to leave a finally` block.
JUnit5 inner test classes should be annotated with @Nested
JUnit5 inner test classes should be annotated with @Nested
If not annotated with `@Nested, an inner class containing some tests will never be executed during tests execution. While you could still be able to manually run its tests in an IDE, it won’t be the case during the build. By contrast, a static nested class containing some tests should not be annotated with @Nested, JUnit5 will not share setup and state with an instance of its enclosing class.
This rule raises an issue on inner classes and static nested classes containing JUnit5 test methods which has a wrong usage of @Nested` annotation.
Note: This rule does not check if the context in which JUnit 5 is running (e.g. Maven Surefire Plugin) is properly configured to execute static nested classes, it could not be the case using the default configuration.
Use switch instead of if-else chain to compare a variable against multiple cases
Use switch instead of if-else chain to compare a variable against multiple cases
Comparing a variable to multiple cases is a frequent operation. This can be done using a sequence of if-else statements. However, for many cases like enums or simple value comparisons, a switch statement is the better alternative. With Java 21, the switch statement has been significantly improved to support pattern matching and record pattern.
Using a switch statement instead of an if-else chain provides benefits like clearer code, certainty of covering all cases, and may even improve performance.
This rule raises an issue when an if-else chain should be replaced by a switch statement.
Static non-final field names should comply with a naming convention
Static non-final field names should comply with a naming convention
The Java Language Specification defines a set of rules called naming conventions that apply to Java programs. These conventions provide recommendations for naming packages, classes, methods, and variables.
By following shared naming conventions, teams can collaborate more efficiently.
This rule checks that static non-final field names match a provided regular expression.
Double.longBitsToDouble should take long as argument
Double.longBitsToDouble should take long as argument
Double.longBitsToDouble converts the bit pattern into its corresponding floating-point representation. The method expects a 64-bit long argument to interpret the bits as a double value correctly.
When the argument is a smaller data type, the cast to long may lead to a different value than expected due to the interpretation of the most significant bit, which, in turn, results in Double.longBitsToDouble returning an incorrect value.
@EnableAutoConfiguration should be fine-tuned
@EnableAutoConfiguration should be fine-tuned
”@EnableAutoConfiguration” is a convenient feature to configure the Spring Application Context by attempting to guess the beans that you are likely to need. The drawback is that it may load and configure beans the application will never use and therefore consume more CPU and RAM than really required. `@EnableAutoConfiguration should be configured to exclude all the beans not required by the application. Alternatively, use the @Import annotation instead of @EnableAutoConfiguration, to explicitly import the useful AutoConfiguration classes.
This rule applies for @SpringBootApplication` as well.
super.finalize() should be called at the end of Object.finalize() implementations
super.finalize() should be called at the end of Object.finalize() implementations
Overriding the `Object.finalize() method must be done with caution to dispose some system resources.
Calling the super.finalize()` at the end of this method implementation is highly recommended in case parent implementations must also dispose some system resources.
Locks should be released on all paths
Locks should be released on all paths
If a lock is acquired and released within a method, then it must be released along all execution paths of that method.
Failing to do so will expose the conditional locking logic to the method’s callers and hence be deadlock-prone.
Exposing HTTP endpoints is security-sensitive
Exposing HTTP endpoints is security-sensitive
Exposing HTTP endpoints is security-sensitive. It has led in the past to the following vulnerabilities:
HTTP endpoints are webservices’ main entrypoint. Attackers will take advantage of any vulnerability by sending crafted inputs for headers (including cookies), body and URI. No input should be trusted and extreme care should be taken with all returned value (header, body and status code).
This rule flags code which creates HTTP endpoint. It guides security code reviews to security-sensitive code.
Regular expressions should not be too complicated
Regular expressions should not be too complicated
Overly complicated regular expressions are hard to read and to maintain and can easily cause hard-to-find bugs. If a regex is too complicated, you should consider replacing it or parts of it with regular code or splitting it apart into multiple patterns at least.
The complexity of a regular expression is determined as follows:
Each of the following operators increases the complexity by an amount equal to the current nesting level and also increases the current nesting level by one for its arguments:
`| - when multiple | operators are used together, the subsequent ones only increase the complexity by 1
&& (inside character classes) - when multiple && operators are used together, the subsequent ones only increase the complexity by 1
Quantifiers (*, +, ?, {n,m}, {n,} or {n})
Non-capturing groups that set flags (such as (?i:some_pattern) or (?i)some_pattern`)
Lookahead and lookbehind assertions
Additionally, each use of the following features increase the complexity by 1 regardless of nesting:
character classes
back references
If a regular expression is split among multiple variables, the complexity is calculated for each variable individually, not for the whole regular expression. If a regular expression is split over multiple lines, each line is treated individually if it is accompanied by a comment (either a Java comment or a comment within the regular expression), otherwise the regular expression is analyzed as a whole.
Jump statements should not be redundant
Jump statements should not be redundant
Jump statements such as return and continue let you change the default flow of program execution, but jump statements that direct the control flow to the original direction are just a waste of keystrokes.
Inappropriate casts should not be made
Inappropriate casts should not be made
Inappropriate casts are errors that will lead to bugs as the members are accessed. This includes casts from one unrelated type to another, as well as untested casts down an inheritance hierarchy.
Using unencrypted databases in mobile applications is security-sensitive
Using unencrypted databases in mobile applications is security-sensitive
Storing data locally is a common task for mobile applications. Such data includes preferences or authentication tokens for external services, among other things. There are many convenient solutions that allow storing data persistently, for example SQLiteDatabase, SharedPreferences, and Realm. By default these systems store the data unencrypted, thus an attacker with physical access to the device can read them out easily. Access to sensitive data can be harmful for the user of the application, for example when the device gets stolen.
Regular expressions should be syntactically valid
Regular expressions should be syntactically valid
Regular expressions have their own syntax that is understood by regular expression engines. Those engines will throw an exception at runtime if they are given a regular expression that does not conform to that syntax.
To avoid syntax errors, special characters should be escaped with backslashes when they are intended to be matched literally and references to capturing groups should use the correctly spelled name or number of the group.
Using publicly writable directories is security-sensitive
Using publicly writable directories is security-sensitive
Operating systems have global directories where any user has write access. Those folders are mostly used as temporary storage areas like `/tmp in Linux based systems. An application manipulating files from these folders is exposed to race conditions on filenames: a malicious user can try to create a file with a predictable name before the application does. A successful attack can result in other files being accessed, modified, corrupted or deleted. This risk is even higher if the application runs with elevated permissions.
In the past, it has led to the following vulnerabilities:
This rule raises an issue whenever it detects a hard-coded path to a publicly writable directory like /tmp (see examples bellow). It also detects access to environment variables that point to publicly writable directories, e.g., TMP and TMPDIR.
/tmp
/var/tmp
/usr/tmp
/dev/shm
/dev/mqueue
/run/lock
/var/run/lock
/Library/Caches
/Users/Shared
/private/tmp
/private/var/tmp
\Windows\Temp
\Temp
\TMP`
Test classes should comply with a naming convention
Test classes should comply with a naming convention
Shared naming conventions allow teams to collaborate efficiently. This rule raises an issue when a test class name does not match the provided regular expression.
Enabling file access for WebViews is security-sensitive
Enabling file access for WebViews is security-sensitive
WebViews can be used to display web content as part of a mobile application. A browser engine is used to render and display the content. Like a web application, a mobile application that uses WebViews can be vulnerable to Cross-Site Scripting if untrusted code is rendered.
If malicious JavaScript code in a WebView is executed this can leak the contents of sensitive files when access to local files is enabled.
Non-capturing groups without quantifier should not be used
Non-capturing groups without quantifier should not be used
Sub-patterns can be wrapped by parentheses to build a group. This enables to restrict alternations, back reference the group or apply quantifier to the sub-pattern.
If this group should not be part of the match result or if no reference to this group is required, a non-capturing group can be created by adding ?: behind the opening parenthesis.
However, if this non-capturing group does not have a quantifier, or does not wrap an alternation, then imaging this group is redundant.
Superfluous curly brace quantifiers should be avoided
Superfluous curly brace quantifiers should be avoided
Curly brace quantifiers in regular expressions can be used to have a more fine-grained control over how many times the character or the sub-expression preceeding them should occur. They can be used to match an expression exactly n times with `{n}, between n and m times with {n,m}, or at least n times with {n,}. In some cases, using such a quantifier is superfluous for the semantic of the regular expression, and it can be removed to improve readability. This rule raises an issue when one of the following quantifiers is encountered:
{1,1} or {1}: they match the expression exactly once. The same behavior can be achieved without the quantifier.
{0,0} or {0}`: they match the expression zero times. The same behavior can be achieved by removing the expression.
Using slow regular expressions is security-sensitive
Using slow regular expressions is security-sensitive
Most of the regular expression engines use backtracking to try all possible execution paths of the regular expression when evaluating an input, in some cases it can cause performance issues, called catastrophic backtracking situations. In the worst case, the complexity of the regular expression is exponential in the size of the input, this means that a small carefully-crafted input (like 20 chars) can trigger catastrophic backtracking and cause a denial of service of the application. Super-linear regex complexity can lead to the same impact too with, in this case, a large carefully-crafted input (thousands chars).
This rule determines the runtime complexity of a regular expression and informs you of the complexity if it is not linear.
Note that, due to improvements to the matching algorithm, some cases of exponential runtime complexity have become impossible when run using JDK 9 or later. In such cases, an issue will only be reported if the project’s target Java version is 8 or earlier.
Expressions should not be too complex
Expressions should not be too complex
The complexity of an expression is defined by the number of &&, || and condition ? ifTrue : ifFalse operators it contains.
A single expression’s complexity should not become too high to keep the code readable.
Disabling CSRF protections is security-sensitive
Disabling CSRF protections is security-sensitive
A cross-site request forgery (CSRF) attack occurs when a trusted user of a web application can be forced, by an attacker, to perform sensitive actions that he didn’t intend, such as updating his profile or sending a message, more generally anything that can change the state of the application.
The attacker can trick the user/victim to click on a link, corresponding to the privileged action, or to visit a malicious web site that embeds a hidden web request and as web browsers automatically include cookies, the actions can be authenticated and sensitive.
Unused type parameters should be removed
Unused type parameters should be removed
Type parameters that aren’t used are dead code, which can only distract and possibly confuse developers during maintenance. Therefore, unused type parameters should be removed.
Searching OS commands in PATH is security-sensitive
Searching OS commands in PATH is security-sensitive
When executing an OS command and unless you specify the full path to the executable, then the locations in your application’s PATH environment variable will be searched for the executable. That search could leave an opening for an attacker if one of the elements in PATH is a directory under his control.
Math operands should be cast before assignment
Math operands should be cast before assignment
When arithmetic is performed on integers, the result will always be an integer. You can assign that result to a `long, double, or float with automatic type conversion, but having started as an int or long, the result will likely not be what you expect.
For instance, if the result of int division is assigned to a floating-point variable, precision will have been lost before the assignment. Likewise, if the result of multiplication is assigned to a long`, it may have already overflowed before the assignment.
In either case, the result will not be what was expected. Instead, at least one operand should be cast or promoted to the final type before the operation takes place.
Having a permissive Cross-Origin Resource Sharing policy is security-sensitive
Having a permissive Cross-Origin Resource Sharing policy is security-sensitive
Having a permissive Cross-Origin Resource Sharing policy is security-sensitive. It has led in the past to the following vulnerabilities:
Same origin policy in browsers prevents, by default and for security-reasons, a javascript frontend to perform a cross-origin HTTP request to a resource that has a different origin (domain, protocol, or port) from its own. The requested target can append additional HTTP headers in response, called CORS, that act like directives for the browser and change the access control policy / relax the same origin policy.
Loops should not contain more than a single break or continue statement
Loops should not contain more than a single break or continue statement
The use of break and continue statements increases the complexity of the control flow and makes it harder to understand the program logic. In order to keep a good program structure, they should not be applied more than once per loop.
This rule reports an issue when there is more than one break or continue statement in a loop. The code should be refactored to increase readability if there is more than one.
Character classes in regular expressions should not contain only one character
Character classes in regular expressions should not contain only one character
Character classes in regular expressions are a convenient way to match one of several possible characters by listing the allowed characters or ranges of characters. If a character class contains only one character, the effect is the same as just writing the character without a character class.
Thus, having only one character in a character class is usually a simple oversight that remained after removing other characters of the class.
Local variables should not be declared and then immediately returned or thrown
Local variables should not be declared and then immediately returned or thrown
Declaring a variable only to immediately return or throw it is considered a bad practice because it adds unnecessary complexity to the code. This practice can make the code harder to read and understand, as it introduces an extra step that doesn’t add any value. Instead of declaring a variable and then immediately returning or throwing it, it is generally better to return or throw the value directly. This makes the code cleaner, simpler, and easier to understand.
Comparisons should only be made in the context of boolean expressions
Comparisons should only be made in the context of boolean expressions
The use of a comparison operator outside of a boolean context is an error. At best it is meaningless code, and should be eliminated. However the far more likely scenario is that it is an assignment gone wrong, and should be corrected.
Delivering code in production with debug features activated is security-sensitive
Delivering code in production with debug features activated is security-sensitive
Development tools and frameworks usually have options to make debugging easier for developers. Although these features are useful during development, they should never be enabled for applications deployed in production. Debug instructions or error messages can leak detailed information about the system, like the application’s path or file names.
Standard outputs should not be used directly to log anything
Standard outputs should not be used directly to log anything
In software development, logs serve as a record of events within an application, providing crucial insights for debugging. When logging, it is essential to ensure that the logs are:
easily accessible
uniformly formatted for readability
properly recorded
securely logged when dealing with sensitive data
Those requirements are not met if a program directly writes to the standard outputs (e.g., {language_std_outputs}). That is why defining and using a dedicated logger is highly recommended.
Type parameters should not shadow other type parameters
Type parameters should not shadow other type parameters
Shadowing makes it impossible to use the type parameter from the outer scope. Also, it can be confusing to distinguish which type parameter is being used.
This rule raises an issue when a type parameter from an inner scope uses the same name as one in an outer scope.
Magic numbers should not be used
Magic numbers should not be used
Magic numbers make the code more complex to understand as it requires the reader to have knowledge about the global context to understand the number itself. Their usage may seem obvious when writing the code, but it may not be the case for another developer or later once the context faded away. -1, 0, and 1 are not considered magic numbers.
Deprecated code should be removed
Deprecated code should be removed
This rule is meant to be used as a way to track code which is marked as being deprecated. Deprecated code should eventually be removed.
Octal values should not be used
Octal values should not be used
Integer literals starting with a zero are octal rather than decimal values. While using octal values is fully supported, most developers do not have experience with them. They may not recognize octal values as such, mistaking them instead for decimal values.
Using pseudorandom number generators (PRNGs) is security-sensitive
Using pseudorandom number generators (PRNGs) is security-sensitive
Using pseudorandom number generators (PRNGs) is security-sensitive. For example, it has led in the past to the following vulnerabilities:
When software generates predictable values in a context requiring unpredictability, it may be possible for an attacker to guess the next value that will be generated, and use this guess to impersonate another user or access sensitive information.
Hard-coded secrets are security-sensitive
Hard-coded secrets are security-sensitive
Because it is easy to extract strings from an application source code or binary, secrets should not be hard-coded. This is particularly true for applications that are distributed or that are open-source.
In the past, it has led to the following vulnerabilities:
Secrets should be stored outside of the source code in a configuration file or a management service for secrets.
This rule detects variables/fields having a name matching a list of words (secret, token, credential, auth, api[_.-]?key) being assigned a pseudorandom hard-coded value. The pseudorandomness of the hard-coded value is based on its entropy and the probability to be human-readable. The randomness sensibility can be adjusted if needed. Lower values will detect less random values, raising potentially more false positives.
Exceptions should not be ignored
Exceptions should not be ignored
When exceptions occur, it is usually a bad idea to simply ignore them. Instead, it is better to handle them properly, or at least to log them.
Utility classes should not have public constructors
Utility classes should not have public constructors
Whenever there are portions of code that are duplicated and do not depend on the state of their container class, they can be centralized inside a “utility class”. A utility class is a class that only has static members, hence it should not be instantiated.
Expanding archive files without controlling resource consumption is security-sensitive
Expanding archive files without controlling resource consumption is security-sensitive
Successful Zip Bomb attacks occur when an application expands untrusted archive files without controlling the size of the expanded data, which can lead to denial of service. A Zip bomb is usually a malicious archive file of a few kilobytes of compressed data but turned into gigabytes of uncompressed data. To achieve this extreme compression ratio, attackers will compress irrelevant data (eg: a long string of repeated bytes).
Allowing both safe and unsafe HTTP methods is security-sensitive
Allowing both safe and unsafe HTTP methods is security-sensitive
An HTTP method is safe when used to perform a read-only operation, such as retrieving information. In contrast, an unsafe HTTP method is used to change the state of an application, for instance to update a user’s profile on a web application.
Common safe HTTP methods are GET, HEAD, or OPTIONS.
Common unsafe HTTP methods are POST, PUT and DELETE.
Allowing both safe and unsafe HTTP methods to perform a specific operation on a web application could impact its security, for example CSRF protections are most of the time only protecting operations performed by unsafe HTTP methods.
Mergeable if statements should be combined
Mergeable if statements should be combined
Nested code - blocks of code inside blocks of code - is eventually necessary, but increases complexity. This is why keeping the code as flat as possible, by avoiding unnecessary nesting, is considered a good practice.
Merging if statements when possible will decrease the nesting of the code and improve its readability.
Methods should not be empty
Methods should not be empty
An empty {operationName} is generally considered bad practice and can lead to confusion, readability, and maintenance issues. Empty {operationName}s bring no functionality and are misleading to others as they might think the {operationName} implementation fulfills a specific and identified requirement.
There are several reasons for a {operationName} not to have a body:
It is an unintentional omission, and should be fixed to prevent an unexpected behavior in production.
It is not yet, or never will be, supported. In this case an exception should be thrown.
The method is an intentionally-blank override. In this case a nested comment should explain the reason for the blank override.
Empty lines should not be tested with regex MULTILINE flag
Empty lines should not be tested with regex MULTILINE flag
One way to test for empty lines is to use the regex `”^$”, which can be extremely handy when filtering out empty lines from collections of Strings, for instance. With regard to this, the Javadoc for Pattern (Line Terminators) states the following:
As emphasized, ^ is not going to match at the end of an input, and the end of the input is necessarily included in the empty string, which might lead to completely missing empty lines, while it would be the initial reason for using such regex.
Therefore, when searching for empty lines using a multi-line regular expression, you should also check whether the string is empty.
This rule is raising an issue every time a pattern that can match the empty string is used with MULTILINE flag and without calling isEmpty()` on the string.
Literal suffixes should be upper case
Literal suffixes should be upper case
Using upper case literal suffixes removes the potential ambiguity between “1” (digit 1) and “l” (letter el) for declaring literals.
Regex lookahead assertions should not be contradictory
Regex lookahead assertions should not be contradictory
Lookahead assertions are a regex feature that makes it possible to look ahead in the input without consuming it. It is often used at the end of regular expressions to make sure that substrings only match when they are followed by a specific pattern.
For example, the following pattern will match an “a” only if it is directly followed by a “b”. This does not consume the “b” in the process:
Unresolved directive in <stdin> - include::{lookahead}[]
However, lookaheads can also be used in the middle (or at the beginning) of a regex. In that case there is the possibility that what comes after the lookahead contradicts the pattern inside the lookahead. Since the lookahead does not consume input, this makes the lookahead impossible to match and is a sign that there’s a mistake in the regular expression that should be fixed.
Using clear-text protocols is security-sensitive
Using clear-text protocols is security-sensitive
Clear-text protocols such as `ftp, telnet, or http lack encryption of transported data, as well as the capability to build an authenticated connection. It means that an attacker able to sniff traffic from the network can read, modify, or corrupt the transported content. These protocols are not secure as they expose applications to an extensive range of risks:
sensitive data exposure
traffic redirected to a malicious endpoint
malware-infected software update or installer
execution of client-side code
corruption of critical information
Even in the context of isolated networks like offline environments or segmented cloud environments, the insider threat exists. Thus, attacks involving communications being sniffed or tampered with can still happen.
For example, attackers could successfully compromise prior security layers by:
bypassing isolation mechanisms
compromising a component of the network
getting the credentials of an internal IAM account (either from a service account or an actual person)
In such cases, encrypting communications would decrease the chances of attackers to successfully leak data or steal credentials from other network components. By layering various security practices (segmentation and encryption, for example), the application will follow the defense-in-depth principle.
Note that using the http` protocol is being deprecated by major web browsers.
In the past, it has led to the following vulnerabilities:
Creating cookies without the HttpOnly flag is security-sensitive
Creating cookies without the HttpOnly flag is security-sensitive
Array designators [] should be on the type, not the variable
Array designators [] should be on the type, not the variable
Array designators should always be located on the type for better code readability. Otherwise, developers must look both at the type and the variable name to know whether or not a variable is an array.
Unused private fields should be removed
Unused private fields should be removed
If a private field is declared but not used locally, its limited visibility makes it dead code.
This is either a sign that some logic is missing or that the code should be cleaned.
Cleaning out dead code decreases the size of the maintained codebase, making it easier to understand and preventing bugs from being introduced.
Redundant casts should not be used
Redundant casts should not be used
Casting expressions are utilized to convert one data type to another, such as transforming an integer into a string. This is especially crucial in strongly typed languages like C, C++, C#, Java, Python, and others.
However, there are instances where casting expressions are not needed. These include situations like:
casting a variable to its own type
casting a subclass to a parent class (in the case of polymorphism)
the programming language is capable of automatically converting the given type to another
These scenarios are considered unnecessary casting expressions. They can complicate the code and make it more difficult to understand, without offering any advantages.
As a result, it’s generally advised to avoid unnecessary casting expressions. Instead, rely on the language’s type system to ensure type safety and code clarity.
Regular expression quantifiers and character classes should be used concisely
Regular expression quantifiers and character classes should be used concisely
A regular expression is a sequence of characters that specifies a match pattern in text. Among the most important concepts are:
Character classes: defines a set of characters, any one of which can occur in an input string for a match to succeed.
Quantifiers: used to specify how many instances of a character, group, or character class must be present in the input for a match.
Wildcard (.): matches all characters except line terminators (also matches them if the s flag is set).
Many of these features include shortcuts of widely used expressions, so there is more than one way to construct a regular expression to achieve the same results. For example, to match a two-digit number, one could write [0-9]{2,2} or \d{2}. The latter is not only shorter but easier to read and thus to maintain.
This rule recommends replacing some quantifiers and character classes with more concise equivalents:
\d for [0-9] and \D for [^0-9]
\w for [A-Za-z0-9_] and \W for `[^A-Za-z0-9_]
. for character classes matching everything (e.g. [\w\W], [\d\D], or [\s\S] with s flag)
x? for x{0,1}, x* for x{0,}, x+ for x{1,}, x{N} for x{N,N}`
Using shell interpreter when executing OS commands is security-sensitive
Using shell interpreter when executing OS commands is security-sensitive
Arbitrary OS command injection vulnerabilities are more likely when a shell is spawned rather than a new process, indeed shell meta-chars can be used (when parameters are user-controlled for instance) to inject OS commands.
switch statements should not contain non-case labels
switch statements should not contain non-case labels
Even if it is legal, mixing case and non-case labels in the body of a switch statement is very confusing and can even be the result of a typing error.
Identical expressions should not be used on both sides of a binary operator
Identical expressions should not be used on both sides of a binary operator
Using the same value on both sides of a binary operator is a code defect. In the case of logical operators, it is either a copy/paste error and, therefore, a bug, or it is simply duplicated code and should be simplified. In the case of bitwise operators and most binary mathematical operators, having the same value on both sides of an operator yields predictable results and should be simplified as well.
Short-circuit logic should be used to prevent null pointer dereferences in conditionals
Short-circuit logic should be used to prevent null pointer dereferences in conditionals
When either the equality operator in a null test or the logical operator that follows it is reversed, the code has the appearance of safely null-testing the object before dereferencing it. Unfortunately the effect is just the opposite - the object is null-tested and then dereferenced only if it is null, leading to a guaranteed null pointer dereference.
Allowing requests with excessive content length is security-sensitive
Allowing requests with excessive content length is security-sensitive
Rejecting requests with significant content length is a good practice to control the network traffic intensity and thus resource consumption in order to prevent DoS attacks.
Assignments should not be made from within sub-expressions
Assignments should not be made from within sub-expressions
A common code smell that can hinder the clarity of source code is making assignments within sub-expressions. This practice involves assigning a value to a variable inside a larger expression, such as within a loop or a conditional statement.
This practice essentially gives a side-effect to a larger expression, thus making it less readable. This often leads to confusion and potential errors.
Related if/else if statements should not have the same condition
Related if/else if statements should not have the same condition
Regex boundaries should not be used in a way that can never be matched
Regex boundaries should not be used in a way that can never be matched
In regular expressions the boundaries `^ and \A can only match at the beginning of the input (or, in case of ^ in combination with the MULTILINE flag, the beginning of the line) and $, \Z and \z only at the end.
These patterns can be misused, by accidentally switching ^ and $` for example, to create a pattern that can never match.
Formatting SQL queries is security-sensitive
Formatting SQL queries is security-sensitive
Formatted SQL queries can be difficult to maintain, debug and can increase the risk of SQL injection when concatenating untrusted values into the query. However, this rule doesn’t detect SQL injections (unlike rule S3649), the goal is only to highlight complex/formatted queries.
Unused method parameters should be removed
Unused method parameters should be removed
A typical code smell known as unused function parameters refers to parameters declared in a function but not used anywhere within the function’s body. While this might seem harmless at first glance, it can lead to confusion and potential errors in your code. Disregarding the values passed to such parameters, the function’s behavior will be the same, but the programmer’s intention won’t be clearly expressed anymore. Therefore, removing function parameters that are not being utilized is considered best practice.
Class variable fields should not have public accessibility
Class variable fields should not have public accessibility
Public fields in public classes do not respect the encapsulation principle and have three main disadvantages:
Additional behavior such as validation cannot be added.
The internal representation is exposed, and cannot be changed afterwards.
Member values are subject to change from anywhere in the code and may not meet the programmer’s assumptions.
To prevent unauthorized modifications, private attributes and accessor methods (set and get) should be used.
Accessing Android external storage is security-sensitive
Accessing Android external storage is security-sensitive
Storing data locally is a common task for mobile applications. Such data includes files among other things. One convenient way to store files is to use the external file storage which usually offers a larger amount of disc space compared to internal storage.
Files created on the external storage are globally readable and writable. Therefore, a malicious application having the permissions WRITE_EXTERNAL_STORAGE or READ_EXTERNAL_STORAGE could try to read sensitive information from the files that other applications have stored on the external storage.
External storage can also be removed by the user (e.g when based on SD card) making the files unavailable to the application.
Unicode Grapheme Clusters should be avoided inside regex character classes
Unicode Grapheme Clusters should be avoided inside regex character classes
When placing Unicode Grapheme Clusters (characters which require to be encoded in multiple Code Points) inside a character class of a regular expression, this will likely lead to unintended behavior.
For instance, the grapheme cluster c̈ requires two code points: one for ‘c’, followed by one for the umlaut modifier ‘\u{0308}’. If placed within a character class, such as [c̈], the regex will consider the character class being the enumeration [c\u{0308}] instead. It will, therefore, match every ‘c’ and every umlaut that isn’t expressed as a single codepoint, which is extremely unlikely to be the intended behavior.
This rule raises an issue every time Unicode Grapheme Clusters are used within a character class of a regular expression.
Package names should comply with a naming convention
Package names should comply with a naming convention
Shared naming conventions improve readability and allow teams to collaborate efficiently. This rule checks that all package names match a provided regular expression.
Constructing arguments of system commands from user input is security-sensitive
Constructing arguments of system commands from user input is security-sensitive
Constructing arguments of system commands from user input is security-sensitive. It has led in the past to the following vulnerabilities:
Arguments of system commands are processed by the executed program. The arguments are usually used to configure and influence the behavior of the programs. Control over a single argument might be enough for an attacker to trigger dangerous features like executing arbitrary commands or writing files into specific directories.
URIs should not be hardcoded
URIs should not be hardcoded
Hard-coding a URI makes it difficult to test a program for a variety of reasons:
path literals are not always portable across operating systems
a given absolute path may not exist in a specific test environment
a specified Internet URL may not be available when executing the tests
production environment filesystems usually differ from the development environment
In addition, hard-coded URIs can contain sensitive information, like IP addresses, and they should not be stored in the code.
For all those reasons, a URI should never be hard coded. Instead, it should be replaced by a customizable parameter.
Further, even if the elements of a URI are obtained dynamically, portability can still be limited if the path delimiters are hard-coded.
This rule raises an issue when URIs or path delimiters are hard-coded.
if ... else if constructs should end with else clauses
if ... else if constructs should end with else clauses
This rule applies whenever an `if statement is followed by one or more else if statements; the final else if should be followed by an else statement.
The requirement for a final else statement is defensive programming.
The else statement should either take appropriate action or contain a suitable comment as to why no action is taken. This is consistent with the requirement to have a final default clause in a switch` statement.
Disabling auto-escaping in template engines is security-sensitive
Disabling auto-escaping in template engines is security-sensitive
To reduce the risk of cross-site scripting attacks, templating systems, such as `Twig, Django, Smarty, Groovy’s template engine, allow configuration of automatic variable escaping before rendering templates. When escape occurs, characters that make sense to the browser (eg: <a>) will be transformed/replaced with escaped/sanitized values (eg: & lt;a& gt; ).
Auto-escaping is not a magic feature to annihilate all cross-site scripting attacks, it depends on the strategy applied and the context, for example a “html auto-escaping” strategy (which only transforms html characters into html entities) will not be relevant when variables are used in a html attribute because ’:’ character is not escaped and thus an attack as below is possible:
<a href=”{{ myLink }}“>link</a> // myLink = javascript:alert(document.cookie)
<a href=“javascript:alert(document.cookie)“>link</a> // JS injection (XSS attack)Regular expressions should not contain multiple spaces
Regular expressions should not contain multiple spaces
Multiple spaces in a regular expression can make it hard to tell how many spaces should be matched. It’s more readable to use only one space and then indicate with a quantifier how many spaces are expected.
HTTP response headers should not be vulnerable to injection attacks
HTTP response headers should not be vulnerable to injection attacks
User-provided data, such as URL parameters, POST data payloads, or cookies, should always be considered untrusted and tainted. Applications constructing HTTP response headers based on tainted data could allow attackers to change security sensitive headers like Cross-Origin Resource Sharing headers.
Web application frameworks and servers might also allow attackers to inject new line characters in headers to craft malformed HTTP response. In this case the application would be vulnerable to a larger range of attacks like HTTP Response Splitting/Smuggling. Most of the time this type of attack is mitigated by default modern web application frameworks but there might be rare cases where older versions are still vulnerable.
As a best practice, applications that use user-provided data to construct the response header should always validate the data first. Validation should be based on a whitelist.
Receiving intents is security-sensitive
Receiving intents is security-sensitive
Android applications can receive broadcasts from the system or other applications. Receiving intents is security-sensitive. For example, it has led in the past to the following vulnerabilities:
Receivers can be declared in the manifest or in the code to make them context-specific. If the receiver is declared in the manifest Android will start the application if it is not already running once a matching broadcast is received. The receiver is an entry point into the application.
Other applications can send potentially malicious broadcasts, so it is important to consider broadcasts as untrusted and to limit the applications that can send broadcasts to the receiver.
Permissions can be specified to restrict broadcasts to authorized applications. Restrictions can be enforced by both the sender and receiver of a broadcast. If permissions are specified when registering a broadcast receiver, then only broadcasters who were granted this permission can send a message to the receiver.
This rule raises an issue when a receiver is registered without specifying any broadcast permission.
Creating cookies without the secure flag is security-sensitive
Creating cookies without the secure flag is security-sensitive
Unused labels should be removed
Unused labels should be removed
If a label is declared but not used in the program, it can be considered as dead code and should therefore be removed.
This will improve maintainability as developers will not wonder what this label is used for.
Using biometric authentication without a cryptographic solution is security-sensitive
Using biometric authentication without a cryptographic solution is security-sensitive
Android comes with Android KeyStore, a secure container for storing key materials. It’s possible to define certain keys to be unlocked when users authenticate using biometric credentials. This way, even if the application process is compromised, the attacker cannot access keys, as presence of the authorized user is required.
These keys can be used, to encrypt, sign or create a message authentication code (MAC) as proof that the authentication result has not been tampered with. This protection defeats the scenario where an attacker with physical access to the device would try to hook into the application process and call the `onAuthenticationSucceeded method directly. Therefore he would be unable to extract the sensitive data or to perform the critical operations protected by the biometric authentication.
Ask Yourself Whether
The application contains:
Cryptographic keys / sensitive information that need to be protected using biometric authentication.
There is a risk if you answered yes to this question.
Recommended Secure Coding Practices
It’s recommended to tie the biometric authentication to a cryptographic operation by using a CryptoObject` during authentication.
Names of regular expressions named groups should be used
Names of regular expressions named groups should be used
Why use named groups only to never use any of them later on in the code?
This rule raises issues every time named groups are:
defined but never called anywhere in the code through their name;
defined but called elsewhere in the code by their number instead;
referenced while not defined.
Control flow statements if, for, while, switch and try should not be nested too deeply
Control flow statements if, for, while, switch and try should not be nested too deeply
Nested control flow statements such as if, for, while, switch, and try are often key ingredients in creating
what’s known as “Spaghetti code”. This code smell can make your program difficult to understand and maintain.
When numerous control structures are placed inside one another, the code becomes a tangled, complex web. This significantly reduces the code’s readability and maintainability, and it also complicates the testing process.
Objects should not be created to be dropped immediately without being used
Objects should not be created to be dropped immediately without being used
There is no good reason to create a new object to not do anything with it. Most of the time, this is due to a missing piece of code and so could lead to an unexpected behavior in production.
If it was done on purpose because the constructor has side-effects, then that side-effect code should be moved into a separate method and called directly.
catch clauses should do more than rethrow
catch clauses should do more than rethrow
A catch clause that only rethrows the caught exception has the same effect as omitting the catch altogether and letting it bubble up automatically.
Unresolved directive in <stdin> - include::{example}[]
Such clauses should either be removed or populated with the appropriate logic.
Unresolved directive in <stdin> - include::{compliant}[]
Overriding methods should do more than simply call the same method in the super class
Overriding methods should do more than simply call the same method in the super class
Overriding a method just to call the same method from the super class without performing any other actions is useless and misleading. The only time this is justified is in final overriding methods, where the effect is to lock in the parent class behavior. This rule ignores such overrides of equals, hashCode and toString.
String literals should not be duplicated
String literals should not be duplicated
Duplicated string literals make the process of refactoring complex and error-prone, as any change would need to be propagated on all occurrences.
Reflection should not be used to increase accessibility of classes, methods, or fields
Reflection should not be used to increase accessibility of classes, methods, or fields
Altering or bypassing the accessibility of classes, methods, or fields through reflection violates the encapsulation principle. This can break the internal contracts of the accessed target and lead to maintainability issues and runtime errors.
This rule raises an issue when reflection is used to change the visibility of a class, method or field, and when it is used to directly update a field value.
Private fields only used as local variables in methods should become local variables
Private fields only used as local variables in methods should become local variables
When the value of a private field is always assigned to in a class’ methods before being read, then it is not being used to store class information. Therefore, it should become a local variable in the relevant methods to prevent any misunderstanding.