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
Per-page X-Frame-Options/Clickjacking protection #2477
Conversation
Not sure about the inherit.... creates an extra query. Would it make sense to make the default either django_default or deny? |
From my perspective it's about not clobbering any site-wide settings, unless specifically and explicitly overridden - effectively mirroring the behaviour of django's own clickjacking-protection view-decorators. Having any 3rd-party reusable app make assumptions (eg. default "deny") about the site as a whole isn't usually very developer-friendly IME. As far as inheritance is concerned: I can imagine a situation where a large subset of a site's pages all need to be embeddable into one or multiple third-party site/s, and with inheritance of this value it makes it easier to control their shared page configurations (template, common plugins, clickjacking, etc.) from a single parent page, while still having the rest of the site obey the global default. However, should inheritance be removed, the only sensible default IMO would be to do nothing ("site default" maybe?); thereby handing responsibility to the site-wide clickjacking middleware. Anything else would cause confusion and/or even more work, should the site-wide default need to be changed. I'm not sure if this was what was meant by "django_default"? |
@yakky what do you think? |
Assigning to me for later review |
The implemented behavior is the saner one: I figure that inheriting is the most common behavior that anyone would like to implement
|
isn't a cache hit almost the same as a query to the db? |
That's not my experience. This allows for policy like:
With default being a settings value |
i like the idea... |
Just so I don't misunderstand your meaning: Do you mean a default behaviour for the x-frame-options header (so a page set to a value of "default" will use whatever the config option is set to), or do you mean a default behaviour for the drop-downs on the cms page admin (so there is no explicit "default" choice, but the config option determines which of the dropdown entries (inherit, allow, deny, same-origin) will be selected initially)? |
2nd option |
+1 for me too |
@xray7224 any help to get this merged? |
Yes, I shall start work on this now. I'll implement the suggestions above and let you know when that's done so you can re-review the changes. |
Thanks! |
So I've run into some issues implementing the caching on the xframe_options settings. The unit tests don't seem to clear the cache which is causing some tests to have values for 'xframe_options' when they shouldn't. This won't really be a problem in the real world but it is in testing. My suggestion is to use |
maybe in the tearDown method? |
I have added caching for get_xframe_options. I think it's ready for re-review |
please remerge with develop |
I've rebased my branch ontop of develop |
seams there is an error in the docs... |
and we have now one query more than before... https://travis-ci.org/divio/django-cms/jobs/19177130 what is the default now? |
The extra query in the tests is expected as the default default is 'inherit' which will trigger the extra query. |
@yakky If we pick something other than "inherit" as the default-default, then we're back to the "should we respect the site-wide configuration by default" question... But if the additional query really is such a point of contention, perhaps an alternative is to have another option of "site default", which would be an explicit per-page "do nothing" option. I can imagine that this could be unintuitive to have as the default-default though, since pages do inherit other settings without explicit user/developer intervention. |
i can live with one extra query. increase the query numbers in the testsuite |
Per-page X-Frame-Options/Clickjacking protection
@raumkraut thank you for the work |
@digi604 thanks, but all I did was give my opinions on the subject ;0) |
This provides a Advanced Setting which allows you to specify a specific X-Frame-Option for a page. The options for X-Frame-Options are:
The default is inherit from parents.