As agreed on by the SafeBrowsing server team:

1. Back off when we encounter a parse error.

2. Ignore commands we don't understand instead of returning an error.

3. Make redirect URL parsing in an update response more robust to changes in format.
Review URL: http://codereview.chromium.org/7329

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@3425 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 14 insertions, 15 deletions

diff --git a/chrome/browser/safe_browsing/protocol_manager.cc b/chrome/browser/safe_browsing/protocol_manager.cc
index fa32b1a..caf2400 100644
--- a/chrome/browser/safe_browsing/protocol_manager.cc
+++ b/chrome/browser/safe_browsing/protocol_manager.cc
@@ -214,14 +214,12 @@ void SafeBrowsingProtocolManager::OnURLFetchComplete(
       if (!parsed_ok) {
         SB_DLOG(INFO) << "SafeBrowsing request for: " << source->url()
                       << "failed parse.";
+        must_back_off = true;
+        chunk_request_urls_.clear();
       }
 
-      if (request_type_ == CHUNK_REQUEST) {
-        if (parsed_ok) {
-          chunk_request_urls_.pop_front();
-        } else {
-          chunk_request_urls_.clear();
-        }
+      if (request_type_ == CHUNK_REQUEST && parsed_ok) {
+        chunk_request_urls_.pop_front();
       } else if (request_type_ == GETKEY_REQUEST && initial_request_) {
         // This is the first request we've made this session. Now that we have
         // the keys, do the regular update request.
diff --git a/chrome/browser/safe_browsing/protocol_parser.cc b/chrome/browser/safe_browsing/protocol_parser.cc
index c3e7b04..d1391c2 100644
--- a/chrome/browser/safe_browsing/protocol_parser.cc
+++ b/chrome/browser/safe_browsing/protocol_parser.cc
@@ -146,7 +146,7 @@ bool SafeBrowsingProtocolParser::ParseUpdate(
     if (cmd_parts.empty())
       return false;
     const std::string& command = cmd_parts[0];
-    if (cmd_parts.size() != 2 && !(cmd_parts.size() == 3 && command[0] == 'u'))
+    if (cmd_parts.size() != 2 && command[0] != 'u')
       return false;
 
     const int consumed = static_cast<int>(cmd_line.size()) + 1;
@@ -197,13 +197,13 @@ bool SafeBrowsingProtocolParser::ParseUpdate(
         break;
 
       case 'u': {
-        // The line providing a URL redirect to a chunk.
-        std::string redirect_url = cmd_parts[1];
-        if (cmd_parts.size() == 3) {
-          redirect_url += ':' + cmd_parts[2];
-        }
-
+        // The redirect command is of the form: u:<url>,<mac> where <url> can
+        // contain multiple colons, commas or any valid URL characters. We scan
+        // backwards in the string looking for the first ',' we encounter and
+        // assume that everything before that is the URL and everything after
+        // is the MAC (if the MAC was requested).
         std::string mac;
+        std::string redirect_url(cmd_line, 2);  // Skip the initial "u:".
         if (!key.empty()) {
           std::string::size_type mac_pos = redirect_url.rfind(',');
           if (mac_pos == std::string::npos)
@@ -211,6 +211,7 @@ bool SafeBrowsingProtocolParser::ParseUpdate(
           mac = redirect_url.substr(mac_pos + 1);
           redirect_url = redirect_url.substr(0, mac_pos);
         }
+
         ChunkUrl chunk_url;
         chunk_url.url = redirect_url;
         if (!key.empty())
@@ -226,8 +227,8 @@ bool SafeBrowsingProtocolParser::ParseUpdate(
         break;
 
       default:
-        // A command we don't understand.
-        return false;
+        // According to the spec, we ignore commands we don't understand.
+        break;
     }
   }