Process review comments from Josh
diff --git a/net/instaweb/http/async_fetch.cc b/net/instaweb/http/async_fetch.cc
index cfd405f..a99b31a 100644
--- a/net/instaweb/http/async_fetch.cc
+++ b/net/instaweb/http/async_fetch.cc
@@ -286,7 +286,7 @@
bool OutputSanitizingAsyncFetch::SanitizeResponseHeaders() {
if (response_headers() != nullptr &&
- response_headers()->RemoveAllWithPrefix("@")) {
+ response_headers()->RemoveAllWithPrefix(ResponseHeaders::kInternalPrefix)) {
response_headers()->ComputeCaching();
return true;
}
diff --git a/net/instaweb/http/async_fetch_test.cc b/net/instaweb/http/async_fetch_test.cc
index 8ad9050..a4952dd 100644
--- a/net/instaweb/http/async_fetch_test.cc
+++ b/net/instaweb/http/async_fetch_test.cc
@@ -143,12 +143,14 @@
ConstStringStarVector foo_headers;
OutputSanitizingAsyncFetch fetch(&string_fetch_);
fetch.response_headers()->set_status_code(HttpStatus::kOK);
- fetch.response_headers()->Add("@foo", "bar");
- EXPECT_TRUE(fetch.response_headers()->Lookup("@foo", &foo_headers));
+ GoogleString internal_header = StrCat(ResponseHeaders::kInternalPrefix,
+ "bar");
+ fetch.response_headers()->Add(internal_header, "bar");
+ EXPECT_TRUE(fetch.response_headers()->Lookup(internal_header, &foo_headers));
EXPECT_EQ(1, foo_headers.size());
foo_headers.clear();
fetch.HeadersComplete();
- EXPECT_FALSE(string_fetch_.response_headers()->Lookup("@foo", &foo_headers));
+ EXPECT_FALSE(string_fetch_.response_headers()->Lookup(internal_header, &foo_headers));
EXPECT_EQ(0, foo_headers.size());
}
diff --git a/net/instaweb/http/public/async_fetch.h b/net/instaweb/http/public/async_fetch.h
index bba7c74..86da28b 100644
--- a/net/instaweb/http/public/async_fetch.h
+++ b/net/instaweb/http/public/async_fetch.h
@@ -316,10 +316,9 @@
DISALLOW_COPY_AND_ASSIGN(SharedAsyncFetch);
};
-// Can be used to sanitize headers and data before forwarding them on to the
-// base fetch. Used to ensure that internal headers, starting with '@', will
-// never be visible outside of PSOL. Note that httpd will send an error page
-// when a response header containing '@' attempts to hit the wire.
+// Used to sanitize headers and data before forwarding them on to the base
+// fetch. Used to ensure that internal headers, starting with
+// ResponseHeaders::kInternalPrefix, will never be visible outside of PSOL.
class OutputSanitizingAsyncFetch : public SharedAsyncFetch {
public:
explicit OutputSanitizingAsyncFetch(AsyncFetch* base_fetch);
@@ -330,8 +329,8 @@
virtual void HandleDone(bool success);
private:
- // Removes any headers starting with '@' from the response.
- // returns true if any changes have been made.
+ // Removes any headers starting with ResponseHeaders::kInternalPrefix from
+ // the response. returns true if any changes have been made.
bool SanitizeResponseHeaders();
DISALLOW_COPY_AND_ASSIGN(OutputSanitizingAsyncFetch);
};
diff --git a/pagespeed/apache/header_util.cc b/pagespeed/apache/header_util.cc
index 32a3778..8ab5496 100644
--- a/pagespeed/apache/header_util.cc
+++ b/pagespeed/apache/header_util.cc
@@ -115,7 +115,7 @@
for (int i = 0, n = response_headers.NumAttributes(); i < n; ++i) {
const GoogleString& name = response_headers.Name(i);
const GoogleString& value = response_headers.Value(i);
- if (strings::StartsWith(name, "@")) {
+ if (strings::StartsWith(name, ResponseHeaders::kInternalPrefix)) {
continue;
}
if (StringCaseEqual(name, HttpAttributes::kContentType)) {
diff --git a/pagespeed/kernel/http/response_headers.cc b/pagespeed/kernel/http/response_headers.cc
index b893157..4c609cf 100644
--- a/pagespeed/kernel/http/response_headers.cc
+++ b/pagespeed/kernel/http/response_headers.cc
@@ -54,6 +54,13 @@
// we'll set it back to 3:00:00 exactly in FixDateHeaders.
const int64 kMaxAllowedDateDriftMs = 3L * net_instaweb::Timer::kMinuteMs;
+// This is illegal from a protocol perspective, see
+// https://tools.ietf.org/html/rfc2616#page-31
+// Basic Rules for 'token' excludes separators, including "@".
+// That's why we can use it as an in-memory sentinal as long as it is
+// never serialized.
+const char ResponseHeaders::kInternalPrefix[] = "@";
+
ResponseHeaders::ResponseHeaders(const ResponseHeaders& other) {
CopyFrom(other);
}
diff --git a/pagespeed/kernel/http/response_headers.h b/pagespeed/kernel/http/response_headers.h
index 3f50e00..1440586 100644
--- a/pagespeed/kernel/http/response_headers.h
+++ b/pagespeed/kernel/http/response_headers.h
@@ -39,6 +39,8 @@
enum VaryOption { kRespectVaryOnResources, kIgnoreVaryOnResources };
enum ValidatorOption { kHasValidator, kNoValidator };
+ static const char kInternalPrefix[];
+
// This constructor with options explicitly set should be used by all callers.
explicit ResponseHeaders(const HttpOptions& options) { Init(options); }
