[OPIK-721] Return more detailed error message for Json deserialization errors #1040
Conversation
| return Response.status(Response.Status.BAD_REQUEST) | ||
| .entity(new ErrorMessage( | ||
| List.of(endIndex == -1 ? "Unable to process JSON" : errorMessage.substring(0, endIndex)))) | ||
| List.of("Unable to process JSON. " + exception.getMessage()))) |
There was a problem hiding this comment.
This can leak more information than necessary I would avoid it
There was a problem hiding this comment.
Currently we just return Unable to process JSON which can't be used for debugging properly by users on SDK side. Discussed it with SDK team, and also we already returned it before for some specific scenario, now it will be for all JSON related exceptions.
As I checked logs, we will now return the following as example:
Cannot deserialize value of type `com.comet.opik.api.DatasetItemSource` from String "https://wonderopolis.org/wonder/will-a-watermelon-grow-in-your-belly-if-you-swallow-a-seed": https://wonderopolis.org/wonder/will-a-watermelon-grow-in-your-belly-if-you-swallow-a-seed was not one of [MANUAL, TRACE, SPAN, SDK]
Cannot deserialize value of type `java.util.UUID` from String "cnn-test-404f859482d47c127868964a9a39d1a7645dd2e9": UUID has to be represented by standard 36-char representation
I don't thing that something important is leaking, it's open source and Java objects and field types are available anyway.
@andrescrz Could you share your opinion?
There was a problem hiding this comment.
I agree with Thiago in the general case, that leaking inner exception messages in REST response objects can lead to important data security issues.
However, we have certainty in this case that this exception mapper is only invoked when an incoming JSON request can't be properly deserialised. Therefore and looking at the examples provided by Borys:
- Datatypes are never a concern as this is an open source application.
- Customer data is a concern. However, this data is already in the request, so clients will get an error response that might contain the same data that is sending.
Therefore, from the backend side, I don't think there's a data security issue.
However, a client like the SDK might be printing these logs and showing data to users that shouldn't be seeing it, or sending it to a third-party service that shouldn't receive sensitive data (typical monitoring tools in the market).
I think if we really want to be paranoid here, my proposal is to remove the sent data from the error message. For example, just printing: Cannot deserialize value of type com.comet.opik.api.DatasetItemSource from String
There was a problem hiding this comment.
let's do the following: do not send the erroneous data unless it's just an id, in which case you can send the id.
Instead, identify which field was invalid and send the proper format that was expected. You can give the item Id that was provided with the request so that the user can find the entire message that was sent in their own database (assuming they have this log).
if what I just suggested costs more than 1/2 day's work, let's try to find a less optimal solution that does not expose customer data in the response.
There was a problem hiding this comment.
I'm happy with that. It should be just a regex to identify an UUID or remove the data otherwise.
| return Response.status(Response.Status.BAD_REQUEST) | ||
| .entity(new ErrorMessage( | ||
| List.of(endIndex == -1 ? "Unable to process JSON" : errorMessage.substring(0, endIndex)))) | ||
| List.of("Unable to process JSON. " + exception.getMessage()))) |
There was a problem hiding this comment.
I agree with Thiago in the general case, that leaking inner exception messages in REST response objects can lead to important data security issues.
However, we have certainty in this case that this exception mapper is only invoked when an incoming JSON request can't be properly deserialised. Therefore and looking at the examples provided by Borys:
- Datatypes are never a concern as this is an open source application.
- Customer data is a concern. However, this data is already in the request, so clients will get an error response that might contain the same data that is sending.
Therefore, from the backend side, I don't think there's a data security issue.
However, a client like the SDK might be printing these logs and showing data to users that shouldn't be seeing it, or sending it to a third-party service that shouldn't receive sensitive data (typical monitoring tools in the market).
I think if we really want to be paranoid here, my proposal is to remove the sent data from the error message. For example, just printing: Cannot deserialize value of type com.comet.opik.api.DatasetItemSource from String
apps/opik-backend/src/main/java/com/comet/opik/api/error/JsonInvalidFormatExceptionMapper.java
Outdated
Show resolved
Hide resolved
...k-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java
Outdated
Show resolved
Hide resolved
BorisTkachenko
deleted the
boryst/OPIK-721-be-add-field-validation-for-dataset-items-provide-better-error-messages
branch
Details
Return more detailed error message for Json deserialization errors
Testing
Covered by integration test