Fix improper cache key on write for IPRO of resources with PageSpeed params

We need to be writing with the base name, with params stripped.
This was causing a test to "flake" under valgrind, since with it we got
to the test showing the bug slowly enough for the entry written out by
an earlier test under proper URL to expire.

src/ngx_pagespeed.cc

@@ -71,6 +71,7 @@
 #include "net/instaweb/util/public/time_util.h"
 #include "net/instaweb/util/stack_buffer.h"
 #include "pagespeed/kernel/base/posix_timer.h"
+#include "pagespeed/kernel/http/query_params.h"
 #include "pagespeed/kernel/html/html_keywords.h"
 #include "pagespeed/kernel/thread/pthread_shared_mem.h"
 
@@ -463,7 +464,7 @@ ngx_command_t ps_commands[] = {
     NULL },
 
   { ngx_string("pagespeed"),
-    NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1|
+    NGX_HTTP_LOC_CONF|NGX_HTTP_LIF_CONF|NGX_CONF_TAKE1|
     NGX_CONF_TAKE2|NGX_CONF_TAKE3|NGX_CONF_TAKE4|NGX_CONF_TAKE5,
     ps_loc_configure,
     NGX_HTTP_SRV_CONF_OFFSET,
@@ -799,19 +800,28 @@ char* ps_merge_srv_conf(ngx_conf_t* cf, void* parent, void* child) {
 }
 
 char* ps_merge_loc_conf(ngx_conf_t* cf, void* parent, void* child) {
-  ps_loc_conf_t* parent_cfg_l = static_cast<ps_loc_conf_t*>(parent);
-
-  // The variant of the pagespeed directive that is acceptable in location
-  // blocks is only acceptable in location blocks, so we should never be merging
-  // in options from a server or main block.
-  CHECK(parent_cfg_l->options == NULL);
-
   ps_loc_conf_t* cfg_l = static_cast<ps_loc_conf_t*>(child);
   if (cfg_l->options == NULL) {
     // No directory specific options.
     return NGX_CONF_OK;
   }
 
+  // While you can't put a "location" block inside a "location" block you can
+  // put an "if" block inside a "location" block, which is implemented by making
+  // a pretend "location" block.  In this case we may have pagespeed options
+  // from the parent "location" block as well as from the current locationish
+  // "if" block.
+  ps_loc_conf_t* parent_cfg_l = static_cast<ps_loc_conf_t*>(parent);
+  if (parent_cfg_l->options != NULL) {
+    // Rebase our options off of the ones defined in the parent location block.
+    ps_merge_options(parent_cfg_l->options, &cfg_l->options);
+    return NGX_CONF_OK;
+  }
+
+  // Pagespeed options are defined in this location block, and it either has no
+  // parent (typical case) or is an if block whose parent location block defines
+  // no pagespeed options.  Base our options off of those in the server block.
+
   ps_srv_conf_t* cfg_s = static_cast<ps_srv_conf_t*>(
       ngx_http_conf_get_module_srv_conf(cf, ngx_pagespeed));
 
@@ -1666,6 +1676,7 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r, bool html_rewrite) {
       }
     }
     ctx->recorder = NULL;
+    ctx->url_string = url_string;
 
     // Set up a cleanup handler on the request.
     ngx_http_cleanup_t* cleanup = ngx_http_cleanup_add(r, 0);
@@ -2155,6 +2166,9 @@ ngx_int_t ps_in_place_check_header_filter(ngx_http_request_t* r) {
   NgxServerContext* server_context = cfg_s->server_context;
   MessageHandler* message_handler = cfg_s->handler;
   GoogleString url = ps_determine_url(r);
+  // The URL we use for cache key is a bit different since it may
+  // have PageSpeed query params removed.
+  GoogleString cache_url = ctx->url_string;
 
   // continue process
   if (status_ok) {
@@ -2175,7 +2189,7 @@ ngx_int_t ps_in_place_check_header_filter(ngx_http_request_t* r) {
         kInfo,
         "Could not rewrite resource in-place "
         "because URL is not in cache: %s",
-        url.c_str());
+        cache_url.c_str());
     const SystemRewriteOptions* options = SystemRewriteOptions::DynamicCast(
         ctx->driver->options());
     RequestHeaders request_headers;
@@ -2186,7 +2200,7 @@ ngx_int_t ps_in_place_check_header_filter(ngx_http_request_t* r) {
     // We do that using an Apache output filter.
     ctx->recorder = new InPlaceResourceRecorder(
         RequestContextPtr(cfg_s->server_context->NewRequestContext(r)),
-        url,
+        cache_url,
         ctx->driver->CacheFragment(),
         request_headers.GetProperties(),
         options->respect_vary(),
@@ -2304,6 +2318,10 @@ class NgxPagespeedConsoleAsyncFetch : public AsyncFetchUsingWriter {
       : AsyncFetchUsingWriter(request_context, writer) { }
   virtual void HandleDone(bool status) { }
   virtual void HandleHeadersComplete() { }
+
+  void FlushToNgx(const GoogleString& output, ngx_http_request_t* r) {
+    send_out_headers_and_body(r, *response_headers(), output);
+  }
 };
 
 }  // namespace
@@ -2317,6 +2335,13 @@ ngx_int_t ps_simple_handler(ngx_http_request_t* r,
   NgxMessageHandler* message_handler = factory->ngx_message_handler();
   StringPiece request_uri_path = str_to_string_piece(r->uri);
 
+  GoogleString url_string = ps_determine_url(r);
+  GoogleUrl url(url_string);
+  QueryParams query_params;
+  if (url.IsWebValid()) {
+    query_params.Parse(url.Query());
+  }
+
   GoogleString output;
   StringWriter writer(&output);
   HttpStatus::Code status = HttpStatus::kOK;
@@ -2338,28 +2363,30 @@ ngx_int_t ps_simple_handler(ngx_http_request_t* r,
     }
     case RequestRouting::kStatistics: {
       bool is_global_request =
-          !factory->use_per_vhost_statistics() ||
           StringCaseStartsWith(
               request_uri_path, "/ngx_pagespeed_global_statistics");
-      error_message = server_context->StatisticsHandler(
-          factory->caches(),
-          is_global_request ?
-              factory->statistics() : server_context->statistics(),
-          NULL,  // No SPDY-specific config in ngx_pagespeed.
-          StringPiece(reinterpret_cast<char*>(r->args.data), r->args.len),
-          &content_type,
-          &writer);
-      break;
+      ps_srv_conf_t* cfg_s = ps_get_srv_config(r);
+      RequestContextPtr request_context(
+          cfg_s->server_context->NewRequestContext(r));
+      NgxPagespeedConsoleAsyncFetch fetch(request_context, &writer);
+      server_context->StatisticsPage(
+          is_global_request,
+          query_params,
+          &fetch);
+      fetch.FlushToNgx(output, r);
+      return NGX_OK;
     }
     case RequestRouting::kConsole: {
       ps_srv_conf_t* cfg_s = ps_get_srv_config(r);
       RequestContextPtr request_context(
           cfg_s->server_context->NewRequestContext(r));
       NgxPagespeedConsoleAsyncFetch fetch(request_context, &writer);
-      server_context->ConsoleHandler(*server_context->config(), &fetch);
-      status = static_cast<HttpStatus::Code>(
-          fetch.response_headers()->status_code());
-      break;
+      server_context->ConsoleHandler(*server_context->config(),
+                                     SystemServerContext::kStatistics,
+                                     query_params,
+                                     &fetch);
+      fetch.FlushToNgx(output, r);
+      return NGX_OK;
     }
     case RequestRouting::kMessages: {
       GoogleString log;

src/ngx_pagespeed.h

@@ -103,6 +103,10 @@ typedef struct {
   RewriteDriver* driver;
   InPlaceResourceRecorder* recorder;
   ResponseHeaders* ipro_response_headers;
+
+  // 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;
 } ps_request_ctx_t;
 
 

src/ngx_rewrite_options.cc

@@ -70,8 +70,6 @@ const char* const main_only_options[] = {
   "UseNativeFetcher"
 };
 
-const char kNgxPagespeedStatisticsHandlerPath[] = "/ngx_pagespeed_statistics";
-
 }  // namespace
 
 RewriteOptions::Properties* NgxRewriteOptions::ngx_properties_ = NULL;
@@ -91,11 +89,6 @@ void NgxRewriteOptions::Init() {
   DCHECK(ngx_properties_ != NULL)
       << "Call NgxRewriteOptions::Initialize() before construction";
   InitializeOptions(ngx_properties_);
-
-  // Nginx-specific default.
-  // TODO(sligocki): Get rid of this line and let both Apache and Nginx use
-  // /pagespeed_statistics as the handler.
-  statistics_handler_path_.set_default(kNgxPagespeedStatisticsHandlerPath);
 }
 
 void NgxRewriteOptions::AddProperties() {

test/nginx_system_test.sh

@@ -304,6 +304,28 @@ function run_post_cache_flush() {
 
 # nginx-specific system tests
 
+start_test Test pagespeed directive inside if block inside location block.
+
+URL="http://if-in-location.example.com/"
+URL+="mod_pagespeed_example/inline_javascript.html"
+
+# When we specify the X-Custom-Header-Inline-Js that triggers an if block in the
+# config which turns on inline_javascript.
+WGET_ARGS="--header=X-Custom-Header-Inline-Js:Yes"
+http_proxy=$SECONDARY_HOSTNAME \
+  fetch_until $URL 'grep -c document.write' 1
+OUT=$(http_proxy=$SECONDARY_HOSTNAME $WGET_DUMP $WGET_ARGS $URL)
+check_from "$OUT" fgrep "X-Inline-Javascript: Yes"
+check_not_from "$OUT" fgrep "inline_javascript.js"
+
+# Without that custom header we don't trigger the if block, and shouldn't get
+# any inline javascript.
+WGET_ARGS=""
+OUT=$(http_proxy=$SECONDARY_HOSTNAME $WGET_DUMP $WGET_ARGS $URL)
+check_from "$OUT" fgrep "X-Inline-Javascript: No"
+check_from "$OUT" fgrep "inline_javascript.js"
+check_not_from "$OUT" fgrep "document.write"
+
 # Tests related to rewritten response (downstream) caching.
 
 if [ "$NATIVE_FETCHER" = "on" ]; then
@@ -489,6 +511,18 @@ if [ "$HOSTNAME" = "localhost:$PRIMARY_PORT" ] ; then
     --header=Cookie2:cookie2-data
   check_from "$IPRO_OUTPUT" fgrep -q '    background: MediumPurple;'
   check_from "$IPRO_OUTPUT" fgrep -q 'Vary: Cookie2'
+
+  start_test authorized resources do not get cached and optimized.
+  URL="$TEST_ROOT/auth/medium_purple.css"
+  AUTH="Authorization:Basic dXNlcjE6cGFzc3dvcmQ="
+  not_cacheable_start=$(scrape_stat ipro_recorder_not_cacheable)
+  echo $WGET_DUMP --header="$AUTH" "$URL"
+  OUT=$($WGET_DUMP --header="$AUTH" "$URL")
+  check_from "$OUT" fgrep -q 'background: MediumPurple;'
+  not_cacheable=$(scrape_stat ipro_recorder_not_cacheable)
+  check [ $not_cacheable = $((not_cacheable_start + 1)) ]
+  URL=""
+  AUTH=""
 fi
 
 WGET_ARGS=""
@@ -716,7 +750,15 @@ fetch_until $URL 'fgrep -c file.exception.ssp.css' 1
 start_test statistics load
 
 OUT=$($WGET_DUMP $STATISTICS_URL)
-check_from "$OUT" grep 'VHost-Specific Statistics'
+check_from "$OUT" grep 'PageSpeed Statistics'
+
+start_test statistics handler full-featured
+OUT=$($WGET_DUMP $STATISTICS_URL?config)
+check_from "$OUT" grep "InPlaceResourceOptimization (ipro)"
+
+start_test statistics handler properly sets JSON content-type
+OUT=$($WGET_DUMP $STATISTICS_URL?json)
+check_from "$OUT" grep "Content-Type: application/javascript"
 
 start_test scrape stats works
 
@@ -770,15 +812,51 @@ start_test UseExperimentalJsMinifier
 URL="$TEST_ROOT/experimental_js_minifier/index.html"
 URL+="?PageSpeedFilters=rewrite_javascript"
 # External scripts rewritten.
-fetch_until -save -recursive \
-  $URL 'grep -c src=.*rewrite_javascript\.js\.pagespeed\.jm\.' 2
-check_not grep removed $WGET_DIR/*.pagespeed.jm.*  # No comments should remain.
-check_file_size $FETCH_FILE -lt 1560               # Net savings
-check grep -q preserved $FETCH_FILE                # Preserves certain comments.
+fetch_until -save -recursive $URL 'grep -c src=.*\.pagespeed\.jm\.' 1
+check_not grep "removed" $WGET_DIR/*   # No comments should remain.
+check grep -q "preserved" $WGET_DIR/*  # Contents of <script src=> element kept.
+ORIGINAL_HTML_SIZE=1484
+check_file_size $FETCH_FILE -lt $ORIGINAL_HTML_SIZE  # Net savings
 # Rewritten JS is cache-extended.
 check grep -qi "Cache-control: max-age=31536000" $WGET_OUTPUT
 check grep -qi "Expires:" $WGET_OUTPUT
 
+start_test Source map tests
+URL="$TEST_ROOT/experimental_js_minifier/index.html"
+URL+="?PageSpeedFilters=rewrite_javascript,include_js_source_maps"
+# All rewriting still happening as expected.
+fetch_until -save -recursive $URL 'grep -c src=.*\.pagespeed\.jm\.' 1
+check_not grep "removed" $WGET_DIR/*  # No comments should remain.
+check_file_size $FETCH_FILE -lt $ORIGINAL_HTML_SIZE  # Net savings
+check grep -qi "Cache-control: max-age=31536000" $WGET_OUTPUT
+check grep -qi "Expires:" $WGET_OUTPUT
+
+# No source map for inline JS
+check_not grep sourceMappingURL $FETCH_FILE
+# Yes source_map for external JS
+check grep -q sourceMappingURL $WGET_DIR/script.js.pagespeed.*
+SOURCE_MAP_URL=$(grep sourceMappingURL $WGET_DIR/script.js.pagespeed.* |
+                 grep -o 'http://.*')
+OUTFILE=$OUTDIR/source_map
+check $WGET_DUMP -O $OUTFILE $SOURCE_MAP_URL
+check grep -qi "Cache-control: max-age=31536000" $OUTFILE  # Long cache
+check grep -q "script.js?PageSpeed=off" $OUTFILE  # Has source URL.
+check grep -q '"mappings":' $OUTFILE  # Has mappings.
+
+start_test IPRO source map tests
+URL="$TEST_ROOT/experimental_js_minifier/script.js"
+URL+="?PageSpeedFilters=rewrite_javascript,include_js_source_maps"
+# Fetch until IPRO removes comments.
+fetch_until -save $URL 'grep -c removed' 0
+# Yes source_map for external JS
+check grep -q sourceMappingURL $FETCH_FILE
+SOURCE_MAP_URL=$(grep sourceMappingURL $FETCH_FILE | grep -o 'http://.*')
+OUTFILE=$OUTDIR/source_map
+check $WGET_DUMP -O $OUTFILE $SOURCE_MAP_URL
+check grep -qi "Cache-control: max-age=31536000" $OUTFILE  # Long cache
+check grep -q "script.js?PageSpeed=off" $OUTFILE  # Has source URL.
+check grep -q '"mappings":' $OUTFILE  # Has mappings.
+
 start_test aris disables js combining for introspective js and only i-js
 URL="$TEST_ROOT/avoid_renaming_introspective_javascript__on/"
 URL+="?PageSpeedFilters=combine_javascript"
@@ -2369,9 +2447,12 @@ OUTFILE=$OUTDIR/ipro_resource_output
 # Fetch the HTML to initiate rewriting and caching of the image.
 http_proxy=$SECONDARY_HOSTNAME check $WGET_DUMP $HTML_URL -O $OUTFILE
 
-# First IPRO resource request after a short wait: it will have the full TTL.
+# First IPRO resource request after a short wait: never be optimized
+# because our non-load-from-file flow doesn't support that, but it will have
+# the full TTL.
 sleep 2
 http_proxy=$SECONDARY_HOSTNAME check $WGET_DUMP $RESOURCE_URL -O $OUTFILE
+check_file_size "$OUTFILE" -gt 15000 # not optimized
 RESOURCE_MAX_AGE=$( \
   extract_headers $OUTFILE | \
   grep 'Cache-Control:' | tr -d '\r' | \
@@ -2379,14 +2460,16 @@ RESOURCE_MAX_AGE=$( \
 check test -n "$RESOURCE_MAX_AGE"
 check test $RESOURCE_MAX_AGE -eq 333
 
-# Second IPRO resource request after a short wait: the TTL will be reduced.
-sleep 2
+# Second IPRO resource request after a short wait: it will still be optimized
+# and the TTL will be reduced.
 http_proxy=$SECONDARY_HOSTNAME check $WGET_DUMP $RESOURCE_URL -O $OUTFILE
+check_file_size "$OUTFILE" -lt 15000 # optimized
 RESOURCE_MAX_AGE=$( \
   extract_headers $OUTFILE | \
   grep 'Cache-Control:' | tr -d '\r' | \
   sed -e 's/^ *Cache-Control: *//' | sed -e 's/^.*max-age=\([0-9]*\).*$/\1/')
 check test -n "$RESOURCE_MAX_AGE"
+
 check test $RESOURCE_MAX_AGE -lt 333
 check test $RESOURCE_MAX_AGE -gt 300
 

test/pagespeed_test.conf.template

@@ -196,6 +196,45 @@ http {
     }
   }
 
+  server {
+    listen @@SECONDARY_PORT@@;
+    server_name if-in-server.example.com;
+    pagespeed FileCachePath "@@SECONDARY_CACHE@@";
+
+    pagespeed RewriteLevel PassThrough;
+    set $inline_javascript "No";
+
+    if ($http_x_custom_header_inline_js) {
+      # TODO(jefftk): Turn on NGX_HTTP_SIF_CONF and figure out how to get
+      # pagespeed directives inside of a server location block to be respected,
+      # then uncomment the following line and duplicate the if-in-location test
+      # for if-in-server.
+      #pagespeed EnableFilters inline_javascript;
+      set $inline_javascript "Yes";
+    }
+
+    add_header "X-Inline-Javascript" $inline_javascript;
+  }
+
+  server {
+    listen @@SECONDARY_PORT@@;
+    server_name if-in-location.example.com;
+    pagespeed FileCachePath "@@SECONDARY_CACHE@@";
+
+
+    location / {
+      set $inline_javascript "No";
+      pagespeed RewriteLevel PassThrough;
+
+      if ($http_x_custom_header_inline_js) {
+        pagespeed EnableFilters inline_javascript;
+        set $inline_javascript "Yes";
+      }
+
+      add_header "X-Inline-Javascript" $inline_javascript;
+    }
+  }
+
   server {
     listen @@SECONDARY_PORT@@;
     server_name mpd.example.com;
@@ -1010,6 +1049,13 @@ http {
       pagespeed InPlaceRewriteDeadlineMs -1;
     }
 
+    # Test to make sure that user-authenticated resources do not get cached and
+    # optimized.
+    location /mod_pagespeed_test/auth/ {
+       auth_basic "Restricted";
+       auth_basic_user_file "@@SERVER_ROOT@@mod_pagespeed_test/auth/passwd.conf";
+    }
+
     location /mod_pagespeed_test/ipro/cookie/ {
       # Add Vary:Cookie.  This should prevent us from optimizing the
       # vary_cookie.css even though PagespeedRespectVary is off.