Per-page X-Frame-Options/Clickjacking protection

[tsyesika]

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:

• Inherit from parent
• Allow
• Deny
• same origin (this website only)

The default is inherit from parents.

[coveralls]

Coverage decreased (-0.13%) when pulling e805e40 on xray7224:XFrameOptions into d7bc2d7 on divio:develop.

[digi604]

Not sure about the inherit.... creates an extra query. Would it make sense to make the default either django_default or deny?

[romlok]

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.
I'm not sure a single extra query is going to break the bank, but since some other page settings are also inheritable, I'm wondering if it might be feasible to fetch the xframe_options value at the same time as those (in the same query)...

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"?

[digi604]

@yakky what do you think?

[yakky]

Assigning to me for later review

[yakky]

The implemented behavior is the saner one: I figure that inheriting is the most common behavior that anyone would like to implement
To mitigate the extra-query issue: how about explicitly caching the inherit value for every page?
E.G.: changing https://github.com/divio/django-cms/pull/2477/files#diff-960fbbc1702d77aa625a17bcb0c37191R1109 and following to something like:

inherit_state = cache_get("cms:x_frame_options:%s" % page.pk)
if not inherit_state:
    [ ... ] #check ancestors
    inherit_state = xframe_options[0]
    cache_set("cms:x_frame_options:%s" % page.pk, inherit_state)
return inherit_state

[digi604]

isn't a cache hit almost the same as a query to the db?

[yakky]

That's not my experience.
A further optimization is to get ancestors data only if self.xframe_options == 'X_FRAME_OPTIONS_INHERIT'
And we can have a configuration option with a default behavior.

This allows for policy like:

• A [default]
  • B [ allow ]
    • C [inherit]
  • D [default]

With default being a settings value

[digi604]

i like the idea...

[romlok]

And we can have a configuration option with a default behavior.

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)?
If the former, then IMO that's better handled by application of the existing clickjacking middleware. If the latter, then I think I agree.

[digi604]

2nd option

[yakky]

+1 for me too

[yakky]

@xray7224 any help to get this merged?

[tsyesika]

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.

[yakky]

Thanks!

[tsyesika]

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 cache.clear() before every test (maybe in the setUp method?) What do you think?

[digi604]

maybe in the tearDown method?

[tsyesika]

I have added caching for get_xframe_options. I think it's ready for re-review

[digi604]

please remerge with develop

[tsyesika]

I've rebased my branch ontop of develop

[digi604]

seams there is an error in the docs...

[digi604]

and we have now one query more than before...

https://travis-ci.org/divio/django-cms/jobs/19177130

what is the default now?

[yakky]

The extra query in the tests is expected as the default default is 'inherit' which will trigger the extra query.
@xray7224 won't be a safer implementation defaulting the default to safe origin?

[romlok]

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

[digi604]

i can live with one extra query. increase the query numbers in the testsuite

[coveralls]

Coverage increased (+0.02%) when pulling d45a131 on xray7224:XFrameOptions into 4a51b2d on divio:develop.

[coveralls]

Coverage remained the same when pulling d1c8726 on xray7224:XFrameOptions into bbc6e0a on divio:develop.

[digi604]

@raumkraut thank you for the work

[romlok]

@digi604 thanks, but all I did was give my opinions on the subject ;0)