1292226 - TelemetryController.submitExternalPing() allows non-object payloads

Status: RESOLVED FIXED

Description

[Georg Fritzsche [:gfritzsche]]

Currently users can submit pings with non-object payloads like:
> TelemetryController.submitExternalPing("type", "payload");

We should reject all payloads that are not objects and log an error:
https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/toolkit/components/telemetry/TelemetryController.jsm#493

We should add test coverage for this similar to this test function:
https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/toolkit/components/telemetry/tests/unit/test_PingAPI.js#428

Comment 1

[Georg Fritzsche [:gfritzsche]]

Robert, would you be interested in working on this?

Comment 2

[rthyberg]

Sounds good to me. Should I be recreating the if logic on line 498 but for the payload? How/where do I log the errors?
https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/toolkit/components/telemetry/TelemetryController.jsm#498

Comment 3

[Georg Fritzsche [:gfritzsche]]

Yes, that would be similar to line 498.
Errors can be logged using `this._log.error()`.

Comment 4

[rthyberg]

Created attachment 8781953 [details] [diff] [review]
bug1292226.patch

Not there yet. Lets see if I am going the right direction

I think my logic for checking if is not an object is correct in TelemetryController. Let me know if it should be different

In the test file:
I dont know what test cases I need.
I dont know what to pass in for the aType into externalSubmitPing.
Test is failing.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8781953 [details] [diff] [review]
bug1292226.patch

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

This looks like a good start.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +504,5 @@
> +    // Enforce that  aPayload is an object.
> +    if (aPayload === null || typeof aPayload !== 'object') {
> +      this._log.error("submitExternalPing - invalid payload type: " + typeof aPayload);
> +      let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PAYLOAD_TYPE_SUBMITTED");
> +      histogram.add(aPayload, 1);

Good thinking on also recording the failure into a histogram here.
I would suggest we use the slightly shorter "TELEMETRY_INVALID_PAYLOAD_SUBMITTED".
We should also use `histogram.add(aType, 1)` here - that way we see in the incoming data which ping type caused this.

For the histogram to work, you need to add the new histogram "TELEMETRY_INVALID_PAYLOAD_SUBMITTED" to toolkit/components/telemetry/Histograms.json.
You can use the entry for "TELEMETRY_INVALID_PING_TYPE_SUBMITTED" as a template.
This page has the background on adding new probes if you are curious:
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

After building you can test this:
* run your build, go to about:telemetry
* open tools -> web developer -> web console
  (on this special page, some privileged operations are available in the web console)
* submit a ping, using e.g.: TelemetryController.submitExternalPing("just-a-test-ping", "bad payload")
* reload about:telemetry
* look at "Keyed Histograms" - your invalid payload submission should be counted in "TELEMETRY_INVALID_PAYLOAD_SUBMITTED"
* look at tools -> web developer -> web console ... this should show the logged error

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +455,5 @@
> +          [1,2,3,4],
> +          null,
> +  ];
> +
> +  for (let type of PAYLOAD_TYPES) {

Nit: Lets use `payload` instead of `type`, to avoid confusion with the "ping type" (which is "payload_test" here).

@@ +460,5 @@
> +    let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PAYLOAD_TYPE_SUBMITTED");
> +    Assert.equal(histogram.snapshot(type).sum, 0,
> +                 "Should not have counted this invalid payload yet: " + type);
> +    Assert.ok(promiseRejects(TelemetryController.submitExternalPing("payload_test", type)),
> +               "Payload type should have been rejected.");

The type "payload_test" contains characters that are not allowed, see:
https://dxr.mozilla.org/mozilla-central/rev/fe895421dfbe1f1f8f1fc6a39bb20774423a6d74/toolkit/components/telemetry/TelemetryController.jsm#190

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to rthyberg from comment #4)
> I dont know what test cases I need.

The test cases look good to me. I'd add `undefined` too though.

Comment 7

[rthyberg]

Created attachment 8783914 [details] [diff] [review]
bug1292226.patch

A bit closer. When i try making the call to submitExternalPing in the browser I am given an error

"[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITelemetry.getKeyedHistogramById]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/TelemetryController.jsm :: send :: line 507"  data: no]"

not really sure what this means

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to rthyberg from comment #7)
> A bit closer. When i try making the call to submitExternalPing in the
> browser I am given an error
> 
> "[Exception... "Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsITelemetry.getKeyedHistogramById]"  nsresult:
> "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
> resource://gre/modules/TelemetryController.jsm :: send :: line 507"  data:
> no]"

For me its working fine with your patch. Running this:
> TelemetryController.submitExternalPing("just-a-test-ping", "bad payload")
... gives me the error:
> 1471957517849	Toolkit.Telemetry	ERROR	TelemetryController::submitExternalPing - invalid payload type: string

I get that error you see when i'm calling getKeyedHistogramById() with an unknown histogram name.
Did you build properly after renaming the histogram? I.e. do `mach build`?

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8783914 [details] [diff] [review]
bug1292226.patch

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

This looks pretty close, below mostly nits and small fixes.

Running tests with this patch (mach xpcshell-test toolkit/components/telemetry/tests/unit), i see a test failure in test_TelemetrySend.js here:
https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/toolkit/components/telemetry/tests/unit/test_TelemetrySend.js#273

This test triggers the new payload type check and we need to fix the test.

::: toolkit/components/telemetry/Histograms.json
@@ +5016,5 @@
> +    "expires_in_version": "never",
> +    "bug_numbers": [1292226],
> +    "kind": "count",
> +    "keyed": true,
> +    "description": "Count of individual invalid payloads that were submitted to Telemetry."

Nit: We should mention what the key is here.
"..., keyed by ping type."?

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +501,4 @@
>        histogram.add(aType, 1);
>        return Promise.reject(new Error("Invalid type string submitted."));
>      }
> +    // Enforce that  aPayload is an object.

Nit: "Enforce that the payload is an object."

