feat: add custom middleware for x-frame-options to allow overrides #422
Conversation
robrap
left a comment
robrap
left a comment
There was a problem hiding this comment.
I did not do a detailed review. I'm just leaving a high-level question. Thanks.
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| # settings.X_FRAME_OPTIONS_OVERRIDES = [['regex', 'SAMEORIGIN']] | ||
| # search.return_value = False | ||
| # middleware.process_response(request, response) | ||
| # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' | ||
| # validate_header.assert_called_with('SAMEORIGIN') | ||
| # search.assert_called_with('regex') | ||
| # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' | ||
| # validate_header.assert_called_with('SAMEORIGIN') | ||
| # search.assert_called_with('regex') | ||
| # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' |
There was a problem hiding this comment.
Code that should be deleted.
| # settings.X_FRAME_OPTIONS_OVERRIDES = [['regex', 'SAMEORIGIN']] | |
| # search.return_value = False | |
| # middleware.process_response(request, response) | |
| # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' | |
| # validate_header.assert_called_with('SAMEORIGIN') | |
| # search.assert_called_with('regex') | |
| # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' | |
| # validate_header.assert_called_with('SAMEORIGIN') | |
| # search.assert_called_with('regex') | |
| # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' |
There was a problem hiding this comment.
Oops sorry. Yeah I was just going through cleanup, fixed that and going through linter errors. I should have marked this as draft perhaps
| header ``X-Frame-Options`` set either to ``DENY`` or to ``SAMEORIGIN``. | ||
| In any case, this middleware allows you both to set the ``X-Frame-Options`` header to any recognized value - | ||
| ``DENY``, ``SAMEORIGIN``, ``ALLOW`` per django setting - but defaults to ``DENY``. | ||
| It also allows you to override the header for specific urls defined via regex. |
There was a problem hiding this comment.
I'm not clear. You are adding a deprecated middleware with a new feature? So this regex feature will not be needed in some future time?
There was a problem hiding this comment.
Okay let me try to explain my intention. Probably there's a better way to do this.
From a security perspective, we prefer CSP over X-Frame-Options for new or flexible projects. That means that it's okay to use X-Frame-Options but only if the project already has them and it's more work than it's worth to change them. That would be way outside the scope of my task for edx-platform - edx-platform still using the X-Frame-Options header.
The new middleware is important because we currently can't support SCORM xblock without setting the header to SAMEORIGIN, but I did not want to blindly do this for all of edx-platform. So I wrote this code to allow us to change the header for specific url patterns.
I wanted to mark this as deprecated because on the one hand we need it and it's not worth the effort to change the whole security headers thing on edx-platform right now, but I want to warn people off from using this middleware together with the outdated header in new projects.
Do you have a better suggestion for this?
There was a problem hiding this comment.
I may have more thoughts, but a quick question is whether this is for edx-platform only, and if it should live there? That would reduce scope of introducing deprecated-from-the-start code.
There was a problem hiding this comment.
I'm happy to move it to edx-platform.
|
This PR can be closed now, right? |
Description:
The SCORM xblocks starting with openedx-scorm-xblock version 18 cannot load because they require the
X-Frame-Optionsheader to be set toSAMEORIGINfor scorm related URLs, e.g.studio.edx.org/media/scorm/........This setting is set to
DENYon edx-platform cms and lms via the settingX_FRAME_OPTIONS.This PR extends django's XFrameOptions middleware to override the cms or lms django setting via an override setting called
X_FRAME_OPTIONS_OVERRIDES = [[regex, header_value]]where regex is a regex that matches urls to override and header_value is one ofDENY,SAMEORIGIN,ALLOW.How to test:
Since this is a library, the automatic tests should suffice.
If you still want to test it with edx-platform to make sure it doesn't break anything, let me know and I can walk you through it.