Add CSRF middleware to enable CSRF protection#3396
Add CSRF middleware to enable CSRF protection#3396lunkwill42 merged 4 commits intoUninett:masterfrom
Conversation
|
1a2b897 to
a7105b7
Compare
ecc8500 to
eaefa8f
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3396 +/- ##
==========================================
+ Coverage 63.32% 63.33% +0.01%
==========================================
Files 619 619
Lines 45873 45875 +2
Branches 43 43
==========================================
+ Hits 29051 29057 +6
+ Misses 16812 16808 -4
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lunkwill42
left a comment
There was a problem hiding this comment.
While this PR in itself is just fine, there's a requirement that all of NAV's forms have been updated to work properly with CSRF before this can be merged.
I tested random parts of NAV's web UI with this branch checked out, and I did find at least one showstopper and one minor issue within 10 minutes of testing:
Showstopper
Netmap allows its view configurations to be saved in named views. It appears the forms to save new or update existing views do not include a CSRF token, causing all of them to fail. We should make an issue for this.
Minor issues
CSRF tokens are added to some GET-based forms, which should be unncessary. First, they do not modify anything, so they should be safe. Second, it looks pretty confusing to the user that the URL now includes a long hexadecimal string (which also makes the URL potentially less bookmarkable).
Specifically, I found the l2trace search form to do this (there might be more, since my tests haven't been exhaustive)
Yes, this was added in #3157, were it was mentioned that "there is no (reliable) way to check whether a Django form is a POST form thats why this redundancy is needed". |
Still not a good idea. Can it be turned off? Putting CSRF tokens in GET requests will leak those tokens in the server access logs, since they are part of the requested URL. |
|
At this point, I've identified and created issues for CSRF problems in at least 4 tools. These have all been linked to this PR. I suspect there may also be something with PortAdmin, but it will take me longer to test, since it requires me to set up working IPv6 in Docker to do the testing from my dev machine against our internal network. |
eaefa8f to
5a26986
Compare
|
@lunkwill42 Should we resolve this bug #3472 before merging this PR? Should be fairly simple by only rendering the |
@Simrayz have you had a look if the cases you solved in #3465 are the only cases where the frontend makes post requests? Because then we can go that route |
|
I searched the code base and made a list of all usage of |
I think it's a good idea to solve #3472 before merging this, yes. We need to do some serious testing of this PR before it's merged (there will be more manual testing, since we don't have proper coverage of the UI) |
5a26986 to
84d51ef
Compare
|
af56ee1 to
baade14
Compare
baade14 to
8252341
Compare
|
|
Just rebased this on the newest master, which includes #3783 I think we should be reasonably safe that this works for most cases by now. |
lunkwill42
left a comment
There was a problem hiding this comment.
I think we've closed most CSRF issues in the backend/frontend now, and are ready to let this loose on users (and let them find any remaining issues).
|
Aaah, not quite, there's #3791 in the way (the JWT forms were a recent addition) |
Closes Uninett/campus-tasks/issues/45.
Reference: https://docs.djangoproject.com/en/4.2/howto/csrf/
Reference for exempting views from needing a CSRF token: https://docs.djangoproject.com/en/4.2/howto/csrf/#disabling-csrf-protection-for-just-a-few-views
Dependent on #3366, #3367, #3384, #3387, #3388, #3389, #3390, #3391, #3394, #3395, #3465.
Blocked by #3472 and #3563.