Make response available on error in rest client #45282

rober710 · 2024-12-24T14:49:17Z

Responses cannot be read if the HTTP status is not in the 2xx range and you request that the response is converted to a specific type, like when calling

httpClient.target("https://example.com/endpoint").request(MediaType.APPLICATION_JSON_TYPE).get(MyBean.class);

In this case, an exception is thrown, but it contains a DummyResponse instance that doesn't give access to the Response object.
Being able to read the response from the server in case of error is extremely valuable in some cases, as it may contain useful information about the problem. Most REST APIs return this information as a JSON object that can be parsed, and the contents examined to see custom error status codes or passing the error message to the user, for example.

This PR sets the response in the exception object, without creating a new field for it, as implementing ResteasyReactiveClientProblem should prevent the server from trying to read the response.

geoand · 2025-02-07T06:22:03Z

Thanks for this.

It would be nice to have some tests covering the functionality

geoand · 2025-02-13T06:22:21Z

@FroMage WDYT?

rober710 · 2025-02-18T16:02:50Z

@geoand @FroMage Do you have any feedback or update on this?

geoand · 2025-02-19T06:48:46Z

I think it makes sense, but I want @FroMage to have a look

FroMage · 2025-02-24T10:59:03Z

...time/src/main/java/org/jboss/resteasy/reactive/client/api/WebClientApplicationException.java

- * Subclass of {@link WebApplicationException} for use by clients, which forbids setting a response that
- * would be used by the server.
- * FIXME: I'd rather this be disjoint from WebApplicationException, so we could store the response info
- * for client usage. Perhaps we can store it in an alternate field?


This explains that we did not want the client response to end up in the WebApplicationException.response field, and you're doing exactly that.
Doing this, we run the risk of user exception mappers that turn that Response into a server response, which is a huge security issue.
I'd much rather this response get added as a new field property getClientReponse(), would this work for you?

I was aware of this, but I decided to put it in the response field anyway because now the exception implements ResteasyReactiveClientProblem to prevent that. Having another field to hold the response may result confusing, but I can add that property.

It is not ideal, but the client web application exception type should be disjoint to the server web application exception type in an ideal world.

I think the (client) Response should go into another property, to make sure nobody ever treats this as a server response. The security risk is too great, in my opinion.

rober710 · 2025-02-26T15:00:03Z

...time/src/main/java/org/jboss/resteasy/reactive/client/api/WebClientApplicationException.java

 * would be used by the server.
- * FIXME: I'd rather this be disjoint from WebApplicationException, so we could store the response info
- * for client usage. Perhaps we can store it in an alternate field?
+ * FIXME: I'd rather this be disjoint from WebApplicationException, but changing the exception type could break existing


I considered not inheriting from WebApplicationException, as there's no use for it if I can't use it's response property... however, it may break code that is still catching it, right?
Let me know if this comment is accurate.
I'd rather have a separate exception type that has it's own response property to avoid confusion, though... right now people should know that the response is available in the new field instead of the standard one.

rober710 · 2025-02-26T15:00:57Z

...time/src/main/java/org/jboss/resteasy/reactive/client/api/WebClientApplicationException.java

