quarkiverse / quarkus-renarde

Server-side Web Framework with Qute templating, magic/easier controllers, auth, reverse-routing
Apache License 2.0
78 stars 19 forks source link

Rename CRSF to CSRF #78

Closed oliv37 closed 1 year ago

oliv37 commented 1 year ago

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.

FroMage commented 1 year ago

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 ?

oliv37 commented 1 year ago

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

ia3andy commented 1 year ago

Any opinion @ia3andy ?

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

FroMage commented 1 year ago

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 commented 1 year ago

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.

ia3andy commented 1 year ago

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

FroMage commented 1 year ago

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 commented 1 year ago

OK, finally, it's done!