fix #76: Safe exception messages provide argument data #77
Conversation
Only logMessage is redacted.
carterkozak
force-pushed
the
ckozak/args_in_exception_message
branch
from
89e8b66
to
ee23917
Compare
|
fyi @uschi2000 @iamdanfox |
| } | ||
| } | ||
| return sb.append('}') | ||
| .toString(); |
| Arg<?> argument = args[i]; | ||
| sb.append(argument.getName()) | ||
| .append("=") | ||
| .append(argument.getValue()); |
| .append(argument.getValue()); | ||
| if (i < args.length - 1) { | ||
| sb.append(", "); | ||
| } |
There was a problem hiding this comment.
maybe we should just copy this code from conjure-java-runtime which already does what you want!
https://github.com/palantir/conjure-java-runtime-api/blob/8d23fc7fb11481033ca4867fdb99a93186b108ac/errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java#L95-L117
There was a problem hiding this comment.
(except the first line, but e.g. it avoids printing out : {} in the trivial case where there are no arguments)
|
Updated, thanks for the feedback |
|
@dansanduleac @cakofony I think this PR is not entirely safe because people may have assumed that the message of a Before this PR, that would have been true because we have baseline-error-prone enforcing that people only write constant messages. However after this PR, the following snippet could cause a leak because try {
throw new SafeRuntimeException("...", UnsafeArg("danger", danger));
// ...
catch (SafeRuntimeException e) {
log.info("Ignoring something and retrying", SafeArg.of("message", e.getMessage());
// ^ 🔥🔥🔥
retry();
}As a compromise, could we just omit all the UnsafeArgs from the exception message and accept that they will just never be visible in logs produced by non-palantir slf4j implementations? Alternatively, I think we need to urgently extend our errorprone checks to prevent the above scenario. |
|
Only including safe args doesn’t provide enough data to debug applications which do not produce sls service logs. Logging a safe arg of getMessage is strictly worse than using getLogMessage, since only the latter is guaranteed to be safe. |
|
Any chance you could pop an extra errorprone check into baseline then? I think this means we could wholesale ban anyone writing |
|
Sure thing |
|
@iamdanfox Cheers palantir/gradle-baseline#444 |
Only logMessage is redacted.