Problem/Motivation

Imagine a scenario, where the main website is served at https://example.com, and the iFrame domain options is set to an alias; https://oembed.example.com

This way when media on a page is viewed, the markup on https://example.com would be something like:

<iframe src="https://oembed.example.com/media/oembed?url=https%3A//youtu.be/Dc3LpT69crc&amp;max_width=0&amp;max_height=0&amp;hash=J3reigEh1oul-MNqUBBvVbfHUo1eI54gZryZJT22g1E" frameborder="0" allowtransparency width="480" height="270" class="media-oembed-content"></iframe>

Steps to reproduce

Out-of the box, this fails with an error:

Refused to display 'https://oembed.example.com/media/oembed?url=https%3A//youtu.be/Dc3LpT69c...' in a frame because it set 'X-Frame-Options' to 'sameorigin'.

Proposed resolution

Unless an appropriate X-Frame-Options header is set, the iframe content will not load.

Should this be an optional setting, addition to iFrame domain in Media settings > Security?

    $form['security']['x_frame_options'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Set HTTP header: X-Frame-Options: allow-from {IFRAME_DOMAIN}'),
      '#default_value' => $this->config('media.settings')->get('x_frame_options'),
      '#description' => $this->t('If the content served from iFrame domain is not displayed try enabling this option.'),
      '#states' => [
        'invisible' => [
          ':input[name="iframe_domain"]' => ['empty' => TRUE],
        ],
      ],
    ];

Then set the HTTP header in an EventSubscriber:

  public function onKernelResponse(FilterResponseEvent $event) {
    $iframe_domain = $this->config->get('iframe_domain', '');
    $x_frame_options = $this->config->get('x_frame_options', 0);

    if ($iframe_domain !== '' && (bool) $x_frame_options) {
      $response = $event->getResponse();
      $response->headers->set('X-Frame-Options', "allow-from ${iframe_domain}");
    }
  }

Remaining tasks

None

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None

Release notes snippet

None

Issue fork drupal-3075685

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

osman created an issue. See original summary.

osman’s picture

osman
Location Copenhagen
commented
Status: Active » Closed (works as designed)

Turns out setting X-Frame-Options alone doesn't resolve the issue by itself. It still seems to require Content-Security-Policy header to be set.

Most importantly, X-Frame-Options is deprecated in favor of Content-Security-Policy.

Seems like though, any solution needs to use a combination of these headers to make it work.

Since every project's solution will have to be custom defined, i'm closing this issue.

a8w4’s picture

a8w4
Location Frankfurt am Main
commented
Version: 8.7.x-dev » 8.9.x-dev
Status: Closed (works as designed) » Active

This is absolutely not satisfactory - you're setting the site-builder up for failure. This configuration will - as it momentarily is - never work!
How can this be "working as designed"? At the very least there should be an informational text, that there is a problem and how to solve it. A minimal solution would be to set at least the Header Content-Security-Policy: frame-ancestors <source> with the iFrame-Domain as <source>. I've found out, that this works with the X-Frame-Options header untouched!

a8w4’s picture

a8w4
Location Frankfurt am Main
commented
Version: 8.9.x-dev » 8.8.x-dev
maosmurf’s picture

maosmurf commented

I agree: the current default setup does not allow embeding oembed iframes from another subdomain.

I can also confirm that we could solve the problem without touching X-Frame-Options (found in \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond)

In order to do so, we installed drupal/csp using default settings + adding frame-ancestors to regular URL (not the oembed one):

$config['csp.settings']['enforce']['directives']['frame-ancestors']['sources'][] = 'https://www.example.com';

EDIT: for sake of completeness, we also had to resolve the problem of unauthenticated access (login not passed down to oembed iframe). A simplified solution could look like this:

  protected function alterRoutes(RouteCollection $collection) {
    if ($route = $collection->get('media.oembed_iframe')) {
      $route->setRequirement('_access', 'TRUE');
      $requirements = $route->getRequirements();
      unset($requirements['_permission']);
      $route->setRequirements($requirements);
    }
  }
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Category: Support request » Bug report
Priority: Normal » Major
Issue tags: +Media Initiative, +Triaged Media Initiative issue, +oembed, +Security, +Needs security review

Bumping the priority here, since I think this is a major bug and prevents oEmbed stuff from working properly.

I'm also tagging this for security review because I'd like a member of security team to confirm that there isn't any harm in Media setting this header. Or, if there is potential danger, what we might do to mitigate it.

pameeela’s picture

pameeela commented
Version: 8.8.x-dev » 8.9.x-dev
tboggia’s picture

tboggia commented

If anyone else is arriving here because YouTube embeds are giving this error, replace /watch?v= with /embed/ in your iframe src (h/t this stackoverflow about the error).

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented

Hi there!
I'm facing this issue too. I enabled the "iFrame domain" setting due to the worrying message in the form.
I installed the CSP module and configured the frame-ancestors setting with the main domain.
I think either the Core should set the appropriate headers or drop this setting to leave it to some contrib modules like CDN.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

newaytech’s picture

newaytech commented

Further discussion here - thanks for the CSP tip!

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

plach made their first commit to this issue’s fork.

plach opened merge request !7691

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Assigned: Unassigned » plach

Working on this.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Assigned: plach » Unassigned
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Can the issue summary be updated please to match standard issue template.

moshe weitzman made their first commit to this issue’s fork.

moshe weitzman’s picture

moshe weitzman commented
Issue summary: View changes
Status: Needs work » Needs review

Updated the MR and now using the Issue summary template.

May I gently suggest that when we ask to use the issue summary template, we don't also switch to Needs Work just for that reason. Its more important to fix a bug than it is to use the official template.

smustgrave’s picture

smustgrave commented

Will disagree if the ticket isn’t correct it can’t move forward and won’t be accepted by committers. Thus delaying it further

smustgrave’s picture

smustgrave commented
Status: Needs review » Postponed

So brought this up in #security-team channel and was recommended by @longwave to postpone this one as the iframe domain feature is planned to be removed from Drupal.

Believe #2965979: Validate alternate domain for oEmbed iFrame is the correct ticket.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.