quarkus-renarde icon indicating copy to clipboard operation
quarkus-renarde copied to clipboard

Rename CRSF to CSRF

Open oliv37 opened this issue 2 years ago • 6 comments

The acronym for Cross-Site Request Forgery is CSRF not CRSF

The class CRSF.java should be renamed as well as all mentions of crsf in the documentation.

oliv37 avatar Nov 06 '22 22:11 oliv37

Haha, yes. I was going to replace it with https://quarkus.io/guides/security-csrf-prevention but I realised this would cause all POST methods to suddenly return a 400 if the token is not included. Which is exactly what we want for HTML/form requests, but not for APIs, so this might break some existing code.

Any opinion @ia3andy ?

FroMage avatar Nov 08 '22 15:11 FroMage

Which is exactly what we want for HTML/form requests, but not for APIs

It's not always true, some APIs might use cookies to store the session id, in this case CSRF protection is needed

oliv37 avatar Nov 08 '22 16:11 oliv37

Any opinion @ia3andy ?

@FroMage I am not sure I get the reason why renaming will make APIs requests fail?

ia3andy avatar Nov 09 '22 08:11 ia3andy

It's not renaming that makes it fail. It's depending on quarkus-security-csrf-reactive because you won't be able to do any POST request without a CSRF token that you first GET.

FroMage avatar Nov 09 '22 10:11 FroMage

In Play it was not automatic, you had to call checkAuthenticity() from your controller methods to check the CSRF token, making it opt-in. Perhaps this is what's missing for an app that does REST APIs and HTML.

FroMage avatar Nov 09 '22 10:11 FroMage

In Play it was not automatic, you had to call checkAuthenticity() from your controller methods to check the CSRF token, making it opt-in. Perhaps this is what's missing for an app that does REST APIs and HTML.

I think making it opt-in with an annotation could be a good solution?

Maybe it should be part of quarkus-security-csrf-reactive to allow a more granular usage if needed (configurable to either all or opt-in with the annotation).

ia3andy avatar Nov 09 '22 10:11 ia3andy

I have a branch resolving this by using the new Quarkus CSRF module, but it's not working ATM so I have to wait for it to be fixed.

FroMage avatar Dec 14 '22 15:12 FroMage

OK, finally, it's done!

FroMage avatar Jun 28 '23 15:06 FroMage