Closed Bug 795317 Opened 7 years ago Closed 7 years ago

wyciwyg:// URLs being passed via mozbrowserlocationchange events

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed

People

(Reporter: benfrancis, Assigned: kk1fff)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

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.
blocking-basecamp: --- → ?
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).
This seems bad enough to block.

Justin, if you don't have time for this, feel free to re-assign.
Assignee: nobody → justin.lebar+bug
blocking-basecamp: ? → +
Maybe Patrick can take this?  I'm trying to focus on memory usage right now.
Assignee: justin.lebar+bug → nobody
Assignee: nobody → pwang
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.
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
Attachment #670357 - Flags: review?(justin.lebar+bug)
Attached patch Test case (obsolete) — Splinter Review
Attachment #670358 - Flags: review?(justin.lebar+bug)
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?
Attachment #670357 - Flags: review?(justin.lebar+bug) → review+
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:iampassword@mochi.test: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.
Attachment #670358 - Flags: review?(justin.lebar+bug) → review-
Fix previous version according to comment 4.
Attachment #670357 - Attachment is obsolete: true
Attachment #671324 - Flags: review+
Attached patch Test case v2 (obsolete) — Splinter Review
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.
Attachment #670358 - Attachment is obsolete: true
Attachment #671325 - Flags: review?(justin.lebar+bug)
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 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.
Attachment #671325 - Flags: review?(justin.lebar+bug) → review+
Fix v2 patch according to comment 11.
Attachment #671324 - Attachment is obsolete: true
Attachment #671793 - Flags: review+
Attached patch Test case v3Splinter Review
Fix nit according to comment 12.
Attachment #671325 - Attachment is obsolete: true
Attachment #671794 - Flags: review+
Whiteboard: [checkin-aurora-needed]
This seems to be the magic incantation we're settling on.
Keywords: checkin-needed
Whiteboard: [checkin-aurora-needed] → [checkin-needed:aurora]
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
Keywords: checkin-needed
Whiteboard: [checkin-needed:aurora]
You need to log in before you can comment on or make changes to this bug.