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

[SR] system_update_1063() doesn't account for existing x_frame_options variable from D7 #4080

Open
klonos opened this issue Sep 23, 2019 · 3 comments

Comments

@klonos
Copy link
Member

klonos commented Sep 23, 2019

This is with re to a change that was implemented in Backdrop 1.9.0, directly via this PR, as part of #2013 (change record).

According to the respective D7 change record, this could have been set with the x_frame_options in settings.php:

...set the 'x_frame_options' variable via any standard method, for example in settings.php:

 // Turn off the X-Frame-Options header entirely, to restore the previous
 // behavior of allowing the site to be embedded in a frame on another site.
 $conf['x_frame_options'] = '';

or

 // Set the "DENY" option to prevent the site from ever being embedded in a
 // frame at all, even on this site itself.
 $conf['x_frame_options'] = 'DENY';

We have added "x_frame_options": "SAMEORIGIN" in our default system.core.json file, so new sites will be set to this, and also in our respective update hook, we are doing this:

/**
 * Set a default value for the X-Frame-Options header. This will not allow the
 * site to be rendered in an iframe on another domain unless specified.
 */
function system_update_1063() {
  $config = config('system.core');
  $config->set('x_frame_options', 'SAMEORIGIN');
  $config->save();
}

Never are we reading from any existing variable set earlier (so D7 to Backdrop upgrades will miss to detect if this was set to something different), nor are we allowing people to override this in their settings.php.


PR by @klonos: backdrop/backdrop#2895

@klonos klonos changed the title system_update_1063() doesn't account for existing variable from D7 system_update_1063() doesn't account for existing x_frame_options variable from D7 Sep 23, 2019
@klonos
Copy link
Member Author

klonos commented Sep 23, 2019

I have filed a PR that:

  • allows setting 'x_frame_options' in settings.php
  • still falls back to the default SAMEORIGIN we set in config via system.core.json
  • updates system_update_1063() so to take into account whether the x_frame_options variable has been previously set (during D7 upgrades)
  • delete the variable after it saves it to config
  • fallback to SAMEORIGIN if the variable has not been previously set in D7

PS: can someone with the right access permissions close backdrop/backdrop#1426 ?

@klonos klonos changed the title system_update_1063() doesn't account for existing x_frame_options variable from D7 [SR] system_update_1063() doesn't account for existing x_frame_options variable from D7 Sep 23, 2019
@klonos
Copy link
Member Author

klonos commented Sep 23, 2019

...the change record in https://api.backdropcms.org/change-records/backdrop-core-now-protected-against-clickjacking-default-x-frame-options-sameorigin should be updated to indicate support for configuring x_frame_options via settings.php.

@indigoxela
Copy link
Member

This branch has conflicts that must be resolved.

I wonder if this issue is still relevant after #2805?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants