Make response available on error in rest client

[FroMage]

Alternate PR for #45282

This removes the duplicate web client exception, and restores the Response it contains.

[sberyozkin]

@FroMage I'd just like to check, as far as I recall, the reason it was done was to avoid sensitive headers contained in the error response leaked to the client. Does this PR change it, or is it totally unrelated to that constraint ?

[FroMage]

Yes, this would add the headers back, but that was already the case, as it turns out, in most of the cases, because we had another type of web client exception with zero protection for hiding the Response like this type has. And that type was used a lot more.

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.

[sberyozkin]

@FroMage

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.

Good to know, thanks, I believe tests are also available

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.

I agree, users who write client exception mappers should be able to see the response headers while Quarkus REST can't prevent the custom mappers leaking them by mistake.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 3b277e7.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/infinispan-cache/deployment

✖ io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls - History

• expected: "thread1" but was: "thread2" - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: 

expected: "thread1"
 but was: "thread2"
	at io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls(InfinispanCacheTest.java:283)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:521)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:435)

---

⚙️ JVM Integration Tests - JDK 17

📦 integration-tests/hibernate-reactive-panache

✖ io.quarkus.it.panache.reactive.DDLGenerationPMT.test - History

• 1 expectation failed. Response body doesn't match expectation. Expected: is "OK" Actual: {"details":"Error id 724b67fa-d6d8-4b93-8b21-41da9cfdc9f7-1","stack":""} - java.lang.AssertionError

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.
Expected: is "OK"
  Actual: {"details":"Error id 724b67fa-d6d8-4b93-8b21-41da9cfdc9f7-1","stack":""}

	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)

[geoand]

@FroMage I assume we should move ahead with this one?

[FroMage]

Yes

[FroMage]

I can't seem to be able to merge ATM, though. I'm getting errors.