BREAKING: don’t expose Exception::getMessage as detail (#156)

Unless the exception is of type ThrowableProblem or UnsatisfiedRouteException

This aims to prevent accidental information leakage.

Fixes: #144

problem-json/build.gradle

@@ -4,11 +4,13 @@ plugins {
 
 dependencies {
     annotationProcessor("io.micronaut.serde:micronaut-serde-processor:$serdeVersion")
+
     api "org.zalando:problem:$problemVersion"
     implementation("io.micronaut.serde:micronaut-serde-api:$serdeVersion")
     implementation "io.micronaut:micronaut-validation"
     implementation "io.micronaut:micronaut-http-server"
 
     testImplementation("io.micronaut:micronaut-http-server-netty")
     testImplementation("io.micronaut:micronaut-http-client")
+    testAnnotationProcessor "io.micronaut:micronaut-inject-java"
 }

problem-json/src/main/java/io/micronaut/problem/ProblemErrorResponseProcessor.java

@@ -27,13 +27,12 @@
 import io.micronaut.http.server.exceptions.response.ErrorResponseProcessor;
 import io.micronaut.problem.conf.ProblemConfiguration;
 import io.micronaut.problem.conf.ProblemConfigurationProperties;
+import io.micronaut.web.router.exceptions.UnsatisfiedRouteException;
 import jakarta.inject.Inject;
 import org.zalando.problem.Problem;
 import org.zalando.problem.StatusType;
 import org.zalando.problem.ThrowableProblem;
-
 import jakarta.inject.Singleton;
-
 import java.net.URI;
 import java.util.Map;
 
@@ -94,7 +93,7 @@ public MutableHttpResponse<Problem> processResponse(@NonNull ErrorContext errorC
     }
 
     /**
-     *
+     * Creates a {@link ThrowableProblem} when the root cause was not an exception of type {@link ThrowableProblem}.
      * @param errorContext Error Context
      * @param httpStatus HTTP Status
      * @return Default problem
@@ -106,12 +105,30 @@ protected ThrowableProblem defaultProblem(@NonNull ErrorContext errorContext,
         if (!errorContext.getErrors().isEmpty()) {
             Error error = errorContext.getErrors().get(0);
             error.getTitle().ifPresent(problemBuilder::withTitle);
-            problemBuilder.withDetail(error.getMessage());
+            if (includeErrorMessage(errorContext)) {
+                problemBuilder.withDetail(error.getMessage());
+            }
             error.getPath().ifPresent(path -> problemBuilder.with("path", path));
         }
         return problemBuilder.build();
     }
 
+    /**
+     * Whether {@link ThrowableProblem}, created when the root cause is not an exception of type {@link ThrowableProblem}, should
+     * contain Error::getMessage in the problem detail.
+     *
+     * To avoid accidental information leakage defaults to false unless the root cause is of type {@link UnsatisfiedRouteException}
+     * which contains helpful information to diagnose the issue (e.g. missing required query value) in the details.
+     *
+     * @param errorContext Error Context
+     * @return To avoid accidental information leakage defaults to false unless the root cause is of type {@link UnsatisfiedRouteException}
+     */
+    protected boolean includeErrorMessage(@NonNull ErrorContext errorContext) {
+        return errorContext.getRootCause()
+                .map(UnsatisfiedRouteException.class::isInstance)
+                .orElse(false);
+    }
+
     @Introspected
     static final class ThrowableProblemWithoutStacktrace implements Problem {
         @JsonUnwrapped

problem-json/src/test/groovy/io/micronaut/problem/DataLeakageOverrideSpec.groovy

@@ -0,0 +1,49 @@
+package io.micronaut.problem
+
+import io.micronaut.context.annotation.Replaces
+import io.micronaut.context.annotation.Requires
+import io.micronaut.core.annotation.NonNull
+import io.micronaut.core.type.Argument
+import io.micronaut.http.HttpRequest
+import io.micronaut.http.annotation.Controller
+import io.micronaut.http.annotation.Get
+import io.micronaut.http.client.exceptions.HttpClientResponseException
+import io.micronaut.http.server.exceptions.response.ErrorContext
+import io.micronaut.problem.conf.ProblemConfiguration
+import io.micronaut.web.router.exceptions.UnsatisfiedRouteException
+import jakarta.inject.Singleton
+import spock.lang.Issue
+
+@Issue("https://github.com/micronaut-projects/micronaut-problem-json/issues/144")
+class DataLeakageOverrideSpec extends EmbeddedServerSpecification {
+
+    @Override
+    String getSpecName() {
+        'DataLeakageOverrideSpec'
+    }
+
+    void "You can replace and override ProblemErrorResponseProcessorReplacement::shouldIncludeErrorMessageAsDetailForDefaultProblem"() {
+        when:
+        Argument<?> okArg = Argument.of(String)
+        Argument<?> errorArg = Argument.of(Map)
+        client.exchange(HttpRequest.GET('/foo'), okArg, errorArg)
+
+        then:
+        HttpClientResponseException thrown = thrown()
+        Optional<Map> bodyOptional = thrown.response.getBody(errorArg)
+        bodyOptional.isPresent()
+        bodyOptional.get().keySet().size() == 3
+        bodyOptional.get()['status'] == 500
+        bodyOptional.get()['type'] == "about:blank"
+        bodyOptional.get()['detail'] == "Internal Server Error: foo data"
+    }
+
+    @Requires(property = "spec.name", value = "DataLeakageOverrideSpec")
+    @Controller('/foo')
+    static class FooController {
+        @Get
+        void doSomething() {
+            throw new FooException("foo data");
+        }
+    }
+}

problem-json/src/test/groovy/io/micronaut/problem/DataLeakageSpec.groovy

@@ -0,0 +1,48 @@
+package io.micronaut.problem
+
+import io.micronaut.context.annotation.Requires
+import io.micronaut.core.type.Argument
+import io.micronaut.http.HttpRequest
+import io.micronaut.http.annotation.Controller
+import io.micronaut.http.annotation.Get
+import io.micronaut.http.client.exceptions.HttpClientResponseException
+import spock.lang.Issue
+import spock.lang.Narrative
+
+@Issue("https://github.com/micronaut-projects/micronaut-problem-json/issues/144")
+@Narrative("""\
+Exceptions that do not extend ThrowableProblem should not include Exception::getMessage in its details part as it 
+may carry sensitive information and cause information disclosure.  
+""")
+class DataLeakageSpec extends EmbeddedServerSpecification {
+
+    @Override
+    String getSpecName() {
+        'DataLeakageSpec'
+    }
+
+    void "No unintended data leakage in instances of Problem"() {
+        when:
+        Argument<?> okArg = Argument.of(String)
+        Argument<?> errorArg = Argument.of(Map)
+        client.exchange(HttpRequest.GET('/foo'), okArg, errorArg)
+
+        then:
+        HttpClientResponseException thrown = thrown()
+        Optional<Map> bodyOptional = thrown.response.getBody(errorArg)
+        bodyOptional.isPresent()
+        bodyOptional.get().keySet().size() == 2
+        bodyOptional.get()['status'] == 500
+        bodyOptional.get()['type'] == "about:blank"
+    }
+
+    @Requires(property = "spec.name", value = "DataLeakageSpec")
+    @Controller('/foo')
+    static class FooController {
+
+        @Get
+        void doSomething() {
+            throw new Exception("foo data");
+        }
+    }
+}

problem-json/src/test/groovy/io/micronaut/problem/NoDuplicatesInNonStackTraceResponseSpec.groovy

@@ -1,10 +1,12 @@
 package io.micronaut.problem
 
 import io.micronaut.context.annotation.Requires
+import io.micronaut.http.HttpResponse
 import io.micronaut.http.annotation.Controller
 import io.micronaut.http.annotation.Get
 import io.micronaut.http.annotation.QueryValue
 import io.micronaut.http.client.exceptions.HttpClientResponseException
+import org.zalando.problem.Problem
 
 class NoDuplicatesInNonStackTraceResponseSpec extends EmbeddedServerSpecification {
 
@@ -22,15 +24,15 @@ class NoDuplicatesInNonStackTraceResponseSpec extends EmbeddedServerSpecificatio
 
     def "properties are not duplicated with a no-stack-trace client error"() {
         when:
-        def response = client.exchange('/client', String, String)
+        HttpResponse<String> response = client.exchange('/client', String, String)
 
         then:
         HttpClientResponseException e = thrown()
         with(e.response.body()) { String it ->
-            it.findAll('"type":').size() == 1
-            it.findAll('"parameters":').size() == 1
-            it.findAll('"detail":').size() == 1
-            it.findAll('"status":').size() == 1
+            it.count('"type":') == 1
+            it.count('"parameters":') == 1
+            it.count('"detail":"Required QueryValue [taskId] not specified"') == 1
+            it.count('"status":') == 1
         }
     }
 

problem-json/src/test/java/io/micronaut/problem/FooException.java

@@ -0,0 +1,7 @@
+package io.micronaut.problem;
+
+public class FooException extends RuntimeException {
+    FooException(String message) {
+        super(message);
+    }
+}

problem-json/src/test/java/io/micronaut/problem/ProblemErrorResponseProcessorReplacement.java

@@ -0,0 +1,28 @@
+package io.micronaut.problem;
+
+import io.micronaut.context.annotation.Replaces;
+import io.micronaut.context.annotation.Requires;
+import io.micronaut.core.annotation.NonNull;
+import io.micronaut.http.server.exceptions.response.ErrorContext;
+import io.micronaut.problem.conf.ProblemConfiguration;
+import io.micronaut.web.router.exceptions.UnsatisfiedRouteException;
+import jakarta.inject.Singleton;
+
+@Requires(property = "spec.name", value = "DataLeakageOverrideSpec")
+//tag::clazz[]
+@Replaces(ProblemErrorResponseProcessor.class)
+@Singleton
+public class ProblemErrorResponseProcessorReplacement
+        extends ProblemErrorResponseProcessor {
+    ProblemErrorResponseProcessorReplacement(ProblemConfiguration config) {
+        super(config);
+    }
+
+    @Override
+    protected boolean includeErrorMessage(@NonNull ErrorContext errorContext) {
+        return errorContext.getRootCause()
+                .map(t -> t instanceof FooException || t instanceof UnsatisfiedRouteException)
+                .orElse(false);
+    }
+}
+//end::clazz[]

src/main/docs/guide/breaks.adoc

@@ -0,0 +1,3 @@
+The default Problem+JSON payload does not include the `detail` field to avoid accidental information disclosure if the exception root cause is not of type `UnsatisfiedRouteException` or `ThrowableProblem` to avoid accidental information disclosure since 2.2.3.
+
+You can <<customizingProblemErrorResponseProcessor, customize it>> to include always the detail or for some scenarios.

src/main/docs/guide/customizingProblemErrorResponseProcessor.adoc

@@ -0,0 +1,9 @@
+The default Problem+JSON payload does not include the `detail` field to avoid accidental information disclosure if the exception root cause is not of type `UnsatisfiedRouteException` or `ThrowableProblem`.
+
+You can extend api:problem.ProblemErrorResponseProcessor[] to customize the behaviour:
+
+[source,java]
+----
+include::problem-json/src/test/java/io/micronaut/problem/ProblemErrorResponseProcessorReplacement.java[tag=clazz]
+----
+

src/main/docs/guide/toc.yml

@@ -1,13 +1,14 @@
 introduction:
   title: Introduction
 releaseHistory: Release History
+breaks: Breaking Changes
 installation:
   Installation
 usage:
   title: Usage
   problemBuilder: Problem Builder
   customProblem: Custom Problem
-configuration:
-  Configuration
+configuration: Configuration
+customizingProblemErrorResponseProcessor: Custom ProblemErrorResponseProcessor
 repository: Repository