Telemetry for percentage of password fields on safe/unsafe pages

RESOLVED FIXED in Firefox 43

Status

()

Core
Security
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tanvi, Assigned: kmckinley)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 13 obsolete attachments)

3.46 KB, patch
MattN
: review+
Details | Diff | Splinter Review
Quite a bit simpler than https://bugzilla.mozilla.org/show_bug.cgi?id=1150605, we should record what percentage of password fields are detected over HTTP.

Add a telemetry probe in https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm#78 _checkIfURIisSecure, add a telemetry probe that reports
1) URL is safe
2) URL is unsafe

We could dig a little deeper and actually check HTTP vs HTTPS by adding a couple more buckets:
1) URL is safe and is HTTPS
2) URL is safe and is not HTTPS
3) URL is not safe and is HTTP
4) URL is not safe and is not HTTP.

Buckets 2 and 4 will have few entries.

Alternatively, we could just report for buckets 1 and 3.

This can help us in making decisions about whether we should interrupt users when they are going to enter credentials on an HTTP page.  See bug 748193 and bug 1135766.

(Not sure if this should be filed under Security or Password Manager)
(Assignee)

Updated

3 years ago
Assignee: nobody → kmckinley
(Assignee)

Comment 1

3 years ago
Created attachment 8640697 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP
Comment on attachment 8640697 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP

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

::: toolkit/components/telemetry/Histograms.json
@@ +8343,5 @@
>      "expires_in_version": "never",
>      "kind": "boolean",
>      "description": "Whether a saved login has a username"
>    },
> +  "PWMGR_PASSWORD_FIELD_ON_SECURE_PAGE": {

Drive-by nit: the PWMGR_ probes are currently in alphabetical order.
(Assignee)

Comment 3

3 years ago
Created attachment 8640723 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP
(Assignee)

Comment 4

3 years ago
(In reply to Matthew N. [:MattN] from comment #2)
> Comment on attachment 8640697 [details] [diff] [review]
> Telemetry for percentage of password fields over HTTP
> 
> Review of attachment 8640697 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +8343,5 @@
> >      "expires_in_version": "never",
> >      "kind": "boolean",
> >      "description": "Whether a saved login has a username"
> >    },
> > +  "PWMGR_PASSWORD_FIELD_ON_SECURE_PAGE": {
> 
> Drive-by nit: the PWMGR_ probes are currently in alphabetical order.

fixed
I'm not sure if you're wanting a review? If so, you need to flag/request that from someone. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Attachment #8640723 - Flags: review?(tanvi)
(Reporter)

Updated

3 years ago
Attachment #8640697 - Attachment is obsolete: true
Comment on attachment 8640723 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP

Hi Kate!  Thanks for this patch!

+    const PWMGR_PASSWORD_FIELD_ON_SECURE_PAGE = Services.telemetry.getHistogramById("PWMGR_PASSWORD_FIELD_ON_SECURE_PAGE");

Can you add a comment above here that is similar to your comment in Histograms.json.

Also, I'm not a toolkit peer so we have to get a second review.  Since Matt has been doing many password manager reviews, I think he can review this.


# Parent  2ee9895e032c492705adaf213706d4260ca172c8
Bug 1174333 - Telemetry for percentage of password fields over HTTP

Before you land the patch, add an "r=tanvi, MattN" to the end of the file.
Attachment #8640723 - Flags: review?(tanvi)
Attachment #8640723 - Flags: review?(MattN+bmo)
Attachment #8640723 - Flags: review+
Comment on attachment 8640723 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP

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

It doesn't seem right to me to be doing this telemetry in _checkIfURIisSecure since it can be called multiple times while checking a single form (with different results) if there is a subframe.

On a related note which doesn't need to be fixed in this bug but will affect data: InsecurePasswordUtils.jsm should consider handling <input type=password> outside of <form>. On the other hand, it seems like to answer the question from comment 0, it would be best to record telemetry once per page. Even better, don't we (also) want to know when users actually interact with/submit the insecure form? We should consider recording this telemetry in a submit handler instead.

Note that before landing you will also need privacy review. See the red box at the top of https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +87,5 @@
>  
>        isSafe = true;
>      }
>  
> +    const PWMGR_PASSWORD_FIELD_ON_SECURE_PAGE = Services.telemetry.getHistogramById("PWMGR_PASSWORD_FIELD_ON_SECURE_PAGE");

Nit: I think this line is >100 characters so it should be split or wrapped. Perhaps use a shorter variable name?

@@ +90,5 @@
>  
> +    const PWMGR_PASSWORD_FIELD_ON_SECURE_PAGE = Services.telemetry.getHistogramById("PWMGR_PASSWORD_FIELD_ON_SECURE_PAGE");
> +    let telemetryValue = 0;
> +    if(isSafe) {
> +	if(!uri.schemeIs("https") {

Nit: spaces after `if` please (x3). See https://developer.mozilla.org/en-US/docs/User%3AGavinSharp_JS_Style_Guidelines and https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: toolkit/components/telemetry/Histograms.json
@@ +8320,5 @@
> +  "PWMGR_PASSWORD_FIELD_ON_SECURE_PAGE": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 4,
> +    "description": "Whether a password field is presented on an secure page. (0=safe and https, 1=safe and not https, 2=unsafe and http, 3=unsafe and not http)"

I find this description/probe confusing because:
a) the name of the probe and first sentence (with "whether") imply it's a boolean probe.
b) the terms "secure", "safe", and "https" can all be confused.
c) the probe is looking at forms, not fields.

If we end up measure this similar to how this patch is I think a better name would be PWMGR_LOGIN_FORM_SECURITY.
Attachment #8640723 - Flags: review?(MattN+bmo) → review-
(Assignee)

Comment 8

3 years ago
Created attachment 8641361 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP
(Assignee)

Updated

3 years ago
Attachment #8640723 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8641362 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP
(Assignee)

Updated

3 years ago
Attachment #8641361 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8641362 - Flags: review?(tanvi)
Attachment #8641362 - Flags: review?(MattN+bmo)
Attachment #8641362 - Flags: feedback?(ally)
Comment on attachment 8641362 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP

+    // The security status of the page where a password field is shown, as a bitfield.
+    //   Bit 0001: safe/unsafe page
+    //   Bit 0010: https/http
+    //   Bit 0100: form action https/http
+    //   Bit 1000 secure/insecure nested document.

Expand on the comments here
+    //   Bit 0001: set if unsafe page, not set if safe page
+    //   Bit 0010: set if page is not https or http, otherwise not set
+    //   Bit 0100: set if form action is http, otherwise not set
+    //   Bit 1000 set if password field is present in an insecure iframe, otherwise not set

+    if (!uri.schemeIs("https") {
+      telemetryValue += 2;
+    }
This check needs to be expanded.  What we want to know is if the scheme is http/https or if it is some other protocol entirely.  So you need to add a check for !uri.schemeIs("http").  This also only checks the top level page uri and the information is not helpful in the 10X0 or the 11X0 states.


Although this method provides more information, it is going to be a lot more difficult to interpret.

What we are mostly interested in is if a password field is on an http page.  To determine this, you'd need to take the sum of values reported for:
0001, 0101, 1000, 1100, 1001, 1101

And compare it against the sum of the rest of the reported values.  And I'm not sure if I even got that right ;)

I'm going to map what we initially wanted to measure with the reported values:
1) URL is safe and is HTTPS - 0000, 0100
2) URL is safe and is not HTTPS - 0010, 0110
3) URL is not safe and is HTTP - 0001, 0101, 1000, 1100, 1001, 1101
4) URL is not safe and is not HTTP - 1011, 1111, 1010, 1110, 0011, 0111

More information is good if we have a use for it.  In this case, I'm not sure we need the extra complexity.  I do see the benefits in moving the telemetry from checkIfURIisSecure (from the previous patch) to checkForInsecurePasswords.
Attachment #8641362 - Flags: review?(tanvi) → review-
Comment on attachment 8641362 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP

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

I agree with tanvi about the complexity

::: toolkit/components/telemetry/Histograms.json
@@ +8309,5 @@
>      "high": 750,
>      "n_buckets" : 50,
>      "description": "Number of HTTP Auth logins"
>    },
> +  "PWMGR_LOGIN_FORM_SECURITY": {

Nit: re-sort this now that it's renamed :P
Attachment #8641362 - Flags: review?(MattN+bmo) → feedback+
Am I flagged for password review or privacy review for this?
(Assignee)

Comment 13

3 years ago
Created attachment 8641869 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP
(Assignee)

Updated

3 years ago
Attachment #8641362 - Attachment is obsolete: true
Attachment #8641362 - Flags: feedback?(ally)
(Assignee)

Comment 14

3 years ago
Back to the original version, but with the telemetry moved into checkForInsecurePasswords.

Allison, I am requesting privacy review for the telemetry.
(Assignee)

Updated

3 years ago
Attachment #8641869 - Flags: review?(tanvi)
Attachment #8641869 - Flags: review?(MattN+bmo)
Attachment #8641869 - Flags: feedback?(ally)
Comment on attachment 8641869 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP


     if (this._checkForInsecureNestedDocuments(domDoc)) {
       this._sendWebConsoleMessage("InsecurePasswordsPresentOnIframe", domDoc);
     }

add isSafePage = false in this if statement.  That way we capture http frames that ask for passwords as well as http top level pages that ask for passwords.

Then for:
+    // 0=safe page;1=safe page, not https;2=unsafe page;3=unsafe page, not http

you can change this to

+    // 0=safe page; 1=safe page, top level not https; 2=unsafe page; 3=unsafe page, top level not http
(with spaces between the semicolons please).  And same below in Histograms.json?



+  "PWMGR_LOGIN_FORM_SECURE": {
+    "expires_in_version": "never",
+    "kind": "enumerated",
+    "n_values": 16,
There are only 4 values now.  We could set it to something higher than 4 if we think we will want to add to the probe later and want to provide some flexibility.  Maybe set it to 8?  Matt, thoughts?
Attachment #8641869 - Flags: review?(tanvi)
(Assignee)

Comment 16

3 years ago
Created attachment 8641966 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP
(Assignee)

Updated

3 years ago
Attachment #8641869 - Attachment is obsolete: true
Attachment #8641869 - Flags: review?(MattN+bmo)
Attachment #8641869 - Flags: feedback?(ally)
Comment on attachment 8641966 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP

page; 1=safe page, top level not https; 2=unsafe page; 3=unsafe page, top level not http"

Hi Kate,
Sorry for so many changes!  I thought about this more and think we should change the description as follows, in attempt to make it more clear

+    "description": "The safety of a page where we see a password field. 0=safe https page; 1=safe page with top level non-https; 2=unsafe http page; 3=unsafe page with top level non-http"

Please apply this change to both Histograms.json and InsecurePasswordUtils.jsm.

Thank you!
(Assignee)

Comment 18

3 years ago
Created attachment 8641975 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP
(Assignee)

Updated

3 years ago
Attachment #8641966 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8641975 - Flags: review?(tanvi)
Attachment #8641975 - Flags: review?(MattN+bmo)
Comment on attachment 8641975 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP

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

* Note that bug 983326 may skew this probe's data (mostly if about:accounts is still affected).
* I filed bug 1191092 for handling <form>-less fields

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +141,4 @@
>      }
>  
>      if (aForm.action.match(/^http:\/\//)) {
>        this._sendWebConsoleMessage("InsecureFormActionPasswordsPresent", domDoc);

So we don't want this probe to include insecure actions, just the security of the document(s)? Just double-checking.

@@ +148,5 @@
> +    // 0=safe page; 1=safe page, top level not https;
> +    // 2=unsafe page; 3=unsafe page, top level not http
> +    let telemetryValue = 0;
> +    if(isSafePage) {
> +      if(!uri.schemeIs("https") {

Nit: missing space after `if` (3x)

@@ +149,5 @@
> +    // 2=unsafe page; 3=unsafe page, top level not http
> +    let telemetryValue = 0;
> +    if(isSafePage) {
> +      if(!uri.schemeIs("https") {
> +         telemetryValue = 1;

What's an example where we would record "1"? It seems like those aren't important and are maybe "secure" for all intents and purposes? e.g. about: pages. i.e. I'm not sure what we will learn by keeping this bucket separate from 0 and I find the complexity of this probe confusing already.

@@ +153,5 @@
> +         telemetryValue = 1;
> +      }
> +    } else {
> +      telemetryValue = 2;
> +      if(!uri.schemeIs("http") {

`uri` is undefined (2x) so I guess you didn't manually test this after moving it? You can do a manual verification using about:telemetry. I think you want pageURI now?

Side note: This function is very hard to read (and causes me to delay review) since the variable names aren't descriptive. I have to keep double-checking whether `isSafePage` and `pageURI`/`uri` are referring to the top-level or the form's document.

@@ +154,5 @@
> +      }
> +    } else {
> +      telemetryValue = 2;
> +      if(!uri.schemeIs("http") {
> +        telemetryValue = 3;

So we record a value of "3" if a form is in an HTTP frame inside an HTTP top-level document? Should that be a 2 or 3?

Taking a step back: I guess I don't understand why we are looking at the scheme at all? Sorry for all the questions but this bug is doing more than just what the title says.
Attachment #8641975 - Flags: review?(MattN+bmo) → feedback+
(Assignee)

Comment 20

3 years ago
Created attachment 8644021 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP
(Assignee)

Updated

3 years ago
Attachment #8641975 - Attachment is obsolete: true
Attachment #8641975 - Flags: review?(tanvi)
(Assignee)

Comment 21

3 years ago
Created attachment 8644025 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP
(Assignee)

Updated

3 years ago
Attachment #8644021 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Depends on: 983326, 1191092
(Assignee)

Comment 22

3 years ago
(In reply to Matthew N. [:MattN] from comment #19)
> Comment on attachment 8641975 [details] [diff] [review]
> Telemetry for percentage of password fields over HTTP
> 
> Review of attachment 8641975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> * Note that bug 983326 may skew this probe's data (mostly if about:accounts
> is still affected).
> * I filed bug 1191092 for handling <form>-less fields
> 
> ::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
> @@ +141,4 @@
> >      }
> >  
> >      if (aForm.action.match(/^http:\/\//)) {
> >        this._sendWebConsoleMessage("InsecureFormActionPasswordsPresent", domDoc);
> 
> So we don't want this probe to include insecure actions, just the security
> of the document(s)? Just double-checking.

Exactly. The purpose of this probe is to understand when the user is presented a password field, whether that is presented on a page we consider safe.
 
> @@ +149,5 @@
> > +    // 2=unsafe page; 3=unsafe page, top level not http
> > +    let telemetryValue = 0;
> > +    if(isSafePage) {
> > +      if(!uri.schemeIs("https") {
> > +         telemetryValue = 1;
> 
> What's an example where we would record "1"? It seems like those aren't
> important and are maybe "secure" for all intents and purposes? e.g. about:
> pages. i.e. I'm not sure what we will learn by keeping this bucket separate
> from 0 and I find the complexity of this probe confusing already.

We would record a 1 when the page is considered safe and is an about:, or local (file:///) URL, for example. It is unlikely to be a high number.

> 
> @@ +153,5 @@
> > +         telemetryValue = 1;
> > +      }
> > +    } else {
> > +      telemetryValue = 2;
> > +      if(!uri.schemeIs("http") {
> 
> `uri` is undefined (2x) so I guess you didn't manually test this after
> moving it? You can do a manual verification using about:telemetry. I think
> you want pageURI now?
> 
> Side note: This function is very hard to read (and causes me to delay
> review) since the variable names aren't descriptive. I have to keep
> double-checking whether `isSafePage` and `pageURI`/`uri` are referring to
> the top-level or the form's document.

It always refers to the form's document. _checkForInsecureNestedDocuments performs the check for top-level documents.

> 
> @@ +154,5 @@
> > +      }
> > +    } else {
> > +      telemetryValue = 2;
> > +      if(!uri.schemeIs("http") {
> > +        telemetryValue = 3;
> 
> So we record a value of "3" if a form is in an HTTP frame inside an HTTP
> top-level document? Should that be a 2 or 3?

We would return a 3 if the form is in an HTTPS document that we consider unsafe, or in an about: page that is not considered safe.

> Taking a step back: I guess I don't understand why we are looking at the
> scheme at all? Sorry for all the questions but this bug is doing more than
> just what the title says.

Changed title to a match more closely with the provided user story.
(Assignee)

Updated

3 years ago
Summary: Telemetry for percentage of password fields over HTTP → Telemetry for percentage of password fields on safe/unsafe pages
(Assignee)

Updated

3 years ago
Attachment #8644025 - Flags: feedback?(MattN+bmo)
Comment on attachment 8641975 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP

Thinking about this more, if we aren't worried about "http vs unsafe" and "https vs safe", then we can easily check the form action.  Something like:


      bool isFormSubmitHTTP = false;
>     if (aForm.action.match(/^http:\/\//)) {
>       this._sendWebConsoleMessage("InsecureFormActionPasswordsPresent", domDoc);
        isFormSubmitHTTP = true;
>     }
>+
>+    // The safety of a page where we see a password field.
>+    // 0=safe page, 1=safe page and unsafe form submit
>+    // 2=unsafe page, 3=unsafe page and unsafe form submit
>+    let telemetryValue;
>+    if(isSafePage) {
>+         telemetryValue = 0;
           if (isFormSubmitHTTP) {
             telemetryValue = 2;
           }
>+    } else {
>+      telemetryValue = 2;
        if (isFromSubmitHTTP) {
           telemetryValue=3;
        }
>+    }

Kate, if you prefer to safe formSubmit for another bug and just change this one to use 2 buckets (one for safe and one for unsafe), that's okay too.
Sorry, I had a midair collision I didn't notice until it was too late.  This below comment should go before comment 23:
(In reply to Matthew N. [:MattN] from comment #19)
> Comment on attachment 8641975 [details] [diff] [review]
> Telemetry for percentage of password fields over HTTP
> 
> Review of attachment 8641975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> * Note that bug 983326 may skew this probe's data (mostly if about:accounts
> is still affected).
> * I filed bug 1191092 for handling <form>-less fields
> 
> ::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
> @@ +141,4 @@
> >      }
> >  
> >      if (aForm.action.match(/^http:\/\//)) {
> >        this._sendWebConsoleMessage("InsecureFormActionPasswordsPresent", domDoc);
> 
> So we don't want this probe to include insecure actions, just the security
> of the document(s)? Just double-checking.
> 
Yes, for now this is fine.  We really want to start by knowing if the top level page is https.  We can add a second probe for insecure forms.
1) http form post on https page
2) http form post on http page
3) https form post on https page
4) https form post on http page
5) other
But this can be done in a separate bug.


> @@ +148,5 @@
> > +    // 0=safe page; 1=safe page, top level not https;
> > +    // 2=unsafe page; 3=unsafe page, top level not http
> > +    let telemetryValue = 0;
> > +    if(isSafePage) {
> > +      if(!uri.schemeIs("https") {
> 
> Nit: missing space after `if` (3x)
> 
> @@ +149,5 @@
> > +    // 2=unsafe page; 3=unsafe page, top level not http
> > +    let telemetryValue = 0;
> > +    if(isSafePage) {
> > +      if(!uri.schemeIs("https") {
> > +         telemetryValue = 1;
> 
> What's an example where we would record "1"? It seems like those aren't
> important and are maybe "secure" for all intents and purposes? e.g. about:
> pages. i.e. I'm not sure what we will learn by keeping this bucket separate
> from 0 and I find the complexity of this probe confusing already.
> 
It separates HTTPS safe pages from non-HTTPS (like about:) safe pages.

> @@ +154,5 @@
> > +      }
> > +    } else {
> > +      telemetryValue = 2;
> > +      if(!uri.schemeIs("http") {
> > +        telemetryValue = 3;
> 
> So we record a value of "3" if a form is in an HTTP frame inside an HTTP
> top-level document? Should that be a 2 or 3?
> 
Ah, I didn't think of that case.  I was thinking that a value of "3" would be an unsafe page that is not using http (so ftp for example).  But you are right, if the top level is https and it has an http frame with a password field, it would fall into category 3.  Which is not what I intended.

> Taking a step back: I guess I don't understand why we are looking at the
> scheme at all? Sorry for all the questions but this bug is doing more than
> just what the title says.

Okay, then it that case we can just go with 2 buckets to reduce complexity.

1) URL is safe
2) URL is not safe

I will comment on the patch as well.
(In reply to Tanvi Vyas [:tanvi] from comment #24)
> I will comment on the patch as well.

I was waiting on this before doing my feedback pass…
Flags: needinfo?(tanvi)
(In reply to Matthew N. [:MattN] from comment #25)
> (In reply to Tanvi Vyas [:tanvi] from comment #24)
> > I will comment on the patch as well.
> 
> I was waiting on this before doing my feedback pass…


I believe this is comment 23.  I also talked to Kate over irc and I think she is planning to update the patch again.  She is out this week though.
Flags: needinfo?(tanvi)
(Assignee)

Comment 27

3 years ago
Created attachment 8647724 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP
(Assignee)

Updated

3 years ago
Attachment #8644025 - Attachment is obsolete: true
Attachment #8644025 - Flags: feedback?(MattN+bmo)
(Assignee)

Comment 28

3 years ago
This version is as discussed above. There are two values, 0 if the page the field appears on is safe and 1 if it is unsafe.

Bug 1194352 created for whether the form action is http/https/other
(Assignee)

Updated

3 years ago
Attachment #8647724 - Flags: review?(tanvi)
Attachment #8647724 - Flags: review?(MattN+bmo)
Comment on attachment 8647724 [details] [diff] [review]
Telemetry for percentage of password fields over HTTP

# HG changeset patch
# User Kate McKinley <kmckinley@mozilla.com>
# Date 1439499681 25200
#      Thu Aug 13 14:01:21 2015 -0700
# Node ID b0dba07d4847bf0b5e86c4924e5aac1d57fd86d6
# Parent  2ee9895e032c492705adaf213706d4260ca172c8
Bug 1174333 - Telemetry for percentage of password fields over HTTP

When you are ready to land, add r=MattN,tanvi at the end of this patch comment.

>     if (aForm.action.match(/^http:\/\//)) {
>       this._sendWebConsoleMessage("InsecureFormActionPasswordsPresent", domDoc);
>     }
>+
>   },

We can remove this extra line.

I think we should do https://bugzilla.mozilla.org/show_bug.cgi?id=1194352 quickly after this to update this probe, instead of adding a second probe.
Attachment #8647724 - Flags: review?(tanvi) → review+
(Reporter)

Updated

3 years ago
Blocks: 1194352
(Assignee)

Comment 30

3 years ago
Created attachment 8648152 [details] [diff] [review]
Telemetry for percentage of password fields on safe/unsafe pages
(Assignee)

Updated

3 years ago
Attachment #8647724 - Attachment is obsolete: true
Attachment #8647724 - Flags: review?(MattN+bmo)
(Assignee)

Updated

3 years ago
Flags: needinfo?(ally)
Welcome to Mozilla & your first Telemetry bug! http://mozillamemes.tumblr.com/tagged/good-first-bug

Since you have cleared the review flag for MattN, I assume this means a new patch will be along shortly. Data Collection review works the same way regular engineering review works (bugzilla allows you to flag multiple reviewers) and would go on your patch as "r=MattN, p=ally". 

Data Review of old(?) patch
- "PWMGR_LOGIN_LAST_USED_DAYS" has expiration:never, which is no longer accepted. I see that you are just moving the block around, but please update that to 55.

- n_values cannot be changed after you land the probe, as the data structure on the server will be set. You probably do want something like 8. Unused buckets effectively incur no cost.

- MattN has already brought up things that might skew your data, so you (the group) are already aware of them.

Please flag me with r? When you have a patch you want to land! You're almost there! :D
Flags: needinfo?(ally)
(Assignee)

Comment 32

3 years ago
Created attachment 8648257 [details] [diff] [review]
Telemetry for percentage of password fields on safe/unsafe pages
(Assignee)

Updated

3 years ago
Attachment #8648152 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8648257 - Flags: review?(ally)
Comment on attachment 8648257 [details] [diff] [review]
Telemetry for percentage of password fields on safe/unsafe pages

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

p=ally

be sure to get an r=MattN before landing.
Attachment #8648257 - Flags: review?(ally) → review+
(Assignee)

Updated

3 years ago
Attachment #8648257 - Flags: review?(MattN+bmo)
(Assignee)

Comment 34

3 years ago
Created attachment 8648296 [details] [diff] [review]
Telemetry for percentage of password fields on safe/unsafe pages
(Assignee)

Updated

3 years ago
Attachment #8648257 - Attachment is obsolete: true
Attachment #8648257 - Flags: review?(MattN+bmo)
(Assignee)

Comment 35

3 years ago
Created attachment 8648297 [details] [diff] [review]
Telemetry for percentage of password fields on safe/unsafe pages
(Assignee)

Updated

3 years ago
Attachment #8648296 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8648297 - Flags: review?(MattN+bmo)
Comment on attachment 8648297 [details] [diff] [review]
Telemetry for percentage of password fields on safe/unsafe pages

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

I made some tweaks since we've gone back and forth so many times and I'm sure you don't want to do so again. Thanks for your patience. I did wonder if we should have just made this a boolean probe but I think you were thinking we may want to add other options in the future.

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +143,5 @@
> +    // The safety of a page where we see a password field.
> +    // 0=safe page; 1=unsafe page
> +    let telemetryValue = (isSafePage) ? 0 : 1;
> +    let hist = Services.telemetry.getHistogramById("PWMGR_LOGIN_PAGE_SAFETY");
> +    hist.add(telemetryValue);

I simplified this into one line since it was more verbose than necessary and the variables names were generic and could conflict with the follow-up.

::: toolkit/components/telemetry/Histograms.json
@@ +8261,5 @@
> +  "PWMGR_LOGIN_PAGE_SAFETY": {
> +    "expires_in_version": "55",
> +    "kind": "enumerated",
> +    "n_values": 8,
> +    "description": "The safety of a page where we see a password field. 0: safe page; 1: unsafe page"

I reversed the values to be more like a boolean probe where "safety" is normally seen as a feature == on == 1.
Attachment #8648297 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/0d81c8f063dd
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.