Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add safelog annotations to SerializableError and RemoteException #1053

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add safelog annotations to SerializableError and RemoteException #1053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
924bcbc
Add safelog annotations to SerializableError and RemoteException
ash211 Nov 7, 2023
e79c4b7
Add generated changelog entries
svc-changelog Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-1053.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Add safelog annotations to SerializableError and RemoteException
links:
- https://github.com/palantir/conjure-java-runtime-api/pull/1053
10 changes: 9 additions & 1 deletion errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import com.palantir.logsafe.Arg;
import com.palantir.logsafe.SafeArg;
import com.palantir.logsafe.SafeLoggable;
import com.palantir.logsafe.Unsafe;
import com.palantir.logsafe.UnsafeArg;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
Expand All @@ -30,12 +32,15 @@ public final class RemoteException extends RuntimeException implements SafeLogga
private static final String ERROR_CODE = "errorCode";
private static final String ERROR_NAME = "errorName";

@Unsafe // because errorName is unsafe
private final String stableMessage;

private final SerializableError error;
private final int status;
private final List<Arg<?>> args;
// Lazily evaluated based on the stableMessage, errorInstanceId, and args.
@SuppressWarnings("MutableException")
@Unsafe
private String unsafeMessage;

/** Returns the error thrown by a remote process which caused an RPC call to fail. */
Expand All @@ -56,10 +61,11 @@ public RemoteException(SerializableError error, int status) {
this.status = status;
this.args = Collections.unmodifiableList(Arrays.asList(
SafeArg.of(ERROR_INSTANCE_ID, error.errorInstanceId()),
SafeArg.of(ERROR_NAME, error.errorName()),
UnsafeArg.of(ERROR_NAME, error.errorName()),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is problematic. The vast majority of the time, an ErrorName is known at compile time and is safe to log. Example value: Product:SomethingBroke.

However, there is a legacy use of the SerializableError that explains why it has deprecated methods getMessage() and getExceptionClass(). At some point in the past, this class must have corresponded more closely with a Java Exception. But now it is broader, and those fields are deprecated in the builder.

Besides marking these methods as @Deprecated, they are also configured to be skipped at read time: access = JsonProperty.Access.WRITE_ONLY, meaning that services receiving JSON objects with message or exceptionClass fields ignore them at read time. The new test testSerDeRoundTripDropsMessage() shows this.

The message is plumbed through as a default to errorName though. See the default implementation of SerializableError.errorName(). This means if errorName isn't set on an object, then the message is used instead. The new test testLegacyMessageUsedAsErrorNameWhenNoErrorNameIsSet() shows this.

Because messages are unsafe, this means the errorName can be unsafe as well. In practice this only happens for SerializableError objects, created using the legacy builder methods that have been marked deprecated, and not yet serialized to a JSON object (still in memory).

I don't think we want to change this line of code to have error name be an Unsafe arg though. I'd rather get it to be Safe 100% of the time. This needs to be done before merging this PR as-is (unless we suppress..)

To prevent message strings from reaching the error name field and causing them to become unsafe, I propose that we keep the Immutables method for setting the message on the ImmutableSerializableError.Builder, but ignore it and replace with a fixed error name, like Default:EmptyErrorNameWithLegacyMessageUsage.

Callers can continue calling .message() on the builder, but it doesn't show anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposed fix for this is at #1054

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just ignore this case. No one should be using servers that emit the legacy error format. It's reasonable to assume that error names are safe.

SafeArg.of(ERROR_CODE, error.errorCode())));
}

