Merge pull request #736 from pagespeed/oschaaf-lff-script-support

script-variables: Support LoadFromFileXXX

src/ngx_pagespeed.cc

@@ -582,7 +582,16 @@ char* ps_init_dir(const StringPiece& directive,
   return NULL;
 }
 
-#define NGX_PAGESPEED_MAX_ARGS 10
+ngx_int_t ps_dollar(
+    ngx_http_request_t *r, ngx_http_variable_value_t *v, uintptr_t data) {
+  v->valid = 1;
+  v->no_cacheable = 0;
+  v->not_found = 0;
+  v->data = reinterpret_cast<u_char*>(const_cast<char*>("$"));
+  v->len = 1;
+  return NGX_OK;
+}
+
 char* ps_configure(ngx_conf_t* cf,
                    NgxRewriteOptions** options,
                    MessageHandler* handler,
@@ -622,8 +631,26 @@ char* ps_configure(ngx_conf_t* cf,
     *options = new NgxRewriteOptions(
         cfg_m->driver_factory->thread_system());
   }
+
+  bool process_script_variables = dynamic_cast<NgxRewriteDriverFactory*>(
+      cfg_m->driver_factory)->process_script_variables();
+
+  if (process_script_variables) {
+    // To be able to use '$', we map '$ps_dollar' to '$' via a script variable.
+    ngx_str_t name = ngx_string("ps_dollar");
+    ngx_http_variable_t* var = ngx_http_add_variable(
+        cf, &name, NGX_HTTP_VAR_CHANGEABLE);
+
+    if (var == NULL) {
+      return const_cast<char*>(
+          "Failed to add global configuration variable for '$ps_dollar'");
+    }
+    var->get_handler = ps_dollar;
+  }
+
   const char* status = (*options)->ParseAndSetOptions(
-      args, n_args, cf->pool, handler, cfg_m->driver_factory, option_scope);
+      args, n_args, cf->pool, handler, cfg_m->driver_factory, option_scope, cf,
+      process_script_variables);
 
   // nginx expects us to return a string literal but doesn't mark it const.
   return const_cast<char*>(status);
@@ -789,6 +816,16 @@ void ps_merge_options(NgxRewriteOptions* parent_options,
     NgxRewriteOptions* child_specific_options = *child_options;
     *child_options = parent_options->Clone();
     (*child_options)->Merge(*child_specific_options);
+
+    if (child_specific_options->clear_inherited_scripts()) {
+      // We don't want to inherit any inherited script lines from the parent
+      // options here, so we just stick to the child specific ones.
+      child_specific_options->CopyScriptLinesTo(*child_options);
+    } else {
+      // We append the child specific script lines to the parent's script lines
+      // so we preserve the order in which they will be executed at request time
+      child_specific_options->AppendScriptLinesTo(*child_options);
+    }
     delete child_specific_options;
   }
 }
@@ -830,6 +867,9 @@ char* ps_merge_srv_conf(ngx_conf_t* cf, void* parent, void* child) {
   // let it do that, then merge in options we got from the config file.
   // Once we do that we're done with cfg_s->options.
   cfg_s->server_context->global_options()->Merge(*cfg_s->options);
+  NgxRewriteOptions* ngx_options = dynamic_cast<NgxRewriteOptions*>(
+      cfg_s->server_context->global_options());
+  cfg_s->options->CopyScriptLinesTo(ngx_options);
   delete cfg_s->options;
   cfg_s->options = NULL;
 
@@ -1380,9 +1420,13 @@ bool ps_determine_options(ngx_http_request_t* r,
 
   // Because the caller takes ownership of any options we return, the only
   // situation in which we can avoid allocating a new RewriteOptions is if the
-  // global options are ok as are.
+  // global options are ok as they are and we don't have script variables we
+  // need to evaluate at this point.
+  NgxRewriteOptions* ngx_global_options =
+      dynamic_cast<NgxRewriteOptions*>(global_options);
   if (!have_request_options && directory_options == NULL &&
-      !global_options->running_experiment()) {
+      !global_options->running_experiment() &&
+      ngx_global_options->script_lines().size() == 0) {
     return true;
   }
 
@@ -1393,6 +1437,18 @@ bool ps_determine_options(ngx_http_request_t* r,
     *options = global_options->Clone();
   }
 
+  NgxRewriteDriverFactory* ngx_factory = dynamic_cast<NgxRewriteDriverFactory*>(
+    cfg_s->server_context->factory());
+  NgxRewriteOptions* ngx_options = dynamic_cast<NgxRewriteOptions*>(*options);
+
+  // ExecuteScriptVariables() sets 'pagespeed off' on ngx_options when execution
+  // fails and then returns false. When that happens we return, as we don't want
+  // to allow enabling pagespeed by request and execute without the intended
+  // configuration.
+  if (!ngx_options->ExecuteScriptVariables(r, cfg_s->handler, ngx_factory)) {
+    return false;
+  }
+
   // Modify our options in response to request options if specified.
   if (have_request_options) {
     (*options)->Merge(*request_options);

src/ngx_rewrite_driver_factory.cc

@@ -81,7 +81,9 @@ NgxRewriteDriverFactory::NgxRewriteDriverFactory(
       use_native_fetcher_(false),
       ngx_shared_circular_buffer_(NULL),
       hostname_(hostname.as_string()),
-      port_(port) {
+      port_(port),
+      process_script_variables_(false),
+      process_script_variables_set_(false) {
   InitializeDefaultOptions();
   default_options()->set_beacon_url("/ngx_pagespeed_beacon");
   SystemRewriteOptions* system_options = dynamic_cast<SystemRewriteOptions*>(

src/ngx_rewrite_driver_factory.h

@@ -120,6 +120,9 @@ class NgxRewriteDriverFactory : public SystemRewriteDriverFactory {
   void set_rate_limit_background_fetches(bool x) {
     rate_limit_background_fetches_ = x;
   }
+  bool process_script_variables() {
+    return process_script_variables_;
+  }
 
   // We use a beacon handler to collect data for critical images,
   // css, etc., so filters should be configured accordingly.
@@ -135,6 +138,15 @@ class NgxRewriteDriverFactory : public SystemRewriteDriverFactory {
 
   virtual void SetCircularBuffer(SharedCircularBuffer* buffer);
 
+  bool SetProcessScriptVariables(bool process_script_variables) {
+    if (!process_script_variables_set_) {
+      process_script_variables_ = process_script_variables;
+      process_script_variables_set_ = true;
+      return true;
+    }
+    return false;
+  }
+
  private:
   Timer* timer_;
 
@@ -166,6 +178,8 @@ class NgxRewriteDriverFactory : public SystemRewriteDriverFactory {
 
   GoogleString hostname_;
   int port_;
+  bool process_script_variables_;
+  bool process_script_variables_set_;
 
   DISALLOW_COPY_AND_ASSIGN(NgxRewriteDriverFactory);
 };

src/ngx_rewrite_options.cc

@@ -31,6 +31,7 @@ extern "C" {
 #include "net/instaweb/rewriter/public/file_load_policy.h"
 #include "net/instaweb/rewriter/public/rewrite_options.h"
 #include "net/instaweb/system/public/system_caches.h"
+#include "net/instaweb/util/public/message_handler.h"
 #include "net/instaweb/util/public/timer.h"
 
 namespace net_instaweb {
@@ -97,6 +98,7 @@ NgxRewriteOptions::NgxRewriteOptions(ThreadSystem* thread_system)
 void NgxRewriteOptions::Init() {
   DCHECK(ngx_properties_ != NULL)
       << "Call NgxRewriteOptions::Initialize() before construction";
+  clear_inherited_scripts_ = false;
   InitializeOptions(ngx_properties_);
 }
 
@@ -250,7 +252,7 @@ const char* ps_error_string_for_option(
 const char* NgxRewriteOptions::ParseAndSetOptions(
     StringPiece* args, int n_args, ngx_pool_t* pool, MessageHandler* handler,
     NgxRewriteDriverFactory* driver_factory,
-    RewriteOptions::OptionScope scope) {
+    RewriteOptions::OptionScope scope, ngx_conf_t* cf, bool compile_scripts) {
   CHECK_GE(n_args, 1);
 
   StringPiece directive = args[0];
@@ -266,6 +268,67 @@ const char* NgxRewriteOptions::ParseAndSetOptions(
         pool, directive, "cannot be set at this scope.");
   }
 
+  ScriptLine* script_line;
+  script_line = NULL;
+  // Only allow script variable support for LoadFromFile for now.
+  // Note that LoadFromFile should not be scriptable on wildcard hosts,
+  // as browsers might be able to manipulate its natural use-case: $http_host.
+  if (!StringCaseStartsWith(directive, "LoadFromFile")) {
+    compile_scripts = false;
+  }
+
+  if (n_args == 1 && StringCaseEqual(directive, "ClearInheritedScripts")) {
+    clear_inherited_scripts_ = true;
+    return NGX_CONF_OK;
+  }
+
+  if (compile_scripts) {
+    CHECK(cf != NULL);
+    int i;
+    // Skip the first arg which is always 'pagespeed'
+    for (i = 1; i < n_args; i++) {
+      ngx_str_t script_source;
+
+      script_source.len = args[i].as_string().length();
+      std::string tmp = args[i].as_string();
+      script_source.data = reinterpret_cast<u_char*>(
+          const_cast<char*>(tmp.c_str()));
+
+      if (ngx_http_script_variables_count(&script_source) > 0) {
+        ngx_http_script_compile_t* sc =
+            reinterpret_cast<ngx_http_script_compile_t*>(
+                ngx_pcalloc(cf->pool, sizeof(ngx_http_script_compile_t)));
+        sc->cf = cf;
+        sc->source = &script_source;
+        sc->lengths = reinterpret_cast<ngx_array_t**>(
+            ngx_pcalloc(cf->pool, sizeof(ngx_array_t*)));
+        sc->values = reinterpret_cast<ngx_array_t**>(
+            ngx_pcalloc(cf->pool, sizeof(ngx_array_t*)));
+        sc->variables = 1;
+        sc->complete_lengths = 1;
+        sc->complete_values = 1;
+        if (ngx_http_script_compile(sc) != NGX_OK) {
+          return ps_error_string_for_option(
+              pool, directive, "Failed to compile script variables");
+        } else {
+          if (script_line == NULL) {
+            script_line = new ScriptLine(args, n_args, scope);
+          }
+          script_line->AddScriptAndArgIndex(sc, i);
+        }
+      }
+    }
+
+    if (script_line != NULL) {
+      script_lines_.push_back(RefCountedPtr<ScriptLine>(script_line));
+      // We have found script variables in the current configuration line, and
+      // prepared the associated rewriteoptions for that.
+      // We will defer parsing, validation and processing of this line to
+      // request time. That means we are done handling this configuration line.
+      return NGX_CONF_OK;
+    }
+  }
+
   GoogleString msg;
   OptionSettingResult result;
   if (n_args == 1) {
@@ -314,6 +377,30 @@ const char* NgxRewriteOptions::ParseAndSetOptions(
     } else if (IsDirective(directive, "StaticAssetPrefix")) {
       driver_factory->set_static_asset_prefix(arg);
       result = RewriteOptions::kOptionOk;
+    } else if (StringCaseEqual("ProcessScriptVariables", args[0])) {
+      if (scope == RewriteOptions::kProcessScopeStrict) {
+        if (StringCaseEqual(arg, "on")) {
+          if (driver_factory->SetProcessScriptVariables(true)) {
+            result = RewriteOptions::kOptionOk;
+          } else {
+            return const_cast<char*>(
+                "pagespeed ProcessScriptVariables: can only be set once");
+          }
+        } else if (StringCaseEqual(arg, "off")) {
+          if (driver_factory->SetProcessScriptVariables(false)) {
+            result = RewriteOptions::kOptionOk;
+          } else {
+            return const_cast<char*>(
+                "pagespeed ProcessScriptVariables: can only be set once");
+          }
+        } else {
+          return const_cast<char*>(
+              "pagespeed ProcessScriptVariables: invalid value");
+        }
+      } else {
+        return const_cast<char*>(
+            "ProcessScriptVariables is only allowed at the top level");
+      }
     } else {
       result = ParseAndSetOptionFromName1(directive, arg, &msg, handler);
     }
@@ -361,13 +448,92 @@ const char* NgxRewriteOptions::ParseAndSetOptions(
   return NULL;
 }
 
+// Execute all entries in the script_lines vector, and hand the result off to
+// ParseAndSetOptions to obtain the final option values.
+bool NgxRewriteOptions::ExecuteScriptVariables(
+    ngx_http_request_t* r, MessageHandler* handler,
+    NgxRewriteDriverFactory* driver_factory) {
+  bool script_error = false;
+
+  if (script_lines_.size() > 0) {
+    std::vector<RefCountedPtr<ScriptLine> >::iterator it;
+    for (it = script_lines_.begin() ; it != script_lines_.end(); ++it) {
+      ScriptLine* script_line = it->get();
+      StringPiece args[NGX_PAGESPEED_MAX_ARGS];
+      std::vector<ScriptArgIndex*>::iterator cs_it;
+      int i;
+
+      for (i = 0; i < script_line->n_args(); i++) {
+        args[i] = script_line->args()[i];
+      }
+
+      for (cs_it = script_line->data().begin();
+           cs_it != script_line->data().end(); cs_it++) {
+        ngx_http_script_compile_t* script;
+        ngx_array_t* values;
+        ngx_array_t* lengths;
+        ngx_str_t value;
+
+        script = (*cs_it)->script();
+        lengths = *script->lengths;
+        values = *script->values;
+
+        if (ngx_http_script_run(r, &value, lengths->elts, 0, values->elts)
+            == NULL) {
+          handler->Message(kError, "ngx_http_script_run error");
+          script_error = true;
+          break;
+        } else  {
+          args[(*cs_it)->index()] = str_to_string_piece(value);
+        }
+      }
+
+      const char* status = ParseAndSetOptions(args, script_line->n_args(),
+          r->pool, handler, driver_factory, script_line->scope(), NULL /*cf*/,
+          false /*compile scripts*/);
+
+      if (status != NULL) {
+        script_error = true;
+        handler->Message(kWarning,
+            "Error setting option value from script: '%s'", status);
+        break;
+      }
+    }
+  }
+
+  if (script_error) {
+    handler->Message(kWarning,
+        "Script error(s) in configuration, disabling optimization");
+    set_enabled(RewriteOptions::kEnabledOff);
+    return false;
+  }
+
+  return true;
+}
+
+void NgxRewriteOptions::CopyScriptLinesTo(
+    NgxRewriteOptions* destination) const {
+  destination->script_lines_ = script_lines_;
+}
+
+void NgxRewriteOptions::AppendScriptLinesTo(
+    NgxRewriteOptions* destination) const {
+  destination->script_lines_.insert(destination->script_lines_.end(),
+                                    script_lines_.begin(), script_lines_.end());
+}
+
 NgxRewriteOptions* NgxRewriteOptions::Clone() const {
   NgxRewriteOptions* options = new NgxRewriteOptions(
       StrCat("cloned from ", description()), thread_system());
+  this->CopyScriptLinesTo(options);
   options->Merge(*this);
   return options;
 }
 
+void NgxRewriteOptions::Merge(const RewriteOptions& src) {
+  SystemRewriteOptions::Merge(src);
+}
+
 const NgxRewriteOptions* NgxRewriteOptions::DynamicCast(
     const RewriteOptions* instance) {
   return dynamic_cast<const NgxRewriteOptions*>(instance);

src/ngx_rewrite_options.h

@@ -27,13 +27,81 @@ extern "C" {
   #include <ngx_http.h>
 }
 
+#include <vector>
+
+#include "net/instaweb/util/public/message_handler.h"
+#include "net/instaweb/util/public/ref_counted_ptr.h"
+#include "net/instaweb/util/public/stl_util.h"          // for STLDeleteElements
 #include "net/instaweb/rewriter/public/rewrite_options.h"
 #include "net/instaweb/system/public/system_rewrite_options.h"
 
+#define NGX_PAGESPEED_MAX_ARGS 10
+
 namespace net_instaweb {
 
 class NgxRewriteDriverFactory;
 
+class ScriptArgIndex {
+ public:
+  explicit ScriptArgIndex(ngx_http_script_compile_t* script, int index)
+    : script_(script), index_(index) {
+      CHECK(script != NULL);
+      CHECK(index > 0 && index < NGX_PAGESPEED_MAX_ARGS);
+  }
+
+  virtual ~ScriptArgIndex() {}
+
+  ngx_http_script_compile_t* script() { return script_; }
+  int index() { return index_; }
+
+ private:
+  // Not owned.
+  ngx_http_script_compile_t* script_;
+  int index_;
+};
+
+// Refcounted, because the ScriptArgIndexes inside data_ can be shared between
+// different rewriteoptions.
+class ScriptLine : public RefCounted<ScriptLine> {
+ public:
+  explicit ScriptLine(StringPiece* args, int n_args,
+                      RewriteOptions::OptionScope scope)
+    : n_args_(n_args),
+      scope_(scope) {
+
+      for (int i = 0; i < n_args; i++) {
+        args_[i] = args[i];
+      }
+  }
+
+  virtual ~ScriptLine() {
+    STLDeleteElements(&data_);
+    data_.clear();
+  }
+
+  void AddScriptAndArgIndex(ngx_http_script_compile_t* script,
+                            int script_index) {
+    CHECK(script != NULL);
+    CHECK(script_index <  NGX_PAGESPEED_MAX_ARGS);
+    data_.push_back(new ScriptArgIndex(script, script_index));
+  }
+
+  int n_args() { return n_args_;}
+  StringPiece* args() { return args_;}
+  RewriteOptions::OptionScope scope() { return scope_; }
+  std::vector<ScriptArgIndex*>& data() {
+    return data_;
+  }
+
+ private:
+  StringPiece args_[NGX_PAGESPEED_MAX_ARGS];
+  int n_args_;
+  RewriteOptions::OptionScope scope_;
+  std::vector<ScriptArgIndex*> data_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScriptLine);
+};
+
 class NgxRewriteOptions : public SystemRewriteOptions {
  public:
   // See rewrite_options::Initialize and ::Terminate
@@ -56,12 +124,23 @@ class NgxRewriteOptions : public SystemRewriteOptions {
   // on failure.
   //
   // pool is a memory pool for allocating error strings.
+  // cf is only required when compile_scripts is true
+  // when compile_scripts is true, the rewrite_options will be prepared
+  // for replacing any script $variables encountered in args. when false,
+  // script variables will be substituted using the prepared rewrite options.
   const char* ParseAndSetOptions(
       StringPiece* args, int n_args, ngx_pool_t* pool, MessageHandler* handler,
-      NgxRewriteDriverFactory* driver_factory, OptionScope scope);
+      NgxRewriteDriverFactory* driver_factory, OptionScope scope,
+      ngx_conf_t* cf, bool compile_scripts);
+  bool ExecuteScriptVariables(
+      ngx_http_request_t* r, MessageHandler* handler,
+      NgxRewriteDriverFactory* driver_factory);
+  void CopyScriptLinesTo(NgxRewriteOptions* destination) const;
+  void AppendScriptLinesTo(NgxRewriteOptions* destination) const;
 
   // Make an identical copy of these options and return it.
   virtual NgxRewriteOptions* Clone() const;
+  virtual void Merge(const RewriteOptions& src);
 
   // Returns a suitably down cast version of 'instance' if it is an instance
   // of this class, NULL if not.
@@ -86,6 +165,12 @@ class NgxRewriteOptions : public SystemRewriteOptions {
   const GoogleString& global_admin_path() const {
     return global_admin_path_.value();
   }
+  const std::vector<RefCountedPtr<ScriptLine> >& script_lines() const {
+    return script_lines_;
+  }
+  const bool& clear_inherited_scripts() const {
+    return clear_inherited_scripts_;
+  }
 
  private:
   // Helper methods for ParseAndSetOptions().  Each can:
@@ -141,6 +226,9 @@ class NgxRewriteOptions : public SystemRewriteOptions {
   Option<GoogleString> admin_path_;
   Option<GoogleString> global_admin_path_;
 
+  bool clear_inherited_scripts_;
+  std::vector<RefCountedPtr<ScriptLine> > script_lines_;
+
   // Helper for ParseAndSetOptions.  Returns whether the two directives equal,
   // ignoring case.
   bool IsDirective(StringPiece config_directive, StringPiece compare_directive);

test/nginx_system_test.sh

@@ -98,6 +98,7 @@ function keepalive_test() {
     | grep -v "^\\* Curl_addHandleToPipeline"\
     | grep -v "^\\* - Conn "\
     | grep -v "^\\* Server "\
+    | grep -v "^\\* Hostname was NOT found in DNS cache"\
     | grep -v "^\\*   Trying.*\\.\\.\\.")
 
   # Nothing should remain after that.

test/pagespeed_test.conf.template

@@ -27,6 +27,7 @@ http {
   proxy_cache_path "@@PROXY_CACHE@@"  levels=1:2   keys_zone=htmlcache:60m inactive=90m  max_size=50m;
   proxy_temp_path "@@TMP_PROXY_CACHE@@";
 
+  pagespeed ProcessScriptVariables on;
   pagespeed StatisticsPath /ngx_pagespeed_statistics;
   pagespeed GlobalStatisticsPath /ngx_pagespeed_global_statistics;
   pagespeed ConsolePath /pagespeed_console;
@@ -1148,9 +1149,12 @@ http {
           http://localhost:@@PRIMARY_PORT@@/https_gstatic_dot_com
           https://www.gstatic.com/psa/static;
     }
-
+    
+    # $host implicitly tests script variable support. I'd love to test it more
+    # directly, but so far this is the best I've come up with and duplicating
+    # the test doesn't seem to make sense.
     pagespeed LoadFromFile
-        "http://localhost:@@PRIMARY_PORT@@/mod_pagespeed_test/ipro/instant/"
+        "http://$host:@@PRIMARY_PORT@@/mod_pagespeed_test/ipro/instant/"
         "@@SERVER_ROOT@@/mod_pagespeed_test/ipro/instant/";
 
     pagespeed EnableFilters remove_comments;
@@ -1164,8 +1168,8 @@ http {
       "@@SERVER_ROOT@@/mod_pagespeed_test/load_from_file/file_\1/";
     pagespeed LoadFromFileRule Disallow
       "@@SERVER_ROOT@@/mod_pagespeed_test/load_from_file/file_dir/httponly/";
-    pagespeed LoadFromFileRuleMatch Disallow \.ssp.css$;
-    pagespeed LoadFromFileRuleMatch Allow exception\.ssp\.css$;
+    pagespeed LoadFromFileRuleMatch Disallow \.ssp.css$ps_dollar;
+    pagespeed LoadFromFileRuleMatch Allow exception\.ssp\.css$ps_dollar;
 
     #charset koi8-r;