CODEWITHSUNDEEP
java
Imesh Chandrasiri
Imesh Chandrasiri

Reputation: 5679

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

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.

Upvotes: 63

Views: 129022

Answers (8)

Akshay Lokur
Akshay Lokur

Reputation: 7506

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

Upvotes: 7

Nicolas Henneaux
Nicolas Henneaux

Reputation: 12245

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);
}

Upvotes: 3

sanbhat
sanbhat

Reputation: 17622

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());
}

Upvotes: 128

St&#233;phane Essayie
St&#233;phane Essayie

Reputation: 11

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());
}

Upvotes: 1

ashish p
ashish p

Reputation: 202

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

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

Upvotes: 3

m.bemowski
m.bemowski

Reputation: 632

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

Upvotes: 51

Narendra Pathai
Narendra Pathai

Reputation: 41955

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());
}

Upvotes: 3

isnot2bad
isnot2bad

Reputation: 24454

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();
}

Upvotes: 1

Related Questions