fix: use CsrfExemptSessionAuthentication in XQueueViewSet#352
fix: use CsrfExemptSessionAuthentication in XQueueViewSet#352blarghmatey wants to merge 5 commits into
Conversation
The XQueue API is a service-to-service interface used by xqueue-watcher. When the watcher POSTs to put_result with a valid session, DRF's SessionAuthentication enforces CSRF for authenticated requests. The watcher has no reliable way to obtain a CSRF token (the login endpoint only accepts POST, so the pre-login GET returns 405 with no cookie set). Extract CsrfExemptSessionAuthentication into submissions/authentication.py and use it in XQueueViewSet. CSRF is not appropriate for this API; authorization is already enforced by the IsXQueueUser permission class. The test module previously defined its own local copy of this class for test setup — update it to import from the production module instead.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @blarghmatey! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #352 +/- ##
==========================================
+ Coverage 95.39% 95.40% +0.01%
==========================================
Files 23 24 +1
Lines 2994 3004 +10
Branches 126 126
==========================================
+ Hits 2856 2866 +10
Misses 124 124
Partials 14 14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR centralizes a CSRF-exempt session authentication class and applies it to the XQueue ViewSet so xqueue-watcher can call the service-to-service API without CSRF tokens.
Changes:
- Added
CsrfExemptSessionAuthenticationto the submissions app. - Switched
XQueueViewSetto use the new CSRF-exempt session authentication. - Updated tests to import the shared authentication class instead of defining a local test-only subclass.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| submissions/views/xqueue.py | Uses the new CSRF-exempt session authentication for XQueue endpoints. |
| submissions/tests/test_viewsets.py | Removes the test-local CSRF-exempt auth class and imports the shared one. |
| submissions/authentication.py | Introduces CsrfExemptSessionAuthentication to bypass CSRF enforcement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Expand docstring to document the security constraint: the 'xqueue' group must be restricted to dedicated service accounts and never granted to regular browser-accessible user accounts, as doing so would reintroduce a CSRF attack surface on the state-changing endpoints (put_result, logout). - Add test_put_result_succeeds_without_csrf_token which uses APIClient(enforce_csrf_checks=True) to exercise the actual DRF-level CSRF enforcement path, confirming that CsrfExemptSessionAuthentication bypasses it and that put_result returns 200 without a CSRF token.
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
Before the login POST it does a pre-fetch GET to the same URL to obtain a CSRF cookie — but the login action is declared methods=['post'] only, so the GET returns 405 with no cookie set. The watcher therefore has no CSRF token to send, and when it POSTs to put_result/ with a valid session, DRF's SessionAuthentication.enforce_csrf() fails the check and returns 403.
Could this be solved by having the CSRF cookie sent in the login GET then? I am a bit nervous about the idea that we are relying on xqueue users never overlapping with real users.
The root cause of the original 403 on put_result was that
xqueue-watcher pre-fetches GET /xqueue/login/ to obtain a CSRF
cookie before POSTing credentials, but the action only accepted
POST, so the GET returned 405 and no cookie was set.
Instead of bypassing CSRF enforcement entirely
(CsrfExemptSessionAuthentication), accept GET on the login action
and call get_token() to ensure the csrftoken cookie is present in
the response. xqueue-watcher already reads that cookie and attaches
its value as the X-CSRFToken header on every subsequent mutating
request, so standard SessionAuthentication now works without any
CSRF exemption.
- Remove submissions/authentication.py (no longer needed)
- Revert XQueueViewSet.authentication_classes to SessionAuthentication
- Replace test_put_result_succeeds_without_csrf_token with:
- test_login_get_sets_csrf_cookie: verifies the GET sets the cookie
- test_xqueue_watcher_csrf_flow: end-to-end flow with
enforce_csrf_checks=True confirming no bypass is required
|
Thanks for the feedback @ormsbee. You're right — the cleaner fix is to support the CSRF flow that xqueue-watcher already implements rather than bypassing CSRF entirely. Root cause recap: xqueue-watcher GETs Updated approach (latest commit):
Why this works end-to-end: xqueue-watcher's
All 19 xqueue-watcher client tests pass unchanged, including the ones that explicitly verify the GET→cookie→POST CSRF flow. The updated edx-submissions tests replace
|
Summary
The
XQueueViewSetwas using DRF's standardSessionAuthentication, which enforces CSRF validation on all mutating requests from authenticated users. This causedPOST /xqueue/put_result/to return a 403 when called byxqueue-watcher.Root cause
xqueue-watcherauthenticates viaPOST /xqueue/login/then uses the resulting session cookie for subsequent calls. Before the login POST it does a pre-fetch GET to the same URL to obtain a CSRF cookie — but the login action is declaredmethods=['post']only, so the GET returns 405 with no cookie set. The watcher therefore has no CSRF token to send, and when it POSTs toput_result/with a valid session, DRF'sSessionAuthentication.enforce_csrf()fails the check and returns 403.get_submissionsucceeds because it is a GET request, which is CSRF-exempt by design.Fix
Extract
CsrfExemptSessionAuthenticationintosubmissions/authentication.pyand use it inXQueueViewSet. CSRF is not appropriate for this service-to-service API; authorization is already enforced by theIsXQueueUserpermission class, which requires the authenticated user to be a member of thexqueuegroup.The test module previously defined its own local copy of this class for test setup — it now imports from the production module instead.
Testing
isortandpycodestylecleanpylintdjango-not-configurederror onsubmissions/__init__.pyis a pre-existing failure onmasterunrelated to this change