Issue #3214208 by gapple, Akhil Babu, larowlan, smustgrave:...

Issue #3214208 by gapple, Akhil Babu, larowlan, smustgrave: FinishResponseSubscriber could create duplicate headers

(cherry picked from commit 208bb878)

core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php

@@ -123,8 +123,10 @@ public function onRespond(ResponseEvent $event) {
     // different from the declared content-type, since that can lead to
     // XSS and other vulnerabilities.
     // https://owasp.org/www-project-secure-headers
-    $response->headers->set('X-Content-Type-Options', 'nosniff', FALSE);
-    $response->headers->set('X-Frame-Options', 'SAMEORIGIN', FALSE);
+    $response->headers->set('X-Content-Type-Options', 'nosniff');
+    if (!$response->headers->has('X-Frame-Options')) {
+      $response->headers->set('X-Frame-Options', 'SAMEORIGIN');
+    }
 
     // If the current response isn't an implementation of the
     // CacheableResponseInterface, we assume that a Response is either

core/tests/Drupal/Tests/Core/EventSubscriber/FinishResponseSubscriberTest.php

+<?php
+
+namespace Drupal\Tests\Core\EventSubscriber;
+
+use Drupal\Core\Cache\Context\CacheContextsManager;
+use Drupal\Core\EventSubscriber\FinishResponseSubscriber;
+use Drupal\Core\Language\Language;
+use Drupal\Core\Language\LanguageManagerInterface;
+use Drupal\Core\PageCache\RequestPolicyInterface;
+use Drupal\Core\PageCache\ResponsePolicyInterface;
+use Drupal\Tests\UnitTestCase;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpFoundation\ResponseHeaderBag;
+use Symfony\Component\HttpKernel\Event\ResponseEvent;
+use Symfony\Component\HttpKernel\HttpKernelInterface;
+
+/**
+ * @coversDefaultClass \Drupal\Core\EventSubscriber\FinishResponseSubscriber
+ * @group EventSubscriber
+ */
+class FinishResponseSubscriberTest extends UnitTestCase {
+
+  /**
+   * The mock Kernel.
+   *
+   * @var \Symfony\Component\HttpKernel\HttpKernelInterface|\PHPUnit\Framework\MockObject\MockObject
+   */
+  protected $kernel;
+
+  /**
+   * The mock language manager.
+   *
+   * @var \Drupal\Core\Language\LanguageManagerInterface|\PHPUnit\Framework\MockObject\MockObject
+   */
+  protected $languageManager;
+
+  /**
+   * The mock request policy.
+   *
+   * @var \Drupal\Core\PageCache\RequestPolicyInterface|\PHPUnit\Framework\MockObject\MockObject
+   */
+  protected $requestPolicy;
+
+  /**
+   * The mock response policy.
+   *
+   * @var \Drupal\Core\PageCache\ResponsePolicyInterface|\PHPUnit\Framework\MockObject\MockObject
+   */
+  protected $responsePolicy;
+
+  /**
+   * The mock cache contexts manager.
+   *
+   * @var \Drupal\Core\Cache\Context\CacheContextsManager|\PHPUnit\Framework\MockObject\MockObject
+   */
+  protected $cacheContextsManager;
+
+  protected function setUp(): void {
+    parent::setUp();
+
+    $this->kernel = $this->createMock(HttpKernelInterface::class);
+    $this->languageManager = $this->createMock(LanguageManagerInterface::class);
+    $this->requestPolicy = $this->createMock(RequestPolicyInterface::class);
+    $this->responsePolicy = $this->createMock(ResponsePolicyInterface::class);
+    $this->cacheContextsManager = $this->createMock(CacheContextsManager::class);
+  }
+
+  /**
+   * Finish subscriber should set some default header values.
+   *
+   * @covers ::onRespond
+   */
+  public function testDefaultHeaders() {
+    $finishSubscriber = new FinishResponseSubscriber(
+      $this->languageManager,
+      $this->getConfigFactoryStub(),
+      $this->requestPolicy,
+      $this->responsePolicy,
+      $this->cacheContextsManager,
+      FALSE
+    );
+
+    $this->languageManager->method('getCurrentLanguage')
+      ->willReturn(new Language(['id' => 'en']));
+
+    $request = $this->createMock(Request::class);
+    $response = $this->createMock(Response::class);
+    $response->headers = new ResponseHeaderBag();
+    $event = new ResponseEvent($this->kernel, $request, HttpKernelInterface::MAIN_REQUEST, $response);
+
+    $finishSubscriber->onRespond($event);
+
+    $this->assertEquals(['en'], $response->headers->all('Content-language'));
+    $this->assertEquals(['nosniff'], $response->headers->all('X-Content-Type-Options'));
+    $this->assertEquals(['SAMEORIGIN'], $response->headers->all('X-Frame-Options'));
+  }
+
+  /**
+   * Finish subscriber should not overwrite existing header values.
+   *
+   * @covers ::onRespond
+   */
+  public function testExistingHeaders() {
+    $finishSubscriber = new FinishResponseSubscriber(
+      $this->languageManager,
+      $this->getConfigFactoryStub(),
+      $this->requestPolicy,
+      $this->responsePolicy,
+      $this->cacheContextsManager,
+      FALSE
+    );
+
+    $this->languageManager->method('getCurrentLanguage')
+      ->willReturn(new Language(['id' => 'en']));
+
+    $request = $this->createMock(Request::class);
+    $response = $this->createMock(Response::class);
+    $response->headers = new ResponseHeaderBag();
+    $event = new ResponseEvent($this->kernel, $request, HttpKernelInterface::MAIN_REQUEST, $response);
+
+    $response->headers->set('X-Content-Type-Options', 'foo');
+    $response->headers->set('X-Frame-Options', 'DENY');
+
+    $finishSubscriber->onRespond($event);
+
+    $this->assertEquals(['en'], $response->headers->all('Content-language'));
+    // 'X-Content-Type-Options' will be unconditionally set by the core.
+    $this->assertEquals(['nosniff'], $response->headers->all('X-Content-Type-Options'));
+    $this->assertEquals(['DENY'], $response->headers->all('X-Frame-Options'));
+  }
+
+}