Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. r=francois.

MozReview-Commit-ID: 1umDhxY5eKl

--HG--
extra : rebase_source : 89dfc9852dc9010afb99d6365b95780c4cce767b

toolkit/components/url-classifier/content/listmanager.js

@@ -411,11 +411,10 @@ PROT_ListManager.prototype.makeUpdateRequest_ = function(updateUrl, tableData) {
 
     let urlUtils = Cc["@mozilla.org/url-classifier/utils;1"]
                      .getService(Ci.nsIUrlClassifierUtils);
-    let requestPayload =  urlUtils.makeUpdateRequestV4(tableArray,
-                                                       stateArray,
-                                                       tableArray.length);
-    // Use a base64-encoded request.
-    streamerMap.requestPayload = btoa(requestPayload);
+
+    streamerMap.requestPayload = urlUtils.makeUpdateRequestV4(tableArray,
+                                                              stateArray,
+                                                              tableArray.length);
     streamerMap.isPostRequest = false;
   } else {
     // Build the request. For each table already in the database, include the

toolkit/components/url-classifier/nsIUrlClassifierUtils.idl

@@ -57,7 +57,7 @@ interface nsIUrlClassifierUtils : nsISupports
    * @param aState An array of states (encoded in base64 format) for each list.
    * @param aCount The array length of aList and aState.
    *
-   * @returns A string to store request. Not null-terminated.
+   * @returns A base64url encoded string.
    */
   ACString makeUpdateRequestV4([array, size_is(aCount)] in string aListNames,
                                [array, size_is(aCount)] in string aStatesBase64,

toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp

@@ -35,6 +35,8 @@ const uint32_t MAX_FILE_SIZE = (32 * 1024 * 1024);
 
 // MOZ_LOG=UrlClassifierStreamUpdater:5
 static mozilla::LazyLogModule gUrlClassifierStreamUpdaterLog("UrlClassifierStreamUpdater");
+
+#define LOG_ENABLED() MOZ_LOG_TEST(gUrlClassifierStreamUpdaterLog, mozilla::LogLevel::Debug)
 #define LOG(args) TrimAndLog args
 
 // Calls nsIURLFormatter::TrimSensitiveURLs to remove sensitive
@@ -678,7 +680,13 @@ nsUrlClassifierStreamUpdater::OnStartRequest(nsIRequest *request,
     }
 
     mDownloadError = true;
+
     status = NS_ERROR_ABORT;
+    if (LOG_ENABLED()) {
+      // We can't return an error just yet because we need to read the body
+      // in order to see the error message.
+      status = NS_OK;
+    }
   } else if (NS_SUCCEEDED(status)) {
     MOZ_ASSERT(mDownloadErrorCallback);
     mBeganStream = true;
@@ -699,8 +707,9 @@ nsUrlClassifierStreamUpdater::OnDataAvailable(nsIRequest *request,
                                               uint64_t aSourceOffset,
                                               uint32_t aLength)
 {
-  if (!mDBService)
+  if (!mDBService && !mDownloadError) {
     return NS_ERROR_NOT_INITIALIZED;
+  }
 
   LOG(("OnDataAvailable (%d bytes)", aLength));
 
@@ -716,6 +725,11 @@ nsUrlClassifierStreamUpdater::OnDataAvailable(nsIRequest *request,
   rv = NS_ConsumeStream(aIStream, aLength, chunk);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  if (mDownloadError) {
+    mDownloadErrorMessage.Append(chunk);
+    return NS_OK;
+  }
+
   //LOG(("Chunk (%d): %s\n\n", chunk.Length(), chunk.get()));
   rv = mDBService->UpdateStream(chunk);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -727,6 +741,11 @@ NS_IMETHODIMP
 nsUrlClassifierStreamUpdater::OnStopRequest(nsIRequest *request, nsISupports* context,
                                             nsresult aStatus)
 {
+  if (mDownloadError) {
+    LOG(("Download error message: %s", mDownloadErrorMessage.get()));
+    return NS_ERROR_ABORT; // The value doesn't matter.
+  }
+
   if (!mDBService)
     return NS_ERROR_NOT_INITIALIZED;
 

toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h

@@ -98,6 +98,8 @@ private:
   nsCOMPtr<nsIUrlClassifierCallback> mSuccessCallback;
   nsCOMPtr<nsIUrlClassifierCallback> mUpdateErrorCallback;
   nsCOMPtr<nsIUrlClassifierCallback> mDownloadErrorCallback;
+
+  nsCString mDownloadErrorMessage;
 };
 
 #endif // nsUrlClassifierStreamUpdater_h_

toolkit/components/url-classifier/nsUrlClassifierUtils.cpp

@@ -118,7 +118,7 @@ InitListUpdateRequest(ThreatType aThreatType,
     nsCString stateBinary;
     nsresult rv = Base64Decode(nsCString(aStateBase64), stateBinary);
     if (NS_SUCCEEDED(rv)) {
-      aListUpdateRequest->set_state(stateBinary.get());
+      aListUpdateRequest->set_state(stateBinary.get(), stateBinary.Length());
     }
   }
 }
@@ -297,7 +297,11 @@ nsUrlClassifierUtils::MakeUpdateRequestV4(const char** aListNames,
   r.SerializeToString(&s);
 
   nsCString out;
-  out.Assign(s.c_str(), s.size());
+  nsresult rv = Base64URLEncode(s.size(),
+                                (const uint8_t*)s.c_str(),
+                                Base64URLEncodePaddingPolicy::Include,
+                                out);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   aRequest = out;
 

toolkit/components/url-classifier/tests/unit/test_listmanager.js

@@ -72,6 +72,9 @@ const NEW_CLIENT_STATE = 'sta\0te';
 
 prefBranch.setBoolPref("browser.safebrowsing.debug", true);
 
+// The "\xFF\xFF" is to generate a base64 string with "/".
+prefBranch.setCharPref("browser.safebrowsing.id", "Firefox\xFF\xFF");
+
 // Register tables.
 TEST_TABLE_DATA_LIST.forEach(function(t) {
   gListManager.registerTable(t.tableName,
@@ -153,7 +156,7 @@ const SERVER_INVOLVED_TEST_CASE_LIST = [
     let requestV4 = gUrlUtils.makeUpdateRequestV4([TEST_TABLE_DATA_V4.tableName],
                                                   [""],
                                                   1);
-    gExpectedQueryV4 = "&$req=" + btoa(requestV4);
+    gExpectedQueryV4 = "&$req=" + requestV4;
 
     forceTableUpdate();
   },
@@ -173,7 +176,7 @@ add_test(function test_partialUpdateV4() {
   let requestV4 = gUrlUtils.makeUpdateRequestV4([TEST_TABLE_DATA_V4.tableName],
                                                 [btoa(NEW_CLIENT_STATE)],
                                                 1);
-  gExpectedQueryV4 = "&$req=" + btoa(requestV4);
+  gExpectedQueryV4 = "&$req=" + requestV4;
 
   forceTableUpdate();
 });
@@ -241,6 +244,8 @@ function run_test() {
 
     // V4 append the base64 encoded request to the query string.
     equal(request.queryString, gExpectedQueryV4);
+    equal(request.queryString.indexOf('+'), -1);
+    equal(request.queryString.indexOf('/'), -1);
 
     // Respond a V2 compatible content for now. In the future we can
     // send a meaningful response to test Bug 1284178 to see if the