Split S1166 to two rules: logger vs. new exception

Clean Code New rules / language support
1

Hello,
Is it possible to split S1166 Exception handlers should preserve the original exceptions to two rules? First will be applied when logger is used and second when developer creates a new exception. Why? Let’s use an example:

try {
  /* ... */
} catch (Exception e) {
  LOGGER.warn(e.getMessage());
}
doSomething()

try {
  /* ... */
} catch (Exception e) {
  throw new RuntimeException("context");
}
doSomething2()

Usually, at first case we consciously hide the stacktrace. We just want to inform that something was wrong, but next we continue the standard flow. At second case we break the flow ( doSomething2 is not executed) and hide the real cause. If somebody wants to:

  • fix the problem - he/she won’t know what is broken
  • ignore the exception - he/she may log warn message (see case no 1)

From our point of view first case is minor/major and second is critical/blocker. Also some security tools forbidden put stacktraces in logs (sensitive information may leak). For example we use HP Fortify which provides a such rule. We have clash problems here :disappointed:

What do you think?

Cheers

7

Hi @agabrys,

Thank you for this improvement idea.

I completely agree that there are cases when exceptions shouldn’t be logged as is, otherwise sensitive informations could be logged. On the other hand, logging too little information creates another vulnerability, see OWASP guidelinesand CWE-778 for more details.

I searched for a middle-ground and created the following ticket: SONARJAVA-3029.

Let me know if this would be a valid solution for you.

Cheers,
Nicolas

1 Like
8

My last answer didn’t explain why we can’t split the rule in two. The minor/major rule you mention would still force developers to call LOGGER.warn(exception). Such a rule would be disabled by developers because it can create vulnerabilities. This makes perfectly sense and we wouldn’t want such a rule to be in SonarJava.

We can instead relax the original rule by accepting two cases:

  • The exception does not contains sensitive information: then the whole exception should be logged. In this case the Fortify rule would be a false positive.
  • The exception contains sensitive information: the exception cannot be logged as is, but a clear message should be logged. Exception.getMessage() is rarely enough as it often does not explain what was attempted. Thus we should explicitly say to the developper to add some context. Any message which is neither a hardcoded string nor Exception.getMessage() should be accepted as it shows that the developer thought this through.

It will be up to the developer and/or a security auditor to decide if the exception is security sensitive, logging either the exception or a hand-crafted message.

Cheers,
Nicolas

2 Likes