Skip to content

Commit

Permalink
Never propagate a WebApplicationException from the client
Browse files Browse the repository at this point in the history
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 quarkusio#19488

(cherry picked from commit 6674bd5)
  • Loading branch information
stuartwdouglas authored and gsmet committed Aug 30, 2021
1 parent 633990e commit 085dad6
Show file tree
Hide file tree
Showing 8 changed files with 235 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,28 @@
<groupId>io.quarkus</groupId>
<artifactId>quarkus-rest-client-reactive-jackson</artifactId>
</dependency>

<!-- test dependencies: -->
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-resteasy-reactive-jackson-deployment</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-junit5-internal</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.rest-assured</groupId>
<artifactId>rest-assured</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
@@ -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<JsonObject> 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;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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> T readEntity(InputStream in,
GenericType<T> responseType, MediaType mediaType,
MultivaluedMap<String, Object> metadata)
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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 {
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down

0 comments on commit 085dad6

Please sign in to comment.