Closed
Bug 1292226
Opened 6 years ago
Closed 6 years ago
TelemetryController.submitExternalPing() allows non-object payloads
Categories
(Toolkit :: Telemetry, defect, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: robertthyberg, Mentored)
Details
(Whiteboard: [measurement:client] [lang=js])
Attachments
(1 file, 10 obsolete files)
5.49 KB,
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•6 years ago
|
||
Robert, would you be interested in working on this?
Flags: needinfo?(robertthyberg)
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
Reporter | ||
Comment 3•6 years ago
|
||
Yes, that would be similar to line 498. Errors can be logged using `this._log.error()`.
Assignee: nobody → robertthyberg
Flags: needinfo?(gfritzsche)
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.
Attachment #8781953 -
Flags: feedback?(gfritzsche)
Reporter | ||
Comment 5•6 years ago
|
||
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
Attachment #8781953 -
Flags: feedback?(gfritzsche)
Reporter | ||
Comment 6•6 years ago
|
||
(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.
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
Attachment #8781953 -
Attachment is obsolete: true
Attachment #8783914 -
Flags: feedback?(gfritzsche)
Reporter | ||
Comment 8•6 years ago
|
||
(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`?
Reporter | ||
Comment 9•6 years ago
|
||
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".
Attachment #8783914 -
Flags: feedback?(gfritzsche) → feedback+
Assignee | ||
Comment 10•6 years ago
|
||
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?
Flags: needinfo?(gfritzsche)
Reporter | ||
Comment 11•6 years ago
|
||
(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.
Flags: needinfo?(gfritzsche)
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #8783914 -
Attachment is obsolete: true
Attachment #8785908 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 13•6 years ago
|
||
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.
Attachment #8785908 -
Flags: review?(gfritzsche) → feedback+
Assignee | ||
Comment 14•6 years ago
|
||
Attachment #8785908 -
Attachment is obsolete: true
Attachment #8786680 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 15•6 years ago
|
||
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.
Attachment #8786680 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 16•6 years ago
|
||
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?
Attachment #8786680 -
Attachment is obsolete: true
Attachment #8787136 -
Flags: feedback?(gfritzsche)
Assignee | ||
Comment 17•6 years ago
|
||
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.
Attachment #8787163 -
Flags: review?(gfritzsche)
Attachment #8787136 -
Flags: feedback?(gfritzsche)
Reporter | ||
Updated•6 years ago
|
Attachment #8787163 -
Attachment is patch: true
Reporter | ||
Updated•6 years ago
|
Attachment #8787136 -
Attachment is obsolete: true
Reporter | ||
Comment 18•6 years ago
|
||
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])
Attachment #8787163 -
Flags: review?(gfritzsche) → review+
Assignee | ||
Comment 19•6 years ago
|
||
Attachment #8787163 -
Attachment is obsolete: true
Attachment #8787573 -
Flags: feedback?(benjamin)
Reporter | ||
Updated•6 years ago
|
Attachment #8787573 -
Attachment is patch: true
Reporter | ||
Comment 20•6 years ago
|
||
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•6 years ago
|
||
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
Attachment #8787573 -
Flags: feedback?(benjamin) → feedback+
Reporter | ||
Comment 22•6 years ago
|
||
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.
Flags: needinfo?(robertthyberg)
Assignee | ||
Comment 23•6 years ago
|
||
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?
Attachment #8790600 -
Flags: feedback?(gfritzsche)
Reporter | ||
Updated•6 years ago
|
Attachment #8790600 -
Attachment is patch: true
Reporter | ||
Comment 24•6 years ago
|
||
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": ["telemetry-client-dev@mozilla.com"], > + "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(...)`.
Attachment #8790600 -
Flags: feedback?(gfritzsche) → feedback+
Assignee | ||
Comment 25•6 years ago
|
||
alright this should be good
Attachment #8787573 -
Attachment is obsolete: true
Attachment #8790600 -
Attachment is obsolete: true
Attachment #8791901 -
Flags: review?(gfritzsche)
Reporter | ||
Updated•6 years ago
|
Attachment #8791901 -
Attachment is patch: true
Reporter | ||
Comment 26•6 years ago
|
||
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.
Attachment #8791901 -
Flags: review?(gfritzsche) → review+
Reporter | ||
Comment 27•6 years ago
|
||
Did you need some information from me? I didn't see any comment.
Flags: needinfo?(gfritzsche)
Assignee | ||
Comment 28•6 years ago
|
||
Yeah I made the changes and I thought I uploaded the patch but I guess I didn't actually submit it.
Attachment #8791901 -
Attachment is obsolete: true
Reporter | ||
Comment 29•6 years ago
|
||
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.
Assignee | ||
Comment 30•6 years ago
|
||
I had a lot of trouble seeing that one.
Attachment #8793032 -
Attachment is obsolete: true
Reporter | ||
Comment 31•6 years ago
|
||
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.
Attachment #8793664 -
Flags: review+
Comment 32•6 years ago
|
||
Pushed by georg.fritzsche@googlemail.com: https://hg.mozilla.org/integration/fx-team/rev/46645a6bb7dc Reject non-object ping payloads submitted to Telemetry. r=gfritzsche, data-r=bsmedberg
Comment 33•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/46645a6bb7dc
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•