artox/incubator-pagespeed-ngx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Don't run add-head or any of the filters that depend on it in ajax request, which

Browse Source 
we can usually detect via request-headers x-requested-with:xml-http-request.

Preserve URLs for ajax requests as well.

Re-establish 'disallow */wp-admin/*' in default set, as a back-stop against the
wp-admin being visited from a browser that does not send that header.

Did a couple of minor cleanups found in the vicinity of the changes reuqired in
rewrite_options.cc: don't statically construct a std::vector (which we totally
did not need in the first place).  Use C++11 loops over static arrays.

Fixes:
  https://github.com/pagespeed/mod_pagespeed/issues/1405
  https://github.com/pagespeed/mod_pagespeed/issues/891
  https://github.com/pagespeed/mod_pagespeed/issues/419
  http://serverfault.com/questions/408057/mod-pagespeed-with-zend-framework-gives-404

Associated ngx_pagespeed pull: https://github.com/pagespeed/ngx_pagespeed/pull/1320
This commit is contained in:
Joshua Marantz
2016-11-14 09:10:07 -05:00
parent e070e55aed
commit c6f01f2d42
9 changed files with 185 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files
install/debug.conf.template
+21
View File
@@ -1849,6 +1849,27 @@ ModPagespeedMessagesDomains Allow localhost
ModPagespeedExperimentSpec "id=1;percent=100;matches_device_type=mobile;enable=recompress_images"
</VirtualHost>
<VirtualHost localhost:@@APACHE_SECONDARY_PORT@@>
ServerName experiment.ajax.example.com
DocumentRoot "@@APACHE_DOC_ROOT@@/"
ModPagespeedFileCachePath "@@MOD_PAGESPEED_CACHE@@"
ModPagespeedRewriteLevel CoreFilters
ModPagespeedDisableFilters add_instrumentation
ModPagespeedRunExperiment on
ModPagespeedUseAnalyticsJs false
ModPagespeedJsInlineMaxBytes 1
ModPagespeedExperimentSpec "id=1;percent=100;level=CoreFilters;enable=collapse_whitespace;options=JsInlineMaxBytes=1"
</VirtualHost>
<VirtualHost localhost:@@APACHE_SECONDARY_PORT@@>
ServerName ajax.example.com
DocumentRoot "@@APACHE_DOC_ROOT@@/"
ModPagespeedFileCachePath "@@MOD_PAGESPEED_CACHE@@"
ModPagespeedJsInlineMaxBytes 1
ModPagespeedRewriteLevel CoreFilters
ModPagespeedDisableFilters add_instrumentation
</VirtualHost>
# Support for embedded configurations, where image flags in the
# VirtualHost serving HTML is not the same as the one serving resources,
# and thus we must embed the image flags in the rewritten image URLs.
install/mod_pagespeed_test/ajax/ajax.html
+5
View File
@@ -0,0 +1,5 @@
<link rel="stylesheet" href="/mod_pagespeed_example/styles/big.css" />
<script type="text/javascript" src="/mod_pagespeed_example/rewrite_javascript.js">
</script>
<img src="/mod_pagespeed_example/images/BikeCrashIcn.png" />
Two spaces.
net/instaweb/rewriter/public/rewrite_options.h
+7
View File
@@ -1388,6 +1388,13 @@ class RewriteOptions {
// Disables all filters that depend on executing custom javascript.
void DisableFiltersRequiringScriptExecution();
// Disables all filters that cannot be run in an Ajax call.
void DisableFiltersThatCantRunInAjax();
// Determines whether any filter is enabled that requires a 'head'
// element to work.
bool RequiresAddHead() const;
// Returns true if any filter benefits from per-origin property cache
// information.
bool UsePerOriginPropertyCachePage() const;
net/instaweb/rewriter/rewrite_driver.cc
+2 -11
View File
@@ -29,7 +29,6 @@
#include <vector>
#include "base/logging.h"
#include "net/instaweb/config/rewrite_options_manager.h"
#include "net/instaweb/http/public/async_fetch.h"
#include "net/instaweb/http/public/cache_url_async_fetcher.h"
#include "net/instaweb/http/public/http_cache.h"
@@ -127,6 +126,7 @@
#include "pagespeed/kernel/base/function.h"
#include "pagespeed/kernel/base/hasher.h"
#include "pagespeed/kernel/base/message_handler.h"
#include "pagespeed/kernel/base/proto_util.h"
#include "pagespeed/kernel/base/request_trace.h"
#include "pagespeed/kernel/base/scoped_ptr.h"
#include "pagespeed/kernel/base/sha1_signature.h"
@@ -1039,16 +1039,7 @@ void RewriteDriver::AddPreRenderFilters() {
AddOwnedPostRenderFilter(resp_filter2);
}
if (rewrite_options->Enabled(RewriteOptions::kAddBaseTag) ||
rewrite_options->Enabled(RewriteOptions::kAddHead) ||
rewrite_options->Enabled(RewriteOptions::kAddInstrumentation) ||
rewrite_options->Enabled(RewriteOptions::kCombineHeads) ||
rewrite_options->Enabled(RewriteOptions::kDeterministicJs) ||
rewrite_options->Enabled(RewriteOptions::kHandleNoscriptRedirect) ||
rewrite_options->Enabled(RewriteOptions::kMakeGoogleAnalyticsAsync) ||
rewrite_options->Enabled(RewriteOptions::kMobilize) ||
rewrite_options->Enabled(RewriteOptions::kMoveCssAboveScripts) ||
rewrite_options->Enabled(RewriteOptions::kMoveCssToHead)) {
if (rewrite_options->RequiresAddHead()) {
// Adds a filter that adds a 'head' section to html documents if
// none found prior to the body.
AddOwnedEarlyPreRenderFilter(new AddHeadFilter(
net/instaweb/rewriter/rewrite_options.cc
+62 -20
View File
@@ -692,6 +692,20 @@ const RewriteOptions::Filter kRequiresScriptExecutionFilterSet[] = {
// in a noscript block and the page will still load / function normally.
};
// List of filters that require a 'head' element to exist.
const RewriteOptions::Filter kAddHeadFilters[] = {
RewriteOptions::kAddBaseTag,
RewriteOptions::kAddHead,
RewriteOptions::kAddInstrumentation,
RewriteOptions::kCombineHeads,
RewriteOptions::kDeterministicJs,
RewriteOptions::kHandleNoscriptRedirect,
RewriteOptions::kMakeGoogleAnalyticsAsync,
RewriteOptions::kMobilize,
RewriteOptions::kMoveCssAboveScripts,
RewriteOptions::kMoveCssToHead,
};
// List of filters which are essential for mobilizing webpages, i.e., for
// making webpages designed for desktop computers look good on mobile devices.
//
@@ -938,11 +952,6 @@ const RenamedOptionMap kRenamedOptionNameData[] = {
"WebpRecompressionQualityForSmallScreens"}
};
std::vector<RenamedOptionMap> kRenamedOptionNameList(
kRenamedOptionNameData,
kRenamedOptionNameData + arraysize(kRenamedOptionNameData)
);
// Will be initialized to a sorted list of headers not allowed in
// AddResourceHeader.
const StringPieceVector* fixed_resource_headers = NULL;
@@ -1131,6 +1140,7 @@ RewriteOptions::RewriteOptions(ThreadSystem* thread_system)
arraysize(kJsPreserveUrlDisabledFilters));
CheckFilterSetOrdering(kCssPreserveUrlDisabledFilters,
arraysize(kCssPreserveUrlDisabledFilters));
CheckFilterSetOrdering(kAddHeadFilters, arraysize(kAddHeadFilters));
// Ensure that all filters have unique IDs.
StringSet id_set;
@@ -2556,6 +2566,9 @@ void RewriteOptions::DisallowTroublesomeResources() {
// ckeditor.js, ckeditor_basic.js, ckeditor_basic_source.js, ...
Disallow("*ckeditor*");
// https://github.com/pagespeed/mod_pagespeed/issues/1405
Disallow("*/wp-admin/*");
// http://github.com/pagespeed/mod_pagespeed/issues/207
// jquery-ui-1.8.2.custom.min.js, jquery-1.4.4.min.js, jquery.fancybox-...
//
@@ -3236,14 +3249,13 @@ RewriteOptions::OptionSettingResult RewriteOptions::ParseAndSetOptionFromName3(
StringPiece RewriteOptions::GetEffectiveOptionName(StringPiece name) {
StringPiece effective_name = name;
std::vector<RenamedOptionMap>::iterator id =
std::lower_bound(kRenamedOptionNameList.begin(),
kRenamedOptionNameList.end(),
name,
RenamedOptionMap::LessThan);
if (id != kRenamedOptionNameList.end() &&
StringCaseEqual(name, id->deprecated_option_name)) {
effective_name = id->new_option_name;
const RenamedOptionMap* end = kRenamedOptionNameData +
arraysize(kRenamedOptionNameData);
const RenamedOptionMap* entry =
std::lower_bound(kRenamedOptionNameData, end, name,
&RenamedOptionMap::LessThan);
if ((entry != end) && StringCaseEqual(name, entry->deprecated_option_name)) {
effective_name = entry->new_option_name;
}
return effective_name;
}
@@ -3546,21 +3558,51 @@ int64 RewriteOptions::MaxImageInlineMaxBytes() const {
void RewriteOptions::GetEnabledFiltersRequiringScriptExecution(
RewriteOptions::FilterVector* filters) const {
for (int i = 0, n = arraysize(kRequiresScriptExecutionFilterSet); i < n;
++i) {
if (Enabled(kRequiresScriptExecutionFilterSet[i])) {
filters->push_back(kRequiresScriptExecutionFilterSet[i]);
for (RewriteOptions::Filter filter : kRequiresScriptExecutionFilterSet) {
if (Enabled(filter)) {
filters->push_back(filter);
}
}
}
void RewriteOptions::DisableFiltersRequiringScriptExecution() {
for (int i = 0, n = arraysize(kRequiresScriptExecutionFilterSet); i < n;
++i) {
DisableFilter(kRequiresScriptExecutionFilterSet[i]);
for (RewriteOptions::Filter filter : kRequiresScriptExecutionFilterSet) {
DisableFilter(filter);
}
}
void RewriteOptions::DisableFiltersThatCantRunInAjax() {
DisableFiltersRequiringScriptExecution();
for (RewriteOptions::Filter filter : kAddHeadFilters) {
DisableFilter(filter);
}
// Note that kPrioritizeCriticalCss does not require script execution for
// correct behavior because it adds its own <noscript> block. This is better
// than a noscript-redirect, which is what we need to do for filters that
// require JS to run for the page to render correctly. But we still need
// to disable it for ajax requests because it's fundamentally a whole-page
// optimization.
DisableFilter(RewriteOptions::kPrioritizeCriticalCss);
// Don't modify URLs for ajax requests, in case they are relative.
//
// TODO(jmarantz): would be better to enable rewriting of absolute URLs but
// disable rewriting of relative URLs.
set_css_preserve_urls(true);
set_image_preserve_urls(true);
set_js_preserve_urls(true);
}
bool RewriteOptions::RequiresAddHead() const {
for (RewriteOptions::Filter filter : kAddHeadFilters) {
if (Enabled(filter)) {
return true;
}
}
return false;
}
bool RewriteOptions::UsePerOriginPropertyCachePage() const {
return Enabled(kMobilize);
}
net/instaweb/rewriter/rewrite_query.cc
+19 -8
View File
@@ -207,23 +207,34 @@ RewriteQuery::Status RewriteQuery::Scan(
? request_headers->GetAllCookies()
: no_cookies);
// For XmlHttpRequests, disable filters that insert js. Otherwise, there
// will be two copies of the same scripts in the html dom -- one from main
// html page and another from html content fetched from ajax --- which will
// generally confuse the heck out of it. The code for this is a little
// special since unlike a PageSpeedFoo= header we should not take it as an
// invitation to turn stuff on.
// For XmlHttpRequests, disable filters that can't run in Ajax.
//
// For example, for filters that insert scripts, there will be two
// copies of the same scripts in the html dom -- one from main html
// page and another from html content fetched from ajax --- which
// will generally confuse the heck out of it. The code for this is a
// little special since unlike a PageSpeedFoo= header we should not
// take it as an invitation to turn stuff on.
//
// Another commmon case is filters that require a <head> element -- we
// don't want to add a <head> in the ajax response, so we can't run
// any filters that might depend on that.
//
// TODO(sriharis): Set a flag in RewriteOptions indicating that we are
// working with Ajax and thus should not assume the base URL is correct.
// Note that there is no guarantee that the header will be set on an ajax
// request and so the option will not be set for all ajax requests.
//
// TODO(jmarantz): Ajax status should probably be in RequestProperties
// rather than RewriteOptions so we don't have to create a new driver
// on an ajax request, but can instead reference properties. However
// that change may be fairly significant.
if (request_headers != NULL && request_headers->IsXmlHttpRequest()) {
if (options_.get() == NULL) {
options_.reset(factory->NewRewriteOptionsForQuery());
}
options_->DisableFiltersRequiringScriptExecution();
options_->DisableFilter(RewriteOptions::kPrioritizeCriticalCss);
options_->DisableFiltersThatCantRunInAjax();
status = kSuccess;
}
// See if anything looks even remotely like one of our options before doing
net/instaweb/rewriter/rewrite_query_test.cc
+29 -13
View File
@@ -82,6 +82,20 @@ class RewriteQueryTest : public RewriteTestBase {
GoogleString* out_query,
GoogleString* out_req_string,
GoogleString* out_resp_string) {
Parse(request_url, in_query, in_cookies, request_headers,
response_headers, out_query, out_req_string, out_resp_string);
return rewrite_query_.options();
}
RewriteQuery::Status Parse(const StringPiece request_url,
const StringPiece& in_query,
const StringPiece& in_cookies,
RequestHeaders* request_headers,
ResponseHeaders* response_headers,
GoogleString* out_query,
GoogleString* out_req_string,
GoogleString* out_resp_string) {
GoogleUrl url(StrCat(request_url, "?", in_query));
if (!in_cookies.empty()) {
// For fidelity we can put multiple cookies per header line. We limit
@@ -107,11 +121,12 @@ class RewriteQueryTest : public RewriteTestBase {
}
}
RequestContextPtr null_request_context;
rewrite_query_.Scan(allow_related_options_,
allow_options_to_be_set_by_cookies_,
request_option_override_, null_request_context,
factory(), server_context(), &url, request_headers,
response_headers, &handler_);
RewriteQuery::Status status = rewrite_query_.Scan(
allow_related_options_,
allow_options_to_be_set_by_cookies_,
request_option_override_, null_request_context,
factory(), server_context(), &url, request_headers,
response_headers, &handler_);
if (out_query != NULL) {
out_query->assign(url.Query().data(), url.Query().size());
}
@@ -121,8 +136,7 @@ class RewriteQueryTest : public RewriteTestBase {
if (out_resp_string != NULL && response_headers != NULL) {
out_resp_string->assign(response_headers->ToString());
}
return rewrite_query_.options();
return status;
}
// Starts with image_, applies the specified image-options, and any
@@ -1072,7 +1086,7 @@ TEST_F(RewriteQueryTest, CacheControlNoTransform) {
EXPECT_TRUE(request_headers.Lookup1(HttpAttributes::kCacheControl) != NULL);
}
TEST_F(RewriteQueryTest, DisableScriptsWithXHR) {
TEST_F(RewriteQueryTest, DisableFiltersWithXHR) {
RequestHeaders request_headers;
request_headers.Replace(HttpAttributes::kXRequestedWith,
HttpAttributes::kXmlHttpRequest);
@@ -1080,17 +1094,19 @@ TEST_F(RewriteQueryTest, DisableScriptsWithXHR) {
ResponseHeaders response_headers;
GoogleString in_query, out_query, out_req_string, out_resp_string;
scoped_ptr<RewriteOptions> options(
ParseAndScan(kHtmlUrl, in_query, StringPiece(), &request_headers,
&response_headers, &out_query, &out_req_string,
&out_resp_string)
->Clone());
EXPECT_EQ(RewriteQuery::kSuccess, Parse(
kHtmlUrl, in_query, StringPiece(), &request_headers,
&response_headers, &out_query, &out_req_string, &out_resp_string));
scoped_ptr<RewriteOptions> options(rewrite_query_.options()->Clone());
// Convert disabled -> forbidden for easier testing.
options->set_forbid_all_disabled_filters(true);
// defer_js, mobilize generaly require JS.
EXPECT_TRUE(options->Forbidden(RewriteOptions::kDeferJavascript));
EXPECT_TRUE(options->Forbidden(RewriteOptions::kMobilize));
EXPECT_TRUE(options->Forbidden(RewriteOptions::kMoveCssToHead));
EXPECT_TRUE(options->Forbidden(RewriteOptions::kAddInstrumentation));
// rewrite_css doesn't, and shouldn't be defaulted on, either.
EXPECT_FALSE(options->Forbidden(RewriteOptions::kRewriteCss));
pagespeed/system/system_test.sh
+1
View File
@@ -85,6 +85,7 @@ if [ $statistics_enabled = "1" ]; then
fi
run_test prioritize_critical_css
if [ "$SECONDARY_HOSTNAME" != "" ]; then
run_test ajax_overrides_experiments
run_test query_params_dont_enable_core_filters
run_test optimize_for_bandwidth
run_test shm_cache
pagespeed/system/system_tests/ajax_overrides_experiments.sh
+39
View File
@@ -0,0 +1,39 @@
start_test Ajax overrides experiments
# Normally, we expect this <head>less HTML file to have a <head> added
# by the experimental infrastructure, and for the double-space between
# "two spaces" to be collapsed to one, due to the experiment.
URL="http://experiment.ajax.example.com/mod_pagespeed_test/ajax/ajax.html"
http_proxy=$SECONDARY_HOSTNAME fetch_until -save "$URL" \
'fgrep -c .pagespeed.' 3
OUT=$(cat "$FETCH_FILE")
check_from "$OUT" fgrep -q '<head'
check_from "$OUT" fgrep -q 'Two spaces.'
# However, we must not add a '<head' in an Ajax request, rewrite any URLs, or
# execute the collapse_whitespace experiment.
start_test Experiments not injected on ajax.html with an Ajax header
AJAX="--header=X-Requested-With:XmlHttpRequest"
http_proxy=$SECONDARY_HOSTNAME fetch_until -save -expect_time_out "$URL" \
'fgrep -c \.pagespeed\.' 3 "$AJAX"
OUT=$(cat "$FETCH_FILE")
check_not_from "$OUT" fgrep -q '<head'
check_not_from "$OUT" fgrep -q '.pagespeed.'
check_from "$OUT" fgrep -q 'Two spaces.'
start_test Ajax disables any filters that add head.
# While we are in here, check also that Ajax requests don't get a 'head',
# even if we are not in an experiment.
URL="http://ajax.example.com/mod_pagespeed_test/ajax/ajax.html"
http_proxy=$SECONDARY_HOSTNAME fetch_until -save "$URL" \
'fgrep -c .pagespeed.' 3
OUT=$(cat "$FETCH_FILE")
check_from "$OUT" fgrep -q '<head'
# However, we must not add a '<head' in an Ajax request or rewrite any URLs.
http_proxy=$SECONDARY_HOSTNAME fetch_until -save -expect_time_out "$URL" \
'fgrep -c \.pagespeed\.' 3 "$AJAX"
OUT=$(cat "$FETCH_FILE")
check_not_from "$OUT" fgrep -q '<head'
check_not_from "$OUT" fgrep -q '.pagespeed.'