@@ +504,5 @@
> +    // Enforce that  aPayload is an object.
> +    if (aPayload === null || typeof aPayload !== 'object') {
> +      this._log.error("submitExternalPing - invalid payload type: " + typeof aPayload);
> +      let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PAYLOAD_SUBMITTED");
> +      histogram.add(aPayload, 1);

`histogram.add(aType, 1);`
We want to know which ping types are causing this.

@@ +505,5 @@
> +    if (aPayload === null || typeof aPayload !== 'object') {
> +      this._log.error("submitExternalPing - invalid payload type: " + typeof aPayload);
> +      let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PAYLOAD_SUBMITTED");
> +      histogram.add(aPayload, 1);
> +      return Promise.reject(new Error("Invalid payload object submitted."));

Nit: "Invalid payload type submitted."

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +451,5 @@
> +add_task(function* test_InvalidPayloadType() {
> +  const PAYLOAD_TYPES = [
> +          19,
> +          "string",
> +          [1,2,3,4],

Nit: spaces after the "," please.
eslint will complain about this: `mach eslint toolkit/components/telemetry`

@@ +461,5 @@
> +    let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PAYLOAD_SUBMITTED");
> +    Assert.equal(histogram.snapshot(payload).sum, 0,
> +                 "Should not have counted this invalid payload yet: " + payload);
> +    Assert.ok(promiseRejects(TelemetryController.submitExternalPing("payload-test", payload)),
> +               "Payload type should have been rejected.");

You need to `...(yield promiseRejects(...`.
Otherwise we just pass a promise object to `Assert.ok()`, not the resulting (async) boolean value.

@@ +463,5 @@
> +                 "Should not have counted this invalid payload yet: " + payload);
> +    Assert.ok(promiseRejects(TelemetryController.submitExternalPing("payload-test", payload)),
> +               "Payload type should have been rejected.");
> +    Assert.equal(histogram.snapshot(payload).sum, 1,
> +                 "Should have counted this as an invalud payload type.");

Nit: "invalid".

Comment 10

[rthyberg]

https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/toolkit/components/telemetry/tests/unit/test_TelemetrySend.js#273

I dont understand what needs to be fixed in this test.


::: toolkit/components/telemetry/Histograms.json
@@ +5016,5 @@
> +    "expires_in_version": "never",
> +    "bug_numbers": [1292226],
> +    "kind": "count",
> +    "keyed": true,
> +    "description": "Count of individual invalid payloads that were submitted to Telemetry."

Nit: We should mention what the key is here.
"..., keyed by ping type."?

I dont understand what needs to be changed here. Do I need to make a comment?

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to rthyberg from comment #10)
> https://dxr.mozilla.org/mozilla-central/rev/
> 24763f58772d45279a935790f732d80851924b46/toolkit/components/telemetry/tests/
> unit/test_TelemetrySend.js#273
> 
> I dont understand what needs to be fixed in this test.

If you look at the payload it submits (OVERSIZED_PAYLOAD), you'll see that it is a string.
We are adding a check here that the payload is an object, so we need to change OVERSIZED_PAYLOAD here and make it an object.

> ::: toolkit/components/telemetry/Histograms.json
> @@ +5016,5 @@
> > +    "expires_in_version": "never",
> > +    "bug_numbers": [1292226],
> > +    "kind": "count",
> > +    "keyed": true,
> > +    "description": "Count of individual invalid payloads that were submitted to Telemetry."
> 
> Nit: We should mention what the key is here.
> "..., keyed by ping type."?
> 
> I dont understand what needs to be changed here. Do I need to make a comment?

I mean, lets change the description by adding "..., keyed by ping type." to it.

Comment 12

[rthyberg]

Created attachment 8785908 [details] [diff] [review]
bug1292226.patch

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8785908 [details] [diff] [review]
bug1292226.patch

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

This looks close, but still needs some changes.

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +451,5 @@
> +add_task(function* test_InvalidPayloadType() {
> +  const PAYLOAD_TYPES = [
> +          19,
> +          "string",
> +          [1,2,3,4],

Note that `typeof [] == "object"`. We can use `Array.isArray()` to catch this in `submitExternalPing()`:
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray

Also, nit: Add a space after the commas in the array.

@@ +458,5 @@
> +  ];
> +
> +  for (let payload of PAYLOAD_TYPES) {
> +    let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PAYLOAD_SUBMITTED");
> +    Assert.equal(histogram.snapshot(payload).sum, 0,

You should use `.snapshot("payload-test")`.
Remember that we use the ping type as the key to record into the histogram.

I think this works better like this:
let histogram = ...;
histogram.clear();
for (let i = 0; i < PAYLOAD_TYPES.length; ++i) {
  Assert.ok(... .submitExternalPing("payload-test", payload[i])...);
  Assert.equal(histogram.snapshot("payload-test"), i + 1, ...);
}

::: toolkit/components/telemetry/tests/unit/test_TelemetrySend.js
@@ +256,4 @@
>    const TEST_PING_TYPE = "test-ping-type";
>  
>    // Generate a 2MB string and create an oversized payload.
> +  const OVERSIZED_PAYLOAD = {"payload": generateRandomString(2 * 1024 * 1024)};

Nit: Lets call this property "data" to avoid confusion with how we use the term "payload" elsewhere.

Comment 14

[rthyberg]

Created attachment 8786680 [details] [diff] [review]
bug1292226.patch

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8786680 [details] [diff] [review]
bug1292226.patch

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

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +460,5 @@
> +    let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PAYLOAD_SUBMITTED");
> +    histogram.clear();
> +    for (let i= 0; i < PAYLOAD_TYPES.length; i++) {
> +      Assert.equal(histogram.snapshot("payload-test").sum, 0,
> +                 "Should not have counted this invalid payload yet: " + payload[i]);

I don't think this will work as there is no variable `payload` in scope.
Please make sure this test runs fine first.

Comment 16

[rthyberg]

Created attachment 8787136 [details] [diff] [review]
bug1292226.patch

Made the changes. Sorry I overlooked that. I still cant test this because of what I mentioned earlier. I am not really sure how to fix it but you said it was working for you?

Comment 17

[rthyberg]

Created attachment 8787163 [details] [diff] [review]
attachment.cgi?id=8787136

I ran the patch on my local machine and noticed some errors. I cant run all the unit tests because a lot of them are broken on my machine but test_PingApi.js did pass.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8787163 [details] [diff] [review]
attachment.cgi?id=8787136

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

This runs fine for me locally, only one change for the code that i'd like to see.
Lets change the commit message be a bit more explicit about the change, say: "Bug 1292226 - Reject non-object ping payloads submitted to Telemetry."

After uploading the updated patch, we still need to get data collection review [1].
We can set the feedback flag for :bsmedberg for that.

1: https://wiki.mozilla.org/Firefox/Data_Collection

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +459,5 @@
> +    let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PAYLOAD_SUBMITTED");
> +    for (let i = 0; i < PAYLOAD_TYPES.length; i++) {
> +      histogram.clear();
> +      Assert.equal(histogram.snapshot("payload-test").sum, 0,
> +                 "Should not have counted this invalid payload yet: " + PAYLOAD_TYPES[i]);

This won't print what you expect (check e.g. ""+[1,2] in the web console). Lets instead use: 
  ... + JSON.stringify(PAYLOAD_TYPES[i])

Comment 19

[rthyberg]

Created attachment 8787573 [details] [diff] [review]
attachment.cgi?id=8787136

Comment 20

[Georg Fritzsche [:gfritzsche]]

Benjamin, this is asking you for data collection review.
We had attempts to submit non-object payloads for pings.
The patch here rejects this and counts the occurences (by ping type) so that we can monitor this.

Comment 21

[Benjamin Smedberg]

Comment on attachment 8787573 [details] [diff] [review]
attachment.cgi?id=8787136

Georg will you be monitoring this as part of the telemetry client health dashboard? This telemetry feels like overkill, especially since we'll raise an exception also. But if you really need it and have a monitoring plan, data-r=me

Comment 22

[Georg Fritzsche [:gfritzsche]]

In general these kind of health metrics are useful for tracking down bad usage - i wouldn't always want to rely on the errors being always caught (what if it is only happen with a non-technical audience?).

The monitoring part is a good point, it would be much better to get automated mail about regressions with this.
For keyed histograms we don't get alerts though.

Robert, can you change it into a non-keyed histogram with "kind":"count"?
That also means changing the `.add()` call to not pass the type.

Comment 23

[rthyberg]

Created attachment 8790600 [details] [diff] [review]
attachment.cgi?id=8787136

I was having trouble with the new histogram like i was before. I recompiled but its saying its not there. I only changed .add() and made the histogram not keyed. Can you take a look?

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8790600 [details] [diff] [review]
attachment.cgi?id=8787136

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

This is nearly there, i actually see the error this time too because of the use of getKeyedHistogramById vs. getHistogramById.
Let's catch up some time again on IRC about your local failures if you still have them.

::: toolkit/components/telemetry/Histograms.json
@@ +5372,5 @@
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",
> +    "bug_numbers": [1292226],
> +    "kind": "count",
> +    "description": "Count of individual invalid payloads that were submitted to Telemetry, keyed by ping type."

We also need to update the description, removing the "keyed by ping type" part.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +503,5 @@
>      }
> +    // Enforce that the payload is an object.
> +    if (aPayload === null || typeof aPayload !== 'object' || Array.isArray(aPayload)) {
> +      this._log.error("submitExternalPing - invalid payload type: " + typeof aPayload);
> +      let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PAYLOAD_SUBMITTED");

This now needs to use `Telemetry.getHistogramById(...)`.

Comment 25

[rthyberg]

Created attachment 8791901 [details] [diff] [review]
attachment.cgi?id=8787136

alright this should be good

Comment 26

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8791901 [details] [diff] [review]
attachment.cgi?id=8787136

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

Thanks, this looks good now!
The last thing is that indentation is now off in the test. Lets fix that and then i'll land it.

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +448,5 @@
>  });
>  
> +add_task(function* test_InvalidPayloadType() {
> +  const PAYLOAD_TYPES = [
> +          19,

The indentation here is off - it should be 2 spaces per indentation level.

@@ +455,5 @@
> +          null,
> +          undefined,
> +  ];
> +
> +    let histogram = Telemetry.getHistogramById("TELEMETRY_INVALID_PAYLOAD_SUBMITTED");

Ditto, this has 4 instead of 2 spaces.

Comment 27

[Georg Fritzsche [:gfritzsche]]

Did you need some information from me? I didn't see any comment.

Comment 28

[rthyberg]

Created attachment 8793032 [details] [diff] [review]
attachment.cgi?id=8787136

Yeah I made the changes and I thought I uploaded the patch but I guess I didn't actually submit it.

Comment 29

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8793032 [details] [diff] [review]
attachment.cgi?id=8787136

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

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +455,5 @@
> +    null,
> +    undefined,
> +  ];
> +
> +    let histogram = Telemetry.getHistogramById("TELEMETRY_INVALID_PAYLOAD_SUBMITTED");

The indentation is still off from here on.

Comment 30

[rthyberg]

Created attachment 8793664 [details] [diff] [review]
attachment.cgi?id=8787136

I had a lot of trouble seeing that one.

Comment 31

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8793664 [details] [diff] [review]
attachment.cgi?id=8787136

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

The indentation is still off for the lines of the for-loop in test_PinAPI.js.
I'll just fix that and land it.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/46645a6bb7dc
Reject non-object ping payloads submitted to Telemetry. r=gfritzsche, data-r=bsmedberg

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46645a6bb7dc