Alessio Placitelli [:Dexter] Reporter
	Description • 9 years ago

Server is receiving weird client IDs in pings.

We're not currently validating the ClientId value on the client. We should probably do that both when loading the client id from file [1] and when loading it from the pref cache [2].

[1] - https://hg.mozilla.org/mozilla-central/annotate/e7434cafdf2f/toolkit/modules/ClientID.jsm#l71
[2] - https://hg.mozilla.org/mozilla-central/annotate/8e5c888d0d89/toolkit/components/telemetry/TelemetryController.jsm#l665

	Georg Fritzsche [:gfritzsche]
	Comment 1 • 9 years ago

Per IRC discussion:
* we already assume the client id is always in UUID format
* we should
  * sanity check the cached client id in the prefs for that, reset if needed
  * overwrite the cached client id from the state-file each session
  * validate the uuid format of the client id in the state file when loading, resetting if needed

Most of that should already happen, lets verify it does and fix where not.

	Georg Fritzsche [:gfritzsche]
	Comment 2 • 9 years ago

There are two main parts to this:
* when loading it from the state file (http://mzl.la/1MIkJAX):
  * we need to validate its in uuid format (regex)
  * if not, we need to generate a new uuid and save/overwrite the state file
* when accessing the cached client id (http://mzl.la/1hVHp5F):
  * we need to verify the cached id is in uuid format
  * if not, reset the caching pref and return null

	Georg Fritzsche [:gfritzsche]
	Comment 3 • 9 years ago

This has test coverage that we should extend to cover this:
https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_client_id.js

	Georg Fritzsche [:gfritzsche]
	Comment 4 • 9 years ago

Iaroslav, would you be interested in taking this?

	Iaroslav Sheptykin Assignee
	Comment 5 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Iaroslav, would you be interested in taking this?

Hi Georg! Sounds like an interesting project thanks for sharing. I will take a closer look on this next couple of days.

	Iaroslav Sheptykin Assignee
	Comment 6 • 9 years ago

Hi Guys!

Sorry for the delay, I have little free time lately. Please checkout my first WIP patch to make sure that I am moving in the right direction. I developed the test_client_id to try different bad client IDs (besides -1). I also added a simple Regex test to the setter of _clientID in order to prevent bad IDs from being used. What do you think about it?

Alessio, can you give me some examples of weird uuid that you are receiving on the server? I would like to test against some real data.

Taking this bug because it seems doable. However as I said earlier I don't have much time lately. So if this has to land quickly let me know, or feel free to reassign.

	Alessio Placitelli [:Dexter] Reporter
	Comment 7 • 9 years ago

Comment on attachment 8658636 [details] [diff] [review]
WIP Part 1: Validate the ClientID on the client.

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

Hi Iaroslav! This looks good to me, there are only a few nits and a suggestion.

I would also say to check that we get a valid client ID from the pref cache here: https://dxr.mozilla.org/mozilla-central/rev/01ae99b53561a2c3b40533d8c1c92bd3efc42d00/toolkit/modules/ClientID.jsm#164

Also, please note that the patch file has an (undesired?) additional comment.

"#***
#added a const"

::: toolkit/modules/ClientID.jsm
@@ +58,5 @@
>    },
>  });
>  
>  let ClientIDImpl = {
> +  __clientID: null,

Nit: I would rather have no "_" than two "__" here. All in all, stuff in ClientIDImpl is already "private" and just exposed by the object above.

@@ +66,5 @@
> +  },
> +
> +  set _clientID(value) {
> +    if (value !== null && !this.isValidID(value))
> +      throw "Invalid client ID (" + value + ")";

Please wrap this line with {} to match the local style (and the style guide https://developer.mozilla.org/en-US/docs/MDN/Contribute/Content/Style_guide ).

@@ +184,5 @@
>      this._clientID = null;
>    }),
> +
> +  /**
> +   * Checkes if client ID.has a valid format. Current implementation tests

Nit: Checkes vs checks

I would drop the part describing the implementation, and just reduce this sentence to "Checks if the client ID has a valid format."

@@ +187,5 @@
> +  /**
> +   * Checkes if client ID.has a valid format. Current implementation tests
> +   * the ID against the UUID pattern.
> +   *
> +   * @param clietID String  a string containing the client ID.

Nit: Since we're using jsdoc here (even though it's not mandatory! ;-) ), the @param should be documented as described in http://usejsdoc.org/tags-param.html

So "@param {String} id The client id to validate".

Same for "@return {Boolean} True when the client ID has a valid format, False otherwise."

	Iaroslav Sheptykin Assignee
	Comment 8 • 9 years ago

Thanks for the quick reply, :dexter! I updated the patch according to your comment 7. How does it look now? To my current (very limited) understanding of Telemetry code this should be enough for fixing this bug. Is it?

	Georg Fritzsche [:gfritzsche]
	Comment 9 • 9 years ago

Comment on attachment 8659843 [details] [diff] [review]
Validate the ClientID on the client

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

::: toolkit/modules/ClientID.jsm
@@ +64,5 @@
> +  get _clientID() {
> +    return this.clientID;
> +  },
> +
> +  set _clientID(value) {

How about we just leave the property as _clientID, lose the getter and make the setter into a function, say |updateClientID(id)|?

@@ +65,5 @@
> +    return this.clientID;
> +  },
> +
> +  set _clientID(value) {
> +    if (value !== null && !this.isValidID(value)) {

The additional null check seems redundant, isValidID() should handle that gracefully.

@@ +66,5 @@
> +  },
> +
> +  set _clientID(value) {
> +    if (value !== null && !this.isValidID(value)) {
> +      throw "Invalid client ID (" + value + ")";

I don't think this should throw (i.e. potentially break calling code if the client id is bad), i'd rather have us log an error here.

@@ +68,5 @@
> +  set _clientID(value) {
> +    if (value !== null && !this.isValidID(value)) {
> +      throw "Invalid client ID (" + value + ")";
> +    }
> +    this.clientID = value;

While we're here, we could move updating the preference here too (|Preferences.set(PREF_CACHED_CLIENTID, ...)|).

@@ +173,5 @@
>      }
>  
>      // Not yet loaded, return the cached client id if we have one.
> +    let id = Preferences.get(PREF_CACHED_CLIENTID, null);
> +    if (id !== null && !this.isValidID(id)) {

Ditto for the null-check.

@@ +190,5 @@
>      this._clientID = null;
>    }),
> +
> +  /**
> +   * Checks if client ID.has a valid format.

Nit: "client ID has"

@@ +196,5 @@
> +   * @param {String} id A string containing the client ID.
> +   * @return {Boolean} True when the client ID has valid format, or False
> +   * otherwise.
> +   */
> +  isValidID: function(id) {

I think this will stay private. Consequently, this can be a normal function, no need to make it a member.

::: toolkit/modules/tests/xpcshell/test_client_id.js
@@ +21,5 @@
>    const drsPath = OS.Path.join(OS.Constants.Path.profileDir, "datareporting", "state.json");
>    const fhrDir  = OS.Path.join(OS.Constants.Path.profileDir, "healthreport");
>    const fhrPath = OS.Path.join(fhrDir, "state.json");
>    const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
> +  const invalidIDs = [-1, 0.5, "INVALID-UUID"];

Let's add e.g.: true, "", "3d1e1560-682a-4043-8cf2-aaaaaaaaaaaZ"

@@ +82,5 @@
> +    Assert.equal(typeof(clientID), 'string');
> +    Assert.ok(uuidRegex.test(clientID));
> +  }
> +
> +  // Assure that cached IDs are being chedked for validity.

"checked"

	Iaroslav Sheptykin Assignee
	Comment 10 • 9 years ago

Hi Georg! Thanks for the detailed review! Here is the new version of the patch.

	Georg Fritzsche [:gfritzsche]
	Comment 11 • 9 years ago

Comment on attachment 8661353 [details] [diff] [review]
Validate the ClientID on the client

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

Thanks, this is nearly there :)
Only some smaller change requests below.

::: toolkit/modules/ClientID.jsm
@@ +65,5 @@
> + * @param {String} id A string containing the client ID.
> + * @return {Boolean} True when the client ID has valid format, or False
> + * otherwise.
> + */
> +function isValidID(id) {

Nit: isValidClientID()

@@ +67,5 @@
> + * otherwise.
> + */
> +function isValidID(id) {
> +  const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
> +  return id === null || UUID_REGEX.test(id);

Nit: id could be undefined too etc., `return id || UUID_REGEX.test(id)`?
Although regex.test(null) etc. should be handled fine (returning false), so i don't think we need that additional check.

@@ +75,5 @@
>    _clientID: null,
> +
> +  updateClientID: function (value){
> +    if (!isValidID(value)) {
> +      Console.error("Invalid client ID (" + value + ")");

I would prefer us to use `Log.error()` here.
We can set up a logger which inherits the Telemetry log settings like this:
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/toolkit/components/telemetry/TelemetryStorage.jsm#576

You can see sample calls in that same file.

@@ +101,5 @@
>      // we first moved the ID management from the FHR implementation to the datareporting
>      // service, then to a common shared module.
>      // Consequently, we try to import an existing FHR ID, so we can keep using it.
>  
> +    let stateHasValidClientID = state => {

Nit: let hasValidClientID ... ?

@@ +116,1 @@
>          Preferences.set(PREF_CACHED_CLIENTID, this._clientID);

As mentioned, lets make updateClientID() set the preference too.
Then this could shrink to (similarly below):
let state = ...
if (this.updateClientID(state)) {
  return this._clientID;
}

@@ +202,5 @@
>     */
>    _reset: Task.async(function* () {
>      yield this._loadClientIdTask;
>      yield this._saveClientIdTask;
> +    this.updateClientID(null);

Doesn't this get rejected by the isValidID() check in updateClientID()?

I don't think this will update this._clientID properly, we probably have to keep doing this manually here.

	Iaroslav Sheptykin Assignee
	Comment 12 • 9 years ago

Hi Georg!

Thanks for the review! Here is a new version of the patch. Please check if I used the Log in a way you meant it.

I also submitted it to the tryserver because it seems were approaching the final version. Here is the treeherder link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0faa2e9c0c73

	Georg Fritzsche [:gfritzsche]
	Comment 13 • 9 years ago

Comment on attachment 8663407 [details] [diff] [review]
Validate the ClientID on the client

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

Thanks, that looks good with the below fixed!
Let me know if anything is unclear about it.

::: toolkit/modules/ClientID.jsm
@@ +14,5 @@
>  Cu.import("resource://gre/modules/Preferences.jsm");
> +Cu.import("resource://gre/modules/Log.jsm");
> +
> +const LOGGER_NAME = "Toolkit.Telemetry";
> +const LOGGER_PREFIX = "TelemetryStorage::";

... = "ClientID::";

@@ +173,5 @@
>      }
>  
>      // Not yet loaded, return the cached client id if we have one.
> +    let id = Preferences.get(PREF_CACHED_CLIENTID, null);
> +    if (!isValidClientID(id)) {

Let's log an error here:
this._log.error("getCachedClientID - invalid client id in preferences, resetting", id);

@@ +199,5 @@
> +   * otherwise.
> +   */
> +  updateClientID: function (id){
> +    if (!isValidClientID(id)) {
> +      this._log.error("Invalid client ID (" + id + ")");

this._log.error("updateClientID - invalid client ID", id);

... that automatically turns into something like:
Toolkit.Telemetry   ERROR   ClientID::updateClientID - invalid client ID: fubar

@@ +202,5 @@
> +    if (!isValidClientID(id)) {
> +      this._log.error("Invalid client ID (" + id + ")");
> +      return false;
> +    }
> +    this._clientID = id;

Nit: empty line before this.

	Iaroslav Sheptykin Assignee
	Comment 14 • 9 years ago

Moin :) Georg!

Thanks for the feedback! I updated the patch. The tests look suspicious. The failures seem unrelated to my unexperienced eye but there are many of those. Should I redo tests?

	Georg Fritzsche [:gfritzsche]
	Comment 15 • 9 years ago

The test failures look unrelated, i don't think we need to block on them here.

	Iaroslav Sheptykin Assignee
	Comment 16 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> The test failures look unrelated, i don't think we need to block on them
> here.

Are we ready for [checkin-needed]?

	Georg Fritzsche [:gfritzsche]
	Comment 17 • 9 years ago

Yes, thats what i mean :)

	Iaroslav Sheptykin Assignee
	Comment 18 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> Yes, thats what i mean :)

Thanks, Georg! Anything else I could do for telemetry?

	Pulsebot
	Comment 19 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/dac49361fae2

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/dac49361fae2