Bug 53320 – [Qt] QtWebKit does not properly handle D&D of a percent-encoded URL

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dawit A.]

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...).

[Dawit A.]

Created attachment 80475 [details]
Test case...

[Benjamin Poulain]

Broken on trunk, good catch.

[Dawit A.]

(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.

[Dawit A.]

(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.

[Dawit A.]

(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...

[Aparna Nandyal]

Created attachment 82235 [details]
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());

[Aparna Nandyal]

..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.

[Andreas Kling]

Comment on attachment 82235 [details]
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());

[Aparna Nandyal]

Created attachment 82272 [details]
Patch with comments implemented

Implemented the changes given in the review.

[Andreas Kling]

Comment on attachment 82272 [details]
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.

[Andreas Kling]

Landed in <http://trac.webkit.org/changeset/78438>