Skip to content
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

fix: CSRF errors in POST requests to LMS #33790

Conversation

regisb
Copy link
Contributor

@regisb regisb commented Nov 24, 2023

This is a Quince backport of #33727 cc @mariajgrimaldi

POST requests to the LMS are failing systematically when HTTPS is enabled. This issue is observed in the Quince release branch. Here is the root cause analysis:

  • CorsCSRFMiddleware overrides the is_secure attribute by setting it to "false".
  • CorsCSRFMiddleware calls the parent process_view method, from the CsrfViewMiddleware.
  • CsrfViewMiddleware checks the Origin header, including the scheme. It is equal to "https://LMSHOST". But because the request is not considered secure, the expected origin is "http://LMSHOST".
  • The check fails with "Origin checking failed"

We resolve this issue by running the CsrfViewMiddleware before the custom CorsCSRFMiddleware. After a successful check of the CsrfViewMiddleware, the request has the "csrf_processing_done = True" attribute, and CorsCSRFMiddleware is short-circuited.

This issue did not happen in the following environments:

  • in Palm because the CsrfViewMiddleware did not check the "Origin" header in Django 3.
  • in the Studio, because the Studio already runs the CsrfViewMiddleware before the CorsCSRFMiddleware.
  • in the master branch because the master branch does not yet run on Django 4. But the issue will happen in the master branch without this proposed change.

To bypass this issue in the master branch, it was proposed that we add "https://LMSHOST" to CSRF_TRUSTED_ORIGINS. This would effectily bypass CSRF checking entirely for all requests that originate from the LMS. Such a solution would not be acceptable, as we would lose the security guarantees offered by CSRF.

See discussion: openedx/wg-build-test-release#325

POST requests to the LMS are failing systematically when HTTPS is
enabled. This issue is observed in the Quince release branch. Here is
the root cause analysis:

- CorsCSRFMiddleware overrides the `is_secure` attribute by setting it
  to "false".
- CorsCSRFMiddleware calls the parent `process_view` method, from the
  CsrfViewMiddleware.
- CsrfViewMiddleware checks the Origin header, including the scheme. It
  is equal to "https://LMSHOST". But because the request is not
  considered secure, the expected origin is "http://LMSHOST".
- The check fails with "Origin checking failed"

We resolve this issue by running the CsrfViewMiddleware *before* the
custom CorsCSRFMiddleware. After a successful check of the
CsrfViewMiddleware, the request has the "csrf_processing_done = True"
attribute, and CorsCSRFMiddleware is short-circuited.

This issue did not happen in the following environments:

- in Palm because the CsrfViewMiddleware did not check the "Origin" header in Django 3.
- in the Studio, because the Studio already runs the CsrfViewMiddleware before
  the CorsCSRFMiddleware.
- in the master branch because the master branch does not yet run on
  Django 4. But the issue will happen in the master branch without this
  proposed change.

To bypass this issue in the master branch, it was proposed that we add
"https://LMSHOST" to CSRF_TRUSTED_ORIGINS. This would effectily bypass
CSRF checking entirely for all requests that originate from the LMS.
Such a solution would not be acceptable, as we would lose the security
guarantees offered by CSRF.

See discussion: openedx/wg-build-test-release#325
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 24, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @regisb! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@mariajgrimaldi mariajgrimaldi merged commit 97b8cb9 into openedx:open-release/quince.master Nov 24, 2023
43 checks passed
@openedx-webhooks
Copy link

@regisb 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@regisb regisb deleted the regisb/fix-lms-csrf branch November 24, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants