Bug 1357798 - Ensure URLs don't end up in sync error telemetry. r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Mon, 12 Jun 2017 13:38:39 -0400
changeset 592698 27a329f1fb6d5c690c7ed612735448160f8245d4
parent 591190 fc905fca340af6f85a9ba95cd024572d6708f7ad
child 632912 99f42e7dbc36d08b048f34daa2331c30605412b1
push id63484
push userbmo:tchiovoloni@mozilla.com
push dateMon, 12 Jun 2017 17:38:50 +0000
reviewersmarkh
bugs1357798
milestone55.0a1
Bug 1357798 - Ensure URLs don't end up in sync error telemetry. r?markh MozReview-Commit-ID: AxFzysv0ks3
services/sync/modules/telemetry.js
services/sync/tests/unit/test_telemetry.js
--- 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"];