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

Duplicate Location headers #725

Closed
akamatgi opened this Issue Jun 11, 2014 · 12 comments

Comments

Projects
None yet
5 participants
@akamatgi
Copy link

akamatgi commented Jun 11, 2014

This issue was introduced in 1.8.31.2
With nginx running as a reverse proxy, if the upstream server responds with a 301 response, an extra Location header is added to the response.
This causes the browser (Chrome) to display an 'Duplicate Headers received from Server' error page.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Jun 12, 2014

@akamatgi
I looked into this, and I think your initial analysis on the list was actually spot on. Could you perhaps add a minimal reproduction configuration with two server{} blocks to this issue?
I can reproduce a case myself where copy_response_headers_from_ngx() is sending the Location: header into PSOL twice, but I would like to make sure we solve your problem.

I am not sure that this duplication in the output is always the case, so I want to look into a few common redirect mechanisms before eliminating these lines:

  if (r->headers_out.location != NULL) {
    headers->Add(HttpAttributes::kLocation,
                 str_to_string_piece(r->headers_out.location->value));
  }

Those lines where added when In-Place-Resource-Optimization landed in ngx_pagespeed, and perhaps they where needed at that point in time. Later on IPRO's separate code for this was de-duplicated by letting copy_response_headers_from_ngx() handle this, but the code that adds the Location header separately was also moved to copy_response_headers_from_ngx() and was new to the non-IPRO flows.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Jun 12, 2014

Attempt to reproduce via the system tests:

  server {
    listen @@SECONDARY_PORT@@;
    server_name redirect.example.com;
    pagespeed FileCachePath "@@FILE_CACHE@@";
    pagespeed off;

    location /redir { 
       return 301 '/foo';
    }
  }

  server {
    listen @@SECONDARY_PORT@@;
    server_name location-header.example.com;
    pagespeed FileCachePath "@@FILE_CACHE@@";
    pagespeed on;
    pagespeed DownstreamCachePurgeLocationPrefix http://localhost:@@SECONDARY_PORT@@/foo;

    location /redir {
      proxy_pass http://127.0.0.1:@@SECONDARY_PORT@@;
      proxy_set_header Host 'redirect.example.com';
      # this line is needed:
      proxy_redirect off;
    }
  }

Executing this:
curl -vv -H "Host: location-header.example.com" http://127.0.0.1:8051/redir

Gives a double location header:

 HTTP/1.1 301 Moved Permanently
* Server nginx/1.6.0 is not blacklisted
< Server: nginx/1.6.0
< Content-Type: text/html
< Transfer-Encoding: chunked
< Connection: keep-alive
< Location: http://redirect.example.com:8051/foo
< Location: http://redirect.example.com:8051/foo
< Date: Thu, 12 Jun 2014 03:37:54 GMT
< X-Page-Speed: 1.8.31.2-3973

Either removing proxy_redirect off; or setting pagespeed off; in the second server{} will give us a single Location header again.

@akamatgi did you, by chance, configure proxy_redirect off?

@akamatgi

This comment has been minimized.

Copy link

akamatgi commented Jun 13, 2014

Otto,
I have not configured the 'proxy_redirect off' directive.
I tried your configuration (internal redirect) and it is not giving a
double Location header.
However, if I use a real upstream server and the upstream server returns
301, I am always getting a duplicate Location header.
My Pagespeed configuration is same as yours, i.e only 2 directives in the
server block:
'pagespeed FileCachePath' and 'pagespeed on'
Hope this information is useful.

Thanks,
-anirudh

On Fri, Jun 13, 2014 at 1:15 AM, Otto van der Schaaf <
notifications@github.com> wrote:

Attempt to reproduce via the system tests:

server {
listen @@SECONDARY_PORT@@;
server_name redirect.example.com;
pagespeed FileCachePath "@@FILE_CACHE@@";
pagespeed off;

location /redir {
   return 301 '/foo';
}

}

server {
listen @@SECONDARY_PORT@@;
server_name location-header.example.com;
pagespeed FileCachePath "@@FILE_CACHE@@";
pagespeed on;
pagespeed DownstreamCachePurgeLocationPrefix http://localhost:@@SECONDARY_PORT@@/foo;

location /redir {
  proxy_pass http://127.0.0.1:@@SECONDARY_PORT@@;
  proxy_set_header Host 'redirect.example.com';
  # this line is needed:
  proxy_redirect off;
}

}

Executing this:
curl -vv -H "Host: location-header.example.com"
http://127.0.0.1:8051/redir

Gives a double location header:

HTTP/1.1 301 Moved Permanently

Either removing proxy_redirect off; or setting pagespeed off; in the
second server{} will give us a single Location header again.

@akamatgi https://github.com/akamatgi did you, by chance, configure proxy_redirect
off?


Reply to this email directly or view it on GitHub
#725 (comment)
.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Jun 16, 2014

