1366357 - (CVE-2018-5142) Media Capture and Streams API permission does not inherit requester's origin

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Jun (Request needinfo for comment, I ignore other updates)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

1. Go to https://vuln.shhnjk.com/iframer.php?url=//test.shhnjk.com/permission.html
2. Click any button says contains word "Cam"


Actual results:

Permission notification says "Unknown protocol" is requesting permission. If you click Geo button, permission inherits origin correctly so it says "test.shhnjk.com".


Expected results:

When Media Capture and Streams API permission is requested from data URL or Blob URL, Firefox does not inherits origin in the permission notification. If top documents is trusted to user and child frame (ads) is untrusted site which requests permission, "Unknow protocol" will not give clear idea to users (Normal user does not what protocol is). Therefore user might allow permission by trusting top document.

So it should inherit origin of requester like Geo location permmision does

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This seems to reproduce with just https://test.shhnjk.com/permission.html so the iframing seems unrelated to the bug. Was it included for illustration of risk?

Florian, do you know if the Geolocation permission prompt has special code here?

If not, is something needed in MediaManager to inherit origins for data urls and Blog urls? Baku, do you know?

Comment 2

[Jun (Request needinfo for comment, I ignore other updates)]

iframer was added just to show the scenario I described about trusted top document (vuln.shhnjk.com) and untrusted framed document (test.shhnjk.com).

Comment 3

[Florian Quèze [:florian]]

Cc'ing Johann who may know how the geolocation prompt works.

Comment 4

[Jun (Request needinfo for comment, I ignore other updates)]

Just FYI, Notification API prompt works same as Geo location. So I guess issue is with Media Capture and Streams API.

https://test.shhnjk.com/notification.html

Comment 5

[Johann Hofmann [:johannh]]

