Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server header dropped #864

Closed
akuryan opened this issue Dec 25, 2014 · 9 comments
Closed

Server header dropped #864

akuryan opened this issue Dec 25, 2014 · 9 comments

Comments

@akuryan
Copy link

@akuryan akuryan commented Dec 25, 2014

My nginx -V output:
nginx version: nginx/1.6.2
built by gcc 4.4.7 20120313 (Red Hat 4.4.7-11) (GCC)
TLS SNI support enabled
configure arguments: --prefix=/etc/nginx --sbin-path=/usr/sbin/nginx --conf-path=/etc/nginx/nginx.conf --error-log-path=/var/log/nginx/error.log --http-log-path=/var/log/nginx/access.log --pid-path=/var/run/nginx.pid --lock-path=/var/run/nginx.lock --http-client-body-temp-path=/var/cache/nginx/client_temp --http-proxy-temp-path=/var/cache/nginx/proxy_temp --http-fastcgi-temp-path=/var/cache/nginx/fastcgi_temp --http-uwsgi-temp-path=/var/cache/nginx/uwsgi_temp --http-scgi-temp-path=/var/cache/nginx/scgi_temp --user=nginx --group=nginx --with-http_ssl_module --with-http_realip_module --with-http_addition_module --with-http_sub_module --with-http_dav_module --with-http_flv_module --with-http_mp4_module --with-http_gunzip_module --with-http_gzip_static_module --with-http_random_index_module --with-http_secure_link_module --with-http_stub_status_module --with-mail --with-mail_ssl_module --with-http_spdy_module --add-module=/tmp/tmp1ra9oc/rpmbuild//BUILD/nginx-1.6.2/headers-more-nginx-module-0.25 --add-module=/tmp/tmp1ra9oc/rpmbuild//BUILD/nginx-1.6.2/ngx_pagespeed-1.8.31.4-beta --with-file-aio --with-ipv6 --with-cc-opt='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic'

If pagespeed is turned on, it sets Server header to 'nginx' for all dynamic resources.
It does not matter if I am trying to modify this header by means of changing nginx sources or by header-more module, as suggested in accepted answer here:
http://stackoverflow.com/questions/246227/how-do-you-change-the-server-header-returned-by-nginx

Is it possible to modify Server header to custom one with pagespeed turned on?

@oschaaf
Copy link
Member

@oschaaf oschaaf commented Dec 25, 2014

I'm surprised that modifying the default Server: response header value in the nginx source code does not work. I'm certain that ngx_pagespeed will not set nginx on any response header value, unless it was provided on its input. Are you sure you tested with the modified and newly build nginx binary after you made the change to update the server header value?

As for overriding via the HttpHeadersMoreModule: In the current release that won't work, but if you are able to build ngx_pagespeed from source, I think the following patch should allow overriding the Server: response header value.

diff --git a/src/ngx_pagespeed.cc b/src/ngx_pagespeed.cc
index f91e48c..5a27221 100644
--- a/src/ngx_pagespeed.cc
+++ b/src/ngx_pagespeed.cc
@@ -401,8 +401,6 @@ ngx_int_t copy_response_headers_to_ngx(
       continue;
     } else if (STR_EQ_LITERAL(name, "Transfer-Encoding")) {
       continue;
-    } else if (STR_EQ_LITERAL(name, "Server")) {
-      continue;
     }

     u_char* name_s = ngx_pstrdup(r->pool, &name);

Loading

@akuryan
Copy link
Author

@akuryan akuryan commented Dec 26, 2014

@oschaaf Thank you, I would try your patch soon and would update an issue

Loading

@akuryan
Copy link
Author

@akuryan akuryan commented Dec 26, 2014

@oschaaf FYI, in test environment it is ok.
Waiting for production results.

Loading

@akuryan
Copy link
Author

@akuryan akuryan commented Jan 16, 2015

@oschaaf Just wanted to inform you that this patch made it's way through and finally are playing on our production

Loading

@akuryan akuryan closed this Jan 16, 2015
@oschaaf
Copy link
Member

@oschaaf oschaaf commented Jan 16, 2015

@akuryan That is nice to hear, thanks!

Loading

oschaaf added a commit that referenced this issue Jan 16, 2015
- Fixes an issue in the html flow that would prevent overriding the
value of the 'Server' response-header.
- Add tests that ensure we emit a single and correct server header
in all flows when not overriding it.
- Add tests that ensure overriding the 'Server' response header
works. The resource and IPRO flow are added to the expected failures
as those are not working yet (and will be adressed in a follow-up).

Fixed #864 (html flow)
oschaaf added a commit that referenced this issue Jan 17, 2015
- Fixes an issue in the html flow that would prevent overriding the
value of the 'Server' response-header.
- Add tests that ensure we emit a single and correct server header
in all flows when not overriding it.
- Add tests that ensure overriding the 'Server' response header
works. The resource and IPRO flow are added to the expected failures
as those are not working yet (and will be adressed in a follow-up).

Fixed #864 (html flow)
oschaaf added a commit that referenced this issue Feb 23, 2015
- Fixes an issue in the html flow that would prevent overriding the
value of the 'Server' response-header.
- Add tests that ensure we emit a single and correct server header
in all flows when not overriding it.
- Add tests that ensure overriding the 'Server' response header
works. The resource and IPRO flow are added to the expected failures
as those are not working yet (and will be adressed in a follow-up).

Fixed #864 (html flow)
@akuryan
Copy link
Author

@akuryan akuryan commented Mar 5, 2015

@oschaaf
Hi
After long run, we found that, though normally Server header is kept intact, still, for some reasons, on some files it is still being dropped in favor of old value ('nginx')
Looks like, if file is being compressed by pagespeed - it is still drops our custom header from HttpHeadersMore

For example (HttpHeadersMore set to modify Server header):
/js/hcsliders/lib/jquery.jcarousel.min.js.pagespeed.ce._veNtSjbOF.js
Server:nginx

/js/hcsliders/skins/tango/skin.css.pagespeed.ce.fenyJITqUy.css
Server:nginx

/published/publicdata/JINANDREY/attachments/SC/images/hotimg/headbg.jpg
Server:CustomHeader

/webAsystSmsSender.php?act=get_phone_field
Server:CustomHeader

Hope this information will help you.
Problem could be reproduced, when following filters is enabled:
pagespeed EnableFilters recompress_jpeg,convert_gif_to_png,convert_jpeg_to_progressive,insert_dns_prefetch,strip_image_meta_data,extend_cache_css,strip_image_color_profile,extend_cache_scripts,local_storage_cache,jpeg_subsampling,extend_cache_images;

Loading

@oschaaf
Copy link
Member

@oschaaf oschaaf commented Mar 5, 2015

@akuryan To get this to work for.pagespeed. resources and IPRO cache hits, ngx_pagespeed should be configured to run at a different position in the module chain.

trunk-tracking has a change that fixes a problem that arises when doing so, and also adds a flag to allow building nginx with ngx_pagespeed repositioned to run in front of the header module(s), see a8141ea

Loading

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Mar 6, 2015

This should be fixed in the next release, right @oschaaf ?

Loading

@oschaaf
Copy link
Member

@oschaaf oschaaf commented Mar 6, 2015

Loading

@jeffkaufman jeffkaufman changed the title ngx_pagespeed overrides Server header Server header dropped Jul 28, 2015
oschaaf added a commit that referenced this issue Jul 28, 2015
- Fixes an issue in the html flow that would prevent overriding the
value of the 'Server' response-header.
- Add tests that ensure we emit a single and correct server header
in all flows when not overriding it.
- Add tests that ensure overriding the 'Server' response header
works. The resource and IPRO flow are added to the expected failures
as those are not working yet (and will be adressed in a follow-up).

Fixed #864 (html flow)
jeffkaufman added a commit that referenced this issue Jul 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants