Only allow a single Location to be set in RESTEasy Reactive

[geoand]

• Fixes: RESTEasy Reactive and security: invalid response after challenge and exception mapper, two Location headers #38523

[FroMage]

I could try that, but I've a feeling this is only going to work for the narrow use-case of Response having a second Location header. Fair enough that was the reported issue, but I have a feeling that if the Response does not have a Location header, the previous one should be removed, no? This would be especially true if the exception handler's overridden Response is not a redirect.

Or perhaps it's the problem really is elsewhere and the security handler's challenge should not set anything on the Vert.x response before it fires an authentication exception? And it's the default exception handler which should build a proper Response? This way any user overriding the exception handler would not have to deal with things already set in the Vert.x reponse?

[geoand]

I don't know how we can decide we should be removing headers

[FroMage]

Well, let's suppose an exception handler for AuthorizationFailedException returns a Response with 200 and some HTML.

Leaving the Location in the headers will at best be ignored, at worse cause an invalid HTTP validation on the client.

So, I wonder if there are cases where we set headers prior to getting a Response that we need to keep? Perhaps auth is the only place where we build headers on the Vert.x response before delegating to RR?

I think it's worth sending a CI run where in the presence of a Response object we log all removed headers and remove them. Then we can see how many tests fail and perhaps get a clue as to what headers we've been setting before the right time?

[geoand]

So, I wonder if there are cases where we set headers prior to getting a Response that we need to keep? Perhaps auth is the only place where we build headers on the Vert.x response before delegating to RR?

Theoretically, any Vertx handler running before RR could set anything it likes. Practically, what you're saying is probably correct.

I think it's worth sending a CI run where in the presence of a Response object we log all removed headers and remove them. Then we can see how many tests fail and perhaps get a clue as to what headers we've been setting before the right time?

Sure, go ahead

[FroMage]

Alright, let's see.

[FroMage]

Fire: #38641

[FroMage]

Well, I suppose Content-Type and Content-Length are definitely set before we look at the Response.

[FroMage]

OK, same player shoots again.

[FroMage]

Also transfer-encoding: chunked but that's also related to the body.

In the TCK:

• Vary: Accept, Accept-Language, Accept-Encoding from JAXRSClient0129.selectVariantResponseVaryTest
• OPERATION: PROCEEDTHROWSWEBAPPEXCEPTION from JAXRSClient0096.proceedThrowsWebApplicationExceptionTest

The first one calls Request.selectVariant which has this NASTY side-effect: requestContext.serverResponse().setResponseHeader(HttpHeaders.VARY, varyHeader); but the spec says A vary header is computed from the supplied list and automatically added to the response so this is a valid side-effect. This is really weird, though. Bad API.

I'm not sure how the second one gets written, though. It appears that the test returns a Response with that header set in it. Then the text suggests that there's a writer interceptor that throws a WebApplicationException, and I presume we set the header first legitimally, then the exception gets thrown and we get to header writing again. I suppose this test might pass if I removed the header on the exception handling. I'll try.

[FroMage]

Let's see how many other cases there are, restarted again with tweaks to let those slide.

[FroMage]

New test failing is CustomFiltersTest which has request and response filters which set headers directly on the Vert.x response.

This makes me realise that there's bound to be filters that will set response headers, either via the JAX-RS API ContainerResponseContext.getHeaders() which is mutable, or directly via Vert.x.

I suppose those use-cases are valid, in many cases. I'm not entirely sure what the JAX-RS spec says about header composition when we have the Vary special-case (as set by side-effect of Request.selectVariant), response filter headers, and Response headers. And then there are content headers, defined by serialisers, before Response headers are added.

This all appears to be a mess with no real semantics defined. In the sense that it's not clear at all how a Response can remove headers previously set.

Perhaps this is not a problem we can solve today. Perhaps indeed you're right and we should concentrate on Location, at the very least making sure there are not two headers, as your PR is doing.

Perhaps we can also remove any pre-existing Location header if the status is not 3xx or 201, since those are the only valid status codes for this header according to the HTTP spec, but the spec doen't say a Location header is invalid for other status codes either, so we can hope it's just ignored in the other cases and delay fixing this until someone reports an issue.

In any case, I think your solution is the best for now, and we've documented why, so in the future we'll have this info at hand.

[geoand]

This all appears to be a mess with no real semantics defined. In the sense that it's not clear at all how a Response can remove headers previously set.

Yeah...

In any case, I think your solution is the best for now, and we've documented why, so in the future we'll have this info at hand.

Thanks for doing a deep dive on this!

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 086cc01.

✅ 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.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

• Assertion condition Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)