795317 - wyciwyg:// URLs being passed via mozbrowserlocationchange events

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Ben Francis [:benfrancis]]

As reported in https://github.com/mozilla-b2g/gaia/issues/5334 wyciwyg:// URLs are somehow making it into the URL bar of the browser app for certain sites.

I hadn't heard of this protocol but apparently it's something Gecko uses internally to load cached content?

It seems that these URLs should never get as far as mozbrowserlocationchange events.

STR:
* Open Gaia's browser app
* Load wired.com
* wyciwyg://0/ is prepended to the URL in the location bar http://www.wired.com URL.

Comment 1

[Honza Bambas (:mayhemer)]

wyciwyg channels are result of JS document.write().  It should never appear in URL bar.  

Can this be reproduced on the Gaia simulator?  (I don't have the phone).

Comment 2

[Andrew Overholt [:overholt]]

This seems bad enough to block.

Justin, if you don't have time for this, feel free to re-assign.

Comment 3

[Justin Lebar (not reading bugmail)]

Maybe Patrick can take this?  I'm trying to focus on memory usage right now.

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: wyciwyg in desktop browser.

In desktop browser, a wyciwyg uri isn't shown in location bar because there's a service "@mozilla.org/docshell/urifixup;1" helps to create an exposable uri. However, the desktop browser still thinks that actual uri is started with 'wyciwyg'. The attached screenshot was made by released version of firefox (ver 16, shipped with ubuntu). Should we treat this as a bug of browser?

STR:
1. Use fennec's user agent string. (This case only happens in mobile version of wired.com)
2. Browse http://wired.com/
3. Type "wire" in location bar. Then the browser shows the opened tabs with their uris.

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Use exposable uri in locationchange event.

This patch uses urifixup service to make exposable uri when firing locationchange event. Despite Comment 4, we might still need to use the service to remove unexposable part.

Hi Justin,

Would you help to review this?

Thanks

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Test case

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 670357 [details] [diff] [review]
Patch: Use exposable uri in locationchange event.

>Bug 795317: Using exposable uri when updating mozbrowser's location r=jlebar

s/Using/Use

>diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js
>+      // Remove password and wyciwyg from uri.
>+      try {
>+        location = Cc["@mozilla.org/docshell/urifixup;1"]
>+          .getService(Ci.nsIURIFixup).createExposableURI(location);
>+      } catch (e) {}

What's the purpose of this try/catch?  If createExposableURI fails, do you
think we should fire a locationchange anyway?  Maybe it would be better not to
fire anything?

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 670358 [details] [diff] [review]
Test case

>diff --git a/dom/browser-element/mochitest/browserElement_ExposableUri.js b/dom/browser-element/mochitest/browserElement_ExposableUri.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/browser-element/mochitest/browserElement_ExposableUri.js

Nit: We seem to prefer "URI" to "Uri" in filenames:

$ git ls-files | grep URI | wc -l
83
$ git ls-files | grep Uri | wc -l
5

>+// Test that an iframe with the |mozbrowser| attribute emits mozbrowserloadX
>+// events when this page is in the whitelist.

I don't think this is the comment you want.

>+function testPassword() {
>+  function locationchange(e) {
>+    var uri = e.detail;
>+    var testStr = uri.substring(0, 27);
>+    isnot(testStr, 'http://iamuser:iampassword@', 'Username and password shouldn\'t be exposed in uri.');

I'd prefer that we assert that the uri is what we expect it to be, rather than
that it isn't what we don't want it to be.  As it is, it's hard to say whether
this actually does the /correct/ thing; we only know it doesn't fail in the way
you're testing.

>+    testEnd();

I don't think we're gaining anything by having a function testEnd() which does
nothing but call SimpleTest.finish().

>+  }
>+
>+  iframe.addEventListener('mozbrowserlocationchange', locationchange);
>+  iframe.src = "http://iamuser:[e-mail]:8888/tests/dom/browser-element/mochitest/file_empty.html";
>+}
>+
>+function testWyciwyg() {
>+  var locationChangeCount = 0;
>+
>+  function locationchange(e) {
>+    // locationChangeCount:
>+    //  0 - the first load.
>+    //  1 - after document.write().
>+    if (locationChangeCount == 0) {
>+      locationChangeCount ++;
>+    } else if (locationChangeCount == 1) {
>+      var uri = e.detail;
>+      var schemeStr = uri.substring(0, 10);
>+      isnot(schemeStr, 'wyciwyg://', 'Scheme in string shouldn\'t be wyciwyg');

I think it's cleaner if you quote strings containing contractions with ", but
whatever you prefer is fine.

Again, I'd prefer that we checked that the whole URI is what we expect here.

>+      iframe.removeEventListener('mozbrowserlocationchange', locationchange);
>+      SimpleTest.executeSoon(testPassword);
>+    }
>+  }

Please add a comment somewhere around here telling us that file_wyciwyg loads
and then calls document.write().

>+  iframe.src = 'file_wyciwyg.html';
>+  iframe.addEventListener('mozbrowserlocationchange', locationchange);
>+}
>+
>+function runTest() {
>+  browserElementTestHelpers.setEnabledPref(true);
>+  browserElementTestHelpers.addPermission();
>+
>+  iframe = document.createElement('iframe');
>+  iframe.mozbrowser = true;
>+  document.body.appendChild(iframe);
>+  testWyciwyg();
>+}
>+
>+addEventListener('load', function() { SimpleTest.executeSoon(runTest); });
>diff --git a/dom/browser-element/mochitest/file_wyciwyg.html b/dom/browser-element/mochitest/file_wyciwyg.html
>new file mode 100644
>--- /dev/null
>+++ b/dom/browser-element/mochitest/file_wyciwyg.html
>@@ -0,0 +1,14 @@
>+<html>
>+<head>
>+<title>test</title>
>+<script type="text/javascript">
>+addEventListener('load', function() {
>+  window.setTimeout(function() {
>+    document.write("test");
>+  }, 1);

Does 0 work here just as well as 1?  If it needs to be 1, I'd be worried we
have a race condition here.

I'd like to have another look at this test.

Comment 9

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Use exposable uri in locationchange event. v2

Fix previous version according to comment 4.

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Test case v2

Fix previous version according to comment 8.
Summarize update:
1. Use whole url as the expected value.
2. Change timeout in window.setTimeout to 0 (it still can reproduce this bug),
3. Change "Uri" in filename of test from to "URI".
4. Fix comments.

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 671324 [details] [diff] [review]
Patch: Use exposable uri in locationchange event. v2

> +      try {
> +        location = Cc["@mozilla.org/docshell/urifixup;1"]
> +          .getService(Ci.nsIURIFixup).createExposableURI(location);
> +      } catch (e) {
> +        return;
> +      }

I'd prefer we just didn't have the try/catch, in this case.  Without the try/catch, there's some hope that our error-reporting code might see the exception and print a warning to the console.  (It's quite bad at this, but it occasionally works properly.)  But with the try/catch, we lose any hope of detecting failure here.

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 671325 [details] [diff] [review]
Test case v2

Looks good to me.  Just two nits on the comments:

>+// Test if browser element fire locationchange with unexposable part URI.

Maybe

  Bug 795317: Test that the browser element sanitizes its URIs by removing the
  "unexposable" parts before sending them in the locationchange event.

>+  // file_wyciwyg.html calls document.write() to make wyciwyg channel created.

  calls document.write() to create a wyciwyg channel.

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Use exposable uri in locationchange event. v3

Fix v2 patch according to comment 11.

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Test case v3

Fix nit according to comment 12.

Comment 15

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2112a3c02c73
https://hg.mozilla.org/integration/mozilla-inbound/rev/66ea6fd30181

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This seems to be the magic incantation we're settling on.

Comment 17

[Justin Lebar (not reading bugmail)]

checkin-needed plus the whiteboard tag?  That's not a little redundant?

Anyway, fixed in Aurora 18.

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b4d24b35f88
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b33ce9ec548

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2112a3c02c73
https://hg.mozilla.org/mozilla-central/rev/66ea6fd30181