Merge pull request #1055 from pagespeed/oschaaf-trunk-tracking-location-header-handling

location-header: Be careful with headers_out->location

src/ngx_pagespeed.cc

@@ -599,7 +599,10 @@ ngx_int_t copy_response_headers_to_ngx(
     } else if (STR_EQ_LITERAL(name, "Last-Modified")) {
       headers_out->last_modified = header;
     } else if (STR_EQ_LITERAL(name, "Location")) {
+      ps_request_ctx_t* ctx = ps_get_request_context(r);
+      if (ctx->location_field_set) {
         headers_out->location = header;
+      }
     } else if (STR_EQ_LITERAL(name, "Server")) {
       headers_out->server = header;
     } else if (STR_EQ_LITERAL(name, "Content-Length")) {
@@ -1264,6 +1267,7 @@ ngx_int_t ps_decline_request(ngx_http_request_t* r) {
 
   ctx->driver->Cleanup();
   ctx->driver = NULL;
+  ctx->location_field_set = false;
 
   // re init ctx
   ctx->html_rewrite = true;
@@ -1833,6 +1837,7 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r,
 
     ctx->recorder = NULL;
     ctx->url_string = url_string;
+    ctx->location_field_set = false;
 
     // Set up a cleanup handler on the request.
     ngx_http_cleanup_t* cleanup = ngx_http_cleanup_add(r, 0);
@@ -2233,6 +2238,8 @@ ngx_int_t ps_html_rewrite_header_filter(ngx_http_request_t* r) {
   }
 
   ps_strip_html_headers(r);
+  // See https://github.com/pagespeed/ngx_pagespeed/issues/819
+  ctx->location_field_set = r->headers_out.location != NULL;
 
   // TODO(jefftk): is this thread safe?
   copy_response_headers_from_ngx(r, ctx->base_fetch->response_headers());

src/ngx_pagespeed.h

@@ -103,6 +103,11 @@ typedef struct {
   // We need to remember the URL here as well since we may modify what NGX
   // gets by stripping our special query params and honoring X-Forwarded-Proto.
   GoogleString url_string;
+
+  // We need to remember if the upstream had headers_out->location set, because
+  // we should mirror that when we write it back. nginx may absolutify
+  // Location: headers that start with '/' without regarding X-Forwarded-Proto.
+  bool location_field_set;
 } ps_request_ctx_t;
 
 ps_request_ctx_t* ps_get_request_context(ngx_http_request_t* r);

test/nginx_system_test.sh

@@ -514,6 +514,17 @@ check $WGET_DUMP -O $FETCHED $HEADERS $URL
 # When enabled, we respect X-Forwarded-Proto and thus list base as https.
 check fgrep -q '<base href="https://' $FETCHED
 
+start_test Relative redirects starting with a forward slash survive.
+URL=http://xfp.example.com/redirect
+# wget seems a bit hairy here, when it comes to handling (relative) redirects.
+# I could not get this test going with wget, and that is why curl is used here.
+# TODO(oschaaf): debug wget some more and swap out curl here.
+OUT=$(curl -v --proxy $SECONDARY_HOSTNAME $URL 2>&1)
+check_from "$OUT" egrep -q '301 Moved Permanently'
+# The important part is that we don't end up with an absolute location here.
+check_from "$OUT" grep -q 'Location: /mod_pagespeed_example'
+check_not_from "$OUT" grep -q 'Location: http'
+
 # Test that loopback route fetcher works with vhosts not listening on
 # 127.0.0.1
 start_test IP choice for loopback fetches.

test/pagespeed_test.conf.template

@@ -621,8 +621,23 @@ http {
     listen [::]:@@SECONDARY_PORT@@;
     server_name xfp.example.com;
     pagespeed FileCachePath "@@FILE_CACHE@@";
-
     pagespeed RespectXForwardedProto on;
+
+    location /redirecting_origin {
+      pagespeed off;
+      # Hack: we clear the response headers using headers_more.
+      # If we don't, nginx will add an extra empty Location: headers here.
+      # It is kind of hard to get nginx to generate a relative location header
+      # that starts with "/".
+      more_clear_headers 'Location';
+      add_header Location /mod_pagespeed_example;
+      return 301;
+    }
+    location /redirect {
+      proxy_method GET;
+      proxy_pass http://127.0.0.1:@@SECONDARY_PORT@@/redirecting_origin;
+      proxy_set_header "Host" "xfp.example.com";
+    }
   }
 
   server {