encryptorcode/iam-oauth

Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.

On the other hand, constants can be referenced from many places, but only need to be updated in a single place.

Noncompliant Code Example

With the default threshold of 3:

public void run() {
  prepare("action1");                              // Noncompliant - "action1" is duplicated 3 times
  execute("action1");
  release("action1");
}

@SuppressWarning("all")                            // Compliant - annotations are excluded
private void method1() { /* ... */ }
@SuppressWarning("all")
private void method2() { /* ... */ }

public String method3(String a) {
  System.out.println("'" + a + "'");               // Compliant - literal "'" has less than 5 characters and is excluded
  return "";                                       // Compliant - literal "" has less than 5 characters and is excluded
}

Compliant Solution

private static final String ACTION_1 = "action1";  // Compliant

public void run() {
  prepare(ACTION_1);                               // Compliant
  execute(ACTION_1);
  release(ACTION_1);
}

Exceptions

To prevent generating some false-positives, literals having less than 5 characters are excluded.

Calling toString() or clone() on an object should always return a string or an object. Returning null instead contravenes the method's implicit contract.

Noncompliant Code Example

public String toString () {
  if (this.collection.isEmpty()) {
    return null; // Noncompliant
  } else {
    // ...

Compliant Solution

public String toString () {
  if (this.collection.isEmpty()) {
    return "";
  } else {
    // ...

See

• MITRE CWE-476 - NULL Pointer Dereference
• CERT, EXP01-J. - Do not use a null in a case where an object is required

Calling toString() or clone() on an object should always return a string or an object. Returning null instead contravenes the method's implicit contract.

Noncompliant Code Example

public String toString () {
  if (this.collection.isEmpty()) {
    return null; // Noncompliant
  } else {
    // ...

Compliant Solution

public String toString () {
  if (this.collection.isEmpty()) {
    return "";
  } else {
    // ...

See

• MITRE CWE-476 - NULL Pointer Dereference
• CERT, EXP01-J. - Do not use a null in a case where an object is required

Even if it is technically possible, Restricted Identifiers should not be used as identifiers. This is only possible for compatibility reasons, using it in Java code is confusing and should be avoided.

Note that this applies to any version of Java, including the one where these identifiers are not yet restricted, to avoid future confusion.

This rule reports an issue when restricted identifiers:

• var
• yield
• record

are used as identifiers.

Noncompliant Code Example

var var = "var"; // Noncompliant: compiles but this code is confusing
var = "what is this?";

int yield(int i) { // Noncompliant
  return switch (i) {
    case 1: yield(0); // This is a yield from switch expression, not a recursive call.
    default: yield(i-1);
  };
}

String record = "record"; // Noncompliant

Compliant Solution

var myVariable = "var";

int minusOne(int i) {
  return switch (i) {
    case 1: yield(0);
    default: yield(i-1);
  };
}

String myRecord = "record";

See

• JLS16, 3.8: Identifiers

Even if it is technically possible, Restricted Identifiers should not be used as identifiers. This is only possible for compatibility reasons, using it in Java code is confusing and should be avoided.

Note that this applies to any version of Java, including the one where these identifiers are not yet restricted, to avoid future confusion.

This rule reports an issue when restricted identifiers:

• var
• yield
• record

are used as identifiers.

Noncompliant Code Example

var var = "var"; // Noncompliant: compiles but this code is confusing
var = "what is this?";

int yield(int i) { // Noncompliant
  return switch (i) {
    case 1: yield(0); // This is a yield from switch expression, not a recursive call.
    default: yield(i-1);
  };
}

String record = "record"; // Noncompliant

Compliant Solution

var myVariable = "var";

int minusOne(int i) {
  return switch (i) {
    case 1: yield(0);
    default: yield(i-1);
  };
}

String myRecord = "record";

See

• JLS16, 3.8: Identifiers

Calling toString() or clone() on an object should always return a string or an object. Returning null instead contravenes the method's implicit contract.

Noncompliant Code Example

public String toString () {
  if (this.collection.isEmpty()) {
    return null; // Noncompliant
  } else {
    // ...

Compliant Solution

public String toString () {
  if (this.collection.isEmpty()) {
    return "";
  } else {
    // ...

See

• MITRE CWE-476 - NULL Pointer Dereference
• CERT, EXP01-J. - Do not use a null in a case where an object is required

Utility classes, which are collections of static members, are not meant to be instantiated. Even abstract utility classes, which can be extended, should not have public constructors.

Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor should be defined.

Noncompliant Code Example

class StringUtils { // Noncompliant

  public static String concatenate(String s1, String s2) {
    return s1 + s2;
  }

}

Compliant Solution

class StringUtils { // Compliant

  private StringUtils() {
    throw new IllegalStateException("Utility class");
  }

  public static String concatenate(String s1, String s2) {
    return s1 + s2;
  }

}

Exceptions

When class contains public static void main(String[] args) method it is not considered as utility class and will be ignored by this rule.

Abstract classes should not have public constructors. Constructors of abstract classes can only be called in constructors of their subclasses. So there is no point in making them public. The protected modifier should be enough.

Noncompliant Code Example

public abstract class AbstractClass1 {
    public AbstractClass1 () { // Noncompliant, has public modifier
        // do something here
    }
}

Compliant Solution

public abstract class AbstractClass2 {
    protected AbstractClass2 () {
        // do something here
    }
}

When encrypting data with the Cipher Block Chaining (CBC) mode an Initialization Vector (IV) is used to randomize the encryption, ie under a given key the same plaintext doesn't always produce the same ciphertext. The IV doesn't need to be secret but should be unpredictable to avoid "Chosen-Plaintext Attack".

To generate Initialization Vectors, NIST recommends to use a secure random number generator.

Noncompliant Code Example

public class MyCbcClass {

  public String applyCBC(String strKey, String plainText) {
    byte[] bytesIV = "7cVgr5cbdCZVw5WY".getBytes("UTF-8");

    /* KEY + IV setting */
    IvParameterSpec iv = new IvParameterSpec(bytesIV);
    SecretKeySpec skeySpec = new SecretKeySpec(strKey.getBytes("UTF-8"), "AES");

    /* Ciphering */
    Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING");
    cipher.init(Cipher.ENCRYPT_MODE, skeySpec, iv);  // Noncompliant: the IV is hard coded and thus not generated with a secure random generator
    byte[] encryptedBytes = cipher.doFinal(plainText.getBytes("UTF-8"));
    return DatatypeConverter.printBase64Binary(bytesIV)
            + ";" + DatatypeConverter.printBase64Binary(encryptedBytes);
  }
}

Compliant Solution

public class MyCbcClass {

  SecureRandom random = new SecureRandom();

  public String applyCBC(String strKey, String plainText) {
    byte[] bytesIV = new byte[16];
    random.nextBytes(bytesIV);

    /* KEY + IV setting */
    IvParameterSpec iv = new IvParameterSpec(bytesIV);
    SecretKeySpec skeySpec = new SecretKeySpec(strKey.getBytes("UTF-8"), "AES");

    /* Ciphering */
    Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING");
    cipher.init(Cipher.ENCRYPT_MODE, skeySpec, iv); // Compliant
    byte[] encryptedBytes = cipher.doFinal(plainText.getBytes("UTF-8"));
    return DatatypeConverter.printBase64Binary(bytesIV)
            + ";" + DatatypeConverter.printBase64Binary(encryptedBytes);
  }
}

See

• OWASP Top 10 2017 Category A6 - Security Misconfiguration
• MITRE, CWE-329 - CWE-329: Not Using an Unpredictable IV with CBC Mode
• MITRE, CWE-330 - Use of Insufficiently Random Values
• NIST, SP-800-38A - Recommendation for Block Cipher Modes of Operation
• Derived from FindSecBugs rule STATIC_IV

Utility classes, which are collections of static members, are not meant to be instantiated. Even abstract utility classes, which can be extended, should not have public constructors.

Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor should be defined.

Noncompliant Code Example

class StringUtils { // Noncompliant

  public static String concatenate(String s1, String s2) {
    return s1 + s2;
  }

}

Compliant Solution

class StringUtils { // Compliant

  private StringUtils() {
    throw new IllegalStateException("Utility class");
  }

  public static String concatenate(String s1, String s2) {
    return s1 + s2;
  }

}

Exceptions

When class contains public static void main(String[] args) method it is not considered as utility class and will be ignored by this rule.

Many consider clone and Cloneable broken in Java, largely because the rules for overriding clone are tricky and difficult to get right, according to Joshua Bloch:

Object's clone method is very tricky. It's based on field copies, and it's "extra-linguistic." It creates an object without calling a constructor. There are no guarantees that it preserves the invariants established by the constructors. There have been lots of bugs over the years, both in and outside Sun, stemming from the fact that if you just call super.clone repeatedly up the chain until you have cloned an object, you have a shallow copy of the object. The clone generally shares state with the object being cloned. If that state is mutable, you don't have two independent objects. If you modify one, the other changes as well. And all of a sudden, you get random behavior.

A copy constructor or copy factory should be used instead.

This rule raises an issue when clone is overridden, whether or not Cloneable is implemented.

Noncompliant Code Example

public class MyClass {
  // ...

  public Object clone() { // Noncompliant
    //...
  }
}

Compliant Solution

public class MyClass {
  // ...

  MyClass (MyClass source) {
    //...
  }
}

See

• Copy Constructor versus Cloning

See Also

• {rule:java:S2157} - "Cloneables" should implement "clone"
• {rule:java:S1182} - Classes that override "clone" should be "Cloneable" and call "super.clone()"

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.

Noncompliant Code Example

int <T> Add(int a, int b) // Noncompliant; <T> is ignored
{
  return a + b;
}

Compliant Solution

int Add(int a, int b)
{
  return a + b;
}

Even if it is technically possible, Restricted Identifiers should not be used as identifiers. This is only possible for compatibility reasons, using it in Java code is confusing and should be avoided.

Note that this applies to any version of Java, including the one where these identifiers are not yet restricted, to avoid future confusion.

This rule reports an issue when restricted identifiers:

• var
• yield
• record

are used as identifiers.

Noncompliant Code Example

var var = "var"; // Noncompliant: compiles but this code is confusing
var = "what is this?";

int yield(int i) { // Noncompliant
  return switch (i) {
    case 1: yield(0); // This is a yield from switch expression, not a recursive call.
    default: yield(i-1);
  };
}

String record = "record"; // Noncompliant

Compliant Solution

var myVariable = "var";

int minusOne(int i) {
  return switch (i) {
    case 1: yield(0);
    default: yield(i-1);
  };
}

String myRecord = "record";

See

• JLS16, 3.8: Identifiers

Using such generic exceptions as Error, RuntimeException, Throwable, and Exception prevents calling methods from handling true, system-generated exceptions differently than application-generated errors.

Noncompliant Code Example

public void foo(String bar) throws Throwable {  // Noncompliant
  throw new RuntimeException("My Message");     // Noncompliant
}

Compliant Solution

public void foo(String bar) {
  throw new MyOwnRuntimeException("My Message");
}

Exceptions

Generic exceptions in the signatures of overriding methods are ignored, because overriding method has to follow signature of the throw declaration in the superclass. The issue will be raised on superclass declaration of the method (or won't be raised at all if superclass is not part of the analysis).

@Override
public void myMethod() throws Exception {...}

Generic exceptions are also ignored in the signatures of methods that make calls to methods that throw generic exceptions.

public void myOtherMethod throws Exception {
  doTheThing();  // this method throws Exception
}

See

• MITRE, CWE-397 - Declaration of Throws for Generic Exception
• CERT, ERR07-J. - Do not throw RuntimeException, Exception, or Throwable

Most of the time a block of code is empty when a piece of code is really missing. So such empty block must be either filled or removed.

Noncompliant Code Example

for (int i = 0; i < 42; i++){}  // Empty on purpose or missing piece of code ?

Exceptions

When a block contains a comment, this block is not considered to be empty unless it is a synchronized block. synchronized blocks are still considered empty even with comments because they can still affect program flow.

Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.

On the other hand, constants can be referenced from many places, but only need to be updated in a single place.

Noncompliant Code Example

With the default threshold of 3:

public void run() {
  prepare("action1");                              // Noncompliant - "action1" is duplicated 3 times
  execute("action1");
  release("action1");
}

@SuppressWarning("all")                            // Compliant - annotations are excluded
private void method1() { /* ... */ }
@SuppressWarning("all")
private void method2() { /* ... */ }

public String method3(String a) {
  System.out.println("'" + a + "'");               // Compliant - literal "'" has less than 5 characters and is excluded
  return "";                                       // Compliant - literal "" has less than 5 characters and is excluded
}

Compliant Solution

private static final String ACTION_1 = "action1";  // Compliant

public void run() {
  prepare(ACTION_1);                               // Compliant
  execute(ACTION_1);
  release(ACTION_1);
}

Exceptions

To prevent generating some false-positives, literals having less than 5 characters are excluded.

When java.io.File#delete fails, this boolean method simply returns false with no indication of the cause. On the other hand, when java.nio.file.Files#delete fails, this void method returns one of a series of exception types to better indicate the cause of the failure. And since more information is generally better in a debugging situation, java.nio.file.Files#delete is the preferred option.

Noncompliant Code Example

public void cleanUp(Path path) {
  File file = new File(path);
  if (!file.delete()) {  // Noncompliant
    //...
  }
}

Compliant Solution

public void cleanUp(Path path) throws NoSuchFileException, DirectoryNotEmptyException, IOException {
  Files.delete(path);
}

A long parameter list can indicate that a new structure should be created to wrap the numerous parameters or that the function is doing too many things.

Noncompliant Code Example

With a maximum number of 4 parameters:

public void doSomething(int param1, int param2, int param3, String param4, long param5) {
...
}

Compliant Solution

public void doSomething(int param1, int param2, int param3, String param4) {
...
}

Exceptions

Methods annotated with :

• Spring's @RequestMapping (and related shortcut annotations, like @GetRequest)
• JAX-RS API annotations (like @javax.ws.rs.GET)
• Bean constructor injection with @org.springframework.beans.factory.annotation.Autowired
• CDI constructor injection with @javax.inject.Inject
• @com.fasterxml.jackson.annotation.JsonCreator

may have a lot of parameters, encapsulation being possible. Such methods are therefore ignored.

When encrypting data with the Cipher Block Chaining (CBC) mode an Initialization Vector (IV) is used to randomize the encryption, ie under a given key the same plaintext doesn't always produce the same ciphertext. The IV doesn't need to be secret but should be unpredictable to avoid "Chosen-Plaintext Attack".

To generate Initialization Vectors, NIST recommends to use a secure random number generator.

Noncompliant Code Example

public class MyCbcClass {

  public String applyCBC(String strKey, String plainText) {
    byte[] bytesIV = "7cVgr5cbdCZVw5WY".getBytes("UTF-8");

    /* KEY + IV setting */
    IvParameterSpec iv = new IvParameterSpec(bytesIV);
    SecretKeySpec skeySpec = new SecretKeySpec(strKey.getBytes("UTF-8"), "AES");

    /* Ciphering */
    Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING");
    cipher.init(Cipher.ENCRYPT_MODE, skeySpec, iv);  // Noncompliant: the IV is hard coded and thus not generated with a secure random generator
    byte[] encryptedBytes = cipher.doFinal(plainText.getBytes("UTF-8"));
    return DatatypeConverter.printBase64Binary(bytesIV)
            + ";" + DatatypeConverter.printBase64Binary(encryptedBytes);
  }
}

Compliant Solution

public class MyCbcClass {

  SecureRandom random = new SecureRandom();

  public String applyCBC(String strKey, String plainText) {
    byte[] bytesIV = new byte[16];
    random.nextBytes(bytesIV);

    /* KEY + IV setting */
    IvParameterSpec iv = new IvParameterSpec(bytesIV);
    SecretKeySpec skeySpec = new SecretKeySpec(strKey.getBytes("UTF-8"), "AES");

    /* Ciphering */
    Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING");
    cipher.init(Cipher.ENCRYPT_MODE, skeySpec, iv); // Compliant
    byte[] encryptedBytes = cipher.doFinal(plainText.getBytes("UTF-8"));
    return DatatypeConverter.printBase64Binary(bytesIV)
            + ";" + DatatypeConverter.printBase64Binary(encryptedBytes);
  }
}

See

• OWASP Top 10 2017 Category A6 - Security Misconfiguration
• MITRE, CWE-329 - CWE-329: Not Using an Unpredictable IV with CBC Mode
• MITRE, CWE-330 - Use of Insufficiently Random Values
• NIST, SP-800-38A - Recommendation for Block Cipher Modes of Operation
• Derived from FindSecBugs rule STATIC_IV