@akamatgi A few questions:

  • Could you elaborate on what you mean with a 'real' upstream server?
  • Would you be able to post or pm your full (anonymized) nginx.conf, plus the response headers for both the reverse proxy and the upstream server when it redirects? I looked at this some more with nginx 1.4.7 plus ngx_pagespeed 1.8.31.2, but no luck yet in reproducing this without proxy_redirect off. This would help with making sure that removing
  if (r->headers_out.location != NULL) {
    headers->Add(HttpAttributes::kLocation,
                 str_to_string_piece(r->headers_out.location->value));
  }

is the correct and all we need to fix your problem.

  • Also, it would be good to know which modules are involved, could you also include the output of nginx -V?

I think I may have found a related potential issue when I look at https://github.com/nginx/nginx/blob/master/src/http/ngx_http_header_filter_module.c#L314: this looks like it could re-add a location header based on a location header which got unset by ngx_pagespeed - but I don't think you are hitting that, as you mentioned that the original Location header included the server name.

@akamatgi

This comment has been minimized.

Copy link

akamatgi commented Jun 17, 2014

Otto,
my response:

  1. By 'real' upstream server, I meant that nginx is behaving as a reverse
    proxy and the response is returned by the upstream server as opposed to the
    response generated by nginx itself.
  2. The nginx.conf is as follows (I have replaced our company name with
    nginx, in the conf file, and also the servername):

Save pid to /var/run

pid /var/run/nginx.pid;

Setup core file size to a maximum of 1Gb

worker_rlimit_core 1024m;

Each worker can open a maximum of 1M fds

worker_rlimit_nofile 1048576;

events {
worker_connections 32768;
}

http {
include /usr/local/nginx/conf/mime.types;
default_type application/octet-stream;

log_format  main  '$remote_addr - $remote_user [$time_local] "$request"

'
'$status $body_bytes_sent "$http_referer" '
'"$http_user_agent" "$http_x_forwarded_for" "$host"'
'"$request_time"';

# proxy_cache_path can be defined in the http block only. This

restriction
# that applies to log_format directive will apply here also.
proxy_cache_path /dev/shm/nginx/cache levels=1:2
keys_zone=nginx-zone:10m inactive=60m max_size=200m;
server_names_hash_bucket_size 128;

sendfile          on;
keepalive_timeout 65;

ssl_session_cache shared:SSL:10m;

# Disable nginx version string from 'Server' header
server_tokens off;

# Pass server header as present in the server
proxy_pass_header Server;

# Default server
server {
    listen 80;
    listen 443 ssl;

    ssl_certificate /usr/local/nginx/security/default.pem;
    ssl_certificate_key /usr/local/nginx/security/default.pem;

    return 404;
}

server {
    listen 80;
    server_name www.example.com;

    # Caching policy Caching
    proxy_cache nginx-zone;
    proxy_cache_valid 200 10m;

    # Compression policy Compression
    gzip on;
    gzip_min_length 1024;
    gzip_comp_level 1;
    gzip_vary on;
    gzip_types text/plain;

    # Pagespeed policy PageSpeed
    pagespeed FileCachePath /usr/local/nginx/ngx_pagespeed_cache/;
    pagespeed CustomFetchHeader Accept "*/*";
    pagespeed on;
    pagespeed EnableFilters

collapse_whitespace,resize_mobile_images,move_css_above_scripts;

    location / {
        proxy_pass http://backend;
        proxy_http_version 1.1;
        proxy_set_header Connection "Keep-Alive";
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header Accept-Encoding "";
    }
}

upstream backend {
    least_conn;
    server 192.168.174.149:80 weight=1 max_fails=5 fail_timeout=300;
}

}

Original server response:

curl -i http://192.168.174.149/images
HTTP/1.1 301 Moved Permanently
Server: nginx/1.4.5
Date: Tue, 17 Jun 2014 06:21:01 GMT
Content-Type: text/html
Content-Length: 184
Location: http://192.168.174.149/images/
Connection: keep-alive

<title>301 Moved Permanently</title>

301 Moved Permanently


nginx/1.4.5

Nginx response (with Pagespeed turned on):

curl -i -H "Host: www.example.com" http://192.168.174.135/images
HTTP/1.1 301 Moved Permanently
Content-Type: text/html
Location: http://www.example.com/images/
Transfer-Encoding: chunked
Connection: keep-alive
Location: /images/
Date: Tue, 17 Jun 2014 10:25:48 GMT
X-Page-Speed: 1.8.31.3-
Cache-Control: max-age=0, no-cache

<title>301 Moved Permanently</title>

301 Moved Permanently


nginx/1.4.5 1. Output of nginx -V:

