From 085dad6eb47e57dc2bfc47616f1429f7c185cd82 Mon Sep 17 00:00:00 2001 From: Stuart Douglas Date: Wed, 25 Aug 2021 07:17:34 +1000 Subject: [PATCH] Never propagate a WebApplicationException from the client If a call fails on the server it should be treated as an internal server error, it should not be propagating the response back to the original request. Fixes #19488 (cherry picked from commit 6674bd51d996eb316704b028f44c0215f954c437) --- .../deployment/pom.xml | 22 ++++ .../test/BadRequestNotPropagatedTestCase.java | 110 ++++++++++++++++++ .../api/WebClientApplicationException.java | 3 +- .../client/impl/RestClientRequestContext.java | 13 +++ .../ClientWebApplicationException.java | 72 ++++++++++++ .../ResteasyReactiveClientProblem.java | 10 ++ .../core/AbstractResteasyReactiveContext.java | 2 +- .../server/core/ExceptionMapping.java | 6 +- 8 files changed, 235 insertions(+), 3 deletions(-) create mode 100644 extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/src/test/java/io/quarkus/rest/client/reactive/jackson/test/BadRequestNotPropagatedTestCase.java create mode 100644 independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/ClientWebApplicationException.java create mode 100644 independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/ResteasyReactiveClientProblem.java diff --git a/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/pom.xml b/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/pom.xml index a7d6ba2b1517f..098e55811abe1 100644 --- a/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/pom.xml +++ b/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/pom.xml @@ -23,6 +23,28 @@ io.quarkus quarkus-rest-client-reactive-jackson + + + + io.quarkus + quarkus-resteasy-reactive-jackson-deployment + test + + + io.quarkus + quarkus-junit5-internal + test + + + io.rest-assured + rest-assured + test + + + org.assertj + assertj-core + test + diff --git a/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/src/test/java/io/quarkus/rest/client/reactive/jackson/test/BadRequestNotPropagatedTestCase.java b/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/src/test/java/io/quarkus/rest/client/reactive/jackson/test/BadRequestNotPropagatedTestCase.java new file mode 100644 index 0000000000000..5ff2441e990ec --- /dev/null +++ b/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/src/test/java/io/quarkus/rest/client/reactive/jackson/test/BadRequestNotPropagatedTestCase.java @@ -0,0 +1,110 @@ +package io.quarkus.rest.client.reactive.jackson.test; + +import java.net.URL; +import java.util.List; + +import javax.inject.Inject; +import javax.ws.rs.Consumes; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.Produces; +import javax.ws.rs.WebApplicationException; +import javax.ws.rs.client.Client; +import javax.ws.rs.client.ClientBuilder; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; + +import org.eclipse.microprofile.rest.client.inject.RegisterRestClient; +import org.eclipse.microprofile.rest.client.inject.RestClient; +import org.jboss.shrinkwrap.api.ShrinkWrap; +import org.jboss.shrinkwrap.api.spec.JavaArchive; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.quarkus.test.QuarkusUnitTest; +import io.quarkus.test.common.http.TestHTTPResource; + +/** + * Tests that a 400 response from jackson on the client is not propagated to the server as a 400, + * but is instead reported as an internal server error + */ +public class BadRequestNotPropagatedTestCase { + @RegisterExtension + static final QuarkusUnitTest TEST = new QuarkusUnitTest() + .setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class)); + + @TestHTTPResource + URL url; + + private Client client; + + @BeforeEach + public void before() { + client = ClientBuilder.newBuilder().build(); + } + + @AfterEach + public void after() { + client.close(); + } + + @Test + public void testBadRequest() { + Response data = client.target(url.toExternalForm() + "/bad-server").request().get(); + Assertions.assertEquals(500, data.getStatus()); + } + + @Path("/bad") + public static class Bad { + + @Produces(MediaType.APPLICATION_JSON) + @Consumes(MediaType.APPLICATION_JSON) + @GET + public JsonObject get(List o) { + return o.get(0); + } + } + + @Path("/bad") + @RegisterRestClient(baseUri = "http://localhost:8081") + public interface BadClient { + + @Produces(MediaType.APPLICATION_JSON) + @GET + JsonObject get(String json); + } + + static class JsonObject { + String name; + } + + @Path("/bad-server") + public static class BadServer { + + @Inject + @RestClient + BadClient badClient; + + @GET + public JsonObject get() { + try { + return badClient.get("{name:foo}"); + } catch (WebApplicationException e) { + if (e.getResponse().getStatus() != 400) { + //this is a bit odd, but we are trying to test that a 400 from jackson won't cause a 400 + //response from the server part + //returning this will cause a 204 response and fail the test, as if the original exception is + //not 400 then something has gone wrong + return null; + } + throw e; + } catch (Throwable t) { + t.printStackTrace(); + return null; + } + } + } +} diff --git a/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/api/WebClientApplicationException.java b/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/api/WebClientApplicationException.java index 71b7598db9e67..04dc7a0679693 100644 --- a/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/api/WebClientApplicationException.java +++ b/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/api/WebClientApplicationException.java @@ -14,6 +14,7 @@ import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.NewCookie; import javax.ws.rs.core.Response; +import org.jboss.resteasy.reactive.ResteasyReactiveClientProblem; import org.jboss.resteasy.reactive.common.jaxrs.StatusTypeImpl; /** @@ -23,7 +24,7 @@ * for client usage. Perhaps we can store it in an alternate field? */ @SuppressWarnings("serial") -public class WebClientApplicationException extends WebApplicationException { +public class WebClientApplicationException extends WebApplicationException implements ResteasyReactiveClientProblem { public WebClientApplicationException(int responseStatus) { this(responseStatus, null); diff --git a/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/RestClientRequestContext.java b/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/RestClientRequestContext.java index b188ec5ee5911..467166cdf2dc9 100644 --- a/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/RestClientRequestContext.java +++ b/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/RestClientRequestContext.java @@ -18,6 +18,7 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executor; import javax.ws.rs.RuntimeType; +import javax.ws.rs.WebApplicationException; import javax.ws.rs.client.Entity; import javax.ws.rs.core.GenericEntity; import javax.ws.rs.core.GenericType; @@ -27,6 +28,7 @@ import javax.ws.rs.ext.MessageBodyWriter; import javax.ws.rs.ext.ReaderInterceptor; import javax.ws.rs.ext.WriterInterceptor; +import org.jboss.resteasy.reactive.ClientWebApplicationException; import org.jboss.resteasy.reactive.client.spi.ClientRestHandler; import org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext; import org.jboss.resteasy.reactive.common.core.Serialisers; @@ -115,6 +117,17 @@ public void abort() { restart(abortHandlerChain); } + @Override + protected Throwable unwrapException(Throwable t) { + var res = super.unwrapException(t); + if (res instanceof WebApplicationException) { + WebApplicationException webApplicationException = (WebApplicationException) res; + return new ClientWebApplicationException(webApplicationException.getMessage(), webApplicationException, + webApplicationException.getResponse()); + } + return res; + } + public T readEntity(InputStream in, GenericType responseType, MediaType mediaType, MultivaluedMap metadata) diff --git a/independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/ClientWebApplicationException.java b/independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/ClientWebApplicationException.java new file mode 100644 index 0000000000000..3183791deb504 --- /dev/null +++ b/independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/ClientWebApplicationException.java @@ -0,0 +1,72 @@ +package org.jboss.resteasy.reactive; + +import javax.ws.rs.WebApplicationException; +import javax.ws.rs.core.Response; + +public class ClientWebApplicationException extends WebApplicationException implements ResteasyReactiveClientProblem { + + public ClientWebApplicationException() { + super(); + } + + public ClientWebApplicationException(String message) { + super(message); + } + + public ClientWebApplicationException(Response response) { + super(response); + } + + public ClientWebApplicationException(String message, Response response) { + super(message, response); + } + + public ClientWebApplicationException(int status) { + super(status); + } + + public ClientWebApplicationException(String message, int status) { + super(message, status); + } + + public ClientWebApplicationException(Response.Status status) { + super(status); + } + + public ClientWebApplicationException(String message, Response.Status status) { + super(message, status); + } + + public ClientWebApplicationException(Throwable cause) { + super(cause); + } + + public ClientWebApplicationException(String message, Throwable cause) { + super(message, cause); + } + + public ClientWebApplicationException(Throwable cause, Response response) { + super(cause, response); + } + + public ClientWebApplicationException(String message, Throwable cause, Response response) { + super(message, cause, response); + } + + public ClientWebApplicationException(Throwable cause, int status) { + super(cause, status); + } + + public ClientWebApplicationException(String message, Throwable cause, int status) { + super(message, cause, status); + } + + public ClientWebApplicationException(Throwable cause, Response.Status status) throws IllegalArgumentException { + super(cause, status); + } + + public ClientWebApplicationException(String message, Throwable cause, Response.Status status) + throws IllegalArgumentException { + super(message, cause, status); + } +} diff --git a/independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/ResteasyReactiveClientProblem.java b/independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/ResteasyReactiveClientProblem.java new file mode 100644 index 0000000000000..aa40c146a2cf1 --- /dev/null +++ b/independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/ResteasyReactiveClientProblem.java @@ -0,0 +1,10 @@ +package org.jboss.resteasy.reactive; + +/** + * Marker interface that is used to indicate that an exception was generated by the client. + * + * This stops the server treating it as a WebApplicationException, so that client errors are not + * interpreted as server responses. + */ +public interface ResteasyReactiveClientProblem { +} diff --git a/independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/common/core/AbstractResteasyReactiveContext.java b/independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/common/core/AbstractResteasyReactiveContext.java index 4ec49cbbda50e..6560d9b203c45 100644 --- a/independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/common/core/AbstractResteasyReactiveContext.java +++ b/independent-projects/resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/common/core/AbstractResteasyReactiveContext.java @@ -323,7 +323,7 @@ public void handleException(Throwable t, boolean keepSameTarget) { } } - private Throwable unwrapException(Throwable t) { + protected Throwable unwrapException(Throwable t) { if (t instanceof UnwrappableException) { return t.getCause(); } diff --git a/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ExceptionMapping.java b/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ExceptionMapping.java index 4680083fc666b..7be4aa24fe6cf 100644 --- a/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ExceptionMapping.java +++ b/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ExceptionMapping.java @@ -17,6 +17,7 @@ import javax.ws.rs.core.Response; import javax.ws.rs.ext.ExceptionMapper; import org.jboss.logging.Logger; +import org.jboss.resteasy.reactive.ResteasyReactiveClientProblem; import org.jboss.resteasy.reactive.common.model.ResourceExceptionMapper; import org.jboss.resteasy.reactive.server.spi.ResteasyReactiveAsyncExceptionMapper; import org.jboss.resteasy.reactive.server.spi.ResteasyReactiveExceptionMapper; @@ -39,7 +40,10 @@ public class ExceptionMapping { @SuppressWarnings({ "unchecked", "rawtypes" }) public void mapException(Throwable throwable, ResteasyReactiveRequestContext context) { Class klass = throwable.getClass(); - boolean isWebApplicationException = throwable instanceof WebApplicationException; + //we don't thread WebApplicationException's thrown from the client as true 'WebApplicationException' + //we consider it a security risk to transparently pass on the result to the calling server + boolean isWebApplicationException = throwable instanceof WebApplicationException + && !(throwable instanceof ResteasyReactiveClientProblem); Response response = null; if (isWebApplicationException) { response = ((WebApplicationException) throwable).getResponse();