apache / incubator-pagespeed-mod

Apache module for rewriting web pages to reduce latency and bandwidth.
http://modpagespeed.com
Apache License 2.0
697 stars 159 forks source link

x-content-type-options nosniff duplicated #1988

Open echu2013 opened 4 years ago

echu2013 commented 4 years ago

Hello! In my scenario, where I choose the following Apache configuration setting: Header set X-Content-Type-Options "nosniff" Te following happens with, for example, Images or CSS rewritten by mod_pagespeed: image Disabling with ModPageSpeed Off reverts the behaviour.

Looking at source code, found the following: https://github.com/apache/incubator-pagespeed-mod/blob/409bd76fd6eafc4cf1c414e679f3e912447a6a31/pagespeed/apache/apache_fetch.cc#L121 Which I suspect that it should be this way:

--- a/pagespeed/apache/apache_fetch.cc
+++ b/pagespeed/apache/apache_fetch.cc
@@ -118,7 +118,10 @@ void ApacheFetch::SendOutHeaders() {
     // "X-Content-Type-Options: nosniff". This is a security feature
     // that helps prevent attacks based on MIME-type confusion.
     if (!is_proxy_) {
-      response_headers()->Add("X-Content-Type-Options", "nosniff");
+      // Replace, as in "add if not already present".  The only valid value for this
+      // header is "nosniff", so we don't have to worry about clobbering existing
+      // usage.
+      response_headers()->Replace("X-Content-Type-Options", "nosniff");
     }

     // TODO(sligocki): Add X-Mod-Pagespeed header.

In short: Replace instead of Add (which duplicates the header if previously added), Unfortunately, I am having trouble trying to build myself the module so I can not confirm that this is where the issue actually resides.

OS:

cat /etc/centos-release CentOS Linux release 7.7.1908 (Core)

Packages version:

mod_fcgid-2.3.9-6.el7.x86_64 mod-pagespeed-stable-1.13.35.2-0.x86_64 mod_dav_svn-1.7.14-14.el7.x86_64 mod_security-2.9.2-1.el7.x86_64 httpd-filesystem-2.4.43-2.codeit.el7.noarch httpd-2.4.43-2.codeit.el7.x86_64 mod_perl-2.0.11-1.el7.x86_64 mod_http2-1.15.7-1.codeit.x86_64 mod_auth_ntlm_winbind-0.0.0-0.15.20070129svn713.el7.x86_64 httpd-tools-2.4.43-2.codeit.el7.x86_64 httpd-devel-2.4.43-2.codeit.el7.x86_64 mod_security_crs-2.2.9-1.el7.noarch mod_ssl-2.4.43-2.codeit.el7.x86_64

Lofesa commented 4 years ago

As a workaround you can set the header conditionaly. See https://stackoverflow.com/questions/42791279/how-to-set-apache-conditional-header-based-on-url

echu2013 commented 4 years ago

Sorry @Lofesa , but that does not apply for me . I also tried to set header if empty without success, page speed applies after any set header directive

Lofesa commented 4 years ago

Hi But pagespeed applies only in rewrited resources aka optimized. When a resource is rewrited by pagespeed, the url contain the word "pagespeed" so you need t put the header only in resources that don´t have pagespeed text in it

echu2013 commented 4 years ago

Hi But pagespeed applies only in rewrited resources aka optimized. When a resource is rewrited by pagespeed, the url contain the word "pagespeed" so you need t put the header only in resources that don´t have pagespeed text in it

I have CSS that are minified (and name is not rewritten) and also some very tiny PNGs that aren´t rewritten but the header is added. See an example: image The second I disable ModPageSpeed, the header ceases to be duplicated.

echu2013 commented 4 years ago

Rewriten content, does not suffer from duplicated headers, see: image I think this is why: https://github.com/apache/incubator-pagespeed-mod/blob/409bd76fd6eafc4cf1c414e679f3e912447a6a31/net/instaweb/rewriter/rewrite_context.cc#L2820

NOTE Replace instead of Add

Lofesa commented 4 years ago

Yes, I know that rewrited resources have only 1 header, so you need to add it to the no rewrited ones. What happens with this css? Is a , half optimized resource. With half optimized I will say: some optimizations are applied but the url is not rewrited. The optimizations come from the IPRO, an in-fligth optimizer, but IPRO don´t add these header. Maybe is in a transient state denoted by the s-max-age=10 in the cache-control header, this make the resource can´t be stored in intermediate caches more than 10 sec. This file can´t change to a rewrited version? If this file can´t change to a rewrited version, some thing is wrong. Have images in this file? if this file have images in it, if a webp capable browser hit it, these images are converted to webp but if a non webp capable hit it after then the file get not optimized and the whole optimization process will start again to set jpg optimizations.

The headers-Replace first do a RemoveAll and then an Add, so this 2nd header mus come from an Add not from a Replace.

The workaround must include a thest for the response header don´t have the PSA string and the url don´t have the pagespeed string.

What happens if you don´t add the header in apache config?

echu2013 commented 4 years ago

What happens if you don´t add the header in apache config?

It just appears once, as desired. But other stuff not processed by mod_pagespeed then lacks of this header which is not my intended behaviour

Lofesa commented 4 years ago

Stuff not processed by pagespeed have any common characterisc? If they have a common characterisc (from the same folder, some file type...) maybe you can add the header selectively to these resources.

I found in the code that x-content-type header is added in 3 files: https://github.com/apache/incubator-pagespeed-mod/blob/b4bf44cc56d8bbf17494c540dfb6ef20dfcf5073/pagespeed/apache/instaweb_handler.cc#L623

https://github.com/apache/incubator-pagespeed-mod/blob/b4bf44cc56d8bbf17494c540dfb6ef20dfcf5073/net/instaweb/rewriter/server_context.cc#L1104

https://github.com/apache/incubator-pagespeed-mod/blob/b4bf44cc56d8bbf17494c540dfb6ef20dfcf5073/pagespeed/apache/apache_fetch.cc#L121

In the server_context.cc file seems not to be related. I don´t have any apache environtment to test if changing these to header->Replace can break any thing. Maybe @oschaaf or @jmarantz can have some clue.