go-pkgz / auth

Authenticator via oauth2, direct, email and telegram

https://go-pkgz.umputun.dev/auth/

MIT License

1.07k stars 84 forks source link

Implement XSRFIgnoreMethods #207

Closed oalexander6 closed 4 months ago

oalexander6 commented 4 months ago

XSRFIgnoreMethods

Background

I discovered while using this library that the current XSRF protections do not allow for a GET request sent by the browser on protected routes, due to requiring the presence of the XSRF token in a header. This is a problem for applications that are not SPAs and send GET requests to the server to retrieve a new HTML page. In these situations, the common pattern is to skip XSRF checks on GET requests.

Solution

To resolve this, I implemented a configuration option called XSRFIgnoreMethods, allowing the user to specify a comma-separated list of HTTP methods for which XSRF checks should be skipped when extracting and validating the JWT. I am open to any feedback, modifications, or discussions about this.

umputun commented 4 months ago

isn't this already exist with https://github.com/go-pkgz/auth/blob/master/auth.go#L106 https://github.com/go-pkgz/auth/blob/master/auth.go#L106 ?

oalexander6 commented 4 months ago

No, in this situation I still definitely want XSRF protections on POST, PUT, DELETE, etc. but specifically not for GET requests. The DisableXSRF option disables it for all methods.

oalexander6 commented 4 months ago

This PR would allow for ignoring just certain HTTP methods, by setting XSRFIgnoreMethods: []string{"GET", "HEAD"} for example, to just skip the XSRF checks for "GET" and "HEAD" only

umputun commented 4 months ago

This PR would allow for ignoring just certain HTTP methods, by setting XSRFIgnoreMethods: []string{"GET", "HEAD"} for example, to just skip the XSRF checks for "GET" and "HEAD" only

Unless I missed something, this code does not expect a []string with all the ignored methods but rather a single string (see my comment in the review above).

umputun commented 4 months ago

one more thing - this change should be also documented in readme. Maybe a section about "Options to ignore XSRF protections" or smth like what showing how to do it and providing a basic explanation why this may be needed

oalexander6 commented 4 months ago

one more thing - this change should be also documented in readme. Maybe a section about "Options to ignore XSRF protections" or smth like what showing how to do it and providing a basic explanation why this may be needed

Will do

oalexander6 commented 4 months ago

The requested changes have been made. Thanks for the feedback.