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

CSRF extension breaks applications due to required config #35530

Closed
FroMage opened this issue Aug 24, 2023 · 7 comments · Fixed by #35538
Closed

CSRF extension breaks applications due to required config #35530

FroMage opened this issue Aug 24, 2023 · 7 comments · Fixed by #35538
Assignees
Labels
Milestone

Comments

@FroMage
Copy link
Member

FroMage commented Aug 24, 2023

CSRF, after #34555 got merged, now requires a config value which should have a default, thus breaking the Quarkus tenet that importing an extension should not break an application by requiring things that have proper defaults.

As such, it breaks every extension user, even if they're not using headers for CSRF, so that's pretty serious.

@FroMage FroMage added the kind/bug Something isn't working label Aug 24, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 24, 2023

/cc @pedroigor (bearer-token), @sberyozkin (bearer-token,jwt,security)

@geoand
Copy link
Contributor

geoand commented Aug 24, 2023

We should definitely have some kind of test for this...

@FroMage are you planning on addressing this issue?

@sberyozkin
Copy link
Member

@FroMage Can you clarify please ? What exactly is broken ?

@sberyozkin
Copy link
Member

sberyozkin commented Aug 24, 2023

@FroMage Or yeah, I'll just make it optional, tokenHeader - it is on main only so we have time to fix

@sberyozkin
Copy link
Member

@FroMage Sigh, the whole optional tokenHeader discussion is back, I've re-opened it at #34555.

One thing that I don't think we can do is to add support for the header at the cost of dropping support for the hidden form fields - which will happen if we have a default header value. Supporting the internal fields for HTML form requests is essential.

@sberyozkin
Copy link
Member

@FroMage Ignore me please, just configuring the default header value does not mean it will be present, should be a simple fix

@sberyozkin
Copy link
Member

@FroMage This is fixed, see the linked PR, I now recall that this property was originally optional and hence I had it configured, then we agreed it should not be optional but I forgot to remove the test configuration which is why I missed that it had no default value...

@quarkus-bot quarkus-bot bot added this to the 3.4 - main milestone Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants