Closed Bug 1345077 Opened 7 years ago Closed 7 years ago

Add telemetry on permission requests from third party origins, handling user input and insecure contexts

Categories

(Firefox :: Site Permissions, enhancement, P3)

Product:
Firefox
Component:
Site Permissions
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 files)

Bug 1345077 - Part 2 - Add telemetry on permission requests from third party origins, handling user input and insecure contexts.
59 bytes, text/x-review-board-request
florian
: review+
liuche
: review+
Details
Bug 1345077 - Part 1 - Add an isHandlingUserInput flag to WebRTC permission requests.
59 bytes, text/x-review-board-request
padenot
: review+
baku
: review+
Details 
With the upcoming importance of this topic, e.g. the changes in bug 1335155 that will restrict the way websites can use iframes to prompt again after being denied by the user and other considerations to limit iframe access to page permissions we should add some telemetry to make better decisions and measure our impact.

I'd like to find out:

- How big is the ratio of permission requests from iframes vs. top-level?
- Are there permission types that tend to be more or less often used in iframes? That means we probably have to key by permission type.

One thing that would have been interesting to know but is now probably moot due to the changes in bug 1206232 is if any iframes request a permission although the same permission was already requested by the parent page.
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Morphing this to cover what I'd like to find out:

- How big is the ratio of permission requests from third-party iframes vs. top-level?
- Are permission prompts requested from user input?
- How many permissions are requested from HTTPS?

All keyed by permission type.
Assignee: nobody → jhofmann
Blocks: 1375683
Status: NEW → ASSIGNED
Summary: Add telemetry on permission requests from iframes → Add telemetry on permission requests from third party origins, handling user input and insecure contexts
Attachment #8941567 - Flags: review?(liuche)
Attachment #8941567 - Flags: review?(florian)
Apologies, I just noticed that I was still missing the user handling input flag on WebRTC. I'll look into it.
Let me note that for WebRTC and Geolocation we already have a SECURE_ORIGIN probe which expires in 60, so I would make bugs to remove these probes once this gets merged.
Comment on attachment 8941567 [details]
Bug 1345077 - Part 2 - Add telemetry on permission requests from third party origins, handling user input and insecure contexts.

https://reviewboard.mozilla.org/r/211836/#review217758

Clearing the flag because I can't decide between r+ and r-, would be f+ on splinter ;).

::: browser/components/nsBrowserGlue.js:2806
(Diff revision 2)
>            Cr.NS_ERROR_FAILURE);
>        }
>  
>        permissionPrompt.prompt();
> +
> +      let schemeHistogram = Services.telemetry.getKeyedHistogramById("PERMISSION_REQUEST_ORIGIN_SCHEME");

nit: I would move each of these 3 variables to be the first line of the block of code using them.

::: browser/components/nsBrowserGlue.js:2811
(Diff revision 2)
> +      let schemeHistogram = Services.telemetry.getKeyedHistogramById("PERMISSION_REQUEST_ORIGIN_SCHEME");
> +      let thirdPartyHistogram = Services.telemetry.getKeyedHistogramById("PERMISSION_REQUEST_THIRD_PARTY_ORIGIN");
> +      let userInputHistogram = Services.telemetry.getKeyedHistogramById("PERMISSION_REQUEST_HANDLING_USER_INPUT");
> +
> +      let scheme = 0;
> +      if (request.principal.URI.scheme == "http") {

nit: I dislike the request.principal.URI.scheme duplication here. Maybe this would look better with a switch/case and 0 in the default: block?

::: browser/components/nsBrowserGlue.js:2816
(Diff revision 2)
> +      if (request.principal.URI.scheme == "http") {
> +        scheme = 1;
> +      } else if (request.principal.URI.scheme == "https") {
> +        scheme = 2;
> +      }
> +      schemeHistogram.add("(all)", scheme);

Is it useful to collect these '(all)' values? Naively, it seems computing them from the collected data should be easy.

::: browser/modules/ContentWebRTC.jsm:219
(Diff revision 2)
>    }
>    aContentWindow.pendingGetUserMediaRequests.set(aCallID, devices);
>  
> +  // Record third party origins for telemetry.
> +  let isThirdPartyOrigin =
> +    aContentWindow.document.location.origin == aContentWindow.top.document.location.origin;

Is this meant to be != rather than ==?

::: browser/modules/webrtcUI.jsm:845
(Diff revision 2)
>               .show(aBrowser, "webRTC-shareDevices", message,
>                     anchorId, mainAction, secondaryActions,
>                     options);
>    notification.callID = aRequest.callID;
> +
> +  let schemeHistogram = Services.telemetry.getKeyedHistogramById("PERMISSION_REQUEST_ORIGIN_SCHEME");

The comments I had in nsBrowserGlue.js apply here too.

::: browser/modules/webrtcUI.jsm:856
(Diff revision 2)
> +    scheme = 2;
> +  } else if (aRequest.documentURI.startsWith("http")) {
> +    scheme = 1;
> +  }
> +
> +  schemeHistogram.add("(all)", scheme);

Hmm, if we have a permission prompt for camera+microphone (which is probably the most common case), you record one entrie in "(all)" but you record once the camera request and once the microphone request. Is this intended? Won't it cause confusion that the (all) count doesn't match the sum of the other counts?

::: browser/modules/webrtcUI.jsm:860
(Diff revision 2)
> +
> +  schemeHistogram.add("(all)", scheme);
> +  thirdPartyHistogram.add("(all)", aRequest.isThirdPartyOrigin);
> +  userInputHistogram.add("(all)", aRequest.isHandlingUserInput);
> +
> +  if (requestTypes.includes("Microphone") || requestTypes.includes("AudioCapture")) {

You can reduce the duplication by doing something like:

for (let requestType of requestTypes) {
  if (requestType == "AudioCapture")
    requestType = "Microphone";
  requestType = requestType.toLowerCase();
  blahHistogram.add(requestType, ...
  ...
}
Attachment #8941567 - Flags: review?(florian)
Hi johannnh, can you fill out the data review request for the new probes and include it in a comment, and then re-flag me for data review? This is so we can document the planned usage and reasons for collecting this telemetry.

https://github.com/mozilla/data-review/blob/master/request.md
Flags: needinfo?(jhofmann)
Attachment #8941567 - Flags: review?(liuche)
1) What questions will you answer with this data?

Under what circumstances/in what way websites are requesting web permissions from users.

2) Why does Mozilla need to answer these questions?  Are there benefits for users? Do we need this information to address product or business requirements? Some example responses:

In the future we would like to make changes to improve our permission model to make it more user friendly and secure. For that we need to know how permissions are used "in the wild".

3) What alternative methods did you consider to answer these questions? Why were they not sufficient?

None.

4) Can current instrumentation answer these questions?

No

5) List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox [data c](https://wiki.mozilla.org/Firefox/Data_Collection)[ollection ](https://wiki.mozilla.org/Firefox/Data_Collection)[categories](https://wiki.mozilla.org/Firefox/Data_Collection) on the found on the Mozilla wiki.   

There are three measurements:

1) For a permission request, did it come from an HTTPS, HTTP or Other page?
2) Did the permission request originate in a third-party iframe?
3) Was the request initiated from user interaction?

I'd say conservatively these are all category 2 measurements.

6) How long will this data be collected?  Choose one of the following:

Until Firefox 64, see Histograms.json

7) What populations will you measure?

Everyone.

8) Please provide a general description of how you will analyze this data.

There is no current plan to analyze this data beyond looking at it.

9) Where do you intend to share the results of your analysis?

Maybe in a public blog post some day.
Flags: needinfo?(jhofmann)
> 8) Please provide a general description of how you will analyze this data.

> There is no current plan to analyze this data beyond looking at it.

Hi Johann, you need a reason to collect data, so this is not going to pass data review. This reason can be to test a specific hypothesis, or establish a baseline for a metric, to give some examples.

This also makes clear when we can stop collecting data on users, so we know when to remove a probe (even if the original person who adds the probe leaves, or changes projects, etc).

From out data policies:
https://wiki.mozilla.org/Firefox/Data_Collection

> Necessity - We collect only as much data as is necessary when we can demonstrate a clear business case for that data

Please give a clearer reason to collect usage data than "we want to look at it" :)
Flags: needinfo?(jhofmann)
Comment on attachment 8941567 [details]
Bug 1345077 - Part 2 - Add telemetry on permission requests from third party origins, handling user input and insecure contexts.

Please flag me for review again when ready.
Attachment #8941567 - Flags: review?(liuche)
Hi,

I'm probably misunderstanding the meaning of question 8). These are boolean values that should require no particular interpretation and I thought I mentioned the reason why we need this in question 2, but let me try wordsmithing this a little:

We will use these probes to determine whether certain security and privacy interventions (namely restricting certain permissions to HTTPS, restricting prompts to user-handling events only, or disallowing cross-origin permission prompts) will affect a large number of websites or not.

Chrome is leading the way with some important UX improvements around permission prompts and I am hesitant to commit to making similar changes without a good idea of how this will affect our user base.

Thanks.
Flags: needinfo?(jhofmann) → needinfo?(liuche)
Comment on attachment 8941567 [details]
Bug 1345077 - Part 2 - Add telemetry on permission requests from third party origins, handling user input and insecure contexts.

https://reviewboard.mozilla.org/r/211836/#review219368

::: browser/modules/webrtcUI.jsm:852
(Diff revision 3)
> +  let userInputHistogram = Services.telemetry.getKeyedHistogramById("PERMISSION_REQUEST_HANDLING_USER_INPUT");
> +
> +  let scheme = 0;
> +  if (aRequest.documentURI.startsWith("https")) {
> +    scheme = 2;
> +  } else if (aRequest.documentURI.startsWith("http")) {

nit: Put aRequest.documentURI in a variable?

::: browser/modules/webrtcUI.jsm:857
(Diff revision 3)
> +  } else if (aRequest.documentURI.startsWith("http")) {
> +    scheme = 1;
> +  }
> +
> +  for (let requestType of requestTypes) {
> +    if (requestType == "AudioCapture" ) {

nit: remove the space before ')'.
Attachment #8941567 - Flags: review?(florian) → review+
Comment on attachment 8941567 [details]
Bug 1345077 - Part 2 - Add telemetry on permission requests from third party origins, handling user input and insecure contexts.

https://reviewboard.mozilla.org/r/211836/#review219464

Data-review only

> Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate? (see here, here, and here for examples). Refer to the appendix for "documentation" if more detail about documentation standards is needed.

Yes, Histograms

> Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Firefox data controls

> If the request is for permanent data collection, is there someone who will monitor the data over time?**
Expires in 64, jhofmann

> Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? **
Type 2

> Is the data collection request for default-on or default-off?
on

> Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?
No

> Is the data collection covered by the existing Firefox privacy notice?
Yes

> Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**
Expires in 64
Attachment #8941567 - Flags: review+
Flags: needinfo?(liuche)
Attachment #8941656 - Flags: review?(padenot)
Comment on attachment 8941656 [details]
Bug 1345077 - Part 1 - Add an isHandlingUserInput flag to WebRTC permission requests.

https://reviewboard.mozilla.org/r/211902/#review219640
Attachment #8941656 - Flags: review?(padenot) → review+
Had some try failures related to testing code that mocked the permission request and used system principals for testing. It's good that these were failing because the probe code is more robust now, even if it's unlikely we would have caught this in production, I think.

I'll go ahead and land.
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 9 changes to 9 files
remote: 
remote: WebIDL file dom/webidl/GetUserMediaRequest.webidl altered in changeset 4ab4475d2fc6 without DOM peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote
Attachment #8941656 - Flags: review?(amarchesini)
It's an internal IDL file... :(
Comment on attachment 8941656 [details]
Bug 1345077 - Part 1 - Add an isHandlingUserInput flag to WebRTC permission requests.

https://reviewboard.mozilla.org/r/211902/#review220516

::: dom/media/GetUserMediaRequest.h:41
(Diff revision 2)
>    nsISupports* GetParentObject();
>  
>    uint64_t WindowID();
>    uint64_t InnerWindowID();
>    bool IsSecure();
> +  bool IsHandlingUserInput();

bool IsHandlingUserInput() const;

::: dom/media/GetUserMediaRequest.cpp:91
(Diff revision 2)
>  bool GetUserMediaRequest::IsSecure()
>  {
>    return mIsSecure;
>  }
>  
> +bool GetUserMediaRequest::IsHandlingUserInput()

bool GetUserMediaRequest::IsHandlingUserInput() const ...
Attachment #8941656 - Flags: review?(amarchesini) → review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae9db2640165
Part 1 - Add an isHandlingUserInput flag to WebRTC permission requests. r=baku,padenot
https://hg.mozilla.org/integration/autoland/rev/530fe27864de
Part 2 - Add telemetry on permission requests from third party origins, handling user input and insecure contexts. r=florian,liuche
https://hg.mozilla.org/mozilla-central/rev/ae9db2640165
https://hg.mozilla.org/mozilla-central/rev/530fe27864de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1450376
Blocks: 1494589
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: