Bypass DRP if a redirect cycle is detected

components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc

Index: components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc
diff --git a/components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc b/components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc
index a8dca30a4f656e1d5a40a5e3703b85387eb506f2..50335495e1db25163bf65ac4960a0c670e660d2b 100644
--- a/components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc
+++ b/components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc
@@ -20,6 +20,7 @@
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
+#include "net/url_request/url_request.h"
 
 using base::StringPiece;
 using base::TimeDelta;
@@ -106,6 +107,26 @@ bool IsPreviewType(const net::HttpResponseHeaders& headers,
   return IsPreviewTypeInHeaderValue(header_value, transform_type);
 }
 
+// Returns true if there is a cycle in |url_chain|.
+bool HasURLRedirectCycle(const std::vector<GURL>& url_chain) {
+  if (url_chain.size() <= 1)
+    return false;
+
+  // If the last entry occurs more than once, then very likely there is a
+  // redirect cycle.
+  GURL last_entry = url_chain.back();
+  size_t last_entry_number_occurences = 0;
+
+  for (const auto& url : url_chain) {
+    if (url == last_entry) {
+      last_entry_number_occurences++;
+      if (last_entry_number_occurences >= 2)

[RyanSturm, 2017/03/28 18:03:19]

This looks confusing with the >=2 condition. Since you are O(n) of the chain
anyway, maybe you should just do something like:

vector<GURL> everything_but_the_back(url_chain.front(), url_chain.back());
for (const auto& url : everything_but_the_back) {
  if (url == last_entry)
    return true;
}
return false;


The most efficient option might be (it doesn't have to walk the whole chain or
copy the chain):

auto last_entry  = std::prev(url_chain.end());
for (auto url : url_chain) {
  if (url == last_entry)
    continue;
  if (*url == *last_entry)
    return true;
}
return false;

You could also re-write the above to use a for loop with the termination
condition of iter != std::prev(vector.end())


My syntax might be a little off.

[tbansal1, 2017/03/28 19:59:48]

Done.

+        return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace
 
 namespace data_reduction_proxy {
@@ -283,10 +304,22 @@ bool HasDataReductionProxyViaHeader(const net::HttpResponseHeaders* headers,
 }
 
 DataReductionProxyBypassType GetDataReductionProxyBypassType(
-    const net::HttpResponseHeaders* headers,
+    const std::vector<GURL>& url_chain,
+    const net::HttpResponseHeaders& headers,
     DataReductionProxyInfo* data_reduction_proxy_info) {
   DCHECK(data_reduction_proxy_info);
-  if (ParseHeadersForBypassInfo(headers, data_reduction_proxy_info)) {
+
+  bool has_via_header = HasDataReductionProxyViaHeader(&headers, nullptr);

[RyanSturm, 2017/03/28 18:03:19]

Any chance you want to change HasDataReductionProxyViaHeader to accept a const
ref to headers?

[tbansal1, 2017/03/28 19:59:48]

I was planning to do it in a separate CL, but I have included those changes in
this CL itself. PTAL. Thanks.

+
+  if (has_via_header && HasURLRedirectCycle(url_chain)) {
+    data_reduction_proxy_info->bypass_all = true;
+    data_reduction_proxy_info->mark_proxies_as_bad = false;
+    data_reduction_proxy_info->bypass_duration = base::TimeDelta();
+    data_reduction_proxy_info->bypass_action = BYPASS_ACTION_TYPE_BLOCK_ONCE;
+    return BYPASS_EVENT_TYPE_URL_REDIRECT_CYCLE;
+  }
+
+  if (ParseHeadersForBypassInfo(&headers, data_reduction_proxy_info)) {
     // A chrome-proxy response header is only present in a 502. For proper
     // reporting, this check must come before the 5xx checks below.
     if (!data_reduction_proxy_info->mark_proxies_as_bad)
@@ -307,20 +340,19 @@ DataReductionProxyBypassType GetDataReductionProxyBypassType(
   data_reduction_proxy_info->bypass_duration = GetDefaultBypassDuration();
 
   // Fall back if a 500, 502 or 503 is returned.
-  if (headers->response_code() == net::HTTP_INTERNAL_SERVER_ERROR)
+  if (headers.response_code() == net::HTTP_INTERNAL_SERVER_ERROR)
     return BYPASS_EVENT_TYPE_STATUS_500_HTTP_INTERNAL_SERVER_ERROR;
-  if (headers->response_code() == net::HTTP_BAD_GATEWAY)
+  if (headers.response_code() == net::HTTP_BAD_GATEWAY)
     return BYPASS_EVENT_TYPE_STATUS_502_HTTP_BAD_GATEWAY;
-  if (headers->response_code() == net::HTTP_SERVICE_UNAVAILABLE)
+  if (headers.response_code() == net::HTTP_SERVICE_UNAVAILABLE)
     return BYPASS_EVENT_TYPE_STATUS_503_HTTP_SERVICE_UNAVAILABLE;
   // TODO(kundaji): Bypass if Proxy-Authenticate header value cannot be
   // interpreted by data reduction proxy.
-  if (headers->response_code() == net::HTTP_PROXY_AUTHENTICATION_REQUIRED &&
-      !headers->HasHeader("Proxy-Authenticate")) {
+  if (headers.response_code() == net::HTTP_PROXY_AUTHENTICATION_REQUIRED &&
+      !headers.HasHeader("Proxy-Authenticate")) {
     return BYPASS_EVENT_TYPE_MALFORMED_407;
   }
-  if (!HasDataReductionProxyViaHeader(headers, NULL) &&
-      (headers->response_code() != net::HTTP_NOT_MODIFIED)) {
+  if (!has_via_header && (headers.response_code() != net::HTTP_NOT_MODIFIED)) {
     // A Via header might not be present in a 304. Since the goal of a 304
     // response is to minimize information transfer, a sender in general
     // should not generate representation metadata other than Cache-Control,
@@ -328,8 +360,8 @@ DataReductionProxyBypassType GetDataReductionProxyBypassType(
 
     // The proxy Via header might also not be present in a 4xx response.
     // Separate this case from other responses that are missing the header.
-    if (headers->response_code() >= net::HTTP_BAD_REQUEST &&
-        headers->response_code() < net::HTTP_INTERNAL_SERVER_ERROR) {
+    if (headers.response_code() >= net::HTTP_BAD_REQUEST &&
+        headers.response_code() < net::HTTP_INTERNAL_SERVER_ERROR) {
       // At this point, any 4xx response that is missing the via header
       // indicates an issue that is scoped to only the current request, so only
       // bypass the data reduction proxy for the current request.