Add x content type options nosniff to ipro

[Lofesa]

Add a x-content-type-options: nosniff to files optimized by IPRO

[oschaaf]

Could you explain the rationale behind this ?

[Lofesa]

Could you explain the rationale behind this ?

IPRO deletes all headers set by the user and then put a fever set of headers. This header is a (relative) security thing that prevents the change of the mime type. A know security issue in wordpress is to upload a image file that is a php script and this help to mitigate this. No matter what the user do to set this header, set by the server or by adding AddResourceHeader directive, when the file is only optimized by IPRO. For example when a file is loaded by a javascript snipet, never is optimized by pagespeed but by IPRO, that deletes this header and then a bunch of security test will fail to not have this header. Maybe the x-xss-protection has the same issue. Maybe thi´s not the rigth way to do and we need to make some type of AddResourceHeader for IPRO.

EDIT: Re-elaborated. Test if resource is a css or a js file, then add the header X-Content-Type-Options. IPRO may change the content-type header in images and may missmatch with the file extension, but css/js files can´t change the content-type or the file extension

[Lofesa]

nice! do you want to take a shot at adding a unit test?

@jmarantz I would like to do it, but I´m lost. Remind you that my skill in c++ is less than nothing. I have take a look at in_place_rewrite_context_test.cc and can´t any test for response headers.

Maybe adding the same logic that the PR in SetDefaultHeaders (line 309) and then add some EXPECT_STREQ in CheckCachingAndContentType (line 441). Thougs? Ping @oschaaf

[jmarantz]

I think it might not be that easy for you to know how to add a test even if you were more experienced in C++ :) It's a complex infrastructure. So first of all, thanks for digging in and improving things.

I don't think changing SetDefaultHeaders makes sense in this case; I think from context (and distant memory), that function should be renamed SetOriginDefaultHeaders. So what the test is trying to do is to set up a mock fetcher where we pre-set what the origin response headers and body should be.

I think you can add a check to https://github.com/apache/incubator-pagespeed-mod/blob/b3349ab11b9a6345f589fba244fcfbc11af69a5a/net/instaweb/rewriter/in_place_rewrite_context_test.cc#L367

which, for JS and CSS files, verifies that the nosniff header is present. Now that is in a helper method which gets called also for jpeg/html files as well, but I think it might be good enough to do only do that check if the URL ends with ".css" or ".js".

  if (strings::EndsWIth(url, ".js") || strings::EndsWith(url, ".css")) {
  ...

let me know if you need more help.

Actually I have another question too: why not put nosniff in for other content types? Of course PageSpeed may change the content-type, but it will be correct coming out of PageSpeed's IPRO flow, and the browser should not second-guess, IMO.

[Lofesa]

Well.... I appreciate the work people have done in this module, I use it for free and get advantanges using it so I try to help to maintain the module so is a take and give "contract".

In a first instance I have set the x-content-type-options nosniff unconditionaly to all resources optimized by IPRO cause I try to solve my own problem with a js file loaded by a js script that have stripped the header by IPRO but then in a response to a issue in the mod-pagespeed-discuss list make me think about this in images optimized by IPRO where the content-type mismacht with the file extension, in this case, if the nosniff stuff is set, the image won´t load.

Why not other types? As far as I know, IPRO only works whit html,css,js and images. Images can have a content-type mismacht so this discard images for set the header, html can be or not a file (think in the pretty url from wordpress) so is hard to determine if if the resource is html or not, css and js files are the good options to set the nosniff, optimized by IPRO or not, their type don´t change.

And at last, the spec for the x-conten-type-options nosniff is intended for css and js files.

[jmarantz]

I most strongly associate nosniff with HTML vs Text. But I've no objection to setting it on css/js too.

You are saying that if a JPEG file is served with Content-Type "image/jpeg", a nosniff header, and a ".png" extension, browsers won't load it?

I didn't think nosniff was going to have an issue with the wrong extension; I thought it was a problem with looking at bytes of content in like Internet Explorer 5 or 6, and "upgrading" text to HTML, thus allowing scary things to happen :)

[Lofesa]

@jmarantz Bad news. Setting if (strings::EndsWIth(url, ".js") || strings::EndsWith(url, ".css")) { in https://github.com/apache/incubator-pagespeed-mod/blob/b3349ab11b9a6345f589fba244fcfbc11af69a5a/net/instaweb/rewriter/in_place_rewrite_context_test.cc#L367

make some test fail cause the nosniff header is empt. Seems that FetchAndCheckResponse is called before call to FixFetchFallbackHeaders, so the header is not added in test. But I don´t find where FixFetchFallbackHeaders is called in test.

I have put a std:cout in in_place_rewrite_context.cc so (new line 423 in the PR) when the header is set, a string is echoed. Same with the if (strings::EndsWIth(url, ".js") || strings::EndsWith(url, ".css")) { in the FetchAndCheckResponse, then in the failed test I gave first the string from FetchAndCheckResponse and later the string from FixFetchFallbackHeaders.

[jmarantz]

can you upload the broken change and I'll comment on the PR?

[Lofesa]

@jmarantz In the new file you can see std::cout << "TEST " ;. A similar echo was set in net/instaweb/rewriter/in_place_rewrite_context.cc, but with text "FILTER". When run the tests, the message TEST is outputed before the message FILTER.

[oschaaf]

Closing this in an effort to clean up stale pull requests (but feel free to re-open if you want to pick this up again).