commit 24519257c6c245a1d6e2ed044d3e2a2715559a6d
parent 5f28dfcc14b5a0c0dc99b268b2386a60e847dcc4
Author: Johannes Salas Schmidt <joschmidt@mozilla.com>
Date: Tue, 2 Dec 2025 11:14:49 +0000
Bug 2002811 - Collect additional information about origin - r=credential-management-reviewers,firefox-desktop-core-reviewers ,dimi,mossop
and actually re-migrate via app data version increment.
Differential Revision: https://phabricator.services.mozilla.com/D274304
Diffstat:
4 files changed, 203 insertions(+), 56 deletions(-)
diff --git a/browser/components/BrowserGlue.sys.mjs b/browser/components/BrowserGlue.sys.mjs
@@ -1607,7 +1607,7 @@ BrowserGlue.prototype = {
// Use an increasing number to keep track of the current state of the user's
// profile, so we can move data around as needed as the browser evolves.
// Completely unrelated to the current Firefox release number.
- const APP_DATA_VERSION = 162;
+ const APP_DATA_VERSION = 163;
const PREF = "browser.migration.version";
let profileDataVersion = Services.prefs.getIntPref(PREF, -1);
diff --git a/toolkit/components/passwordmgr/LoginManagerRustMirror.sys.mjs b/toolkit/components/passwordmgr/LoginManagerRustMirror.sys.mjs
@@ -7,25 +7,127 @@ ChromeUtils.defineESModuleGetters(lazy, {
LoginHelper: "resource://gre/modules/LoginHelper.sys.mjs",
});
-const rustMirrorTelemetryVersion = "2";
+const rustMirrorTelemetryVersion = "3";
-/* Check if an url has punicode encoded hostname */
-function isPunycodeOrigin(origin) {
+// checks validity of an origin
+function checkOrigin(origin) {
try {
- return origin && new URL(origin).hostname.startsWith("xn--");
- } catch (_) {
+ new URL(origin);
+ return true;
+ } catch (e) {
return false;
}
}
-/* Check if an origin is a single dot */
-function isSingleDot(str) {
- return str === ".";
+/**
+ * Validate an origin string.
+ *
+ * Returns:
+ * [
+ * "ErrorName" or null,
+ * fixedOrigin or null,
+ * ]
+ *
+ * Possible ErrorName values include:
+ * - FalsyOrigin
+ * - SurroundingWhitespace
+ * - SingleDot
+ * - ProtocolNameOnly
+ * - ProtocolFragmentOnly
+ * - ProtocolOnly
+ * - MissingProtocol
+ * - ProtocolTypo
+ * - MissingProtocol
+ * - UnknownError
+ */
+function validateOrigin(origin) {
+ // valid origin
+ if (checkOrigin(origin)) {
+ return [null, null];
+ }
+
+ // falsy origin
+ if (!origin) {
+ return ["FalsyOrigin", null];
+ }
+
+ // surrounding white-space
+ {
+ const fixedOrigin = origin.trim();
+ if (checkOrigin(fixedOrigin)) {
+ return ["SurroundingWhitespace", fixedOrigin];
+ }
+ }
+
+ const lower = origin.toLowerCase();
+
+ // some protocol-only urls we won't try to fix
+ const wontfix = {
+ ".": "SingleDot",
+
+ http: "ProtocolNameOnly",
+ "http:": "ProtocolFragmentOnly",
+ "http://": "ProtocolOnly",
+
+ https: "ProtocolNameOnly",
+ "https:": "ProtocolFragmentOnly",
+ "https://": "ProtocolOnly",
+
+ file: "ProtocolNameOnly",
+ "file:": "ProtocolFragmentOnly",
+ "file://": "ProtocolOnly",
+ };
+ if (lower in wontfix) {
+ return [wontfix[lower], null];
+ }
+
+ // leading "//"
+ if (origin.startsWith("//")) {
+ const fixedOrigin = "https:" + origin;
+ if (checkOrigin(fixedOrigin)) {
+ return ["MissingProtocol", fixedOrigin];
+ }
+ }
+
+ // protocol typos
+ const brokenPrefixes = [
+ "http//",
+ "https//",
+ "htp//",
+ "htttp//",
+ "hptts//",
+ "htpps//",
+ "http:/",
+ "https:/",
+ ];
+ for (const prefix of brokenPrefixes) {
+ if (lower.startsWith(prefix)) {
+ const fixedOrigin = "https://" + origin.slice(prefix.length);
+ if (checkOrigin(fixedOrigin)) {
+ return ["ProtocolTypo", fixedOrigin];
+ }
+ }
+ }
+
+ // no protocol
+ if (!lower.match(/^[a-z]{2,20}\:\/\//)) {
+ const fixedOrigin = "https://" + origin;
+ if (checkOrigin(fixedOrigin)) {
+ return ["MissingProtocol", fixedOrigin];
+ }
+ }
+
+ // the rest is unknown
+ return ["UnknownError", null];
}
-/* Check if an origin does not contain http protocol */
-function isHttpOrigin(str) {
- return str.startsWith("https://") || str.startsWith("http://");
+/* Check if an url has punicode encoded hostname */
+function isPunycodeOrigin(origin) {
+ try {
+ return origin && new URL(origin).hostname.startsWith("xn--");
+ } catch (_) {
+ return false;
+ }
}
/* Check if a string contains line breaks */
@@ -64,26 +166,37 @@ function recordMirrorFailure(runId, operation, error, login = null) {
error_message: normalizeRustStorageErrorMessage(error),
is_deleted: false,
- has_empty_password: false,
+
+ origin_error: null,
+ origin_fixable: false,
+ form_action_origin_error: null,
+ form_action_origin_fixable: false,
+
has_punycode_origin: false,
- has_single_dot_origin: false,
- has_non_http_origin: false,
+ has_punycode_form_action_origin: false,
+
+ has_empty_password: false,
has_username_line_break: false,
has_username_nul: false,
};
if (login) {
data.is_deleted = login.deleted;
- data.has_empty_password = !login.password;
- data.has_punycode_origin = isPunycodeOrigin(
- login.origin || login.formActionOrigin
- );
- data.has_single_dot_origin = isSingleDot(
- login.origin || login.formActionOrigin
- );
- data.has_non_http_origin = !isHttpOrigin(
- login.origin || login.formActionOrigin
+
+ const [originError, fixableOriginError] = validateOrigin(login.origin);
+ data.origin_error = originError;
+ data.origin_fixable = !!fixableOriginError;
+ const [formActionOriginError, fixableFormActionOriginError] =
+ validateOrigin(login.formActionOrigin);
+ data.form_action_origin_error = formActionOriginError;
+ data.form_action_origin_fixable = !!fixableFormActionOriginError;
+
+ data.has_punycode_origin = isPunycodeOrigin(login.origin);
+ data.has_punycode_form_action_origin = isPunycodeOrigin(
+ login.formActionOrigin
);
+
+ data.has_empty_password = !login.password;
data.has_username_line_break = containsLineBreaks(login.username);
data.has_username_nul = containsNul(login.username);
}
diff --git a/toolkit/components/passwordmgr/metrics.yaml b/toolkit/components/passwordmgr/metrics.yaml
@@ -934,21 +934,33 @@ pwmgr:
description: >
The error message or exception string from the failure.
type: string
- has_empty_password:
+ origin_error:
description: >
- Whether the login to add/modify/remove has an empty password
+ a string providing more information about an invalid origin, can be "EmptyString", "SurroundingWhitespace", "MissingScheme", "InvalidScheme", "IllegalCharacters", "InvalidPercentEncoding", "InvalidPort" or "UrlParseError"
+ type: string
+ origin_fixable:
+ description: >
+ Whether our logic would be able to fix the origin
+ type: boolean
+ form_action_origin_error:
+ description: >
+ a string providing more information about an invalid origin, can be "EmptyString", "SurroundingWhitespace", "MissingScheme", "InvalidScheme", "IllegalCharacters", "InvalidPercentEncoding", "InvalidPort" or "UrlParseError"
+ type: string
+ form_action_origin_fixable:
+ description: >
+ Whether our logic would be able to fix the form action origin
type: boolean
has_punycode_origin:
description: >
- Whether the login to add/modify/remove has a punycode origin or form_action_origin
+ Whether the login to add/modify/remove has a punycode origin
type: boolean
- has_single_dot_origin:
+ has_punycode_form_action_origin:
description: >
- Whether the login to add/modify/remove has an origin or form_action_origin consisting of a single dot only
+ Whether the login to add/modify/remove has a punycode form_action_origin
type: boolean
- has_non_http_origin:
+ has_empty_password:
description: >
- Whether the login to add/modify/remove has an origin or form_action_origin which does not start with http:// or https://
+ Whether the login to add/modify/remove has an empty password
type: boolean
has_username_line_break:
description: >
diff --git a/toolkit/components/passwordmgr/test/browser/browser_rust_mirror.js b/toolkit/components/passwordmgr/test/browser/browser_rust_mirror.js
@@ -683,37 +683,59 @@ add_task(async function test_punycode_formActionOrigin_metric() {
});
/*
- * Tests that we collect telemetry for single dot in origin
+ * Tests that we collect telemetry about several origin errors
*/
-add_task(async function test_single_dot_in_origin() {
- // ensure mirror is on
- await SpecialPowers.pushPrefEnv({
- set: [["signon.rustMirror.enabled", true]],
- });
+const originsToTest = {
+ "//example.com": "MissingProtocol",
+ "//example.com/path": "MissingProtocol",
+ "example.com": "MissingProtocol",
+ "example.com/path": "MissingProtocol",
+ "hptts//example.com": "ProtocolTypo",
+ "htp//example.com": "ProtocolTypo",
+ "htpps//example.com": "ProtocolTypo",
+ "http//example.com": "ProtocolTypo",
+ "http//example.com/path": "ProtocolTypo",
+ https: "ProtocolNameOnly",
+ "https//example.com": "ProtocolTypo",
+ "https//example.com:abc": "MissingProtocol",
+ "https:": "ProtocolFragmentOnly",
+ "https:// example.com": "UnknownError",
+ "https://": "ProtocolOnly",
+ "https:///": "UnknownError",
+ "https://exa mple.com": "UnknownError",
+ "htttp//example.com": "ProtocolTypo",
+ "www.example.com": "MissingProtocol",
+};
+for (const origin in originsToTest) {
+ add_task(async function () {
+ // ensure mirror is on
+ await SpecialPowers.pushPrefEnv({
+ set: [["signon.rustMirror.enabled", true]],
+ });
- Services.fog.testResetFOG();
+ Services.fog.testResetFOG();
- const badOrigin = ".";
- const login = LoginTestUtils.testData.formLogin({
- origin: badOrigin,
- formActionOrigin: "https://example.com",
- username: "user1",
- password: "pass1",
- });
+ const login = LoginTestUtils.testData.formLogin({
+ origin,
+ formActionOrigin: "https://example.com",
+ username: "user1",
+ password: "pass1",
+ });
- const waitForGleanEvent = BrowserTestUtils.waitForCondition(
- () => Glean.pwmgr.rustMirrorStatus.testGetValue()?.length == 1,
- "event has been emitted"
- );
- await Services.logins.addLoginAsync(login);
- await waitForGleanEvent;
+ const waitForGleanEvent = BrowserTestUtils.waitForCondition(
+ () => Glean.pwmgr.rustMirrorStatus.testGetValue()?.length == 1,
+ "event has been emitted"
+ );
+ await Services.logins.addLoginAsync(login);
+ await waitForGleanEvent;
- const [evt] = Glean.pwmgr.rustWriteFailure.testGetValue();
- Assert.ok(evt.extra?.has_single_dot_origin);
+ const [evt] = Glean.pwmgr.rustWriteFailure.testGetValue();
+ Assert.equal(evt.extra?.origin_error, originsToTest[origin]);
- LoginTestUtils.clearData();
- await SpecialPowers.flushPrefEnv();
-});
+ LoginTestUtils.clearData();
+ await SpecialPowers.flushPrefEnv();
+ });
+}
/*
* Tests that we collect telemetry if the username contains line breaks.
