Updated error response to hide error stack in case of JsonMappingException

[vivek807]

Updated error response to hide error stack in case of JsonMappingException if druid.server.http.showDetailedJsonMappingError is configured as false

Description

Currently when Jackson failed to map request payload either due to required fields or invalid types, it end up exposing too much details about the internal model with 400 response. This can be concerning from security point of view.

It is likely that an attacker may be able to exploit this indeterminate state to access unauthorized functionality, or worse, create, modify or destroy data. Error messages may also aid in the identification of other attacks such as buffer overflows and SQL injection, and can generally contribute to an overall weaker security posture.

The same physical paths, versioning information, stack traces' content, and other data can be gathered and used to help further an attack.

Fixed the bug

Added flag druid.server.http.showDetailedJsonMappingError similar druid.server.http.showDetailedJettyError to configure error message detail.

This PR has:

• [ ] been self-reviewed.
  • [ ] using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
• [ ] added documentation for new or modified features or behaviors.
• [ ] a release note entry in the PR description.
• [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• [ ] added or updated version, license, or notice information in licenses.yaml
• [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• [ ] added integration tests.
• [ ] been tested in a test Druid cluster.

[FrankChen021]

1. What kind of response now is if such exception is raised?
2. IMO, there's no need to provide a configuration to hide the error stack, we can always hide the internal errors and show better messages. If we design it in this way, in the future, we would have lots of configurations like showDetailedXXXError

[vivek807]

1. What kind of response now is if such exception is raised?
2. IMO, there's no need to provide a configuration to hide the error stack, we can always hide the internal errors and show better messages. If we design it in this way, in the future, we would have lots of configurations like showDetailedXXXError

Current error response looks something like this:

{"error":"Cannot construct instance of
`org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexIOConfig`,
problem: At least one of properties[[firehose, inputSource]] must be present\n at
[Source: (org.eclipse.jetty.server.HttpInputOverHTTP); line: 1, column: 69] (through
reference chain:
org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexSupervisorTask[\"spe
c\"]-
>org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexIngestionSpec[\"ioC
onfig\"])"}

We added the flag because we may need to view detailed errors for debugging or other purposes. Initially, we used showDetailedJettyError, but that name was too specific to Jetty errors. To avoid such flag, I think we can add a generic flag or live without it by adding detail message to logs only.

[FrankChen021]

1. What kind of response now is if such exception is raised?
2. IMO, there's no need to provide a configuration to hide the error stack, we can always hide the internal errors and show better messages. If we design it in this way, in the future, we would have lots of configurations like showDetailedXXXError

Current error response looks something like this:

{"error":"Cannot construct instance of
`org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexIOConfig`,
problem: At least one of properties[[firehose, inputSource]] must be present\n at
[Source: (org.eclipse.jetty.server.HttpInputOverHTTP); line: 1, column: 69] (through
reference chain:
org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexSupervisorTask[\"spe
c\"]-
>org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexIngestionSpec[\"ioC
onfig\"])"}

We added the flag because we may need to view detailed errors for debugging or other purposes. Initially, we used showDetailedJettyError, but that name was too specific to Jetty errors. To avoid such flag, I think we can add a generic flag or live without it by adding detail message to logs only.

Thanks for the info. For the debugging purpose, we can add these information to logs. For the HTTP Response, we can keep only the info like:

Cannot construct instance of
`org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexIOConfig`,
problem: At least one of properties[[firehose, inputSource]] must be present

[FrankChen021]

But if the http response is like:

Cannot construct instance of
`org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexIOConfig`,
problem: At least one of properties[[firehose, inputSource]] must be present

I think such information is clear for the users for troubleshooting and there's no need to add stack trace to logs.

[vivek807]

But if the http response is like:

Cannot construct instance of
`org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexIOConfig`,
problem: At least one of properties[[firehose, inputSource]] must be present

I think such information is clear for the users for troubleshooting and there's no need to add stack trace to logs.

Done, thanks

[vivek807]

Hi @FrankChen021 / @cryptoe , Just checking in—could you please review the changes once more and let me know if anything is still pending?

[cryptoe]

@vivek807 Thank you for the patience. The change LGTM.