+    }
+
+    public WebClientApplicationException(Response clientResponse) {
+        super("Server response is: " + clientResponse.getStatus(), new DummyResponse(clientResponse.getStatus(),


Preserved the dummy response in this case, so that it's not null and breaks existing code that expects to read the status code from the response field.

This is not right, it will set a Response property from the client's response, which is a security risk. The super.response field must always be null, and the response should be stored in the clientResponse property only.

🤔 That's how it was done before my change, and it looks like that's why the DummyResponse class was created for, so that clients can look at the status of the response from the server... If I set the response to null, then should I remove the DummyResponse class as well?

FroMage · 2025-03-11T16:32:17Z

...time/src/main/java/org/jboss/resteasy/reactive/client/api/WebClientApplicationException.java

+    }
+
+    public WebClientApplicationException(Response clientResponse) {
+        super("Server response is: " + clientResponse.getStatus(), new DummyResponse(clientResponse.getStatus(),


This is not right, it will set a Response property from the client's response, which is a security risk. The super.response field must always be null, and the response should be stored in the clientResponse property only.

FroMage · 2025-03-11T16:36:43Z

.../runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/RestClientRequestContext.java

                        + invokedMethod.getName() + "'";
            }
+            Response response = webApplicationException instanceof WebClientApplicationException wcae ? wcae.getClientResponse()
+                    : webApplicationException.getResponse();


@geoand I can't remember why we have ClientWebApplicationException and WebClientApplicationException when they implement the same types…

I don't remember at all, but the commit that introduced the former was 6674bd5

I wonder if this is not simply a mistake, I wonder what happens if we remove one of the two types…

We can always try :)

So, I can remove the lesser-used exception. Now, as it turns out, the other type has zero protection for hiding the Response like this type has.

If I add the protections to the other type, lots of tests fail, because clients expect the Response to be set when throwing, and unless we tell our users to stop using getResponse() and use getClientResponse() this won't work. And they likely will care that we break their applications this way, now.

Now, given the fact that we have the most used type of client exception actually contain a Response for years, and we've solved the security risk by marking both types of exception with a special marker interface, Quarkus REST is not vulnerable if the client exception's Response is set.

The only risk remaining is a user risk of adding their own ExceptionMapper or any type of filter and extracting client exception responses to turn them into a server response. I'm not sure we can fix that risk. Well, we could, I suppose also mark the client Response implementations and refuse to accept them as server responses. I'm not sure it's worth the effort.

WDYT @geoand ?

I've made an alternate PR at #46826 to remove the duplicate exception, and restore the client Response. Let's see what the full CI says. And what you think about the risk for users.

FroMage · 2025-03-18T13:56:54Z

We merged #46826 which has your commits, so in theory this should be fixed. Thanks a lot for your help and patience :)

rober710 · 2025-03-18T15:20:03Z

@FroMage Thank you very much! 👍

quarkus-bot bot added the area/rest-client label Dec 24, 2024

rober710 force-pushed the rest-client-response branch from b234baa to 4bb672d Compare December 24, 2024 14:51

This comment has been minimized.

Sign in to view

rober710 force-pushed the rest-client-response branch from 4bb672d to 8c7a5c3 Compare February 12, 2025 16:15

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Feb 12, 2025

FroMage requested changes Feb 24, 2025

View reviewed changes

rober710 force-pushed the rest-client-response branch from c719a91 to 881f277 Compare February 26, 2025 14:54

rober710 commented Feb 26, 2025

View reviewed changes

rober710 requested a review from FroMage February 26, 2025 15:01

This comment has been minimized.

Sign in to view

rober710 added 3 commits March 11, 2025 10:30

Make response available on error in rest client.

38ff374

Add tests to check response body can be read on error.

b7ca5bb

Move response to a separate property.

149d77b

rober710 force-pushed the rest-client-response branch from 881f277 to 149d77b Compare March 11, 2025 15:36

FroMage requested changes Mar 11, 2025

View reviewed changes

FroMage mentioned this pull request Mar 14, 2025

Make response available on error in rest client #46826

Merged

FroMage closed this Mar 18, 2025

quarkus-bot bot added the triage/invalid This doesn't seem right label Mar 18, 2025

rober710 deleted the rest-client-response branch March 18, 2025 15:20

Make response available on error in rest client #45282

Make response available on error in rest client #45282

Uh oh!

Conversation

rober710 commented Dec 24, 2024

Uh oh!

This comment has been minimized.

geoand commented Feb 7, 2025

Uh oh!

This comment has been minimized.

geoand commented Feb 13, 2025

Uh oh!

rober710 commented Feb 18, 2025

Uh oh!

geoand commented Feb 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FroMage commented Mar 18, 2025

Uh oh!

rober710 commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants