Bug 1162476 - Telemetry should reject external pings with invalid types. r=vladan
authorGeorg Fritzsche <georg.fritzsche@googlemail.com>
Wed, 13 May 2015 21:57:06 +0200
changeset 243867 a74157e2190b62e6961131dcf098396ba8112453
parent 243866 b15e349abce424b6a62116b075def9a93bd279b1
child 243868 c4280fa46aa5bfe3aa04362051b4b3000e214ac1
push id28754
push userkwierso@gmail.com
push dateThu, 14 May 2015 22:34:49 +0000
treeherdermozilla-central@9e97d75e5b1b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvladan
bugs1162476
milestone41.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1162476 - Telemetry should reject external pings with invalid types. r=vladan
toolkit/components/telemetry/Histograms.json
toolkit/components/telemetry/TelemetryController.jsm
toolkit/components/telemetry/tests/unit/test_PingAPI.js
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -354,29 +354,16 @@
     "description": "Geolocation on OS X is either MLS or CoreLocation"
   },
   "JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT": {
     "expires_in_version": "never",
     "kind": "enumerated",
     "n_values": 10,
     "description": "Use of SpiderMonkey's deprecated language extensions in web content: ForEach=0, DestructuringForIn=1, LegacyGenerator=2, ExpressionClosure=3, LetBlock=4, LetExpression=5, NoSuchMethod=6, FlagsArgument=7, RegExpSourceProp=8"
   },
-  "TELEMETRY_PING": {
-    "expires_in_version": "never",
-    "kind": "exponential",
-    "high": "3000",
-    "n_buckets": 10,
-    "extended_statistics_ok": true,
-    "description": "Time taken to submit telemetry info (ms)"
-  },
-  "TELEMETRY_SUCCESS": {
-    "expires_in_version": "never",
-    "kind": "boolean",
-    "description": "Successful telemetry submission"
-  },
   "XUL_CACHE_DISABLED": {
     "expires_in_version": "default",
     "kind": "flag",
     "description": "XUL cache was disabled"
   },
   "MEMORY_RESIDENT": {
     "alert_emails": ["memshrink-telemetry-alerts@mozilla.com"],
     "expires_in_version": "never",
@@ -4396,16 +4383,36 @@
   },
   "TELEMETRY_FILES_EVICTED": {
     "alert_emails": ["perf-telemetry-alerts@mozilla.com", "rvitillo@mozilla.com"],
     "expires_in_version": "never",
     "kind": "enumerated",
     "n_values": 30,
     "description": "Number of telemetry pings evicted at startup"
   },
+  "TELEMETRY_PING": {
+    "expires_in_version": "never",
+    "kind": "exponential",
+    "high": "3000",
+    "n_buckets": 10,
+    "extended_statistics_ok": true,
+    "description": "Time taken to submit telemetry info (ms)"
+  },
+  "TELEMETRY_SUCCESS": {
+    "expires_in_version": "never",
+    "kind": "boolean",
+    "description": "Successful telemetry submission"
+  },
+  "TELEMETRY_INVALID_PING_TYPE_SUBMITTED": {
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "never",
+    "kind": "count",
+    "keyed": true,
+    "description": "Count of individual invalid ping types that were submitted to Telemetry."
+  },
   "TELEMETRY_TEST_FLAG": {
     "expires_in_version": "never",
     "kind": "flag",
     "description": "a testing histogram; not meant to be touched"
   },
   "TELEMETRY_TEST_COUNT": {
     "expires_in_version": "never",
     "kind": "count",
--- a/toolkit/components/telemetry/TelemetryController.jsm
+++ b/toolkit/components/telemetry/TelemetryController.jsm
@@ -198,16 +198,21 @@ this.TelemetryController = Object.freeze
   },
 
   /**
    * Submit ping payloads to Telemetry. This will assemble a complete ping, adding
    * environment data, client id and some general info.
    * Depending on configuration, the ping will be sent to the server (immediately or later)
    * and archived locally.
    *
+   * To identify the different pings and to be able to query them pings have a type.
+   * A type is a string identifier that should be unique to the type ping that is being submitted,
+   * it should only contain alphanumeric characters and '-' for separation, i.e. satisfy:
+   * /^[a-z0-9][a-z0-9-]+[a-z0-9]$/i
+   *
    * @param {String} aType The type of the ping.
    * @param {Object} aPayload The actual data payload for the ping.
    * @param {Object} [aOptions] Options object.
    * @param {Boolean} [aOptions.addClientId=false] true if the ping should contain the client
    *                  id, false otherwise.
    * @param {Boolean} [aOptions.addEnvironment=false] true if the ping should contain the
    *                  environment data.
    * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
@@ -571,16 +576,25 @@ let Impl = {
    *                  environment data.
    * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
    * @returns {Promise} A promise that is resolved with the ping id once the ping is stored or sent.
    */
   submitExternalPing: function send(aType, aPayload, aOptions) {
     this._log.trace("submitExternalPing - type: " + aType + ", server: " + this._server +
                     ", aOptions: " + JSON.stringify(aOptions));
 
+    // Enforce the type string to only contain sane characters.
+    const typeUuid = /^[a-z0-9][a-z0-9-]+[a-z0-9]$/i;
+    if (!typeUuid.test(aType)) {
+      this._log.error("submitExternalPing - invalid ping type: " + aType);
+      let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PING_TYPE_SUBMITTED");
+      histogram.add(aType, 1);
+      return Promise.reject(new Error("Invalid type string submitted."));
+    }
+
     const pingData = this.assemblePing(aType, aPayload, aOptions);
     this._log.trace("submitExternalPing - ping assembled, id: " + pingData.id);
 
     // Always persist the pings if we are allowed to.
     let archivePromise = TelemetryArchive.promiseArchivePing(pingData)
       .catch(e => this._log.error("submitExternalPing - Failed to archive ping " + pingData.id, e));
     let p = [ archivePromise ];
 
--- a/toolkit/components/telemetry/tests/unit/test_PingAPI.js
+++ b/toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ -165,16 +165,39 @@ add_task(function* test_clientId() {
   id = yield TelemetryController.submitExternalPing("test-type", {}, {addClientId: true});
   ping = yield TelemetryArchive.promiseArchivedPingById(id);
   Assert.equal(ping.clientId, clientId);
 
   // Finish setup.
   yield promiseSetup;
 });
 
+add_task(function* test_InvalidPingType() {
+  const TYPES = [
+    "a",
+    "-",
+    "¿€€€?",
+    "-foo-",
+    "-moo",
+    "zoo-",
+    ".bar",
+    "asfd.asdf",
+  ];
+
+  for (let type of TYPES) {
+    let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PING_TYPE_SUBMITTED");
+    Assert.equal(histogram.snapshot(type).sum, 0,
+                 "Should not have counted this invalid ping yet: " + type);
+    Assert.ok(promiseRejects(TelemetryController.submitExternalPing(type, {})),
+              "Ping type should have been rejected.");
+    Assert.equal(histogram.snapshot(type).sum, 1,
+                 "Should have counted this as an invalid ping type.");
+  }
+});
+
 add_task(function* test_currentPingData() {
   yield TelemetrySession.setup();
 
   // Setup test data.
   let h = Telemetry.getHistogramById("TELEMETRY_TEST_RELEASE_OPTOUT");
   h.clear();
   h.add(1);
   let k = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_RELEASE_OPTOUT");