/usr/local/nginx/sbin/nginx -V ( I have replaced the path to the nginx
modules with ...)
nginx version: nginx/1.4.7
built by gcc 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC)
TLS SNI support enabled
configure arguments: --prefix= --without-select_module
--without-poll_module --with-ipv6 --with-http_ssl_module
--with-http_spdy_module --add-module=.../lua-nginx-module-0.9.4
--add-module=.../ngx_devel_kit-0.2.19
--add-module=.../nginx-goodies-nginx-sticky-module-ng-45973be2b64e
--add-module=.../ngx_pagespeed-1.8.31.3-beta

Hope this information is helpful.
Thanks,
-anirudh

On Mon, Jun 16, 2014 at 6:39 PM, Otto van der Schaaf <
notifications@github.com> wrote:

@akamatgi https://github.com/akamatgi A few questions:

  • Could you elaborate on what you mean with a 'real' upstream server?

  • Would you be able to post or pm your full (anonymized) nginx.conf,
    plus the response headers for both the reverse proxy and the upstream
    server when it redirects? I looked at this some more with nginx 1.4.7 plus
    ngx_pagespeed 1.8.31.2, but no luck yet in reproducing this without
    proxy_redirect off. This would help with making sure that removing

    if (r->headers_out.location != NULL) {
    headers->Add(HttpAttributes::kLocation,
    str_to_string_piece(r->headers_out.location->value));
    }

is the correct and all we need to fix your problem.

  • Also, it would be good to know which modules are involved, could you
    also include the output of nginx -V?

I think I may have found a related potential issue when I look at
https://github.com/nginx/nginx/blob/master/src/http/ngx_http_header_filter_module.c#L314:
this looks like it could re-add a location header based on a location
header which got unset by ngx_pagespeed - but I don't think you are hitting
that, as you mentioned that the original Location header included the
server name.


Reply to this email directly or view it on GitHub
#725 (comment)
.

oschaaf added a commit that referenced this issue Jun 30, 2014

location-header: tweak location header handling
- Fix potentially sending the location header into PSOL twice.
- Be more thorough when unsetting the location header

Attempts to fix #725
@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Jun 30, 2014

@akamatgi Could you try if #744 fixes the problem?

@akamatgi

This comment has been minimized.

Copy link

akamatgi commented Jul 1, 2014

Sure, I will let you know in a couple of days.
Thanks,
-anirudh

On Mon, Jun 30, 2014 at 7:51 PM, Otto van der Schaaf <
notifications@github.com> wrote:

@akamatgi https://github.com/akamatgi Could you try if #744
#744 fixes the problem?


Reply to this email directly or view it on GitHub
#725 (comment)
.

@akamatgi

This comment has been minimized.

Copy link

akamatgi commented Jul 3, 2014

Hi @oschaaf,
Yes, your fix works for me.
Thanks,
-anirudh

On Tue, Jul 1, 2014 at 11:04 AM, Anirudh Kamatgi akamatgi@gmail.com wrote:

Sure, I will let you know in a couple of days.
Thanks,
-anirudh

On Mon, Jun 30, 2014 at 7:51 PM, Otto van der Schaaf <
notifications@github.com> wrote:

@akamatgi https://github.com/akamatgi Could you try if #744
#744 fixes the problem?


Reply to this email directly or view it on GitHub
#725 (comment)
.

@oschaaf oschaaf closed this in #744 Jul 15, 2014

@olemartinorg

This comment has been minimized.

Copy link

olemartinorg commented Dec 31, 2015

I had this exact problem in 1.10.33.2, and found this issue. Seems that part of #744 is gone from master now? I tested in 1.9.32.11 and that worked fine.

crowell added a commit that referenced this issue Dec 31, 2015

reapply: location-header: tweak location header handling
- Fix potentially sending the location header into PSOL twice.
- Be more thorough when unsetting the location header

Attempts to fix #725

from oschaaf

crowell added a commit that referenced this issue Dec 31, 2015

reapply: location-header: tweak location header handling
- Fix potentially sending the location header into PSOL twice.
- Be more thorough when unsetting the location header

Attempts to fix #725

from oschaaf
@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented Dec 31, 2015

yep, looks like that commit didn't make it into the 1.10.33.x branch. i've reapplied it to master and trunk-tracking so it should be fixed in the next release.
@olemartinorg if you build from master now it should be fixed.

jeffkaufman added a commit that referenced this issue Jan 26, 2016

reapply: location-header: tweak location header handling
- Fix potentially sending the location header into PSOL twice.
- Be more thorough when unsetting the location header

Attempts to fix #725

from oschaaf

jeffkaufman added a commit that referenced this issue Feb 12, 2016

reapply: location-header: tweak location header handling
- Fix potentially sending the location header into PSOL twice.
- Be more thorough when unsetting the location header

Attempts to fix #725

from oschaaf
@WesCossick

This comment has been minimized.

Copy link

WesCossick commented Mar 2, 2016

From my testing, the duplicate location header problem occurred in 1.10.33.4 but works in 1.10.33.5. Stumped me for a bit because I initially thought 1.10.33.4 was new enough to have the fix applied, but that was not the case.

@oschaaf

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment