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

LoginView missing CSRF protection #49

Open
flaeppe opened this issue Feb 12, 2020 · 3 comments · May be fixed by #74
Open

LoginView missing CSRF protection #49

flaeppe opened this issue Feb 12, 2020 · 3 comments · May be fixed by #74

Comments

@flaeppe
Copy link
Contributor

flaeppe commented Feb 12, 2020

Correct me if I'm wrong or missing something, although:

It seems to me that the LoginView.create is lacking CSRF protection.

Citing the DRF docs regarding SessionAuthentication:

CSRF validation in REST framework works slightly differently to standard Django due to the need to support both session and non-session based authentication to the same views. This means that only authenticated requests require CSRF tokens, and anonymous requests may be sent without CSRF tokens. This behaviour is not suitable for login views, which should always have CSRF validation applied.

We can see that DRF's GenericViewSet inherits skipping of CSRF checking and the SessionAuthentication class skips CSRF checking as the login view's permission class requires the user to be anonymous. (LoginView inherits the SessionAuthentication from here)

The csrf_protect decorator should probably be set similarly to how Django's LoginView does it.

@Mojken
Copy link

Mojken commented Nov 11, 2021

This seems like a severe security issue as well as a one-line fix, how come it remains an ignored issue almost a year later? Should I go ahead and add the decorator as a PR?

@flaeppe
Copy link
Contributor Author

flaeppe commented Nov 11, 2021

@Mojken I don't think it's any major issue in general, as long as the site doesn't allow visitors to register themselves for the Django admin site.

Should I go ahead and add the decorator as a PR?

Sure, go ahead if you like!

@Mojken Mojken linked a pull request Nov 11, 2021 that will close this issue
@Mojken
Copy link

Mojken commented Nov 11, 2021

I did not realize it was the Django admin login only, that does explain why this wasn't resolved earlier. Still, nice to have this in case somebody accidentally exposes the page or some such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants