Jakarta - EE 10 - RestClientExceptionTest.testExceptionCaught failure

[gsmet]

With the current state of jakarta-rewrite (for more information, see https://github.com/quarkusio/quarkus/tree/main/jakarta#build-locally), we have the following failure:

[ERROR] io.quarkus.restclient.exception.RestClientExceptionTest.testExceptionCaught  Time elapsed: 0.01 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <null> but was: <http://localhost/private-service>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertNull.failNotNull(AssertNull.java:50)
	at org.junit.jupiter.api.AssertNull.assertNull(AssertNull.java:35)
	at org.junit.jupiter.api.AssertNull.assertNull(AssertNull.java:30)
	at org.junit.jupiter.api.Assertions.assertNull(Assertions.java:275)
	at io.quarkus.restclient.exception.RestClientExceptionTest.testExceptionCaught(RestClientExceptionTest.java:44)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:486)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:400)

My guess is that a behavior has changed in RESTEasy but I want us to get more details about what is exactly going on before involving James Perkins.

[gsmet]

@Sgitario I wonder if you could have a look at that one and try to understand the change of behavior in RESTEasy 6.1?

[quarkus-bot]

/cc @Sgitario, @cescoffier, @geoand

[Sgitario]

This failure is related to this change: resteasy/resteasy#3034.
Before these changes, the response was sanitized (see third parameter in line https://github.com/resteasy/resteasy/blob/61fa657f7612f2897cd10e0f761dfbbd514161ee/resteasy-client-api/src/main/java/org/jboss/resteasy/client/exception/ResteasyWebApplicationException.java#L25).
After these changes, the response is simply propagated and then all the previous headers are propagated as well, including the location (see the same line: https://github.com/resteasy/resteasy/blob/4e41a4386f00b5291a5336397ff64716ebc07251/resteasy-client-api/src/main/java/org/jboss/resteasy/client/exception/ResteasyWebApplicationException.java#L26).

Checking the PR and the linked JIRA, this change was intended. Note that this change is mentioned in the release notes, in the section of bugs "[RESTEASY-3096] - Resteasy new WebApplicationExceptions behavior".

Therefore, we could fix this error by changing our FrontendService.exceptionCaught (see class in here) to:

@GET
    @Path("/exception-caught")
    public String exceptionCaught() {
        try {
            return client.getData();
        } catch (WebApplicationException ex) {
            Response r = ex.getResponse();
            if (ex instanceof ResteasyWebApplicationException) {
                throw new WebApplicationException(((ResteasyWebApplicationException) ex).getSanitizedResponse());
            }

            throw new WebApplicationException(r);
        }
    }

Now, all the tests pass.

Let me know if you want me to proceed with this change.

[gsmet]

@Sgitario thanks for digging into this. TBH, I'm not sure what to do. Fixing the test means that anyone using this pattern might have information leaking to the users... which is quite bad. Especially since this test is there exactly to make sure this behavior is not happening. And that we made explicit changes in RESTEasy for it not to happen.

@jamezp could we discuss this change with @sberyozkin and @Sgitario when you're back from PTO? I'm pretty sure it opens a can of worms.

[jamezp]

I will try to create a reproducer test in RESTEasy. The tests RESTEasy has passed without changes. However, that doesn't mean we're testing it the same way. The tests RESTEasy has are pretty complex, so I'll create a new one which does something similar to what Quarkus is testing.

As far as changing the Quarkus test, I agree that seems wrong. Especially having to use some internal RESTEasy API to get the test to pass.

[jamezp]

I'm struggling a bit with this one. I was only marginally involved in the original decision on sanitizing like this. It is a bit painful from a user perspective to get a real result of the failed response. It would require knowledge of the implementation, RESTEasy in this case, which makes applications not very portable.

I do however understand the possible security issues. The question to me is how far to do we go to protect a user from bad practices?

@ronsigal What do you think here?

[gsmet]

I haven't followed the original discussion closely either but my understanding is that this private information exposure was actually the result of a usage that was not considered bad practice but normal practice. At least, it wasn't immediately obvious for the users that they were doing crazy things.

[jamezp]

Since there is a way to disable the feature, I think for now we should revert resteasy/resteasy#3034. The idea made sense when I looked at it, but I can see clearer now with the Quarkus test how this needs to be re-thought.

[jamezp]

The revert PR resteasy/resteasy#3279