FinishResponseSubscriber could create duplicate headers

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

MR rebased to 10.1

Very very small change requested.

When I run the tests locally without the fix
testDefaultHeaders passes should it fail?

Fixed the small issue, and removed the X-UA-Compatible from the existing header test, since it's no longer added by core.

testDefaultHeaders() should pass both before and after the change; testExistingHeaders() should only pass after the change.

Thank you for the quick follow up!

Adding review credit for @smustgrave

According to the documentation, there is only one possible value for X-Content-Type-Options, and that's nosniff

Can you elaborate on the scenario where you need to set an alternative value?

Similarly for X-FRAME-OPTIONS there's only two options, SAMEORIGIN or DENY

If you switch it to DENY, you will break things like media library embedding of oembed videos.

Can you elaborate on the use-case for that change too?

Putting back to needs work, pending response above, however I would expect we'd need to reverse the X-Content-Type-Options changes given that header only takes one possible value.

As I noted in the summary, the current code is unlikely to cause issues in practice since a module that wants to alter the value is likely to execute after core's handler and also replace the value set by core - core setting the $replace parameter is just incorrect for what the code intends to do. Were a module to be prioritized before core's event handler and also set the same value it would also result in a duplicate header, just with the same value twice.

The values set in FinishResponseSubscriber are not changed by this issue (X-Frame-Options: SAMEORIGIN is still the default set by core, as now tested by FinishResponseSubscriberTest::testDefaultHeaders()), the test just sets them to different values to best ensure it is actually testing what's expected.

The headers could just unconditionally set the header values by removing the $replace parameter value (like is done for Content-language just before) - having the conditional just matches what appears to be the intent behind why the $replace parameter was originally set.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed missing parent:setUp() in test that bot flagged.

I found a super small code nitpick, but this change looks super solid otherwise. +1 for rtbc after that is fixed.
Great job on the test coverage, that looks great!

david.muffley → made their first commit to this issue’s fork.

Removed the nitpicks from #31 so moving to RTBC.

I think we should do #26/ #28 for the x-content-type-options header - i.e just remove the replace variable and set it unconditionally - there's no use-case for anything else per the spec.

fine to self-RTBC after that, will keep an eye out for the change

Akhil Babu → made their first commit to this issue’s fork.

Akhil Babu → changed the visibility of the branch 11.x to hidden.

Akhil Babu → changed the visibility of the branch 3214208-finishresponsesubscriber-could-create to hidden.

Added the changes as per #35 and created a new merge request against 11.x branch

Appears feedback to only set X-Frame-Options is empty is addressed

larowlan → closed merge request !673

Compared the two MRs

here's the diff

diff --git a/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
index 6d93ec91915..6fc54a38c00 100644
--- a/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -123,9 +123,7 @@ 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
-    if (!$response->headers->has('X-Content-Type-Options')) {
-      $response->headers->set('X-Content-Type-Options', 'nosniff');
-    }
+    $response->headers->set('X-Content-Type-Options', 'nosniff');
     if (!$response->headers->has('X-Frame-Options')) {
       $response->headers->set('X-Frame-Options', 'SAMEORIGIN');
     }
diff --git a/core/tests/Drupal/Tests/Core/EventSubscriber/FinishResponseSubscriberTest.php b/core/tests/Drupal/Tests/Core/EventSubscriber/FinishResponseSubscriberTest.php
index 790c291c26e..a77403daf41 100644
--- a/core/tests/Drupal/Tests/Core/EventSubscriber/FinishResponseSubscriberTest.php
+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/FinishResponseSubscriberTest.php
@@ -125,7 +125,8 @@ public function testExistingHeaders() {
     $finishSubscriber->onRespond($event);
 
     $this->assertEquals(['en'], $response->headers->all('Content-language'));
-    $this->assertEquals(['foo'], $response->headers->all('X-Content-Type-Options'));
+    // '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'));
   }

Committed to 11.x and 10.2.x

Made a minor cleanup/grammar commit as follows:

diff --git a/core/tests/Drupal/Tests/Core/EventSubscriber/FinishResponseSubscriberTest.php b/core/tests/Drupal/Tests/Core/EventSubscriber/FinishResponseSubscriberTest.php
index a77403daf41..f83951cc85a 100644
--- a/core/tests/Drupal/Tests/Core/EventSubscriber/FinishResponseSubscriberTest.php
+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/FinishResponseSubscriberTest.php
@@ -125,7 +125,7 @@ public function testExistingHeaders() {
     $finishSubscriber->onRespond($event);
 
     $this->assertEquals(['en'], $response->headers->all('Content-language'));
-    // 'X-Content-Type-Options' will be unconditionally set by the core.
+    // 'X-Content-Type-Options' will be unconditionally set by core.
     $this->assertEquals(['nosniff'], $response->headers->all('X-Content-Type-Options'));
     $this->assertEquals(['DENY'], $response->headers->all('X-Frame-Options'));
   }

larowlan → closed merge request !5969

I bet this also fixed a bug reported against the BigPipe Sessionless module I maintain: 🐛 Security headers are duplicated (X-UA-Compatible, X-Content-Type-Options, X-Frame-Options) Needs review .

Automatically closed - issue fixed for 2 weeks with no activity.