Bug 1762374 - Part 1: Add a Histogram to count amount of query parameters stripped per navigation. r=anti-tracking-reviewers,necko-reviewers,dragana,timhuang

Differential Revision: https://phabricator.services.mozilla.com/D146592
This commit is contained in:
Paul Zuehlcke 2022-05-24 18:57:17 +00:00
Родитель 13ff336c18
Коммит 3cf0011c96
7 изменённых файлов: 203 добавлений и 76 удалений

Просмотреть файл

@ -616,13 +616,15 @@ void nsDocShellLoadState::MaybeStripTrackerQueryStrings(
Telemetry::LABELS_QUERY_STRIPPING_COUNT::Navigation);
nsCOMPtr<nsIURI> strippedURI;
if (URLQueryStringStripper::Strip(URI(), aContext->UsePrivateBrowsing(),
strippedURI)) {
uint32_t numStripped = URLQueryStringStripper::Strip(
URI(), aContext->UsePrivateBrowsing(), strippedURI);
if (numStripped) {
mUnstrippedURI = URI();
SetURI(strippedURI);
Telemetry::AccumulateCategorical(
Telemetry::LABELS_QUERY_STRIPPING_COUNT::StripForNavigation);
Telemetry::Accumulate(Telemetry::QUERY_STRIPPING_PARAM_COUNT, numStripped);
} else if (LoadType() & nsIDocShell::LOAD_CMD_RELOAD) {
// Preserve the Unstripped URI if it's a reload. By doing this, we can
// restore the stripped query parameters once the ETP has been toggled to
@ -635,8 +637,8 @@ void nsDocShellLoadState::MaybeStripTrackerQueryStrings(
// string could be different.
if (mUnstrippedURI) {
nsCOMPtr<nsIURI> uri;
URLQueryStringStripper::Strip(mUnstrippedURI,
aContext->UsePrivateBrowsing(), uri);
Unused << URLQueryStringStripper::Strip(
mUnstrippedURI, aContext->UsePrivateBrowsing(), uri);
bool equals = false;
Unused << URI()->Equals(uri, &equals);
MOZ_ASSERT(equals);

Просмотреть файл

@ -5149,15 +5149,21 @@ nsresult nsHttpChannel::AsyncProcessRedirection(uint32_t redirectType) {
isRedirectURIInAllowList);
}
nsCOMPtr<nsIURI> strippedURI;
if (!isRedirectURIInAllowList &&
URLQueryStringStripper::Strip(mRedirectURI, mPrivateBrowsing,
strippedURI)) {
mUnstrippedRedirectURI = mRedirectURI;
mRedirectURI = strippedURI;
if (!isRedirectURIInAllowList) {
nsCOMPtr<nsIURI> strippedURI;
uint32_t numStripped = URLQueryStringStripper::Strip(
mRedirectURI, mPrivateBrowsing, strippedURI);
Telemetry::AccumulateCategorical(
Telemetry::LABELS_QUERY_STRIPPING_COUNT::StripForRedirect);
if (numStripped) {
mUnstrippedRedirectURI = mRedirectURI;
mRedirectURI = strippedURI;
// Record telemetry, but only if we stripped any query params.
Telemetry::AccumulateCategorical(
Telemetry::LABELS_QUERY_STRIPPING_COUNT::StripForRedirect);
Telemetry::Accumulate(Telemetry::QUERY_STRIPPING_PARAM_COUNT,
numStripped);
}
}
}
}

Просмотреть файл

@ -9,6 +9,7 @@
#include "mozilla/StaticPrefs_privacy.h"
#include "mozilla/StaticPtr.h"
#include "mozilla/Unused.h"
#include "mozilla/Telemetry.h"
#include "nsEffectiveTLDService.h"
#include "nsISupportsImpl.h"
@ -42,22 +43,22 @@ URLQueryStringStripper* URLQueryStringStripper::GetOrCreate() {
}
/* static */
bool URLQueryStringStripper::Strip(nsIURI* aURI, bool aIsPBM,
nsCOMPtr<nsIURI>& aOutput) {
uint32_t URLQueryStringStripper::Strip(nsIURI* aURI, bool aIsPBM,
nsCOMPtr<nsIURI>& aOutput) {
if (aIsPBM) {
if (!StaticPrefs::privacy_query_stripping_enabled_pbmode()) {
return false;
return 0;
}
} else {
if (!StaticPrefs::privacy_query_stripping_enabled()) {
return false;
return 0;
}
}
RefPtr<URLQueryStringStripper> stripper = GetOrCreate();
if (stripper->CheckAllowList(aURI)) {
return false;
return 0;
}
return stripper->StripQueryString(aURI, aOutput);
@ -79,8 +80,8 @@ void URLQueryStringStripper::Shutdown() {
mService = nullptr;
}
bool URLQueryStringStripper::StripQueryString(nsIURI* aURI,
nsCOMPtr<nsIURI>& aOutput) {
uint32_t URLQueryStringStripper::StripQueryString(nsIURI* aURI,
nsCOMPtr<nsIURI>& aOutput) {
MOZ_ASSERT(aURI);
nsCOMPtr<nsIURI> uri(aURI);
@ -89,13 +90,14 @@ bool URLQueryStringStripper::StripQueryString(nsIURI* aURI,
nsresult rv = aURI->GetQuery(query);
NS_ENSURE_SUCCESS(rv, false);
uint32_t numStripped = 0;
// We don't need to do anything if there is no query string.
if (query.IsEmpty()) {
return false;
return numStripped;
}
URLParams params;
bool hasStripped = false;
URLParams::Parse(query, [&](nsString&& name, nsString&& value) {
nsAutoString lowerCaseName;
@ -103,7 +105,7 @@ bool URLQueryStringStripper::StripQueryString(nsIURI* aURI,
ToLowerCase(name, lowerCaseName);
if (mList.Contains(lowerCaseName)) {
hasStripped = true;
numStripped += 1;
return true;
}
@ -112,8 +114,8 @@ bool URLQueryStringStripper::StripQueryString(nsIURI* aURI,
});
// Return if there is no parameter has been stripped.
if (!hasStripped) {
return false;
if (!numStripped) {
return numStripped;
}
nsAutoString newQuery;
@ -123,7 +125,7 @@ bool URLQueryStringStripper::StripQueryString(nsIURI* aURI,
.SetQuery(NS_ConvertUTF16toUTF8(newQuery))
.Finalize(aOutput);
return true;
return numStripped;
}
bool URLQueryStringStripper::CheckAllowList(nsIURI* aURI) {

Просмотреть файл

@ -32,9 +32,10 @@ class URLQueryStringStripper final : public nsIURLQueryStrippingListObserver {
NS_DECL_ISUPPORTS
NS_DECL_NSIURLQUERYSTRIPPINGLISTOBSERVER
// Strip the query parameters that are in the strip list. Return true if there
// is any query parameter has been stripped. Otherwise, false.
static bool Strip(nsIURI* aURI, bool aIsPBM, nsCOMPtr<nsIURI>& aOutput);
// Strip the query parameters that are in the strip list. Return the amount of
// query parameters that have been stripped. Returns 0 if no query parameters
// have been stripped or the feature is disabled.
static uint32_t Strip(nsIURI* aURI, bool aIsPBM, nsCOMPtr<nsIURI>& aOutput);
private:
URLQueryStringStripper() = default;
@ -46,7 +47,7 @@ class URLQueryStringStripper final : public nsIURLQueryStrippingListObserver {
void Init();
void Shutdown();
bool StripQueryString(nsIURI* aURI, nsCOMPtr<nsIURI>& aOutput);
uint32_t StripQueryString(nsIURI* aURI, nsCOMPtr<nsIURI>& aOutput);
bool CheckAllowList(nsIURI* aURI);

Просмотреть файл

@ -19,6 +19,9 @@ const LABEL_REDIRECT = 1;
const LABEL_STRIP_FOR_NAVIGATION = 2;
const LABEL_STRIP_FOR_REDIRECT = 3;
const QUERY_STRIPPING_COUNT = "QUERY_STRIPPING_COUNT";
const QUERY_STRIPPING_PARAM_COUNT = "QUERY_STRIPPING_PARAM_COUNT";
async function clearTelemetry() {
// There's an arbitrary interval of 2 seconds in which the content
// processes sync their data with the parent process, we wait
@ -27,7 +30,8 @@ async function clearTelemetry() {
await new Promise(resolve => setTimeout(resolve, 2000));
Services.telemetry.getSnapshotForHistograms("main", true /* clear */);
Services.telemetry.getHistogramById("QUERY_STRIPPING_COUNT").clear();
Services.telemetry.getHistogramById(QUERY_STRIPPING_COUNT).clear();
Services.telemetry.getHistogramById(QUERY_STRIPPING_PARAM_COUNT).clear();
// Ensure the data is cleared in content.
await TestUtils.waitForCondition(() => {
@ -36,7 +40,11 @@ async function clearTelemetry() {
false /* clear */
).content;
return !histograms || !histograms.QUERY_STRIPPING_COUNT;
return (
!histograms ||
(!histograms[QUERY_STRIPPING_COUNT] &&
!histograms[QUERY_STRIPPING_PARAM_COUNT])
);
});
}
@ -49,8 +57,8 @@ async function verifyQueryString(browser, expected) {
});
}
async function getTelemetryProbe(probeInParent, label, checkCntFn) {
let queryStrippingHistogram;
async function getTelemetryProbe(probeInParent, key, label, checkCntFn) {
let histogram;
// Wait until the telemetry probe appears.
await TestUtils.waitForCondition(() => {
@ -66,25 +74,24 @@ async function getTelemetryProbe(probeInParent, label, checkCntFn) {
false /* clear */
).content;
}
queryStrippingHistogram = histograms.QUERY_STRIPPING_COUNT;
histogram = histograms[key];
let checkRes = false;
if (queryStrippingHistogram) {
checkRes = checkCntFn
? checkCntFn(queryStrippingHistogram.values[label])
: true;
if (histogram) {
checkRes = checkCntFn ? checkCntFn(histogram.values[label]) : true;
}
return checkRes;
});
return queryStrippingHistogram.values[label];
return histogram.values[label];
}
async function checkTelemetryProbe(probeInParent, expectedCnt, label) {
async function checkTelemetryProbe(probeInParent, key, expectedCnt, label) {
let cnt = await getTelemetryProbe(
probeInParent,
key,
label,
cnt => cnt == expectedCnt
);
@ -96,7 +103,10 @@ add_setup(async function() {
await SpecialPowers.pushPrefEnv({
set: [
["privacy.query_stripping.enabled", true],
["privacy.query_stripping.strip_list", "paramToStrip"],
[
"privacy.query_stripping.strip_list",
"paramToStrip paramToStripB paramToStripC paramToStripD",
],
],
});
@ -115,13 +125,20 @@ add_task(async function testQueryStrippingNavigationInParent() {
// Verify the telemetry probe. The stripping for new tab loading would happen
// in the parent process, so we check values in parent process.
await checkTelemetryProbe(true, 1, LABEL_STRIP_FOR_NAVIGATION);
await checkTelemetryProbe(
true,
QUERY_STRIPPING_COUNT,
1,
LABEL_STRIP_FOR_NAVIGATION
);
await checkTelemetryProbe(true, QUERY_STRIPPING_PARAM_COUNT, 1, "1");
// Because there would be some loading happening during the test and they
// could interfere the count here. So, we only verify if the counter is
// increased, but not the exact count.
let newNavigationCnt = await getTelemetryProbe(
true,
QUERY_STRIPPING_COUNT,
LABEL_NAVIGATION,
cnt => cnt > 0
);
@ -152,11 +169,73 @@ add_task(async function testQueryStrippingNavigationInContent() {
});
// Verify the telemetry probe in content process.
await checkTelemetryProbe(false, 1, LABEL_STRIP_FOR_NAVIGATION);
await checkTelemetryProbe(
false,
QUERY_STRIPPING_COUNT,
1,
LABEL_STRIP_FOR_NAVIGATION
);
await checkTelemetryProbe(false, QUERY_STRIPPING_PARAM_COUNT, 1, "1");
// Check if the navigation count is increased.
let newNavigationCnt = await getTelemetryProbe(
false,
QUERY_STRIPPING_COUNT,
LABEL_NAVIGATION,
cnt => cnt > 0
);
ok(newNavigationCnt > 0, "There is navigation count added.");
await clearTelemetry();
});
add_task(async function testQueryStrippingNavigationInContentQueryCount() {
let testThirdPartyURI =
TEST_THIRD_PARTY_URI +
"?paramToStrip=value&paramToStripB=valueB&paramToStripC=valueC&paramToStripD=valueD";
await BrowserTestUtils.withNewTab(TEST_URI, async browser => {
// Create the promise to wait for the location change.
let locationChangePromise = BrowserTestUtils.waitForLocationChange(
gBrowser,
TEST_THIRD_PARTY_URI
);
// Trigger the navigation by script.
await SpecialPowers.spawn(browser, [testThirdPartyURI], async url => {
content.postMessage({ type: "script", url }, "*");
});
await locationChangePromise;
// Verify if the query string was happened.
await verifyQueryString(browser, "");
});
// Verify the telemetry probe in content process.
await checkTelemetryProbe(
false,
QUERY_STRIPPING_COUNT,
1,
LABEL_STRIP_FOR_NAVIGATION
);
await getTelemetryProbe(false, QUERY_STRIPPING_PARAM_COUNT, "0", cnt => !cnt);
await getTelemetryProbe(false, QUERY_STRIPPING_PARAM_COUNT, "1", cnt => !cnt);
await getTelemetryProbe(false, QUERY_STRIPPING_PARAM_COUNT, "2", cnt => !cnt);
await getTelemetryProbe(false, QUERY_STRIPPING_PARAM_COUNT, "3", cnt => !cnt);
await getTelemetryProbe(
false,
QUERY_STRIPPING_PARAM_COUNT,
"4",
cnt => cnt == 1
);
await getTelemetryProbe(false, QUERY_STRIPPING_PARAM_COUNT, "5", cnt => !cnt);
// Check if the navigation count is increased.
let newNavigationCnt = await getTelemetryProbe(
false,
QUERY_STRIPPING_COUNT,
LABEL_NAVIGATION,
cnt => cnt > 0
);
@ -188,8 +267,14 @@ add_task(async function testQueryStrippingRedirect() {
// Verify the telemetry probe in parent process. Note that there is no
// non-test loading is using redirect. So, we can check the exact count here.
await checkTelemetryProbe(true, 1, LABEL_STRIP_FOR_REDIRECT);
await checkTelemetryProbe(true, 1, LABEL_REDIRECT);
await checkTelemetryProbe(
true,
QUERY_STRIPPING_COUNT,
1,
LABEL_STRIP_FOR_REDIRECT
);
await checkTelemetryProbe(true, QUERY_STRIPPING_COUNT, 1, LABEL_REDIRECT);
await checkTelemetryProbe(true, QUERY_STRIPPING_PARAM_COUNT, 1, "1");
await clearTelemetry();
});
@ -210,10 +295,16 @@ add_task(async function testQueryStrippingDisabled() {
// Verify the telemetry probe. There should be no stripped navigation count in
// parent.
await checkTelemetryProbe(true, undefined, LABEL_STRIP_FOR_NAVIGATION);
await checkTelemetryProbe(
true,
QUERY_STRIPPING_COUNT,
undefined,
LABEL_STRIP_FOR_NAVIGATION
);
// Check if the navigation count is increased.
let newNavigationCnt = await getTelemetryProbe(
true,
QUERY_STRIPPING_COUNT,
LABEL_NAVIGATION,
cnt => cnt > 0
);
@ -242,10 +333,16 @@ add_task(async function testQueryStrippingDisabled() {
// Verify the telemetry probe in content process. There should be no stripped
// navigation count in content.
await checkTelemetryProbe(false, undefined, LABEL_STRIP_FOR_NAVIGATION);
await checkTelemetryProbe(
false,
QUERY_STRIPPING_COUNT,
undefined,
LABEL_STRIP_FOR_NAVIGATION
);
// Check if the navigation count is increased.
newNavigationCnt = await getTelemetryProbe(
false,
QUERY_STRIPPING_COUNT,
LABEL_NAVIGATION,
cnt => cnt > 0
);
@ -273,8 +370,13 @@ add_task(async function testQueryStrippingDisabled() {
});
// Verify the telemetry probe. The stripped redirect count should not exist.
await checkTelemetryProbe(true, undefined, LABEL_STRIP_FOR_REDIRECT);
await checkTelemetryProbe(true, 1, LABEL_REDIRECT);
await checkTelemetryProbe(
true,
QUERY_STRIPPING_COUNT,
undefined,
LABEL_STRIP_FOR_REDIRECT
);
await checkTelemetryProbe(true, QUERY_STRIPPING_COUNT, 1, LABEL_REDIRECT);
await clearTelemetry();
});

Просмотреть файл

@ -23,18 +23,18 @@ static const char kPrefQueryStrippingList[] =
"privacy.query_stripping.strip_list";
void DoTest(const nsACString& aTestURL, const bool aIsPBM,
const nsACString& aExpectedURL, bool aExpectedResult) {
const nsACString& aExpectedURL, uint32_t aExpectedResult) {
nsCOMPtr<nsIURI> testURI;
NS_NewURI(getter_AddRefs(testURI), aTestURL);
bool result;
nsCOMPtr<nsIURI> strippedURI;
result = URLQueryStringStripper::Strip(testURI, aIsPBM, strippedURI);
uint32_t numStripped =
URLQueryStringStripper::Strip(testURI, aIsPBM, strippedURI);
EXPECT_TRUE(result == aExpectedResult);
EXPECT_TRUE(numStripped == aExpectedResult);
if (!result) {
if (!numStripped) {
EXPECT_TRUE(!strippedURI);
} else {
EXPECT_TRUE(strippedURI->GetSpecOrDefault().Equals(aExpectedURL));
@ -85,9 +85,9 @@ TEST(TestURLQueryStringStripper, TestPrefDisabled)
Preferences::SetBool(kPrefQueryStrippingEnabledPBM, false);
for (bool isPBM : {false, true}) {
DoTest("https://example.com/"_ns, isPBM, ""_ns, false);
DoTest("https://example.com/?Barfoo=123"_ns, isPBM, ""_ns, false);
DoTest("https://example.com/?fooBar=123&foobaz"_ns, isPBM, ""_ns, false);
DoTest("https://example.com/"_ns, isPBM, ""_ns, 0);
DoTest("https://example.com/?Barfoo=123"_ns, isPBM, ""_ns, 0);
DoTest("https://example.com/?fooBar=123&foobaz"_ns, isPBM, ""_ns, 0);
}
}
@ -100,7 +100,7 @@ TEST(TestURLQueryStringStripper, TestEmptyStripList)
// To create the URLQueryStringStripper, we need to run a dummy test after
// the query stripping is enabled. By doing this, the stripper will be
// initiated and we are good to test.
DoTest("https://example.com/"_ns, false, ""_ns, false);
DoTest("https://example.com/"_ns, false, ""_ns, 0);
// Set the strip list to empty and wait until the pref setting is set to the
// stripper.
@ -112,16 +112,16 @@ TEST(TestURLQueryStringStripper, TestEmptyStripList)
[&]() -> bool { return !observer->IsStillWaiting(); }));
for (bool isPBM : {false, true}) {
DoTest("https://example.com/"_ns, isPBM, ""_ns, false);
DoTest("https://example.com/?Barfoo=123"_ns, isPBM, ""_ns, false);
DoTest("https://example.com/?fooBar=123&foobaz"_ns, isPBM, ""_ns, false);
DoTest("https://example.com/"_ns, isPBM, ""_ns, 0);
DoTest("https://example.com/?Barfoo=123"_ns, isPBM, ""_ns, 0);
DoTest("https://example.com/?fooBar=123&foobaz"_ns, isPBM, ""_ns, 0);
}
}
TEST(TestURLQueryStringStripper, TestStripping)
{
// A dummy test to initiate the URLQueryStringStripper.
DoTest("https://example.com/"_ns, false, ""_ns, false);
DoTest("https://example.com/"_ns, false, ""_ns, 0);
// Set the pref and create an observer to wait the pref setting is set to the
// stripper.
@ -144,24 +144,25 @@ TEST(TestURLQueryStringStripper, TestStripping)
for (bool isPBM : {false, true}) {
bool expectStrip = (prefPBM && isPBM) || (pref && !isPBM);
DoTest("https://example.com/"_ns, isPBM, ""_ns, false);
DoTest("https://example.com/?Barfoo=123"_ns, isPBM, ""_ns, false);
DoTest("https://example.com/"_ns, isPBM, ""_ns, 0);
DoTest("https://example.com/?Barfoo=123"_ns, isPBM, ""_ns, 0);
DoTest("https://example.com/?fooBar=123"_ns, isPBM,
"https://example.com/"_ns, expectStrip);
"https://example.com/"_ns, expectStrip ? 1 : 0);
DoTest("https://example.com/?fooBar=123&foobaz"_ns, isPBM,
"https://example.com/"_ns, expectStrip);
"https://example.com/"_ns, expectStrip ? 2 : 0);
DoTest("https://example.com/?fooBar=123&Barfoo=456&foobaz"_ns, isPBM,
"https://example.com/?Barfoo=456"_ns, expectStrip);
"https://example.com/?Barfoo=456"_ns, expectStrip ? 2 : 0);
DoTest("https://example.com/?FOOBAR=123"_ns, isPBM,
"https://example.com/"_ns, expectStrip);
"https://example.com/"_ns, expectStrip ? 1 : 0);
DoTest("https://example.com/?barfoo=foobar"_ns, isPBM,
"https://example.com/?barfoo=foobar"_ns, false);
"https://example.com/?barfoo=foobar"_ns, 0);
DoTest("https://example.com/?foobar=123&nostrip=456&FooBar=789"_ns,
isPBM, "https://example.com/?nostrip=456"_ns, expectStrip);
isPBM, "https://example.com/?nostrip=456"_ns,
expectStrip ? 2 : 0);
DoTest("https://example.com/?AfoobazB=123"_ns, isPBM,
"https://example.com/?AfoobazB=123"_ns, false);
"https://example.com/?AfoobazB=123"_ns, 0);
}
}
}
@ -177,11 +178,11 @@ TEST(TestURLQueryStringStripper, TestStripping)
"TEST(TestURLQueryStringStripper, TestStripping)"_ns,
[&]() -> bool { return !observer->IsStillWaiting(); }));
DoTest("https://example.com/?fooBar=123"_ns, false, ""_ns, false);
DoTest("https://example.com/?fooBar=123&foobaz"_ns, false, ""_ns, false);
DoTest("https://example.com/?fooBar=123"_ns, false, ""_ns, 0);
DoTest("https://example.com/?fooBar=123&foobaz"_ns, false, ""_ns, 0);
DoTest("https://example.com/?bazfoo=123"_ns, false, "https://example.com/"_ns,
true);
1);
DoTest("https://example.com/?fooBar=123&Barfoo=456&foobaz=abc"_ns, false,
"https://example.com/?fooBar=123&foobaz=abc"_ns, true);
"https://example.com/?fooBar=123&foobaz=abc"_ns, 1);
}

Просмотреть файл

@ -13268,9 +13268,22 @@
"StripForRedirect"
],
"description": "A count of the query stripping. ('Navigation' = There was a top-level loading via navigation, 'Redirect' = There was a top-level loading via redirect, 'StripForNavigation' = There was a stripping happened for a top-level navigation, 'StripForRedirect' = There was a stripping happened for a top-level redirect.",
"alert_emails": ["tihuang@mozilla.com", "aedelstein@mozilla.com", "jhofmann@mozilla.com", "pbz@mozilla.com", "seceng-telemetry@mozilla.com"],
"alert_emails": ["tihuang@mozilla.com", "pbz@mozilla.com", "seceng-telemetry@mozilla.com"],
"bug_numbers": [1706616]
},
"QUERY_STRIPPING_PARAM_COUNT": {
"record_in_processes": ["main", "content"],
"products": ["firefox", "fennec"],
"expires_in_version": "never",
"releaseChannelCollection": "opt-out",
"kind": "exponential",
"low": 1,
"high": 100,
"n_buckets": 25,
"description": "If query params get stripped, how many per navigation/redirect.",
"alert_emails": ["tihuang@mozilla.com", "pbz@mozilla.com", "seceng-telemetry@mozilla.com"],
"bug_numbers": [1762374]
},
"SERVICE_WORKER_LAUNCH_TIME_2": {
"record_in_processes": ["main", "content"],
"products": ["firefox", "fennec"],