Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
FroMage opened this issue Feb 1, 2024 · 9 comments · Fixed by #38554

Comments

@FroMage
Copy link
Member

FroMage commented Feb 1, 2024

I'm getting an auth challenge from HttpAuthenticationMechanism.ChallengeSender which sets the response code and adds a Location header directly to the Vert.x response, and then an AuthenticationFailedException is thrown, resulting in my @ServerExceptionMapper being called and also setting a Location by returning a Response.seeOther() and we end up with two Location headers:

HTTP/1.1 303 See Other
Location: http://localhost:8080/_renarde/security/login
Location: http://localhost:8080/
Set-Cookie: QuarkusUser=;Version=1;Path=/;Max-Age=0
content-length: 0
set-cookie: _renarde_flash=eyJtZXNzYWdlIjoiSW52YWxpZCBzZXNzaW9uIChiYWQgc2lnbmF0dXJlKSwgeW91J3ZlIGJlZW4gbG9nZ2VkIG91dCJ9; Path=/; HTTPOnly; SameSite=Lax
set-cookie: quarkus-redirect-location=http://localhost:8080/; Path=/

I suspect that at the very least, any Response data set by the exception mapper should override the ones set previously, if only because the Response is not meant to come on top of anything prior to it, but override it from scratch. So, the exception mapper's Location should "win" and override the security challenge one.

I wonder what other headers are set directly on the Vert.x response, before we translate the Response headers onto it?

This was first reported in quarkiverse/quarkus-renarde#194

Copy link

quarkus-bot bot commented Feb 1, 2024

/cc @geoand (resteasy-reactive), @sberyozkin (security), @stuartwdouglas (resteasy-reactive)

@sberyozkin
Copy link
Member

Hi @FroMage Where is the Location header set in Quarkus, which code set its value to http://location:8080/ ?

I suspect that at the very least, any Response data set by the exception mapper should override the ones set previously, if only because the Response is not meant to come on top of anything prior to it, but override it from scratch.

It does appear to be the right thing to do

@FroMage
Copy link
Member Author

FroMage commented Feb 1, 2024

Hi @FroMage Where is the Location header set in Quarkus, which code set its value to http://location:8080/ ?

HttpAuthenticationMechanism.ChallengeSender

@sberyozkin
Copy link
Member

sberyozkin commented Feb 1, 2024

@FroMage Sorry, yes, you mentioned it, I wonder why are header values different, looks like some authentication mechanism (which one ?) and the custom exception mapper use different Location header value calculation... Just curious if there is something in Quarkus that may need to be tuned...
Duplication is still a problem though...

@FroMage
Copy link
Member Author

FroMage commented Feb 1, 2024

That's not really the point, the issue is that RESTEasy Reactive should overwrite any header already set in the vert.x response, when it maps the JAX-RS Response onto the Vert.x response.

@geoand
Copy link
Contributor

geoand commented Feb 1, 2024

I agree, although I do have a fear that we've faced the inverse problem in the past and hence the current behavior

@FroMage
Copy link
Member Author

FroMage commented Feb 1, 2024

If that's the case, I pray we added a test for the behaviour, which we'll break and find the issue we wanted to fix with this behaviour. Then discuss this further :)

@geoand
Copy link
Contributor

geoand commented Feb 1, 2024

Yeah, I have to check

@geoand
Copy link
Contributor

geoand commented Feb 2, 2024

@FroMage can you try #38554 ?

geoand added a commit to geoand/quarkus that referenced this issue Feb 9, 2024
geoand added a commit that referenced this issue Feb 9, 2024
Only allow a single Location to be set in RESTEasy Reactive
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants