Duplicate Location headers during OIDC redirection

[acoburn]

Describe the bug

For a Quarkus webapp using the oidc extension, during an OIDC authorization code flow, an app may be configured with a quarkus.oidc.authentication.redirect-path value.

When a client lands on the path defined in that configuration, as part of a standard OIDC flow, the Quarkus app will send a 302 redirect. In addition to the redirection status code, the response also includes two Location headers (Pragma and Cache-Control are also duplicated). Chrome and FF seem to handle this duplicated Location header fine, but Safari attempts to concatenate the two URLs into a composite URL, which generally will result in a 404 on an app.

This issue was not present in 2.14.2, but has been present since.

The relevant commit appears to be related to 07b1e09#diff-ff8824242a94a73be9ed56f63521b1d690e611927625d46c06d227a3e1846596R342 where the AuthenticationRedirectException proceeds to a further step instead of just ending, which was the earlier behavior.

Expected behavior

The path defined at quarkus.oidc.authentication.redirect-path should produce a redirection response with a single Location header

Actual behavior

The path defined at quarkus.oidc.authentication.redirect-path should produce a redirection response with two Location headers

How to Reproduce?

https://github.com/acoburn/qurakus-oidc-demo

1. Build the project.
2. Add the relevant configuration, pointing to a running authorization server as described in the README
3. Visit http://localhost:8080 and login, observing the network tab in a browser (or try using Safari)

Output of uname -a or ver

No response

Output of java -version

openjdk version "11.0.17" 2022-10-18

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.15.1 (this affects versions starting with 2.14.3)

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)

Additional information

Viewing the network tab while using Chrome on Ubuntu:

[quarkus-bot]

/cc @pedroigor(oidc), @sberyozkin(oidc)

[sberyozkin]

Hmm. Looks like AuthenticationRedirectException is handled twice. @michalvavrik Does it ring any bell ? Np if not, I'll have a look soon then

[michalvavrik]

@sberyozkin it rings a bell, I'm uncertain as Aaron mentions it started with 2.14.3 as this is RR + proactive auth, which should not be issue there. It must be because both headers set on RoutingEvent (default auth failure handler) and AuthenticationRedirectExceptionMapper are kept (merged). I remember testing this, but... I'll check shortly if I'm right.

[sberyozkin]

Thanks @michalvavrik May be the simplest option is to make sure the headers are not added but set, but I guess we don't want a double handling for the authentication exceptions (not sure if the problem is only related to the auth redirect exception)

[michalvavrik]

I have pretty good idea what happens in all scenarios, only thing that surprised me is that RR adds headers to existing map which means everything that has been set before stays there but it if exception mappers sets same headers, they override it. Now that I read the code, it makes absolute sense and it's a good thing for other scenarios - user can override response status with exception mappers, but if only thing that he does is sets custom response body, result of challenge sticks - safer.

This issue

AuthenticationRedirectExceptionMapper : To prevent double handling means to have exceptions mappers aware of vertx-http security as the mapper would have to know it should not act in certain scenarios. The mapper does exactly same thing as auth failure handler does - sets headers and response status. Only difference between them is case sensitivity.

If we prevent mapper from handling this exception, we only should do it when routing context already has headers set, as this mapper is called in other scenarios too. Please bear in mind exception handling flow is very complex, I was quite happy it works. Also it is a good thing that event is handled by that mappers as it's default one, and if user defined custom one, if would replace this mapper (btw = workaround for @acoburn ), so we should not prevent flow to go there, question is what we do there!

I suggest we makes sure the mapper and auth failure handler sets same case sensitive headers (lower case would be easier) or in mapper we remove incorrect ones. I think I could find situations where same header is set twice in security code too.

not sure if the problem is only related to the auth redirect exception

AuthenticationCompletionException doesn't set headers, only status and AuthenticationFailedException sets headers that are result of challenge and no others in io.quarkus.resteasy.reactive.server.runtime.exceptionmappers.SecurityExceptionMapperUtil#handleWithAuthenticator. I don't think there is an issue, but it strictly depends on definition on "double handling". You need to be judge here :-) I can see AuthenticationFailedException has location twice (same case). I'll to proper way how to prevent it.

[michalvavrik]

@sberyozkin ^^

[michalvavrik]

I wrote long text above, I can try to make it shorter: IMHO it's a good thing that auth failure handler handles the exception when proactive=true (this case) and than let user/extensions to customize response via failure handler. Here, RR failure handler handles ex. to mapper that sets same headers with different case sensitivity. To this point is IMHO not double handling (not arguing, I'm interested in other opinions, please). Now exception mappers is called in all scenarios and the behavior is close to correct - it sets correct headers and status, but case differs.

UPDATE: Pragma is twice in response with same case, response headers are multimap

[michalvavrik]

re reproducer - it is possible to skip step 2. as dev services helps there. just run quarkus dev

[michalvavrik]

@sberyozkin if you want I can create PR and we can discuss solutions there in case you won't be complacent with mine. I won't do it for now as you mentioned you are going to look into issue and I'm interested in your responses to ^^^.

[sberyozkin]

Sorry @michalvavrik, struggling with my laptop's display going blank, which does not help.

So, as far as the possible fix is concerned, you propose to make sure that both the AuthenticationRedirectionExceptionMapper and the default handler use the same case for these headers (I propose the camel case) - makes sense. What I'm not sure if why both handlers, the exception mapper and the default handler react to this exception, sorry if I'm missing something

[michalvavrik]

@sberyozkin I've moved on meanwhile, I'll prepare solution where we prevent double handling. sorry for comments above, it was WIP. you'll see tomorrow.

[sberyozkin]

Thanks @michalvavrik, good night