@Unsafe
@Override
public String getMessage() {
// This field is not used in most environments so the cost of computation may be avoided.
Expand All @@ -71,6 +77,7 @@ public String getMessage() {
return messageValue;
}

@Unsafe
private String renderUnsafeMessage() {
StringBuilder builder = new StringBuilder()
.append(stableMessage)
Expand All @@ -89,6 +96,7 @@ private String renderUnsafeMessage() {
return builder.toString();
}

@Unsafe
@Override
public String getLogMessage() {
return stableMessage;
Expand Down
8 changes: 8 additions & 0 deletions errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import com.palantir.logsafe.Arg;
import com.palantir.logsafe.Safe;
import com.palantir.logsafe.Unsafe;
import com.palantir.logsafe.exceptions.SafeIllegalStateException;
import java.io.Serializable;
import java.util.Map;
Expand All @@ -34,6 +36,7 @@
* transport errors through RPC channels such as HTTP responses.
*/
// Automatically suppressed to unblock enforcement in new code
@Unsafe
@SuppressWarnings("ImmutablesStyle")
@JsonDeserialize(builder = SerializableError.Builder.class)
@JsonSerialize(as = ImmutableSerializableError.class)
Expand All @@ -48,6 +51,7 @@ public abstract class SerializableError implements Serializable {
* the server-side error code via {@link RemoteException#getError} and typically switch&dispatch on the error code
* and/or name.
*/
@Safe
@JsonProperty("errorCode")
@Value.Default
public String errorCode() {
Expand All @@ -61,6 +65,7 @@ public String errorCode() {
* {@link ErrorType#name} and is part of the service's API surface. Clients are given access to the service-side
* error name via {@link RemoteException#getError} and typically switch&dispatch on the error code and/or name.
*/
@Unsafe // because message is unsafe
@JsonProperty("errorName")
@Value.Default
public String errorName() {
Expand All @@ -74,6 +79,7 @@ public String errorName() {
* {@link #errorName}, the {@link #errorInstanceId} identifies a specific occurrence of an error, not a class of
* errors. By convention, this field is a UUID.
*/
@Safe
@JsonProperty("errorInstanceId")
@Value.Default
@SuppressWarnings("checkstyle:designforextension")
Expand All @@ -89,6 +95,7 @@ public String errorInstanceId() {
*
* @deprecated Used by the serialization-mechanism for back-compat only. Do not use.
*/
@Safe
@Deprecated
@JsonProperty(value = "exceptionClass", access = JsonProperty.Access.WRITE_ONLY)
@Value.Auxiliary
Expand All @@ -100,6 +107,7 @@ public String errorInstanceId() {
*
* @deprecated Used by the serialization-mechanism for back-compat only. Do not use.
*/
@Unsafe
@Deprecated
@JsonProperty(value = "message", access = JsonProperty.Access.WRITE_ONLY)
@Value.Auxiliary
Expand Down
3 changes: 2 additions & 1 deletion errors/src/test/java/com/palantir/conjure/java/api/errors/RemoteExceptionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.assertj.core.api.Assertions.assertThat;

import com.palantir.logsafe.SafeArg;
import com.palantir.logsafe.UnsafeArg;
import org.apache.commons.lang3.SerializationUtils;
import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -130,6 +131,6 @@ public void testArgsContainsOnlyErrorInstanceId() {
.containsExactlyInAnyOrder(
SafeArg.of("errorInstanceId", "errorId"),
SafeArg.of("errorCode", "errorCode"),
SafeArg.of("errorName", "errorName"));
UnsafeArg.of("errorName", "errorName"));
}
}
30 changes: 28 additions & 2 deletions errors/src/test/java/com/palantir/conjure/java/api/errors/SerializableErrorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.palantir.conjure.java.api.ext.jackson.ObjectMappers;
import com.palantir.logsafe.SafeArg;
Expand Down Expand Up @@ -146,18 +147,39 @@ public void forException_optionalArgValue_serializesWithToString() {

@Test
public void testSerializationContainsRedundantParameters() throws Exception {
assertThat(mapper.writeValueAsString(ERROR))
assertThat(serialize(ERROR))
.isEqualTo("{\"errorCode\":\"PERMISSION_DENIED\",\"errorName\":\"Product:SomethingBroke\","
+ "\"errorInstanceId\":\"\",\"parameters\":{}}");

assertThat(mapper.writeValueAsString(SerializableError.builder()
assertThat(serialize(SerializableError.builder()
.from(ERROR)
.errorInstanceId("errorId")
.build()))
.isEqualTo("{\"errorCode\":\"PERMISSION_DENIED\",\"errorName\":\"Product:SomethingBroke\","
+ "\"errorInstanceId\":\"errorId\",\"parameters\":{}}");
}

@Test
public void testSerDeRoundTripDropsMessage() throws Exception {
SerializableError error = SerializableError.builder()
.from(ERROR)
.message("the secret is 42")
.build();

assertThat(error.getMessage()).hasValue("the secret is 42");
assertThat(deserialize(serialize(error)).getMessage()).isEmpty();
}

@Test
public void testLegacyMessageUsedAsErrorNameWhenNoErrorNameIsSet() {
SerializableError error = SerializableError.builder()
.errorCode("errorCode")
.message("the secret is 42")
.build();

assertThat(error.errorName()).isEqualTo("the secret is 42");
}

@Test
public void testDeserializesWhenRedundantParamerersAreGiven() throws Exception {
String serialized =
Expand Down Expand Up @@ -194,4 +216,8 @@ public void testDeserializationFailsWhenNeitherErrorNameNorMessageIsSet() throws
private static SerializableError deserialize(String serialized) throws IOException {
return mapper.readValue(serialized, SerializableError.class);
}

private static String serialize(SerializableError error) throws JsonProcessingException {
return mapper.writeValueAsString(error);
}
}