Yes, because Geolocation (like e.g. Desktop Notifications) implements nsIContentPermissionPrompt which has a "principal" attribute that is apparently properly set (I'm not sure about the details off-hand). That's a perfect opportunity for me to voice that I really dislike WebRTC not implementing the same interface as the other permission prompts. It makes implementing everything generic over permission prompt a double-effort.

I realize that asking for a complete rewrite of the WebRTC prompts isn't constructive, so we should look for a way to solve this. I think constructing a new URI from the documentURI isn't the right way to go (https://searchfox.org/mozilla-central/rev/02cae69058d41e2c6b635edccbec91a6ca2cb57f/browser/modules/webrtcUI.jsm#372). We should rather try to get ahold of the content principal that triggered the permission prompt. E.g. running gBrowser.selectedBrowser.contentPrincipal gives us the correct host to display. But I'm not sure if we should do exactly that, because it could be susceptible to timing attacks.

This is just what immediately comes to my mind, being a bit sleepy. I can probably say more about this next week :)

Generally, IMO: data and blob shouldn't get any permissions at all.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: uriWithPrincipal.patch

Talking about blobs:

When prompting the permission for WebRTC, we use the URI of the document. This is a blob URL. In order to retrieve the correct host, we should QI nsIURIWithPrincipal as this patch does.

This is basically what Geolocation prompting does in Location::CheckURL(), when calling ssm->CheckLoadURIFromScript(cx, aURI). That method ends up calling ContentPrincipal::MayLoadInternal() which uses nsIURIWithPrincipal.

A quick fix is this patch. At least for blob URL.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8869925 [details] [diff] [review]
uriWithPrincipal.patch

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

::: browser/modules/webrtcUI.jsm
@@ +320,5 @@
>      }
>      host = uri.host;
>    } catch (ex) {}
>    if (!host) {
> +    let uriWithPrincipal = uri.QueryInterface(Ci.nsIURIWithPrincipal);

In JS QueryInterface throws if the queried interface isn't supported.

I think you meant to write:

  if ((uri instanceof Ci.nsIURIWithPrincipal) && uri.principalUri) {
    return getHost(uri.principalUri);
  }

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8869925 [details] [diff] [review]
uriWithPrincipal.patch

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

Lgtm, module florian's comment. He's probably the right reviewer here in any case.

::: browser/modules/webrtcUI.jsm
@@ +322,5 @@
>    } catch (ex) {}
>    if (!host) {
> +    let uriWithPrincipal = uri.QueryInterface(Ci.nsIURIWithPrincipal);
> +    if (uriWithPrincipal && uriWithPrincipal.principalUri) {
> +      return getHost(uriWithPrincipal.principalUri);

Nit: Should probably follow existing if-else pattern or restructure rest of code to early-bail.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Also, maybe rename getHost() to something else to avoid confusion?

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

eh these are special cases anyway, ignore comment 9.

Comment 11

[Florian Quèze [:florian]]

You will also want to add a comment giving a hint about which case this handles, so that the next person refactoring the code doesn't remove this by accident. Would be even nicer to include a test of course.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: uriWithPrincipal.patch

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8870411 [details] [diff] [review]
uriWithPrincipal.patch

Comment 7 has not been addressed.

And I agree with jib's nit in comment 8: this should follow the existing if-else pattern.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: uriWithPrincipal.patch

Sorry I didn't see those comments. I prefer the code to be before anything else instead adding this line in the if-else block.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8870752 [details] [diff] [review]
uriWithPrincipal.patch

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

I think we'll want this to be covered by a test, or it'll regress eventually.

::: browser/modules/webrtcUI.jsm
@@ +314,5 @@
>  
>  function getHost(uri, href) {
> +  // If this is a blobURL, we have to retrieve the owner URI from the
> +  // principal. blobURL implements nsIURIWithPrincipal for this purpouse.
> +  if ((uri instanceof Ci.nsIURIWithPrincipal) && uri.principalUri) {

uri can be null here if the function has been called from http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/browser/modules/webrtcUI.jsm#965

The easiest solution may be to just call Services.io.newURI in onTabSharingMenuPopupShowing, and remove the 'href' parameter here. I don't know if we need to keep a try/catch block around that call. Maybe keep one to be safe.

@@ +315,5 @@
>  function getHost(uri, href) {
> +  // If this is a blobURL, we have to retrieve the owner URI from the
> +  // principal. blobURL implements nsIURIWithPrincipal for this purpouse.
> +  if ((uri instanceof Ci.nsIURIWithPrincipal) && uri.principalUri) {
> +    return getHost(uri.principalUri);

This can be simplified to:
  uri = uri.principalUri;

Or is uri.principalUri a chain, which means we do need the recursive call?

Comment 16

[Johann Hofmann [:johannh]]

I don't understand the reason for this approach. Shouldn't we fix this for data: AND blob: URLs? This patch also doesn't work when testing it on https://vuln.shhnjk.com/iframer.php?url=//test.shhnjk.com/permission.html

Maybe there's a more generic approach to this that eliminates our usage of URIs entirely?

Comment 17

[Andrea Marchesini [:baku]]

Attachment: uriWithPrincipal.patch

In theory it cannot happen that we have a uriWithPrincipal where the principalURI implements uriWithPrincipal, but I don't think this is the place to enforce that check.

I applied the comments, but I have idea how to write a tests for this prompt.
And I haven't found any example.

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8870763 [details] [diff] [review]
uriWithPrincipal.patch

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

Looks good for blob uris, but as Johann points out, we also need a solution for data: urls, and it may be better to implement a more generic solution.

The existing tests for this prompt (and the webrtc UI in general) are in browser/base/content/test/webrtc/

Comment 19

[Daniel Veditz [:dveditz]]

Is it only the prompt that gets the origin wrong, or would we save the granted permission (if remembered) incorrectly as well?

Comment 20

[Jun (Request needinfo for comment, I ignore other updates)]

As far as I tested, we can't choose to remember permission in data URL or blob URL.

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Right, even with the https link in comment 1 the prompt won't allow me to remember the decision for data or even the https blob.

I'll have to check if the origin-unique deviceId API is affected.

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I've verified that deviceIds match the page even for data and blob urls with https://jsfiddle.net/jib1/whb39gyb/

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

As of FF53, even though you get the prompt in the STRs in comment 1, you no longer get the camera stream (bug 1367805), due to our e10s camera permissions double-check, which accidentally prevents this and causes things to fail with NotReadable error after the user grants access.

However, microphone access still works, as you can tell by copy-pasting this url:

data:text/html;base64,PHZpZGVvIGlkPSJ2aWRlb1RhZyIgYXV0b3BsYXk+PC92aWRlbz4NCjxzY3JpcHQ+DQpuYXZpZ2F0b3IubWVkaWFEZXZpY2VzLmdldFVzZXJNZWRpYSh7YXVkaW86IHRydWV9KQ0KLnRoZW4oc3RyZWFtID0+IGRvY3VtZW50LmdldEVsZW1lbnRCeUlkKCJ2aWRlb1RhZyIpLnNyY09iamVjdCA9IHN0cmVhbSkNCi5jYXRjaChlID0+IGNvbnNvbGUubG9nKGUpKTsNCjwvc2NyaXB0Pg0K

Data URLs can be pasted directly, which means they won't have any "owning" principal, which means there's no way around asking:

    Will you allow Unknown origin to share your microphone?

To move forward on this and bug 1367805, I think we need to sort out what our policy on nullprincipal permissions is, and what we want the user experience to be. This would seem to apply to all permissions, not just WebRTC ones.

FWIW, in Google Chrome, getUserMedia requires https, and therefore doesn't work in data urls. The blob one works though, which makes me think our blob patch here is good at least.

Comment 24

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

See also bug 1369482.

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Interestingly, the [Get Data] button in the STRs in comment 1 shows the owning origin somehow in the prompt. Johann, any idea how it does that?

Also, if I cut and paste the resulting data url from that into another tab and hit ENTER, then it doesn't prompt for location. No error, nothing.

Comment 26

[Boris Zbarsky [:bzbarsky]]

If you load a data: URL in a tab that is already showing some other URL, that data: URL will inherit the principal of the other URL right now.

Comment 27

[Johann Hofmann [:johannh]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #25)
> Interestingly, the [Get Data] button in the STRs in comment 1 shows the
> owning origin somehow in the prompt. Johann, any idea how it does that?

Did you mean Geo Data? As bz said and as I tried to outline in comment 5, geolocation/nsIContentPermissionPrompt correctly only considers the content principal of the request. Because data: URLs inherit their opener principal the request is considered to be coming from a "secure" https context, hence geolocation works and it shows the correct URL. We should also use the content principal for webrtc.

> Also, if I cut and paste the resulting data url from that into another tab
> and hit ENTER, then it doesn't prompt for location. No error, nothing.

If you just paste it then it's a null principal, and then it rightfully fails because geolocation is limited to https (you should see a warning in the console).

IMO we should just disallow any permission for data: and blob: urls, irrespective of their parent principal, but since that sounds complicated we might just wait for bug 1331351 to be resolved.

Comment 28

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Maybe we can untangle the issues, to apply fixes flexibly.

 1. We allow permission today (modulo bug 1367805) so fix "Unknown origin" in prompts where there are inherited principals.
 2. Fix cam access in bug 1367805 by either restoring 52-like allowance or move SecurityError ahead of prompt.
 3. Consider denying all permissions (geo, gum, etc.) for blob urls as a category.
 4. Consider denying all permissions (geo, gum, etc.) for data urls as a category.
 5. Consider denying all permissions (geo, gum, etc.) for nullprincipals (modulo sandboxed iframe with allow="camera" etc.)
 6. Figure out our policy on nullprincipal permissions & UX, like sandboxed iframe with allow="camera" etc.

It seems to me #1 is this issue, and can hopefully be uplifted far, irrespective of the other issues.

#2 is bug 1367805 and may be upliftable as well, depending on approach taken. #3 through #6 seem like they should be new bugs.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> 3. Consider denying all permissions (geo, gum, etc.) for blob urls as a category.

Why are blob urls special?  Should we deny for srcdoc too?

That is, why are we doing denials based on url instead of origin?

Comment 30

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #29)
> That is, why are we doing denials based on url instead of origin?

I've filed bug 1371741 to disallow getUserMedia on nullprincipals.

Once that lands, it should avoid the "Unknown origin" prompt problems here, replacing them with SecurityError.

In light of that, we should take baku's patch here, to continue to allow blob urls with inherited principals, as that would match Chrome behavior IMHO.

I think I'd take a patch that did the same for data urls, though I don't feel strongly about it. Chrome disallows it, but for https reasons, and we don't implement the https restriction today.

Comment 31

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Florian, may we consider your f+ an r+ for landing?

I'm happy to punt on data urls (have them stop working once bug 1371741 lands) if we don't have a good solution there.

Comment 32

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #31)
> Florian, may we consider your f+ an r+ for landing?

No, this needs a test. But I'm otherwise OK with landing this change as it's a net improvement over the current situation. I'm just afraid the solution for data URL will require us to use the principal instead of the URL, and will also fix the blob case in a different way (which will mean this patch could then be reverted).

Comment 33

[Johann Hofmann [:johannh]]

This patch does not fix any of the issues originally reported on https://vuln.shhnjk.com/iframer.php?url=//test.shhnjk.com/permission.html, it's just a hack to solve a small subset of the problem. I agree we can land this since it's an improvement, but it's not ok to resolve this bug with that patch. Either add leave-open or open a new bug (which would have the exact same description as this one).

Comment 34

[Johann Hofmann [:johannh]]

Attachment: Use origin instead of documentURI

Is there anything wrong with this approach?

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8877097 [details] [diff] [review]
Use origin instead of documentURI

>+  let uri = Services.io.newURI(aRequest.origin);

What is the expected behavior when that throws (e.g. if aRequest.origin is "null")?

Comment 36

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8877097 [details] [diff] [review]
Use origin instead of documentURI

If baku is ok with it then I am. Lateraling to florian.

Are we happy with tests for this? Bug 1371741 adds some.

Comment 37

[Johann Hofmann [:johannh]]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #35)
> Comment on attachment 8877097 [details] [diff] [review]
> Use origin instead of documentURI
> 
> >+  let uri = Services.io.newURI(aRequest.origin);
> 
> What is the expected behavior when that throws (e.g. if aRequest.origin is
> "null")?

Ah, I expected .origin to return a null principal origin, but it returns "null", indeed. Since that only happens for null principals, we could just use a null principal, right?

We should probably remove the checkbox offering to permanently save a permission for null principals. It doesn't even work :D

Comment 38

[Johann Hofmann [:johannh]]

I'll fix the null origin problem and add a test and put the patch up for review for Florian.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> Since that only happens for null principals

No, happens for file:// too for example.

Whether using a null principal in the file:// case and others like that (no hostname) is the right thing, I can't tell you.

Comment 40

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Ooh yeah I think we want gUM to continue to work for file:// urls. I also see app and localhost in our telemetry reporting [1].

That's going to be a problem with bug 1371741 as well...

[1] https://dxr.mozilla.org/mozilla-central/rev/91134c95d68cbcfe984211fa3cbd28d610361ef1/dom/media/MediaManager.cpp#2111

Comment 41

[Boris Zbarsky [:bzbarsky]]

> That's going to be a problem with bug 1371741 as well...

Why?  file:// doesn't get a nullprincipal.  It just gets a principal that _serializes_ to "null", which is not the same thing at all.

Comment 42

[Johann Hofmann [:johannh]]

We should probably wait for bug 1371741 to be resolved.

Comment 43

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 44

[Randell Jesup [:jesup] (needinfo me)]

Mass change P1->P2 to align with new Mozilla triage process.

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

Any action here on solving the wider issue?  I see bug 1371741 is still open with no action

Comment 46

[Johann Hofmann [:johannh]]

I guess we could solve this independently of bug 1371741 by working around the file URI/null principal case. I might have some time to look into this next week. We will now also have to solve this differently for data: documents, since they also get a null principal since bug 1371741. We can probably just introduce magic strings for both, since bug 1371741 seems stalled.

Comment 47

[Johann Hofmann [:johannh]]

Stealing this from baku (I hope that's okay) to remind myself to look at it.

Comment 48

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks Johann! Bug 1371741 likely won't get looked at until 60 when we hope to do bug 1389198.

Comment 49

[Johann Hofmann [:johannh]]

Attachment: Use origin instead of documentURI for WebRTC permission requests

MozReview-Commit-ID: IkccA65Ma3a

Comment 50

[Florian Quèze [:florian]]

Comment on attachment 8932062 [details] [diff] [review]
Use origin instead of documentURI for WebRTC permission requests

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

::: browser/base/content/test/popupNotifications/browser_displayURI.js
@@ +1,2 @@
>  /*
> + * Make sure that the correct origin is shown for permission prompts.

I'm confused about what this test covers or not. On IRC you told me that we'll sometimes show the "Unknown origin" message, but this test checks for example.org in the permission panel in all cases.

Given that the code changes are in webrtc specific code, I wonder if you should add a test in browser/base/content/test/webrtc covering the edge cases that we have to support.

@@ +7,2 @@
>      let popupShownPromise = waitForNotificationPanel();
> +    await contentTask(browser);

This could be:
    await ContentTask.spawn(browser, null, testFunction)
and then all the check callers can be simplified.

@@ +38,5 @@
> +});
> +
> +add_task(async function test_displayURI_geo_blob() {
> +  await check(browser => ContentTask.spawn(browser, null, async function() {
> +    let text = "\u003cscript\u003enavigator.geolocation.getCurrentPosition(() => {})\u003c/script\u003e";

What requires this escaping of < and > here?

::: browser/modules/webrtcUI.jsm
@@ +370,5 @@
>  
> +  let uri = null;
> +  try {
> +    uri = Services.io.newURI(aRequest.origin);
> +  } catch (e) { /* This fails for principals that serialize to "null". */ }

I doubt the code will work correctly if uri is null.

Especially https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/browser/modules/webrtcUI.jsm#416 when scope == SitePermissions.SCOPE_PERSISTENT

and https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/browser/modules/webrtcUI.jsm#506

Comment 51

[Johann Hofmann [:johannh]]

Attachment: Use origin instead of documentURI for WebRTC permission requests

MozReview-Commit-ID: IkccA65Ma3a

Comment 52

[Johann Hofmann [:johannh]]

(In reply to Florian Quèze [:florian] from comment #50)
> Comment on attachment 8932062 [details] [diff] [review]
> Use origin instead of documentURI for WebRTC permission requests
> 
> Review of attachment 8932062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/popupNotifications/browser_displayURI.js
> @@ +1,2 @@
> >  /*
> > + * Make sure that the correct origin is shown for permission prompts.
> 
> I'm confused about what this test covers or not. On IRC you told me that
> we'll sometimes show the "Unknown origin" message, but this test checks for
> example.org in the permission panel in all cases.

I added a test for file:// URIs, that's a good idea.

> Given that the code changes are in webrtc specific code, I wonder if you
> should add a test in browser/base/content/test/webrtc covering the edge
> cases that we have to support.

I'd say the coverage we have in test/popupNotifications is sufficient. What additional edge cases would you cover?

> @@ +38,5 @@
> > +});
> > +
> > +add_task(async function test_displayURI_geo_blob() {
> > +  await check(browser => ContentTask.spawn(browser, null, async function() {
> > +    let text = "\u003cscript\u003enavigator.geolocation.getCurrentPosition(() => {})\u003c/script\u003e";
> 
> What requires this escaping of < and > here?

Ah, indeed, you don't need to escape for blob URLs (I just copied from the original PoC).

> ::: browser/modules/webrtcUI.jsm
> @@ +370,5 @@
> >  
> > +  let uri = null;
> > +  try {
> > +    uri = Services.io.newURI(aRequest.origin);
> > +  } catch (e) { /* This fails for principals that serialize to "null". */ }
> 
> I doubt the code will work correctly if uri is null.

Good catch, thanks! I had assumed it was only used for displaying (that function is way too large).

Comment 53

[Florian Quèze [:florian]]

Comment on attachment 8934528 [details] [diff] [review]
Use origin instead of documentURI for WebRTC permission requests

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

::: browser/base/content/test/popupNotifications/browser_displayURI.js
@@ +20,5 @@
> +    let panel = await popupShownPromise;
> +    let notification = panel.children[0];
> +    let body = document.getAnonymousElementByAttribute(notification,
> +                                                       "class",
> +                                                       "popup-notification-body");

There's a smell of code duplication here (but not a reason for r- because this is just a test).

@@ +44,5 @@
> +
> +add_task(async function test_displayURI_camera() {
> +  await check(async function() {
> +    // For some reason fake camera requests get instantly allowed when requesting
> +    // them through direct gUM queries in content tasks. We really want to see the prompt here,

should we file a bug on this?

Comment 54

[Florian Quèze [:florian]]

Note: the r+ in comment 53 is without testing the patch myself locally, which I think is fine because the patch adds test coverage.

Comment 55

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8934528 [details] [diff] [review]
Use origin instead of documentURI for WebRTC permission requests

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

::: browser/base/content/test/popupNotifications/browser_displayURI.js
@@ +44,5 @@
> +
> +add_task(async function test_displayURI_camera() {
> +  await check(async function() {
> +    // For some reason fake camera requests get instantly allowed when requesting
> +    // them through direct gUM queries in content tasks. We really want to see the prompt here,

This is by design. Please use the pref "media.navigator.permission.fake" here to get a prompt, and remove this comment.

Comment 56

[Johann Hofmann [:johannh]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #55)
> Comment on attachment 8934528 [details] [diff] [review]
> Use origin instead of documentURI for WebRTC permission requests
> 
> Review of attachment 8934528 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/popupNotifications/browser_displayURI.js
> @@ +44,5 @@
> > +
> > +add_task(async function test_displayURI_camera() {
> > +  await check(async function() {
> > +    // For some reason fake camera requests get instantly allowed when requesting
> > +    // them through direct gUM queries in content tasks. We really want to see the prompt here,
> 
> This is by design. Please use the pref "media.navigator.permission.fake"
> here to get a prompt, and remove this comment.

Yup, I'm using that pref already. It's still auto-accepting my request.

Comment 57

[Andreas Pehrson [:pehrsons]]

(In reply to Johann Hofmann [:johannh] from comment #56)
> Yup, I'm using that pref already. It's still auto-accepting my request.

Two ways this could happen still:
- If the pref "media.navigator.permission.force" is true
- Your profile has persistent permissions for the origin the test is running on

I hope it's the former. I'd doubt a bug as we haven't touched this logic recently.

Comment 58

[Johann Hofmann [:johannh]]

(In reply to Andreas Pehrson [:pehrsons] from comment #57)
> (In reply to Johann Hofmann [:johannh] from comment #56)
> > Yup, I'm using that pref already. It's still auto-accepting my request.
> 
> Two ways this could happen still:
> - If the pref "media.navigator.permission.force" is true
> - Your profile has persistent permissions for the origin the test is running
> on
> 
> I hope it's the former. I'd doubt a bug as we haven't touched this logic
> recently.

So, uh, here's a weird thing: setting "media.navigator.permission.force" to true FIXES the issue. No idea how that's happening, but I'll update the patch unless you want to look into it...

Comment 59

[Johann Hofmann [:johannh]]

It makes sense when you look at the logic here and assuming that the call into ContentTask.spawn should be privileged:

https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/dom/media/MediaManager.cpp#2515

Comment 60

[Johann Hofmann [:johannh]]

Attachment: Use origin instead of documentURI for WebRTC permission requests

MozReview-Commit-ID: IkccA65Ma3a

Comment 61

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Yes there are two prefs to force the prompts:

 1. "media.navigator.permission.fake" forces it for fake devices (but still doesn't affect chrome calls) 
 2. "media.navigator.permission.force" forces it even for chrome calls

We need the distinction because the gUM prompt calls gUM itself from chrome to render a preview in the case of screensharing, and we have tests [1] for that.

[1] https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/dom/media/tests/mochitest/test_getUserMedia_scarySources.html#40

Comment 62

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d592ebf8cdca4e2139e23f1cda15c391f40c2e
Bug 1366357 - Use origin instead of documentURI for WebRTC permission requests. r=florian

Comment 63

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for browser-chrome failure in browser_displayURI.js on Windows (both with and without e10s):

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d279371d35bd824ca5a854097b92c6b8f9b738

First push with failure (previous were busted): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f54b084de4f907a0d5edc220179ca3a1ea5f1349&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=149968017&repo=mozilla-inbound

19:57:13     INFO -  75 INFO TEST-START | browser/base/content/test/popupNotifications/browser_displayURI.js
19:57:14     INFO -  GECKO(3968) | JavaScript error: chrome://global/content/browser-child.js, line 359: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.loadURIWithOptions]
19:57:30     INFO -  GECKO(3968) | [DEBUG SHUTDOWN] Shutdown: decoder=012BCB60 state machine=01224800
19:57:30     INFO -  GECKO(3968) | [DEBUG SHUTDOWN] Enter: state machine=08E75CC8 reader=08E5F6D0
19:57:30     INFO -  GECKO(3968) | [DEBUG SHUTDOWN] Shutdown: reader=012B8000 shutdown demuxer=1689B258
19:57:30     INFO -  GECKO(3968) | [DEBUG SHUTDOWN] Shutdown: pool=168E4240 count=1
19:57:30     INFO -  GECKO(3968) | [DEBUG SHUTDOWN] operator (): pool=168E4240 shutdown=true count=0
19:57:30     INFO -  GECKO(3968) | [DEBUG SHUTDOWN] TearDownDecoders: reader=012B8000 shut down audio task queue
19:57:30     INFO -  GECKO(3968) | [DEBUG SHUTDOWN] TearDownDecoders: reader=012B8000 shut down video task queue
19:57:30     INFO -  GECKO(3968) | [DEBUG SHUTDOWN] FinishShutdown: state machine=01224800
19:57:30     INFO -  GECKO(3968) | [DEBUG SHUTDOWN] Unregister: decoder=012BCB60, count=0
19:57:58     INFO -  TEST-INFO | started process screenshot
19:57:58     INFO -  TEST-INFO | screenshot: exit 0
19:57:58     INFO -  Buffered messages logged at 19:57:13
19:57:58     INFO -  76 INFO Entering test bound setup
19:57:58     INFO -  77 INFO Leaving test bound setup
19:57:58     INFO -  78 INFO Entering test bound test_displayURI_geo
19:57:58     INFO -  79 INFO Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 174}]
19:57:58     INFO -  80 INFO Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 174}]
19:57:58     INFO -  Buffered messages logged at 19:57:14
19:57:58     INFO -  81 INFO TEST-PASS | browser/base/content/test/popupNotifications/browser_displayURI.js | Check that at least the eTLD+1 is present in the markup -
19:57:58     INFO -  82 INFO Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 174}]
19:57:58     INFO -  83 INFO Console message: [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.loadURIWithOptions]" {file: "chrome://global/content/browser-child.js" line: 359}]
19:57:58     INFO -  Buffered messages finished
19:57:58    ERROR -  84 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/popupNotifications/browser_displayURI.js | Test timed out -

Comment 64

[Johann Hofmann [:johannh]]

Attachment: Use origin instead of documentURI for WebRTC permission requests

MozReview-Commit-ID: IkccA65Ma3a

Comment 65

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c1f2c895949b1dae617b0c2a1039a5494cd8b2a

Comment 66

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/784e8f4a3209082492f44f511180c0168e7c38eb
Bug 1366357 - Use origin instead of documentURI for WebRTC permission requests. r=florian

Comment 67

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/784e8f4a3209

Comment 68

[Julien Cristau [:jcristau]]

Do we want to ship this change on 58?  OTOH this bug has been around for a while so maybe there's no need to rush the fix out.

Comment 69

[Johann Hofmann [:johannh]]

I think we can let it ride the trains :)

Comment 70

[Randell Jesup [:jesup] (needinfo me)]

Fixed in 59; should this be unhidden?