Closed Bug 1320502 Opened 3 years ago Closed 3 years ago

"Paste and Go" does not work if CRLF exists at the end of clipboard url.

Categories

(Firefox :: Address Bar, defect)

53 Branch
x86
Windows 10
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 - verified

People

(Reporter: alice0775, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached file url.txt
Affected Nightly53.0a1.
Not affected Aurora52.0a2.

If CRLF exists at the end of clipboard url.
"Paste and Go" does not work. It only perform paste.

Steps To Reproduce:
1. Copy url to Clipboard from notepad
   Open attached with notepad
   Click left side of "h" and hold mouse down and move downward to select a line.
   Copy to clipboard ( Ctrl+C )
   
2. Open New Tab
3. Right Click on Location Bar and Choose "Paste and Go"

Actual Results:
The url is pasted. The trailing CRLF seems to be ignored and stripped.
However, Nothing loads.

Expected Results:
The trailing CRLF shuld be ignored and stripped.
The url should load.
Regressed by: 37a877e840fc	Gijs Kruitbosch — Bug 1229426 - avoid dnd of js URIs, r=mak
Flags: needinfo?(gijskruitbosch+bugs)
[Tracking Requested - why for this release]: Broken all function related clipboard of Location Bar.

Steps To Reproduce:
1. Copy url to Clipboard:
     Open attached with notepad.exe
     Click left side of "h" and hold mouse down and move downward to select a line.
     Copy to clipboard ( Ctrl+C )
   
2. Open New Tab
3. Attempt to open with clipboard url:
     Right Click on Location Bar and Choose "Paste & Go"
     Focus Location Bar, paste ( Ctrk+v ) , keypress [Enter] or click Go button[->] 

Actual Results:
The url is pasted. The trailing CRLF seems to be ignored and stripped.
However, Nothing loads.

Expected Results:
The trailing CRLF shuld be ignored and stripped.
The url should load.
Attached file test.html
STR
1. Copy url to Clipboard:
     Open attached html
     Click left side of "h" and hold mouse down and move downward to select a line.
     Copy to clipboard ( Ctrl+C )
2. ...
3. ...
hm, it's the change to stripUnsafeProtocolOnPaste, it now also strips newlines, but that means it will fail the str == fixed_str check we use with it.
(In reply to Marco Bonardo [::mak] from comment #4)
> hm, it's the change to stripUnsafeProtocolOnPaste, it now also strips
> newlines, but that means it will fail the str == fixed_str check we use with
> it.

Ugh, sorry for missing that. I'll check if I can add a test.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8814941 [details]
Bug 1320502 - fix paste (and go) issues regressed in bug 1229426,

https://reviewboard.mozilla.org/r/96000/#review96076

::: browser/base/content/browser.js:5639
(Diff revision 1)
> -  return pasteData.replace(/\r?\n/g, "").replace(/^(?:\s*javascript:)+/i, "");
> +  let sanitizedPasteData = pasteData.replace(/\r?\n/g, "");
> +  let pasteDataNoJS = sanitizedPasteData.replace(/^(?:\s*javascript:)+/i, "");
> +  if (pasteDataNoJS.length != sanitizedPasteData.length) {
> +    return pasteDataNoJS;
> +  }
> +  return pasteData;

nit: in case you would like to avoid the temp var and length comparison, an alternative could be:

let changed = false;
let pasteDataNoJS = pasteData.replace(/\r?\n/g, "")
                             .replace(/^(?:\s*javascript:)+/i,
                                      () => { changed = true;
                                              return ""; })
return changed ? pasteDataNoJS : pasteData;

::: browser/base/content/test/urlbar/browser_pasteAndGo.js:18
(Diff revision 1)
> +      gURLBar.focus();
> +      yield new Promise((resolve, reject) => {
> +        waitForClipboard(url, function() {
> +          clipboardHelper.copyString(url);
> +        }, resolve,
> +          reject.bind(new Error("Failed to copy string '" + url + "' to clipboard"))

does this really work? I've never seen binding a reject to a new Error...
Either you meant () => reject(New Error
or I learned something new!
Attachment #8814941 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #7)
> > +          reject.bind(new Error("Failed to copy string '" + url + "' to clipboard"))
> 
> does this really work? I've never seen binding a reject to a new Error...
> Either you meant () => reject(New Error
> or I learned something new!

Re-reading, I think I meant reject.bind(null, new Error(...)). That should work, I think, though the arrow function might be clearer. Why wouldn't it?
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3a64450cc56a
fix paste (and go) issues regressed in bug 1229426, r=mak
https://hg.mozilla.org/mozilla-central/rev/3a64450cc56a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Tracking 53- as this is resolved fixed.
I have reproduced this bug with Firefox nightly 53.0a1 (2016-11-26)(64-bit) on Windows 10, 64 Bit.

The Bug's fix is now verified on latest nightly 53.0a1.

Build ID 	 20161207030204

User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0.

[bugday-20161207]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.