-
Notifications
You must be signed in to change notification settings - Fork 716
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
ENFORCE CSRF verification in API to be accessed by a browser strictly #11978
ENFORCE CSRF verification in API to be accessed by a browser strictly #11978
Conversation
Build Artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking promising - a few places where I don't think it's needed, and a few more places where I think it may be.
I would like to see some unit tests of these API endpoints just to guarantee this CSRF enforcement in the future.
kolibri/core/content/api.py
Outdated
@@ -1387,6 +1388,7 @@ class Meta: | |||
fields = ("contentnode_id", "contentnode_id__in") | |||
|
|||
|
|||
@method_decorator(csrf_protect, name="dispatch") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - I think we should actually just add the IsAuthenticated
permissions class to this viewset, as it requires a logged in user in the get_queryset
method, so would just give a 500 for an anonymous user, I'd guess?
This might be worth doing in 0.16 instead, if you could open a separate PR to add just that onto the release-v0.16.x branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i will add IsAuthenticated permissions to this in a seperate PR
And will remove csrf changes from this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I have added some unit tests for the API's those are modified by this PR. If any of them are not doing the thing or is not at the right file, let me know, i will change it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent - love the tests, just a little bit more cleanup to do, and clarity over the GET behaviour of the session viewset.
Excellent work, thank you! |
Summary
Enforces CSRF Verification more strongly in post api endpoints which are supposed to be accessed only by browser
…
References
Fixes #7389
…
Reviewer guidance
I performed Manual QA on the following API's:(except for setupwizard, i was confused with the actual api of that)
api/auth/session/
api/auth/facility/
api/content/contentrequest/
api/auth/signup/
api/tasks/tasks/
api/device/deviceprovision/
I ran curl commands like:
I did the above for all API's i have changed here, without my changes i was in the API but with it i wasn't able to access it
…
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)