Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. r=francois.
MozReview-Commit-ID: GPkTVESJrDI
--- a/toolkit/components/url-classifier/content/listmanager.js
+++ b/toolkit/components/url-classifier/content/listmanager.js
@@ -406,24 +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);
- // Encode as base64url including padding since it's going to
- // be a part of the URL.
- streamerMap.requestPayload = btoa(requestPayload).replace(/\+/g, '-')
- .replace(/\//g, '_');
+ 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,19 @@ 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;
+
+ // Continue to receive error message only when log is enabled.
+ status = LOG_ENABLED() ? NS_OK : NS_ERROR_ABORT;
} 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 +698,54 @@ 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_OK;
+ }
+
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
@@ -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);
@@ -235,18 +238,19 @@ 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.replace(/\+/g, '-')
- .replace(/\//g, '_'));
+ 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");