From 40c05b4a4ff7c8731c6ee23deea82416408dfc73 Mon Sep 17 00:00:00 2001
From: Otto van der Schaaf <oschaaf@we-amp.com>
Date: Mon, 30 Nov 2015 20:00:08 +0100
Subject: [PATCH] location-header: Be careful with headers_out->location

Only set headers_out->location when the upstream originally did
as well. If the Location: header value involved starts with "/"
nginx will absolutify it, ignoring any X-Forwarded-Proto header
in the process.

Fixes https://github.com/pagespeed/ngx_pagespeed/issues/819
(Confirmed: https://github.com/pagespeed/ngx_pagespeed/issues/1029)
Hopefully fixes https://github.com/pagespeed/ngx_pagespeed/issues/711
---
 src/ngx_pagespeed.cc              | 15 +++++++++++----
 src/ngx_pagespeed.h               |  5 +++++
 test/nginx_system_test.sh         | 17 ++++++++++++++---
 test/pagespeed_test.conf.template | 17 ++++++++++++++++-
 4 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/src/ngx_pagespeed.cc b/src/ngx_pagespeed.cc
index 8645694e4..1f8f2e214 100644
--- a/src/ngx_pagespeed.cc
+++ b/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")) {
-      headers_out->location = header;
+      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);
@@ -1902,12 +1907,12 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r,
     bool is_proxy = false;
     GoogleString mapped_url;
     GoogleString host_header;
-  
+
     if (options->domain_lawyer()->MapOriginUrl(
             url, &mapped_url, &host_header, &is_proxy) && is_proxy) {
       ps_create_base_fetch(ctx, request_context, request_headers.release(),
                           kPageSpeedProxy);
-                          
+
       RewriteDriver* driver;
       if (custom_options.get() == NULL) {
         driver = cfg_s->server_context->NewRewriteDriver(
@@ -1926,7 +1931,7 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r,
 
       return ps_async_wait_response(r);
     }
-      
+
   }
 
   if (html_rewrite) {
@@ -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());
diff --git a/src/ngx_pagespeed.h b/src/ngx_pagespeed.h
index 00fd00e34..cae895a8a 100644
--- a/src/ngx_pagespeed.h
+++ b/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);
diff --git a/test/nginx_system_test.sh b/test/nginx_system_test.sh
index 547901c82..ee976f5f2 100644
--- a/test/nginx_system_test.sh
+++ b/test/nginx_system_test.sh
@@ -322,8 +322,8 @@ fi
 #
 # TODO(sligicki): When the prioritize critical css race condition is fixed, the
 # two prioritize_critical_css tests no longer need to be listed here.
-# TODO(oschaaf): Now that we wait after we send a SIGHUP for the new worker 
-# process to handle requests, check if we can remove more from the expected 
+# TODO(oschaaf): Now that we wait after we send a SIGHUP for the new worker
+# process to handle requests, check if we can remove more from the expected
 # failures here under valgrind.
 if $USE_VALGRIND; then
     PAGESPEED_EXPECTED_FAILURES+="
@@ -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.
@@ -566,7 +577,7 @@ check test $(scrape_stat image_rewrite_total_original_bytes) -ge 10000
 start_test "Reload config"
 
 # Fire up some heavy load if ab is available to test a stressed reload.
-# TODO(oschaaf): make sure we wait for the new worker to get ready to accept 
+# TODO(oschaaf): make sure we wait for the new worker to get ready to accept
 # requests.
 fire_ab_load
 
diff --git a/test/pagespeed_test.conf.template b/test/pagespeed_test.conf.template
index 8143e776f..c2aa6dffc 100644
--- a/test/pagespeed_test.conf.template
+++ b/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 {