Regression: Image markup copied to clipboard broken by URL encoding |
|||||||||||||
Issue descriptionChrome Version: 65.0.3293.0 and later, up to ToT 66.0.3331.0 Broken out from https://bugs.chromium.org/p/chromium/issues/detail?id=758523#c25 Regressing CL: https://chromium.googlesource.com/chromium/src/+/fdd23baca689698b9f209d20a420be712c197ef4 Existing version in clipboard_utils.cc: std::string URLToImageMarkup(const blink::WebURL& url, const blink::WebString& title) { std::string markup("<img src=\""); markup.append(net::EscapeForHTML(url.GetString().Utf8())); markup.append("\""); if (!title.IsEmpty()) { markup.append(" alt=\""); markup.append(net::EscapeForHTML(title.Utf8())); markup.append("\""); } markup.append("/>"); return markup; } New version in WebClipboardImpl.cpp calls EncodeWithURLEscapeSequences: // TODO(slangley): crbug.com/775830 Remove the implementation of // URLToImageMarkup from clipboard_utils.h once we can delete // MockWebClipboardImpl. WTF::String URLToImageMarkup(const WebURL& url, const WTF::String& title) { WTF::String markup("<img src=\""); markup.append(EncodeWithURLEscapeSequences(url.GetString())); markup.append("\""); if (!title.IsEmpty()) { markup.append(" alt=\""); markup.append(EncodeWithURLEscapeSequences(title)); markup.append("\""); } markup.append("/>"); return markup; } Notably, this *only* happens in the CopyImage codepath. If you select markup before and after the image too and copy that, the URL is correct. https://bayden.com/webp.html The extra URL-encoding pass breaks the image.
,
Jan 25 2018
,
Jan 26 2018
Adding some context: For ARC++ this is an important BUG since most of the copy&paste functionality from Chrome to ARC relies on this feature. Would be great, once this is fixed, to have it backported in M65. thanks.
,
Jan 26 2018
,
Jan 26 2018
The |EncodeWithURLEscapeSequences| function lacks a comment, but it's meant to be used when you're embedding text content into a URL (e.g. a filesystem or data URI); this is called URL-Escaping. To fix this regression, we need to replace that call with a function that performs HTML-Escaping instead. I assume we can't use net::EscapeForHTML for layering reasons; /WebKit/Source/core/dom/trustedtypes/TrustedHTML.cpp has an implementation, but I'm not sure whether it's suitable.
,
Jan 27 2018
Tentatively tagging with Release-Block-Beta based on the observation in #3.
,
Jan 28 2018
,
Feb 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/41dfe13dc2cfcfd4962e6147dbfc6b4f8e49bdc3 commit 41dfe13dc2cfcfd4962e6147dbfc6b4f8e49bdc3 Author: Stuart Langley <slangley@chromium.org> Date: Wed Feb 07 00:58:31 2018 Fix regression introduced into copying images using the clipboard. The mojo work for web clipboard has introduced a regression when copying images. Originally the code was in content and was using net::EscapeForHTML to escape the URL of the image and the title. When converted to mojo and moved inside of blink this was changed to EncodeWithURLEscapeSequences which is a different encoding scheme. This change reverts to using an escaping the same as net::EscapeForHTML, however as net::EscapeForHTML uses std::string and we are working with WTF::String we have had to copy the code rather than re-use it. Bug: 806076 Change-Id: Ic51835c555c071571a837018d74a27fd455362cf Reviewed-on: https://chromium-review.googlesource.com/896762 Commit-Queue: Stuart Langley <slangley@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#534858} [modify] https://crrev.com/41dfe13dc2cfcfd4962e6147dbfc6b4f8e49bdc3/third_party/WebKit/Source/platform/DEPS [modify] https://crrev.com/41dfe13dc2cfcfd4962e6147dbfc6b4f8e49bdc3/third_party/WebKit/Source/platform/exported/WebClipboardImpl.cpp
,
Feb 7 2018
,
Feb 7 2018
Tried testing the issue on Win-10 using chrome version #66.0.3342.0 by downloading the clipSpy software from internet. But it showed some malware threat, hence couldn't proceed with the issue. slangley@ - Could you please provide any sample web clipboard tool url to test the issue from TE-end. Thanks...!!
,
Feb 7 2018
Verified the fix in 66.0.3342.0 on Windows.
,
Feb 7 2018
,
Feb 7 2018
We won't block Chrome Desktop this week beta release as we already have RC ready and comment #3 is more applicable to Chrome OS. We will take this fix in next week M65 Beta for desktop once approved and merged. Thank you.
,
Feb 8 2018
This bug requires manual review: DEPS changes referenced in bugdroid comments. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 8 2018
LGTM for 65, DEPS does not appear to have a roll.
,
Feb 9 2018
Pls merge your change to M65 branch 3325 ASAP so we can pick it up for next M65 Beta release. Thank you.
,
Feb 9 2018
Can you let me know how to do that?
,
Feb 9 2018
RE #17: I'll merge this for you (save a day due to time zone differences). In general, see https://www.chromium.org/developers/how-tos/drover. See "Using Gerrit to cherry-pick CLs Instead". Use a branch target of "refs/branch-heads/3325"
,
Feb 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/77d57008436eeee89b5c3fb2c8ca8634bdf7c3ff commit 77d57008436eeee89b5c3fb2c8ca8634bdf7c3ff Author: Stuart Langley <slangley@chromium.org> Date: Fri Feb 09 16:35:39 2018 Fix regression introduced into copying images using the clipboard. M65 merge. The mojo work for web clipboard has introduced a regression when copying images. Originally the code was in content and was using net::EscapeForHTML to escape the URL of the image and the title. When converted to mojo and moved inside of blink this was changed to EncodeWithURLEscapeSequences which is a different encoding scheme. This change reverts to using an escaping the same as net::EscapeForHTML, however as net::EscapeForHTML uses std::string and we are working with WTF::String we have had to copy the code rather than re-use it. Bug: 806076 Change-Id: Ic51835c555c071571a837018d74a27fd455362cf Reviewed-on: https://chromium-review.googlesource.com/896762 Commit-Queue: Stuart Langley <slangley@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#534858}(cherry picked from commit 41dfe13dc2cfcfd4962e6147dbfc6b4f8e49bdc3) Reviewed-on: https://chromium-review.googlesource.com/911848 Reviewed-by: Eric Lawrence <elawrence@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#403} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/77d57008436eeee89b5c3fb2c8ca8634bdf7c3ff/third_party/WebKit/Source/platform/DEPS [modify] https://crrev.com/77d57008436eeee89b5c3fb2c8ca8634bdf7c3ff/third_party/WebKit/Source/platform/exported/WebClipboardImpl.cpp
,
Feb 11 2018
Thanks for merging this and for the instructions.
,
Feb 14 2018
Able to reproduce the issue on Win-10 using chrome reported version #66.0.3332.0 using the merged issue #806088 . Verified the fix on Win-10 using Chrome version #65.0.3325.73 as per the the merged issue #806088 . Note: Unable to verify the fix on OS-Mac as after downloading "clipspy" from internet did not support on mac. Attaching screen cast and screenshot for reference. Observed that "Copy Image" did not generate invalid HTML clip. Hence, the fix is working as expected. Adding the verified labels as per OS-Win. Thanks...!! |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by elawrence@chromium.org
, Jan 25 2018