Closed Bug 1162476 Opened 4 years ago Closed 4 years ago

Telemetry should reject external pings with invalid types

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- unaffected
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

(Whiteboard: [uplift])

Attachments

(1 file, 1 obsolete file)

Currently TelemetryController.submitExternalPing() blindly to use a type string of the format we expect.
For simplicity we should reject everything which is not of the format: /^[a-z0-9][a-z0-9-]+[a-z0-9]$/i
Simple patch, i think that's pretty obvious. If you start submitting a new ping type you should test that anyway, so this rejecting on invalid types should not break anything released.
Attachment #8602704 - Flags: review?(vdjeric)
Assignee: nobody → gfritzsche
Status: NEW → ASSIGNED
Should we do this rejection on the server-side to potentially discover bugs or improperly-added ping types?
Ok, there is two problems there:
* discovering bugs
* keeping the Telemetry client code working properly

For discovering bugs we could use a keyed histogram counter ("count of rejected pings", key being the type).
We use the type internally for things like archiving filenames, so we shouldn't leave that through.
An alternative approach:
If an external ping with an invalid ping type is submitted, set the ping type to "invalid" and store the invalid type in a different field (say ping.submittedType).
I think a keyed histogram counter (with proper monitoring / alerting on the server) would cover the case I mentioned in Comment 2.
This adds the probe for counting invalid ping types. I also moved the two lonely TELEMETRY_* histograms up to the rest and added alerts.
Attachment #8602860 - Flags: review?(vdjeric)
Attachment #8602704 - Attachment is obsolete: true
Attachment #8602704 - Flags: review?(vdjeric)
I think that measuring invalid ping types is over-engineering this. They won't work, and that will be caught in QA. And we don't have the staffing to really monitor this anyway.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> I think that measuring invalid ping types is over-engineering this. They
> won't work, and that will be caught in QA. And we don't have the staffing to
> really monitor this anyway.

Just as an aside, changes in histogram distributions are monitored automatically by Roberto's change detection tool
Comment on attachment 8602860 [details] [diff] [review]
Telemetry should reject external pings with invalid types

Review of attachment 8602860 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +4388,5 @@
>      "n_values": 30,
>      "description": "Number of telemetry pings evicted at startup"
>    },
> +  "TELEMETRY_PING": {
> +    "alert_emails": ["telemetry-alerts@mozilla.com"],

You don't need to set telemetry-alerts@mozilla.com, all histograms generate alerts even without an alert_emails field, and the alerts always get sent to the default notification address. The alert_emails field is intended for identifying teams which own the probe

Btw, I might have mislead you last week, the default notification address is actually dev-telemetry-alerts@lists.mozilla.org, the alerts get sent FROM telemetry-alerts@mozilla.com

@@ +4403,5 @@
> +    "kind": "boolean",
> +    "description": "Successful telemetry submission"
> +  },
> +  "TELEMETRY_INVALID_PING_TYPE_SUBMITTED": {
> +    "alert_emails": ["telemetry-alerts@mozilla.com"],

You might want to create a new alert email alias for you & Alessio, e.g. telemetry-client-dev@mozilla.com

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +194,5 @@
>     * 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, each user needs to submit
> +   * a different ping type.

"each user needs to submit a different ping type"?
Attachment #8602860 - Flags: review?(vdjeric) → review+
Whiteboard: [uplift]
https://hg.mozilla.org/mozilla-central/rev/a74157e2190b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8602860 [details] [diff] [review]
Telemetry should reject external pings with invalid types

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.
Attachment #8602860 - Flags: approval-mozilla-aurora?
Comment on attachment 8602860 [details] [diff] [review]
Telemetry should reject external pings with invalid types

Unified telemetry is an important new feature. It is blocking some other projects. Taking it.
Attachment #8602860 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.