Malicious code vulnerability - May expose internal representation by incorporating reference to mutable object

Question

I have the following code in my dto class.

public void setBillDate(Date billDate) {
    this.billDate = billDate;
}

And I get an error in sonar stated as such and I'm not sure what I'm doing wrong here.

Malicious code vulnerability - May expose internal representation by incorporating reference to mutable object   

The class is a dto and the method is automatically created setter method. What am I doing wrong here. if anyone could explain. it would be a great help.

Answer 1

Date is mutable

Using that setter, someone can modify the date instance from outside unintentionally

Consider this

class MyClass {

   private Date billDate;


   public void setBillDate(Date billDate) {
      this.billDate = billDate;
   }

}

now some one can set it

MyClass m = new MyClass();

Date dateToBeSet = new Date();
m.setBillDate(dateToBeSet); //The actual dateToBeSet is set to m

dateToBeSet.setYear(...); 
//^^^^^^^^ Un-intentional modification to dateToBeSet, will also modify the m's billDate 

To avoid this, you may want to Deep-copy before setting

public void setBillDate(Date billDate) {
    this.billDate = new Date(billDate.getTime());
}

Answer 2

I wonder why none of the solutions takes null into consideration. A general, null-safe solution should look like this:

public void setBillDate(Date billDate) {
    this.billDate = billDate != null ? new Date(billDate.getTime()) : null;
}

Answer 3

A counter argument can be, why one would one unintentionally modify the date? If client sets the value and then modifies it, then our code should reflect it, isn't it? If not then is it not confusing?

I prefer to just ignore this FindBugs warning.

In case if you want to do that, just add following Maven dependencies in your pom.xml:

<!-- Findbugs -->
        <dependency>
            <groupId>com.google.code.findbugs</groupId>
            <artifactId>annotations</artifactId>
            <version>3.0.1</version>
            <scope>provided</scope>
        </dependency>
        <dependency>
            <groupId>com.google.code.findbugs</groupId>
            <artifactId>annotations</artifactId>
            <version>3.0.1</version>
            <scope>provided</scope>
        </dependency>
        <dependency>
            <groupId>com.google.code.findbugs</groupId>
            <artifactId>jsr305</artifactId>
            <version>3.0.1</version>
            <scope>provided</scope>
        </dependency>

and then these annotations at class or member field level in your POJO:

@SuppressFBWarnings(value = { "EI_EXPOSE_REP", "EI_EXPOSE_REP2" }, justification = "I prefer to suppress these FindBugs warnings")

Cheers

Akshay

Answer 4

Date is mutable

and you are not creating a copy of Date that came in to you are parameter. So if the client code will change the value of the Date object, it will affect your class too.

Solution is to create a copy of Date

public setBillDate(Date billDate){
   this.billDate = new Date(billDate.getTime());
}

Answer 5

Consider using a clone as well. Don't forget null check.

public void setBillDate(Date billDate) {
    this.billDate = billDate == null ? null : billDate.clone();
}

Answer 6

In addition to the existing answers, I propose a new version based on Optional class from Java 8.

public void setBillDate(Date billDate) {
    this.billDate = Optional
            .ofNullable(billDate)
            .map(Date::getTime)
            .map(Date::new)
            .orElse(null);
}

Answer 7

Date is not immutable, i.e. your billDate can be changed after it has been set on your DTO object. Or, in code:

Date billDate = new Date();
dto.setBillDate(billDate);
billDate.setYear(1990);
// now, dto.getBillDate().getYear() == 1990

You can make your setter more secure:

public void setBillDate(Date billDate) {
    this.billDate = (Date)billDate.clone();
}

Answer 8

Top answer number 37 is not the correct answer : nobody cares about NullPointerExceptions???

You should try this instead :

public void setBillDate(Date billDate) {
    this.billDate = billDate == null ? billDate : new Date(billDate.getTime());
}