672128 - copying entire URL shouldn't copy "wyciwyg://" URIs or username/password

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Alice0775 White]

Build Identifier:

http://hg.mozilla.org/mozilla-central/rev/da362a07a5ee
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110717 Firefox/8.0a1 ID:20110717030802

When I was testing Bug 672119, I found this problem.

When I copy selected text in the location bar and paste, wyciwyg://x/ is added extra.
I am not sure this is intentional or not.

*This also happens Latest Aurora7.0a2.
*This is not happens on Firefox6.0beta build2.

Reproducible: Always

Steps to Reproduce:
1. Start Firefox with New Profile (without addons)
2. Go to URL and Reload once.
3. Click "Recherche" on the left to display search fields.
4. Type "40" below "entrer le numero de l'adresse recherchée" and "rue blanche" below "entrer le nom de la rue".
5. Click "Rechercher".
6. Select text in the Locationbar of the popup window.
7. Copy selected text in the Locationbar
8. Paste it elsewhere.

Actual Results: 
  Copied text is as follows.
  wyciwyg://9/http://cartelie.application.equipement.gouv.fr/cartelie/voir.do?carte=IAL&service=DRIEA_IF

Expected Results:
  Should not expose wyciwyg://x/.
  Copied text should be as follows.
  http://cartelie.application.equipement.gouv.fr/cartelie/voir.do?carte=IAL&service=DRIEA_IF

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/48e72227c2fa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110623 Firefox/7.0a1 ID:20110623030811
Fails:
http://hg.mozilla.org/mozilla-central/rev/5028de4c95e9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110623 Firefox/7.0a1 ID:20110623013442
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=48e72227c2fa

Comment 1

[Thomas Ahlblom]

Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110717 Firefox/8.0a1

wyciwyg://1/http://cartelie.application.equipement.gouv.fr/cartelie/voir.do?carte=IAL&service=DRIEA_IF

Comment 2

[Ed Morley [:emorley]]

(In reply to comment #0)
> Pushlog:
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=48e72227c2fa

Link truncated, working version:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=48e72227c2fa&tochange=5028de4c95e9

Comment 3

[Alice0775 White]

Attachment: test html

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Thanks for the great bug report, Alice!

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 546710 [details] [diff] [review]
patch

>diff --git a/browser/base/content/test/browser_wyciwyg_urlbarCopying.js b/browser/base/content/test/browser_wyciwyg_urlbarCopying.js

>+      isnot(currentURL.slice(wyciwyg.length), wyciwyg, currentURL + " is a wyciwyg URI");

Oops, this is obviously wrong (started with an isnot(indexOf("wyciwyg"), -1)), will fix locally.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Changed to:
ok(/^wyciwyg:\/\//i.test(currentURL), currentURL + " is a wyciwyg URI");

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 546710 [details] [diff] [review]
patch

>--- a/browser/base/content/urlbarBindings.xml
>+++ b/browser/base/content/urlbarBindings.xml
>@@ -508,16 +508,20 @@
>           var selectedVal = inputVal.substring(this.selectionStart, this.selectionEnd);
> 
>           // If the selection doesn't start at the beginning or URL bar is
>           // modified, nothing else to do here.
>           if (this.getAttribute("pageproxystate") != "valid" || this.selectionStart > 0)
>             return selectedVal;
> 
>           let uri = gBrowser.currentURI;
>+          // Only copy exposable URIs
>+          try {
>+            uri = Cc["@mozilla.org/docshell/urifixup;1"].getService(Ci.nsIURIFixup).createExposableURI(uri);
>+          } catch (ex) {}

Should selectedVal be returned in the exception case? I guess not, after glancing at docshell/base/nsIURIFixup.idl.
Alternatively, add a comment on why it's ok to use the original uri when createExposableURI throws?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

The IDL documentation looks wrong, it seems like it will only really throw for malformed wyciwyg URIs (I can't imagine this actually occurs in practice). I'm tempted to just remove the try/catch, but I guess I'll just leave it as-is to match the behavior of XULBrowserWindow.onLocationChange.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/fx-team/rev/e8c0e73885ee

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 546710 [details] [diff] [review]
patch

I think we should get this fix on Aurora (primarily for the password copying variant). It only affects copying from the location bar, and it makes us match the behavior we have when setting the URL bar value, so the risk is quite low.

Comment 11

[LegNeato]

Comment on attachment 546710 [details] [diff] [review]
patch

approved for releases/mozilla-aurora

Comment 12

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/e8c0e73885ee

Comment 13

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/e05bfc2a8b29

Comment 14

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110818 Firefox/9.0a1

Tested with the steps provided in the description, as well as with the test html. The issue could not be reproduced anymore.

Setting status to verified fixed.