Skip to content

Sets default X-Frame-Options for login and registration forms to environment setting - #25338

Merged
ormsbee merged 1 commit into
openedx:masterfrom
open-craft:nizar/xframe_allow_whitelist
Dec 18, 2020
Merged

Sets default X-Frame-Options for login and registration forms to environment setting#25338
ormsbee merged 1 commit into
openedx:masterfrom
open-craft:nizar/xframe_allow_whitelist

Conversation

@nizarmah

@nizarmah nizarmah commented Oct 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Update the X-Frame-Options default value for login and registration forms to the existing EDXAPP_X_FRAME_OPTIONS environment settings to match the default setting across the whole platform.

JIRA tickets: SE-3437, OSPR-5053

Sandbox URL:

Testing instructions:

  1. Send a request to the Login page
  2. Verify that the response header contains X-Frame-Options: ALLOW

Reviewers

Settings

EDXAPP_X_FRAME_OPTIONS: "ALLOWALL"

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Oct 13, 2020
@openedx-webhooks

openedx-webhooks commented Oct 13, 2020 •
edited
Loading

Copy link
Copy Markdown

Thanks for the pull request, @nizarmah! I've created OSPR-5053 to keep track of it in JIRA, where we prioritize reviews. 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:

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

gabor-boros
gabor-boros approved these changes Oct 13, 2020
View reviewed changes

@gabor-boros gabor-boros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 🎉

  • I tested this: Send a request to the Login page with a specific Referer: customhostname.com header
  • I tested this: Verify that the response header contains X-Frame-Options: ALLOW
  • I read through the code
    - [ ] I checked for accessibility issues
    - [ ] Includes documentation
    - [ ] I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

Comment thread common/djangoapps/third_party_auth/decorators.py Outdated
@natabene

natabene commented Oct 15, 2020

Copy link
Copy Markdown
Contributor

@nizarmah Thank you for your contribution. Please let me know once it is ready for our review.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Oct 15, 2020
@nizarmah
nizarmah marked this pull request as draft October 29, 2020 08:26
@natabene

natabene commented Nov 10, 2020

Copy link
Copy Markdown
Contributor

@nizarmah When do you think this PR will be ready?

@nizarmah

nizarmah commented Nov 10, 2020

Copy link
Copy Markdown
Contributor Author

@natabene sorry about this PR. It might be a while since it is ready.

There are a couple of changes I'm trying to upstream at this moment, which include different fixes, such as the ones open at the moment to the Video Abstraction Layer repository (edx-val).

This is PR is one of the changes that I'd like to upstream, but there's a high chance I'll be changing the approach that I used on this PR, which is why I converted it to a Draft PR.

If it would be an issue, I can close it and reopen a new one. Let me know please 🙂

@natabene

natabene commented Nov 10, 2020

Copy link
Copy Markdown
Contributor

@nizarmah Thanks for the update, it is fine to keep it open, just wanted to make sure this was not abandoned.

@nizarmah nizarmah changed the title Adds Whitelist Env Setting for X-Frame-Options for Login and Registration Forms Sets default X-Frame-Options for login and registration forms to environment setting Nov 18, 2020
@nizarmah nizarmah mentioned this pull request Nov 18, 2020
Merged
2 tasks
@nizarmah
nizarmah marked this pull request as ready for review November 18, 2020 17:35
@openedx-webhooks openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Nov 18, 2020
@natabene natabene added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Nov 24, 2020
@nizarmah

nizarmah commented Nov 26, 2020

Copy link
Copy Markdown
Contributor Author

@natabene this is ready for review 👍

@natabene

natabene commented Nov 30, 2020

Copy link
Copy Markdown
Contributor

@nizarmah Thank you for letting me know.

@openedx-webhooks openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 10, 2020
bradenmacdonald
bradenmacdonald approved these changes Dec 12, 2020
View reviewed changes

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Has potential security / clickjacking implications, but I have checked:

  • This change just makes the X_FRAME_OPTIONS setting work more consistently as described, and does not affect the security of the default nor production configurations.
  • This really only affects IE 11 / old browsers, because a "Content Security Policy" with "frame-ancestors" is used for newer browsers and allows more fine-grained control.
  • Tested this on local devstack to confirm DENY is still the default header sent.

So: 👍

  • I tested this: as described above and on the sandbox provided
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a, setting is already documented

@bradenmacdonald

bradenmacdonald commented Dec 12, 2020

Copy link
Copy Markdown
Contributor

@nizarmah Can you please rebase + squash this down to one commit with a clear commit message, in preparation for merge?

@nizarmah
nizarmah force-pushed the nizar/xframe_allow_whitelist branch from 46fb6df to 3d1f4e5 Compare December 12, 2020 01:02
@nizarmah
nizarmah force-pushed the nizar/xframe_allow_whitelist branch from 3d1f4e5 to 6e0b304 Compare December 12, 2020 01:04
@nizarmah

nizarmah commented Dec 12, 2020

Copy link
Copy Markdown
Contributor Author

@bradenmacdonald thanks for your review! 😄
I've rebased and squashed down to a single commit with a clear commit message 👍

I'll keep an eye on the builds, in case they fail (even though they should not)

@edx-status-bot

edx-status-bot commented Dec 12, 2020

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

@natabene

natabene commented Dec 15, 2020

Copy link
Copy Markdown
Contributor

@bradenmacdonald Ready to merge?

@bradenmacdonald

bradenmacdonald commented Dec 15, 2020

Copy link
Copy Markdown
Contributor

@natabene Yes, this is ready to merge; I have asked @ormsbee if he can do that.

@ormsbee
ormsbee merged commit bb0492e into openedx:master Dec 18, 2020
@openedx-webhooks

openedx-webhooks commented Dec 18, 2020

Copy link
Copy Markdown

@nizarmah 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot

edx-pipeline-bot commented Dec 18, 2020

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

edx-pipeline-bot commented Dec 18, 2020

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@ormsbee ormsbee mentioned this pull request Dec 18, 2020
Closed
@nizarmah
nizarmah deleted the nizar/xframe_allow_whitelist branch December 19, 2020 04:30
nizarmah pushed a commit to open-craft/openedx-platform that referenced this pull request Jan 20, 2021
David Ormsbee @nizarmah
Merge pull request openedx#25338 from open-craft/nizar/xframe_allow_w…
b8c567f 
…hitelist

Sets default X-Frame-Options for login and registration forms to environment setting

(cherry picked from commit bb0492e)
0x29a pushed a commit to open-craft/openedx-platform that referenced this pull request Feb 15, 2021
David Ormsbee @0x29a
Merge pull request openedx#25338 from open-craft/nizar/xframe_allow_w…
e752907 
…hitelist

Sets default X-Frame-Options for login and registration forms to environment setting

(cherry picked from commit bb0492e)
0x29a pushed a commit to open-craft/openedx-platform that referenced this pull request Apr 20, 2021
David Ormsbee @0x29a
Merge pull request openedx#25338 from open-craft/nizar/xframe_allow_w…
54e3ea1 
…hitelist

Sets default X-Frame-Options for login and registration forms to environment setting

(cherry picked from commit bb0492e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bradenmacdonald bradenmacdonald bradenmacdonald approved these changes
+1 more reviewer
@gabor-boros gabor-boros gabor-boros approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@nizarmah @openedx-webhooks @natabene @bradenmacdonald @edx-status-bot @edx-pipeline-bot @gabor-boros @ormsbee