FinishResponseSubscriber could create duplicate headers

From what I can tell it should be working. I am re-running the test.

This merge request does not fix this bug on my site. I keep getting a duplicate IE=edge entry in the x-ua-compatible header.

Commenting out the line in FinishResponseSubscriber fixes the issue. It obviously removes the whole HTTP X-UA-Compatible header.

I also replaced it with 'IE=edgetest'. The result of doing that, is that I first get the error "X-UA-Compatible HTTP header must have the value IE=edge, was IE=edgetest." after a cache flush. On multiple page visits, the error changes to "X-UA-Compatible HTTP header must have the value IE=edge, was IE=edgetest, IE=edgetest."

Personally, I would not mind dropping any IE features like this. Microsoft already dropped support for their browser and that browser is not used much. I just checked on my site. Last year, only 0.3% of visitors to my site were using IE. Since November 1st 2021 this was only 0.11%.

Well, my site is optimized for performance and I use several modules for that. It sounds possible to me that (a combination of) one of these modules or their configuration is triggering this bug. I have attached a list of non-core modules that are enabled on my site. If this can help, I can temporarily disable some of them on my test site to see if that makes a difference.

The issue does still appear in the latest 9.3.11 release. Commenting out the line in FinishResponseSubscriber fixes the issue as in the past.

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.

Very very small change requested.

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

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.

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.

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!

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

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

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'));
   }

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

I just noticed this on an up-to-date Drupal 11 site returning this header:

This appears to imply that such duplicate headers can still happen with D11 on Apache without such a header in the global config, since the header does not appear on other vhosts on the server, appears once on public files for the D11, and twice on D11 pages.

In our specific case, that happened because a directory block in the vhost already included that header. Not sure Drupal can actually do anything to prevent the double header being emitted.

Not sure Drupal can actually do anything to prevent the double header being emitted.

It can, and it does (for X-Content-Type-Options), see https://git.drupalcode.org/project/drupal/-/blob/11.x/.htaccess?ref_type...