53320 2011-01-28 11:59:17 -0800 [Qt] QtWebKit does not properly handle D&D of a percent-encoded URL 2011-02-13 12:14:26 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED EasyFix, Qt, QtTriaged P2 Normal --- 1 adawit webkit-unassigned aparna.nand benjamin cmarcelo kling oldest_to_newest 341599 0 adawit 2011-01-28 11:59:17 -0800 When a percent encoded URL is dragged and dropped into QtWebKit (read: QWebView), the QNetworkRequest that is generated will contain an invalid URL. That is because the URL is not properly encoded before being used to create the request. To test, open the test case attached in QtTestBrowser and drag "drag me" link to a second instance of QtTestBrowser. I believe this bug is caused by the fact that the dragged URL is passed in unencoded format (QUrl::fromEncoded), but the attempt re-encode in QtWebKit's implementation of DragData::asURL in WebCore/platform/qt/DragDataQt.cpp does not seem to do the job properly. In fact to me the call to encodeWithURLEscapeSequences with the string representation of of the URL seems totally wrong to me since the characters allowed within a URL vary based in each of its separate components (scheme, authority, path, query etc...). 341603 1 80475 adawit 2011-01-28 12:00:54 -0800 Created attachment 80475 Test case... 341729 2 benjamin 2011-01-28 14:15:54 -0800 Broken on trunk, good catch. 341769 3 adawit 2011-01-28 15:09:13 -0800 (In reply to comment #2) > Broken on trunk, good catch. Thanks but I cannot take credit for this... It was reported downstream. See https://bugs.kde.org/show_bug.cgi?id=263788. 341777 4 adawit 2011-01-28 15:14:28 -0800 (In reply to comment #0) > I believe this bug is caused by the fact that the dragged URL is passed in unencoded format (QUrl::fromEncoded), but the attempt re-encode in QtWebKit's implementation of DragData::asURL in WebCore/platform/qt/DragDataQt.cpp does not seem to do the job properly. In fact to me the call to encodeWithURLEscapeSequences with the string representation of of the URL seems totally wrong to me since the characters allowed within a URL vary based in each of its separate components (scheme, authority, path, query etc...). I am not longer sure whether the cause for the bug is the call to encodeWithURLEscapeSequences. Maninly because I fail to see how it can be that different from QUrl::toPrecentEncoding... However, I can confirm that the URL sent through the QDropEvent received when one reimplements QWebPage::dropEvent is most definitely not precent encoded. 341783 5 adawit 2011-01-28 15:17:55 -0800 (In reply to comment #2) > Broken on trunk, good catch. For the record it is also broken in QtWebKit 2.2 which is what I tested this one. I am almost certain the original bug reporter is using the version of QtWebKit that came with Qt. As such it is likely broken in QtWebKit 2.0 as well... 350276 6 82235 aparna.nand 2011-02-12 09:41:50 -0800 Created attachment 82235 Patch for review This bug is due to the wrong encoding done in encodeWithURLEscapeSequences() function. For example - The url passed - https://www.google.com/accounts/ServiceLogin?service=mail&passive=true&rm=false&continue=http://mail.google.com/mail/?ui=html&zy=l&bsv=llya694le36z&scc=1&ltmpl=default&ltmplcache=2 - The url returned - https://www.google.com/accounts/ServiceLogin%3Fservice=mail&passive=true&rm=false&continue=http://mail.google.com/mail/%3Fui=html&zy=l&bsv=llya694le36z&scc=1&ltmpl=default&ltmplcache=2 - The url expected - https://www.google.com/accounts/ServiceLogin?service=mail&passive=true&rm=false&continue=http%3A%2F%2Fmail.google.com%2Fmail%2F%3Fui%3Dhtml%26zy%3Dl&bsv=llya694le36z&scc=1&ltmpl=default&ltmplcache=2 The expected URL is obtained when using QUrl's function toEncoded(). The expected URL (in the encoded form) is infact the url that is there in the html href tag. As per this analysis there are 2 ways to solve this bug. 1. As in the patch encode the url using toEncoded() 2. Do not do any encoding. Convert the input QUrl to String and pass it back. Replace the fix line with return String(urls().first().toString()); 350277 7 aparna.nand 2011-02-12 09:46:14 -0800 ..Continuing the analysis part (pressed commit button by mistake) The second approach is the approach taken by the gtk port for example. In this approach the string does not get encoded at this point. I took the first approach: 1. It is always better to encode at this point rather then passing the human readable url 2. The encoded url is same as the one in html. 350391 8 82235 kling 2011-02-13 06:23:47 -0800 Comment on attachment 82235 Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=82235&action=review Patch looks sensible, just one thing: > Source/WebCore/platform/qt/DragDataQt.cpp:128 > + return String(urls.first().toEncoded().constData()); You should use the String constructor that takes a length parameter in addition to the const char*, i.e: QByteArray encodedUrl = urls.first().toEncoded(); return String(encodedUrl.constData(), encodedUrl.length()); 350430 9 82272 aparna.nand 2011-02-13 11:45:04 -0800 Created attachment 82272 Patch with comments implemented Implemented the changes given in the review. 350434 10 82272 kling 2011-02-13 11:50:00 -0800 Comment on attachment 82272 Patch with comments implemented View in context: https://bugs.webkit.org/attachment.cgi?id=82272&action=review r=me > Source/WebCore/ChangeLog:5 > + [Qt] QtWebKit does not properly handle D&D of a precent encoded URL... Typo, percent. 350435 11 kling 2011-02-13 12:14:26 -0800 Landed in <http://trac.webkit.org/changeset/78438> 80475 2011-01-28 12:00:54 -0800 2011-01-28 12:00:54 -0800 Test case... test_link_drag_drop.html text/html 304 adawit PGh0bWw+CjxoZWFkPgo8dGl0bGU+VGVzdCBMaW5rIERyYWcgYW5kIERyb3A8L3RpdGxlPgo8L2hl YWQ+Cjxib2R5Pgo8YSBocmVmPSJodHRwczovL3d3dy5nb29nbGUuY29tL2FjY291bnRzL1NlcnZp Y2VMb2dpbj9zZXJ2aWNlPW1haWwmcGFzc2l2ZT10cnVlJnJtPWZhbHNlJmNvbnRpbnVlPWh0dHAl M0ElMkYlMkZtYWlsLmdvb2dsZS5jb20lMkZtYWlsJTJGJTNGdWklM0RodG1sJTI2enklM0RsJmJz dj1sbHlhNjk0bGUzNnomc2NjPTEmbHRtcGw9ZGVmYXVsdCZsdG1wbGNhY2hlPTIiPkRyYWcgbWU8 L2E+CjwvYm9keT4KPC9odG1sPg== 82235 2011-02-12 09:41:50 -0800 2011-02-13 06:23:47 -0800 Patch for review patch_53320 text/plain 1308 aparna.nand ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBjNWM1OTRjLi43NzI0MWQyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTEtMDItMTIgIEFwYXJuYSBOYW5keWFsICA8YXBhcm5hLm5hbmRAd2lwcm8uY29tPgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtRdF0gUXRX ZWJLaXQgZG9lcyBub3QgcHJvcGVybHkgaGFuZGxlIEQmRCBvZiBhIHByZWNlbnQgZW5jb2RlZCBV UkwuLi4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUz MzIwCisKKyAgICAgICAgVGhlIGVuY29kaW5nIHRoYXQgd2FzIGRvbmUgaXMgY29ycmVjdGVkIGlu IHRoZSBmaXguICAKKyAgICAgICAgUmVwbGFjZWQgdGhlIEtVUkwgZW5jb2RpbmcgZnVuY3Rpb24g d2l0aCBRVXJsIEFQSS4KKworICAgICAgICAqIHBsYXRmb3JtL3F0L0RyYWdEYXRhUXQuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6RHJhZ0RhdGE6OmFzVVJMKToKKwogMjAxMS0wMi0xMSAgQ2hyaXMg Um9nZXJzICA8Y3JvZ2Vyc0Bnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEtlbm5l dGggUnVzc2VsbC4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3F0L0RyYWdE YXRhUXQuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vcXQvRHJhZ0RhdGFRdC5jcHAKaW5k ZXggZjY4YWQxZC4uODVmMWI3ZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v cXQvRHJhZ0RhdGFRdC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vcXQvRHJhZ0Rh dGFRdC5jcHAKQEAgLTEyNSw3ICsxMjUsNyBAQCBTdHJpbmcgRHJhZ0RhdGE6OmFzVVJMKEZyYW1l KiwgRmlsZW5hbWVDb252ZXJzaW9uUG9saWN5IGZpbGVuYW1lUG9saWN5LCBTdHJpbmcqKQogICAg IGlmICh1cmxzLmlzRW1wdHkoKSkKICAgICAgICAgcmV0dXJuIFN0cmluZygpOwogCi0gICAgcmV0 dXJuIGVuY29kZVdpdGhVUkxFc2NhcGVTZXF1ZW5jZXModXJscy5maXJzdCgpLnRvU3RyaW5nKCkp OworICAgIHJldHVybiBTdHJpbmcodXJscy5maXJzdCgpLnRvRW5jb2RlZCgpLmNvbnN0RGF0YSgp KTsKIH0KIAogUGFzc1JlZlB0cjxEb2N1bWVudEZyYWdtZW50PiBEcmFnRGF0YTo6YXNGcmFnbWVu dChGcmFtZSogZnJhbWUsIFBhc3NSZWZQdHI8UmFuZ2U+LCBib29sLCBib29sJikgY29uc3QK 82272 2011-02-13 11:45:04 -0800 2011-02-13 11:50:00 -0800 Patch with comments implemented patch_updated_53320 text/plain 1370 aparna.nand ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBjNWM1OTRjLi43NzI0MWQyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTEtMDItMTIgIEFwYXJuYSBOYW5keWFsICA8YXBhcm5hLm5hbmRAd2lwcm8uY29tPgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtRdF0gUXRX ZWJLaXQgZG9lcyBub3QgcHJvcGVybHkgaGFuZGxlIEQmRCBvZiBhIHByZWNlbnQgZW5jb2RlZCBV UkwuLi4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUz MzIwCisKKyAgICAgICAgVGhlIGVuY29kaW5nIHRoYXQgd2FzIGRvbmUgaXMgY29ycmVjdGVkIGlu IHRoZSBmaXguICAKKyAgICAgICAgUmVwbGFjZWQgdGhlIEtVUkwgZW5jb2RpbmcgZnVuY3Rpb24g d2l0aCBRVXJsIEFQSS4KKworICAgICAgICAqIHBsYXRmb3JtL3F0L0RyYWdEYXRhUXQuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6RHJhZ0RhdGE6OmFzVVJMKToKKwogMjAxMS0wMi0xMSAgQ2hyaXMg Um9nZXJzICA8Y3JvZ2Vyc0Bnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEtlbm5l dGggUnVzc2VsbC4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3F0L0RyYWdE YXRhUXQuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vcXQvRHJhZ0RhdGFRdC5jcHAKaW5k ZXggZjY4YWQxZC4uMjU1NGRmNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v cXQvRHJhZ0RhdGFRdC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vcXQvRHJhZ0Rh dGFRdC5jcHAKQEAgLTEyNSw3ICsxMjUsOCBAQCBTdHJpbmcgRHJhZ0RhdGE6OmFzVVJMKEZyYW1l KiwgRmlsZW5hbWVDb252ZXJzaW9uUG9saWN5IGZpbGVuYW1lUG9saWN5LCBTdHJpbmcqKQogICAg IGlmICh1cmxzLmlzRW1wdHkoKSkKICAgICAgICAgcmV0dXJuIFN0cmluZygpOwogCi0gICAgcmV0 dXJuIGVuY29kZVdpdGhVUkxFc2NhcGVTZXF1ZW5jZXModXJscy5maXJzdCgpLnRvU3RyaW5nKCkp OworICAgIFFCeXRlQXJyYXkgZW5jb2RlZFVybCA9IHVybHMuZmlyc3QoKS50b0VuY29kZWQoKTsK KyAgICByZXR1cm4gU3RyaW5nKGVuY29kZWRVcmwuY29uc3REYXRhKCksIGVuY29kZWRVcmwubGVu Z3RoKCkpOwogfQogCiBQYXNzUmVmUHRyPERvY3VtZW50RnJhZ21lbnQ+IERyYWdEYXRhOjphc0Zy YWdtZW50KEZyYW1lKiBmcmFtZSwgUGFzc1JlZlB0cjxSYW5nZT4sIGJvb2wsIGJvb2wmKSBjb25z dAo=