Issue fork drupal-3116482

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:

Comments

Liam Morland created an issue. See original summary.

liam morland’s picture

Assigned: liam morland » Unassigned
Status: Active » Needs review
StatusFileSize
new515 bytes
klonos’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Liam Morland 🙏

Although I could not reproduce the issue with the duplicate headers reported in #2854817: Duplicate X-Content-Type-Options headers both with the value nosniff (tried on my local with both ngnix and apache), this is a simple, straight-forward change that makes sense and mimics what has been added to the .htaccess file in Drupal core 10.1.x (minus the Symfony-related changes that are not relevant in D7). Based on that, I am going to go ahead and mark this as RTBC.

poker10’s picture

Thanks for working on this. Any reason not to use the same code (and comment) as in D10?

D10:

 <IfModule mod_headers.c>
-  # Disable content sniffing, since it's an attack vector.
+  # Disable content sniffing for all responses, since it's an attack vector.
+  # This header is also set in FinishResponseSubscriber, which depending on
+  # Apache configuration might get placed in the 'onsuccess' table. To prevent
+  # header duplication, unset that one prior to setting in the 'always' table.
+  # See "To circumvent this limitation..." in
+  # https://httpd.apache.org/docs/current/mod/mod_headers.html.
+  Header onsuccess unset X-Content-Type-Options
   Header always set X-Content-Type-Options nosniff

D7:

 <IfModule mod_headers.c>
+  # This header is also set in drupal_page_header(). Unset before adding to
+  # ensure there is no header duplication.
+  Header unset X-Content-Type-Options
   # Disable content sniffing, since it's an attack vector.
poker10’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch does not apply anymore, as there were recent changes in .htaccess file.

When doing reroll, please consider my question from #4. Thanks!

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

andypost’s picture

Assigned: Unassigned » andypost
Issue tags: -Needs reroll

Created MR but instead of patch backported commited changes https://git.drupalcode.org/project/drupal/-/commit/e7b87b5c2027f97d7a31a...

Gonna test it as in related images enables mod_headers and .htaccess started to work

See failure https://git.drupalcode.org/issue/drupal-3443234/-/jobs/1432789

andypost’s picture

Priority: Normal » Major
Related issues: +#3226187: Enable mod_headers and mod_expires on Apache

gonna use :dev images to make sure that issue fixed, then will remove testing commit

If new images with enabled headers module will be commited then tests should start to fail on PHP 8.1+

andypost’s picture

Assigned: andypost » Unassigned
Issue summary: View changes
Status: Needs work » Needs review

Probably now it's critical, updated IS

poker10’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Pending Drupal 7 commit

Thanks for working on this @andypost! I confirm that HEAD tests for 7.x are now failing due to the changes in PHP 8.1+ images (so it affects all 7.x issues, see the example in this (random) pipeline https://git.drupalcode.org/project/drupal/-/pipelines/148353).

Also the draft no-op MR in this issue is failing (https://git.drupalcode.org/project/drupal/-/merge_requests/7748), to confirm the issue.

I have checked the parent D10 issue and it looks like the change is the same as in D10 - https://git.drupalcode.org/project/drupal/-/blob/11.x/.htaccess?ref_type...

We are setting X-Content-Type-Options = nosniff header on two places, but this was kept in D10 as well, so I think just the .htaccess change is OK (it is explained in the parent issue in comment #108 https://www.drupal.org/project/drupal/issues/2854817#comment-14888372).

Just a minor think, I changed a mention about FinishResponseSubscriber in the comment, because we do not have this in D7, so updated to the D7 function which sets that header. Otherwise looks good to me. Created a draft CR here: https://www.drupal.org/node/3443661 (based on D10).

Adding a tag for final review, but moving to RTBC. Pipeline with the change is green: https://git.drupalcode.org/project/drupal/-/pipelines/157245

andypost’s picture

@poker10 thank you, missed subscriber(

  • poker10 committed 7539b239 on 7.x
    Issue #3116482 by andypost, poker10, Liam Morland: Duplicate X-Content-...

poker10 credited Fabianx.

poker10’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

This got +1 from @Fabianx on Slack. Committed and pushed, thanks all!

7.x pipelines are all green: https://git.drupalcode.org/project/drupal/-/pipelines?scope=branches&page=1

andypost’s picture

Thank you! Hope it will help to keep HEAD 7 green for Portland)

Status: Fixed » Closed (fixed)

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