Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. r=francois. draft
authorHenry <hchang@mozilla.com>
Tue, 27 Sep 2016 11:48:11 -0700
changeset 418700 89dfc9852dc9010afb99d6365b95780c4cce767b
parent 417649 c55bcb7c777ea09431b4d16903ed079ae5632648
child 532420 d937b676c18d67edb967ad6145c6a9453f17e01c
push id30759
push userhchang@mozilla.com
push dateThu, 29 Sep 2016 00:28:58 +0000
reviewersfrancois
bugs1305567
milestone52.0a1
Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. r=francois. MozReview-Commit-ID: 1umDhxY5eKl
toolkit/components/url-classifier/content/listmanager.js
toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
toolkit/components/url-classifier/tests/unit/test_listmanager.js
--- a/toolkit/components/url-classifier/content/listmanager.js
+++ b/toolkit/components/url-classifier/content/listmanager.js
@@ -406,21 +406,20 @@ PROT_ListManager.prototype.makeUpdateReq
       // "saving states to HashStore".
       let statePrefName = "browser.safebrowsing.provider.google4.state." + listName;
       let stateBase64 = this.prefs_.getPref(statePrefName, "");
       stateArray.push(stateBase64);
     });
 
     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
     // chunk data from the database
     var lines = tableData.split("\n");
     for (var i = 0; i < lines.length; i++) {
       var fields = lines[i].split(";");
       var name = fields[0];
--- a/toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
+++ b/toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
@@ -52,14 +52,14 @@ interface nsIUrlClassifierUtils : nsISup
 
   /**
    * Make update request for given lists and their states.
    *
    * @param aListNames An array of list name represented in string.
    * @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,
                                in uint32_t aCount);
 };
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ -30,16 +30,18 @@ static const char* gQuitApplicationMessa
 
 // Limit the list file size to 32mb
 const uint32_t MAX_FILE_SIZE = (32 * 1024 * 1024);
 
 #undef LOG
 
 // 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
 // info from the logging message.
 static void TrimAndLog(const char* aFmt, ...)
 {
   nsString raw;
 
@@ -673,17 +675,23 @@ nsUrlClassifierStreamUpdater::OnStartReq
     LOG(("nsUrlClassifierStreamUpdater::Download error [this=%p]", this));
 
     // It's possible for mDownloadErrorCallback to be null on shutdown.
     if (mDownloadErrorCallback) {
       mDownloadErrorCallback->HandleEvent(strStatus);
     }
 
     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;
     LOG(("nsUrlClassifierStreamUpdater::Beginning stream [this=%p]", this));
     rv = mDBService->BeginStream(mStreamTable);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
@@ -694,44 +702,55 @@ nsUrlClassifierStreamUpdater::OnStartReq
 
 NS_IMETHODIMP
 nsUrlClassifierStreamUpdater::OnDataAvailable(nsIRequest *request,
                                               nsISupports* context,
                                               nsIInputStream *aIStream,
                                               uint64_t aSourceOffset,
                                               uint32_t aLength)
 {
-  if (!mDBService)
+  if (!mDBService && !mDownloadError) {
     return NS_ERROR_NOT_INITIALIZED;
+  }
 
   LOG(("OnDataAvailable (%d bytes)", aLength));
 
   if (aSourceOffset > MAX_FILE_SIZE) {
     LOG(("OnDataAvailable::Abort because exceeded the maximum file size(%lld)", aSourceOffset));
     return NS_ERROR_FILE_TOO_BIG;
   }
 
   nsresult rv;
 
   // Copy the data into a nsCString
   nsCString chunk;
   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);
 
   return NS_OK;
 }
 
 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;
 
   LOG(("OnStopRequest (status %x, beganStream %s, this=%p)", aStatus,
        mBeganStream ? "true" : "false", this));
 
   nsresult rv;
 
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
@@ -93,11 +93,13 @@ private:
     nsCString mUrl;
     nsCString mTable;
   };
   nsTArray<PendingUpdate> mPendingUpdates;
 
   nsCOMPtr<nsIUrlClassifierCallback> mSuccessCallback;
   nsCOMPtr<nsIUrlClassifierCallback> mUpdateErrorCallback;
   nsCOMPtr<nsIUrlClassifierCallback> mDownloadErrorCallback;
+
+  nsCString mDownloadErrorMessage;
 };
 
 #endif // nsUrlClassifierStreamUpdater_h_
--- a/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
@@ -113,17 +113,17 @@ InitListUpdateRequest(ThreatType aThreat
   contraints->add_supported_compressions(RAW);
   aListUpdateRequest->set_allocated_constraints(contraints);
 
   // Only set non-empty state.
   if (aStateBase64[0] != '\0') {
     nsCString stateBinary;
     nsresult rv = Base64Decode(nsCString(aStateBase64), stateBinary);
     if (NS_SUCCEEDED(rv)) {
-      aListUpdateRequest->set_state(stateBinary.get());
+      aListUpdateRequest->set_state(stateBinary.get(), stateBinary.Length());
     }
   }
 }
 
 static ClientInfo*
 CreateClientInfo()
 {
   ClientInfo* c = new ClientInfo();
@@ -292,17 +292,21 @@ nsUrlClassifierUtils::MakeUpdateRequestV
     InitListUpdateRequest(static_cast<ThreatType>(threatType), aStatesBase64[i], lur);
   }
 
   // Then serialize.
   std::string s;
   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;
 
   return NS_OK;
 }
 
 /////////////////////////////////////////////////////////////////////////////
 // non-interface methods
--- a/toolkit/components/url-classifier/tests/unit/test_listmanager.js
+++ b/toolkit/components/url-classifier/tests/unit/test_listmanager.js
@@ -67,16 +67,19 @@ let gHttpServV4 = null;
 // (in terms of "update URL") in test_update_all_tables().
 let gUpdatedCntForTableData = 0; // For TEST_TABLE_DATA_LIST.
 let gIsV4Updated = false;   // For TEST_TABLE_DATA_V4.
 
 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,
                              t.providerName,
                              t.updateUrl,
                              t.gethashUrl);
 });
 
@@ -148,17 +151,17 @@ const SERVER_INVOLVED_TEST_CASE_LIST = [
     gUpdateResponse = "n:1000\n";
 
     // We test the request against the query string since v4 request
     // would be appened to the query string. The request is generated
     // by protobuf API (binary) then encoded to base64 format.
     let requestV4 = gUrlUtils.makeUpdateRequestV4([TEST_TABLE_DATA_V4.tableName],
                                                   [""],
                                                   1);
-    gExpectedQueryV4 = "&$req=" + btoa(requestV4);
+    gExpectedQueryV4 = "&$req=" + requestV4;
 
     forceTableUpdate();
   },
 
 ];
 
 SERVER_INVOLVED_TEST_CASE_LIST.forEach(t => add_test(t));
 
@@ -168,17 +171,17 @@ add_test(function test_partialUpdateV4()
   gListManager.enableUpdate(TEST_TABLE_DATA_V4.tableName);
 
   // Since the new client state has been responded and saved in
   // test_update_all_tables, this update request should send
   // a partial update to the server.
   let requestV4 = gUrlUtils.makeUpdateRequestV4([TEST_TABLE_DATA_V4.tableName],
                                                 [btoa(NEW_CLIENT_STATE)],
                                                 1);
-  gExpectedQueryV4 = "&$req=" + btoa(requestV4);
+  gExpectedQueryV4 = "&$req=" + requestV4;
 
   forceTableUpdate();
 });
 
 // Tests nsIUrlListManager.getGethashUrl.
 add_test(function test_getGethashUrl() {
   TEST_TABLE_DATA_LIST.forEach(function (t) {
     equal(gListManager.getGethashUrl(t.tableName), t.gethashUrl);
@@ -236,16 +239,18 @@ function run_test() {
     // Not on the spec. Found in Chromium source code...
     equal(request.getHeader("X-HTTP-Method-Override"), "POST");
 
     // V4 update request uses GET.
     equal(request.method, "GET");
 
     // 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
     // update is successfully stored to database.
     response.setHeader("Content-Type",
                        "application/vnd.google.safebrowsing-update", false);
     response.setStatusLine(request.httpVersion, 200, "OK");