apache / incubator-pagespeed-ngx

Automatic PageSpeed optimization module for Nginx
http://ngxpagespeed.com/
Apache License 2.0
4.37k stars 363 forks source link

404 on some assets when UrlSigningKey is enabled. #1019

Open rfnx opened 8 years ago

rfnx commented 8 years ago

Hello,

If I enable UrlSigningKey, some assets results in 404 errors. I did some tests with IE 11, Chrome and Firefox and it only happens on images that are rewritten for IE.

These images ends with HASH@2x.extension, unlike other assets that ends with HASH.extension.

Example : HASH@2x.png will results in 404 if UrlSigningKey is enabled.

crowell commented 8 years ago

can you give an example page, or at least the source of the page when visited with IE11 and Chrome/Firefox?

is it all .pagespeed resources?

Thanks, Jeff

rfnx commented 8 years ago

You can see it on this page : https://www.lidocs.me You will see the 404 in your browser tools, and you will see that all the blocks of code in the webpage don't have icon on the top right corner.

It's not all pagespeed resources, only some images I think because css and js works fine. Also, Chrome and Firefox have no issue.

Related question : should I change the location block in nginx from location ~ "\.pagespeed\.([a-z]\.)?[a-z]{2}\.[^.]{10}\.[^.]+" { to location ~ "\.pagespeed\.([a-z]\.)?[a-z]{2}\.[^.]{20}\.[^.]+" { when I enable UrlSigningKey ? The documentation doesn't mention it but it seems necessary.

crowell commented 8 years ago

@rfnx thanks for pointing out the issue in the docs. I'll update that in the morning.

I set my UA string to Mozilla/5.0 (Windows NT 6.3; Trident/7.0; rv:11.0) like Gecko in Chrome to masquerade as IE 11, and I don't see any 404.

img

I don't currently have access to a machine with IE 11, and I'll check that tomorrow as well.

Does your server log mention invalid signatures when you request pages with IE11?

rfnx commented 8 years ago

There is no error in the ngins error_log. But I can easily see the bug when I'm using a Lumia (Windows Phone) or IE on Windows 10.

crowell commented 8 years ago

what is the windows 10 IE user agent string? check here http://whatsmyuseragent.com/

crowell commented 8 years ago

ah, i see, set my UA to Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.71 Safari/537.36 Edge/12.0 (edge browser) UA, and the beacon posts fail.

rfnx commented 8 years ago

UA of the Lumia : Mozilla/5.0 (Mobile; Windows Phone 8.1; Android 4.0; ARM; Trident/7.0; Touch; rv:11.0; IEMobile/11.0; NOKIA; Lumia 820) like iPhone OS 7_0_3 Mac OS X AppleWebKit/537 (KHTML, like Gecko) Mobile Safari/537

rfnx commented 8 years ago

But the beacon does not fail for me, only the asset "xbuttons(...)HASH@2x.png"

EDIT : Also, I think the regex for the nginx location can't work with @2x after the hash, because of the "." (but this is not related to the 404 issue)

crowell commented 8 years ago

the beacon failing was actually unrelated, it doesn't seem like a UA issue, but actually a renaming issue.

The signature is failing because the @2x is being added (probably by wordpress?) and being counted as part of the signature, and CountCharacterMismatches ( https://github.com/pagespeed/mod_pagespeed/blob/4b3fc6d5a175de21a001697fad2a20c9d0ff31bc/pagespeed/kernel/base/string_util.cc#L455 ) counts this as invalid.

The current behavior should probably be changed to stop checking at the end of the computed signature instead of marking additional characters as mismatches.

I'll look changing the behavior.

Thanks for the report!

rfnx commented 8 years ago

You are right, the plugin used for the code blocks is adding @2x for retina screens.

Thanks !

crowell commented 8 years ago

I'm not really familiar with wordpress plugins or wordpress in general, but it sounds like the url signing is working as intended, and plugins that change the url in front of pagespeed are just incompatible with this feature.

What plugin is this, so that I can verify the behavior locally?

jmarantz commented 8 years ago

The plugin is adding that signature in JavaScript? Is that right? If it was being added at HTML generation time then I think PageSpeed would see that as part of the URL and incorporate it in the signing.

Independent of the signing, I don't think a WP plugin that appends to URLs in JavaScript will work well with PageSpeed's image optimization.

-Josh

On Wed, Oct 14, 2015 at 1:34 AM, rfnx notifications@github.com wrote:

You are right, the plugin used for the code blocks is adding @2x https://github.com/2x for retina screens.

Thanks !

— Reply to this email directly or view it on GitHub https://github.com/pagespeed/ngx_pagespeed/issues/1019#issuecomment-147940057 .

rfnx commented 8 years ago

The plugin is Crayon Syntax Highlighter : https://wordpress.org/plugins/crayon-syntax-highlighter/

EDIT : I forgot to mention that if i disable UrlSigningKey, the asset does not 404 anymore but it still has @2x after its hash. So in this case, it seems that PageSpeed works well with javascript that appends text to URL. Only the UrlSigningKey seems to be the problem.

Like @crowell said, I think the behaviour of PageSpeed should be changed to ignore characters after the 20th. Also, why read more characters than we should ? it's only a waste of time (very small, but still a waste of time).

rfnx commented 8 years ago

In https://github.com/pagespeed/mod_pagespeed/blob/master/net/instaweb/rewriter/output_resource.cc#L303 , can this change be a solution :

= full_name_.signature();

to

= StringPiece(full_name_.signature(),0,10);

?

jmarantz commented 8 years ago

Scanning over the home page for that plugin, I suspect that this is related to the "retina buttons" feature. I'm not sure if this is the right URL, but from looking at your page, I think you should just get PageSpeed out of the way for the plugin's retina buttons, maybe with this:

pagespeed Disallow http*:// www.lidocs.me/wp-content/plugins/crayon-syntax-highlighter/css/images/toolbar/buttons.png

To the extent that PageSpeed and the crayon-syntax-highlighter are incompatible in other ways, you might broaden this:

pagespeed Disallow http:// www.lidocs.me/wp-content/plugins/crayon-syntax-highlighter/

but please see if just excluding buttons.png is enough first, and let us know what you find.

I actually think the URL-signing thing is just symptom of the basic trouble you can get into when mix PageSpeed's URL-rewriting functionality with JavaScript that also rewrites URLs. You can also consider using PageSpeed in simple OptimizeForBandwidth https://developers.google.com/speed/pagespeed/module/optimize-for-bandwidthmode, but doing that limits what PageSpeed can do in terms of latency, and your site looks like it's otherwise benefiting from PageSpeed url-modifying features, including combining, cache extension, etc, which are not in OptimizeForBandwidth.

-Josh

On Wed, Oct 14, 2015 at 1:12 PM, rfnx notifications@github.com wrote:

In https://github.com/pagespeed/mod_pagespeed/blob/master/net/instaweb/rewriter/output_resource.cc#L303 , can this change be a solution :

= fullname.signature();

to

= StringPiece(fullname.signature(),0,10);

?

— Reply to this email directly or view it on GitHub https://github.com/pagespeed/ngx_pagespeed/issues/1019#issuecomment-148122173 .

rfnx commented 8 years ago

@jmarantz You are right, when I add pagespeed Disallow "https://www.lidocs.me/wp-content/plugins/crayon-syntax-highlighter/css/images/toolbar/buttons.png"; the problem is fixed !

It's strange because the problem comes from buttons@2x.png, not buttons.png, and if I replace the previous pagespeed configuration with pagespeed Disallow "https://www.lidocs.me/wp-content/plugins/crayon-syntax-highlighter/css/images/toolbar/buttons@2x.png"; then the problem is not fixed.

jmarantz commented 8 years ago

This is because PageSpeed runs on your server and sees <img src="buttons.png"/>. PageSpeed does not see the URL as modified in the client by the plugin JS. The disallow is processed on your server.

-Josh

On Wed, Oct 14, 2015 at 2:29 PM, rfnx notifications@github.com wrote:

@jmarantz https://github.com/jmarantz You are right, when I add

pagespeed Disallow " https://www.lidocs.me/wp-content/plugins/crayon-syntax-highlighter/css/images/toolbar/buttons.png ";

the problem is fixed !

It's strange because the problem comes from buttons@2x.png, not buttons.png, and if I replace the previous pagespeed configuration with

pagespeed Disallow " https://www.lidocs.me/wp-content/plugins/crayon-syntax-highlighter/css/images/toolbar/buttons@2x.png ";

then the problem is not fixed.

— Reply to this email directly or view it on GitHub https://github.com/pagespeed/ngx_pagespeed/issues/1019#issuecomment-148146174 .

rfnx commented 8 years ago

@jmarantz Thanks for your explanation. But don't you think, like @crowell said, that pagespeed should stop checking at the end of the computed signature ? It seems to be a sane change.

jmarantz commented 8 years ago

Making the C++ change you suggested probably makes the signature-test pass. However there's still a functional problem with adding "@2x" to a pagespeed-rewritten image. I haven't sorted out all the ways that can fail, but at a minimum, PageSpeed will ignore the "@2x" part of the URL in that case, and you will not get the "retina" functionality.

PageSpeed might also 404 the request (even with the code you suggested), and it also might serve it but give it a short private cache lifetime. That's the behavior you'd get if you were not enforcing signatures, but the md5 sum of the resource did not match the actual content. PageSpeed would assume the resource was being updated by the web site, and that it had a stale copy cached. To limit poisoning client caches it would private-cache it for 5 minutes.

On another topic, the next release of PageSpeed will include automatic generation of srcset attributes for img tags, to improve their appearance on retina displays without bloating their size on non-retina displays. That way the PageSpeed optimization will be integrated properly with retina handling on the server, and you could turn off that feature of your WP plugin.

-Josh

On Wed, Oct 14, 2015 at 2:41 PM, rfnx notifications@github.com wrote:

@jmarantz https://github.com/jmarantz Thanks for your explanation. But don't you think, like @crowell https://github.com/crowell said, that pagespeed should stop checking at the end of the computed signature ?

— Reply to this email directly or view it on GitHub https://github.com/pagespeed/ngx_pagespeed/issues/1019#issuecomment-148151626 .