Bug 1357798 - Ensure URLs don't end up in sync error telemetry. r?markh
MozReview-Commit-ID: AxFzysv0ks3
--- a/services/sync/modules/telemetry.js
+++ b/services/sync/modules/telemetry.js
@@ -397,16 +397,32 @@ class TelemetryRecord {
log.error(`Notification for engine ${engineName} but it isn't current`);
return true;
}
}
return false;
}
}
+function cleanErrorMessage(error) {
+ // There's a chance the profiledir is in the error string which is PII we
+ // want to avoid including in the ping.
+ error = error.replace(reProfileDir, "[profileDir]");
+ // MSG_INVALID_URL from /dom/bindings/Errors.msg -- no way to access this
+ // directly from JS.
+ if (error.endsWith("is not a valid URL.")) {
+ error = "<URL> is not a valid URL.";
+ }
+ // Try to filter things that look somewhat like a URL (in that they contain a
+ // colon in the middle of non-whitespace), in case anything else is including
+ // these in error messages.
+ error = error.replace(/\S+:\S+/g, "<URL>");
+ return error;
+}
+
class SyncTelemetryImpl {
constructor(allowedEngines) {
log.level = Log.Level[Svc.Prefs.get("log.logger.telemetry", "Trace")];
// This is accessible so we can enable custom engines during tests.
this.allowedEngines = allowedEngines;
this.current = null;
this.setupObservers();
@@ -661,19 +677,17 @@ class SyncTelemetryImpl {
return { name: "shutdownerror" };
}
if (typeof error === "string") {
if (error.startsWith("error.")) {
// This is hacky, but I can't imagine that it's not also accurate.
return { name: "othererror", error };
}
- // There's a chance the profiledir is in the error string which is PII we
- // want to avoid including in the ping.
- error = error.replace(reProfileDir, "[profileDir]");
+ error = cleanErrorMessage(error);
return { name: "unexpectederror", error };
}
if (error.failureCode) {
return { name: "othererror", error: error.failureCode };
}
if (error instanceof AuthenticationError) {
@@ -693,17 +707,16 @@ class SyncTelemetryImpl {
}
if (error.result) {
return { name: "nserror", code: error.result };
}
return {
name: "unexpectederror",
- // as above, remove the profile dir value.
- error: String(error).replace(reProfileDir, "[profileDir]")
- }
+ error: cleanErrorMessage(String(error))
+ };
}
}
/* global SyncTelemetry */
this.SyncTelemetry = new SyncTelemetryImpl(ENGINES);
--- a/services/sync/tests/unit/test_telemetry.js
+++ b/services/sync/tests/unit/test_telemetry.js
@@ -379,16 +379,48 @@ add_task(async function test_engine_fail
ok(!failureReason.error.includes(OS.Constants.Path.profileDir), failureReason.error);
ok(failureReason.error.includes("[profileDir]"), failureReason.error);
} finally {
await cleanAndGo(engine, server);
Service.engineManager.unregister(engine);
}
});
+add_task(async function test_clean_urls() {
+ enableValidationPrefs();
+
+ Service.engineManager.register(SteamEngine);
+ let engine = Service.engineManager.get("steam");
+ engine.enabled = true;
+ let server = serverForFoo(engine);
+ await SyncTestingInfrastructure(server);
+ engine._errToThrow = new TypeError("http://www.google .com is not a valid URL.");
+
+ try {
+ _(`test_clean_urls: Steam tracker contents: ${
+ JSON.stringify(engine._tracker.changedIDs)}`);
+ let ping = await sync_and_validate_telem(true);
+ equal(ping.status.service, SYNC_FAILED_PARTIAL);
+ let failureReason = ping.engines.find(e => e.name === "steam").failureReason;
+ equal(failureReason.name, "unexpectederror");
+ equal(failureReason.error, "<URL> is not a valid URL.");
+ // Handle other errors that include urls.
+ engine._errToThrow = "Other error message that includes some:url/foo/bar/ in it.";
+ ping = await sync_and_validate_telem(true);
+ equal(ping.status.service, SYNC_FAILED_PARTIAL);
+ failureReason = ping.engines.find(e => e.name === "steam").failureReason;
+ equal(failureReason.name, "unexpectederror");
+ equal(failureReason.error, "Other error message that includes <URL> in it.");
+ } finally {
+ await cleanAndGo(engine, server);
+ Service.engineManager.unregister(engine);
+ }
+});
+
+
add_task(async function test_initial_sync_engines() {
enableValidationPrefs();
Service.engineManager.register(SteamEngine);
let engine = Service.engineManager.get("steam");
engine.enabled = true;
// These are the only ones who actually have things to sync at startup.
let engineNames = ["clients", "bookmarks", "prefs", "tabs"];