search

5monkeys / django-bananas

Django extensions the monkey way
MIT License
66 stars 17 forks source link

LoginView missing CSRF protection #49

Open flaeppe opened 4 years ago

flaeppe commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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.