Skip to content

Conversation

@johannaengland
Copy link
Contributor

@johannaengland johannaengland commented Jun 18, 2025

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.

@johannaengland johannaengland requested review from a team, hmpf and lunkwill42 June 18, 2025 12:23
@johannaengland johannaengland self-assigned this Jun 18, 2025
@sonarqubecloud
Copy link

@johannaengland johannaengland force-pushed the add-csrf-middleware branch 2 times, most recently from ecc8500 to eaefa8f Compare August 11, 2025 12:45
@sonarqubecloud
Copy link

@codecov
Copy link

codecov bot commented Aug 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.21%. Comparing base (35ca868) to head (84d51ef).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3396      +/-   ##
==========================================
+ Coverage   62.20%   62.21%   +0.01%     
==========================================
  Files         611      611              
  Lines       44939    44941       +2     
  Branches       43       43              
==========================================
+ Hits        27955    27961       +6     
+ Misses      16974    16970       -4     
  Partials       10       10              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@johannaengland
Copy link
Contributor Author

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

@lunkwill42
Copy link
Member

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.

@lunkwill42
Copy link
Member

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.

@Simrayz
Copy link
Contributor

Simrayz commented Aug 26, 2025

I have added fixes for #3437, #3438, #3439 and #3441 in PR #3465

I have not attempted to fix #3444, as I'm not sure how to get the PortAdmin tool to work locally (the views crash for me).

@Simrayz
Copy link
Contributor

Simrayz commented Sep 22, 2025

@lunkwill42 Should we resolve this bug #3472 before merging this PR? Should be fairly simple by only rendering the {% csrf_token %} if the current method is not get

@johannaengland
Copy link
Contributor Author

@lunkwill42 Should we resolve this bug #3472 before merging this PR? Should be fairly simple by only rendering the {% csrf_token %} if the current method is not get

@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

@Simrayz
Copy link
Contributor

Simrayz commented Sep 23, 2025

I searched the code base and made a list of all usage of {% csrf_token %} with a method="get" form.
#3472 (comment)

@johannaengland johannaengland linked an issue Oct 2, 2025 that may be closed by this pull request
@lunkwill42
Copy link
Member

@lunkwill42 Should we resolve this bug #3472 before merging this PR? Should be fairly simple by only rendering the {% csrf_token %} if the current method is not get

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)

@sonarqubecloud
Copy link

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 this pull request may close these issues.

Look into Django's CSRF protection

4 participants