Last Comment Bug 672128 - copying entire URL shouldn't copy "wyciwyg://" URIs or username/password
: copying entire URL shouldn't copy "wyciwyg://" URIs or username/password
Status: VERIFIED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
http://cartelie.application.equipemen...
Depends on:
Blocks: 665580
  Show dependency treegraph
 
Reported: 2011-07-17 12:29 PDT by Alice0775 White
Modified: 2011-08-19 08:03 PDT (History)
6 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed


Attachments
test html (880 bytes, text/html)
2011-07-17 15:44 PDT, Alice0775 White
no flags Details
patch (6.68 KB, patch)
2011-07-18 22:15 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
dao+bmo: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Alice0775 White 2011-07-17 12:29:46 PDT
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 2011-07-17 12:40:18 PDT
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 3 Alice0775 White 2011-07-17 15:44:03 PDT
Created attachment 546446 [details]
test html
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-18 22:15:04 PDT
Created attachment 546710 [details] [diff] [review]
patch

Thanks for the great bug report, Alice!
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-18 22:38:28 PDT
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: gavin@gavinsharp.com] 2011-07-18 22:42:10 PDT
Changed to:
ok(/^wyciwyg:\/\//i.test(currentURL), currentURL + " is a wyciwyg URI");
Comment 7 Dão Gottwald [:dao] 2011-07-19 05:39:22 PDT
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: gavin@gavinsharp.com] 2011-07-21 11:04:28 PDT
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: gavin@gavinsharp.com] 2011-07-21 11:20:03 PDT
https://hg.mozilla.org/integration/fx-team/rev/e8c0e73885ee
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-21 11:49:29 PDT
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 christian 2011-07-21 14:50:33 PDT
Comment on attachment 546710 [details] [diff] [review]
patch

approved for releases/mozilla-aurora
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-21 18:08:58 PDT
http://hg.mozilla.org/mozilla-central/rev/e8c0e73885ee
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-21 18:15:45 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/e05bfc2a8b29
Comment 14 Virgil Dicu [:virgil] [QA] 2011-08-19 08:03:35 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.