Access private setters in codebase

Question

I have a DTO object which I used in the controller method which initially had all the getters and setters as public. Due to a SAST scanning (Unsafe object binding) in checkmarx tool, I have to make the setters as private in that DTO. Now checkmarx is not complaining. But in my codebase, I have used some of those setters so now my build is failing. How to resolve this problem, or what technique should be used so that the checkmarx tool doesnot complain and my code builds and runs fine at the same time.

@SuppressWarnings("serial")
@NoArgsConstructor @AllArgsConstructor @ToString
@Builder(toBuilder=true)
public class PartDto implements Serializable {

    private Date autoCsoCommit;
    private Integer statusId;

    public Integer getStatusId() {
        return statusId;
    }

    private void setStatusId(Integer statusId) {
        this.statusId = statusId;
    }
    public Date getAutoCsoCommit() {
        return autoCsoCommit;
    }

    private void setAutoCsoCommit(Date autoCsoCommit) {
        this.autoCsoCommit = autoCsoCommit;
    }

Code where its used:

    public PartDto createPart(final PartDto partDto) {
            log.debug("PartDto: {}", partDto);
            if (partExists(partDto.getPartNumber(), partDto.getEsnId(), partDto.getPoNumber()))
                throw new ValidationException("For this ESN, the Part Number and PO Number combination already exists.");
            log.debug("Part: {}", getPart(partDto, new Part()));
    
            if (partDto.getStatusId() == null) 
                partDto.setStatusId(0);
            
            Part finalPart = savePartDtoUpdateAutoCso(partDto, new Part());
            

return getPartDtoByPartId(finalPart.getPartId());
    }

PartDto updateAutoCsoCommitDateForPartDto(PartDto partDto) {
    
    if (partDto == null) {
        log.error("partDto is null");
        return null;
    }
                
    partDto.setAutoCsoCommit(retrieveAutoCsoDate(
            partDto.getMaterialStream(), 
            partDto.getSource(), 
            partDto.getSourceId(), 
            partDto.getPartNumber(), 
            partDto.getPoNumber())
            );

    return partDto;
};

Error in build:

$ gradle bootrun

> Task :compileJava
C:\Users\502622018\moa_workspace\moa-svc\src\main\java\com\ge\digital\oa\moa\service\PartService.java:123: error: setStatusId(Integer) has private access in PartDto
                        partDto.setStatusId(0);
                               ^
C:\Users\502622018\moa_workspace\moa-svc\src\main\java\com\ge\digital\oa\moa\service\PartService.java:366: error: setAutoCsoCommit(Date) has private access in PartDto
                partDto.setAutoCsoCommit(retrieveAutoCsoDate(
                       ^
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: C:\Users\502622018\moa_workspace\moa-svc\src\main\java\com\ge\digital\oa\common\config\aws\SecretsManagerPropertySourceLocator.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
2 errors

> Task :compileJava FAILED

FAILURE: Build failed with an exception.

The method where the partDTO object is saved is

private Part savePartDtoUpdateAutoCso(final PartDto partDto, final Part existingPart) {
    
    // we need to save and retrieve the part so all string fields
    // have values, since we use them for the auto cso commit determination
    Part savedPart = partRepo.saveAndFlush(getPart(partDto, existingPart));
    entityManager.refresh(savedPart);
    
    log.debug("saved domain part: {}",savedPart);
    PartDto savedPartDto = getPartDtoByPartId(savedPart.getPartId());
    log.debug("Part after save: {}", savedPartDto);
    updateAutoCsoCommitDateForPartDto(savedPartDto);
    
    Part finalPart = partRepo.save(getPart(savedPartDto, savedPart));

return finalPart;
}

PostMapping code:

@PostMapping
public HttpEntity<PartDto> createPart(@RequestBody @Valid PartDto partDto) {
    if (!securityService.isAdmin()) {
        return new ResponseEntity<>(HttpStatus.UNAUTHORIZED);
    }
    log.debug("Start..." + partDto);
    return new ResponseEntity<>(partService.createPart(partDto), HttpStatus.CREATED);
}

Understand the tools you are using. Don't follow warnings blindly. — Turing85, Dec 21 '21 at 18:38
Agreed..The tool is complaining - "The partDto at src\main\java\com\ge\digital\oa\moa\controller\PartController.java in line 104 may unintentionally allow setting the value of saveAndFlush in savePartDtoUpdateAutoCso, in the object src\main\java\com\ge\digital\oa\moa\service\PartService.java at line 136." It is true, I am saving the partDto object in the savePartDtoUpdateAutoCso() method. Isnt it a good practise? — sromit, Dec 21 '21 at 18:54

score 1 · Answer 1 · answered Dec 25 '21 at 23:21

1

There's a risk of mass assignment vulnerability (unsafe object binding) in your code. It is advised to be granular on the parameters you passed to prevent unwanted property and member value tampering.

While there's only a couple of properties/members of the PartDto class, it might change due to a requirement in the future.

So instead of passing the whole PartDto object, just pass each member explicitly (what's only needed). Here's a pseudo code as a guide:

    public PartDto createPart(string partNumber, integer esnId, integer poNumber) {
            //log.debug("PartDto: {}", partDto);
            if (partExists(partNumber, esnId, poNumber))
                throw new ValidationException("For this ESN, the Part Number and PO Number combination already exists.");
             // create a PartDto object and save the values 

       }

answered Dec 25 '21 at 23:21

securecodeninja

2,497
3
16
22

Actually that method is of PostMapping of a RestController and the partDTO object has 50 fields.. I gave only 2 for simplicity... so I cannot pass 50 fields in the createPart() method..any other suggestion you can suggest..updated the post with the Postmapping – sromit Dec 27 '21 at 02:33
from a structural standpoint, if you have 50 fields in a class it does look complex so you may want to revisit your class design. i don't think its efficient to create 50 params but are you going to update ALL of them? if yes, then it is only right to pass the PartDTO object as a single parameter – securecodeninja Dec 29 '21 at 04:11
Yeah..I am updating all the fields of the part table via partDTO object in this method - Part finalPart = partRepo.save(getPart(savedPartDto, savedPart)); The getPart method is for mapping the DTO fields to domain feilds...so I think its proper that we pass the partDTO object as a whole in the controller and mark the checkmarx alert as false positive unless you advice otherwise – sromit Dec 29 '21 at 15:12
i would rely on what your security team will say, the false positive marking should be based on their judgement and your deliberation with them – securecodeninja Dec 29 '21 at 19:27

Access private setters in codebase

1 Answers1