Bug 36232 – [Qt] User name/password stripped from URLs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Doug Scheirer]

When I send a url of the format http://user@server/path into the webkit (using the QWebView::load() method), when the code gets to ResourceHandle::start(), the method uses the class's internal "d" pointer instead of using getInternal(). See ResourceHandleQt.cpp, lines 134-147 in Qt 4.6.1. At line 147 the method calls getInternal() to get a locally defined "d", but above it is using the object's "d", which is not initialized. See also loadResourceSynchronously(), which properly sets a local "d" pointer from which to get the user/password.
The net result is that by the time the network reply handler gets the URL, it does not have authentication data.

[Tor Arne Vestbø]

Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs

[Simon Hausmann]

AFAICS d and the return value of getInternal() point to the same thing: ResourceHandle's private data pointer.

I agree the last call to getInternal() is redundant, but otherwise I don't see the bug. What am I missing?

[Doug Scheirer]

(In reply to comment #2)
> AFAICS d and the return value of getInternal() point to the same thing:
> ResourceHandle's private data pointer.
> 
> I agree the last call to getInternal() is redundant, but otherwise I don't see
> the bug. What am I missing?

That I'm getting mislead by my debugger.  I think that the actual bug might be the if statement on 134:

if (!(d->m_user.isEmpty() || d->m_pass.isEmpty())

If a username is provided but not a password (which is my case), if (!(false || true)) == if (!(true)) == if (false).  Should this and other user/password checks be:

if (!(d->m_user.isEmpty() && d->m_pass.isEmpty())

[Simon Hausmann]

(In reply to comment #3)
> (In reply to comment #2)
> > AFAICS d and the return value of getInternal() point to the same thing:
> > ResourceHandle's private data pointer.
> > 
> > I agree the last call to getInternal() is redundant, but otherwise I don't see
> > the bug. What am I missing?
> 
> That I'm getting mislead by my debugger.  I think that the actual bug might be
> the if statement on 134:
> 
> if (!(d->m_user.isEmpty() || d->m_pass.isEmpty())
> 
> If a username is provided but not a password (which is my case), if (!(false ||
> true)) == if (!(true)) == if (false).  Should this and other user/password
> checks be:
> 
> if (!(d->m_user.isEmpty() && d->m_pass.isEmpty())

Yes, exactly! :)

Or slightly more readable:

    if (!d->m_user.isEmpty() || !d->m_pass.isEmpty()) {


Want to make a patch with ChangeLog and testcase/unit test in WebKit/qt/tests?

[Alexey Proskuryakov]

Can't this be tested with cross-platform tests? I'm actually surprised that we don't have an XMLHttpRequest test for empty password.

[Jan Erik Hanssen]

Created attachment 74709 [details]
Patch

[Jan Erik Hanssen]

Note that the case where you have no username but do have a password is broken, this is due to a bug in Qt which has been separately reported in http://bugreports.qt.nokia.com/browse/QTBUG-15566.

The test case for this has been added to the Skipped file in the proposed patch for the time being.

[Antonio Gomes]

what about gtk and chromium port? no need to skip? if so, what is the point of adding a test that do not run on any DRT?

[Alexey Proskuryakov]

+# mac presents an authentication dialog when no username or no password is present
+http/tests/xmlhttprequest/basic-auth-nouser.html
+http/tests/xmlhttprequest/basic-auth-nopassword.html

I'm not quite sure what behavior you are after, but it's possible to simulate authentication dialog handling in DumpRenderTree with layoutTestController.setAuthenticationUsername()/setAuthenticationPassword().

[Jan Erik Hanssen]

(In reply to comment #8)
> what about gtk and chromium port? no need to skip? if so, what is the point of adding a test that do not run on any DRT?

Works fine on chromium.

I just built gtk and that one seems to have the same issue as on Mac, meaning that this test implementation would have to be skipped there as well.

[Jan Erik Hanssen]

(In reply to comment #9)
> I'm not quite sure what behavior you are after, but it's possible to simulate authentication dialog handling in DumpRenderTree with layoutTestController.setAuthenticationUsername()/setAuthenticationPassword().

Ah right, that's probably what I should do then. I don't need to test the XMLHttpRequest behavior in particular. I'll update the test and propose a new patch.

Thanks.

[Jan Erik Hanssen]

Created attachment 74792 [details]
Patch

[Jan Erik Hanssen]

(In reply to comment #12)
> Created an attachment (id=74792) [details]
> Patch

I'm not entirely sure if it's safe to use http://localhost:8000/ in a test, will the http server always listen on that port? Works fine on mac, qt and chromium with the default settings at least.

[Jan Erik Hanssen]

Created attachment 74798 [details]
Patch

[Alexey Proskuryakov]

For future reference, see Jan Erik's comment in <http://bugreports.qt.nokia.com/browse/QTBUG-15566>: "certain routers (at least recent Cisco models like the E2100L) require you to log in with username blank and password 'admin' in order to change the username / password to something else. This won't work using QtWebKit."

> I'm not entirely sure if it's safe to use http://localhost:8000/ in a test

Theoretically, there is a way to change the port in run-webkit-tests, but a lot of existing tests just hardcode 8000. So yes, it's OK.

+        * platform/mac-leopard/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added.
+        * platform/mac-snowleopard/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added.
+        * platform/mac-tiger/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added.

I don't see any difference between these results, are there any? Otherwise, the -expected file should just go to platform/mac/http/tests/xmlhttprequest.

+        layoutTestController.queueLoad("http://:password@localhost:8000/xmlhttprequest/resources/basic-auth-nouserpass/basic-auth-nouserpass.php");

I don't know what queueLoad is - but the explanatory text says "Tests for XMLHttpRequest authentication", so maybe you should actually use XMLHttpRequest?

+            layoutTestController.setHandlesAuthenticationChallenges(true);
+            layoutTestController.setAuthenticationPassword("password");

Since we're really after empty username, perhaps it would be cleaner to call setAuthenticationUsername(""), too.

[Jan Erik Hanssen]

(In reply to comment #15)
> +        * platform/mac-leopard/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added.
> +        * platform/mac-snowleopard/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added.
> +        * platform/mac-tiger/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added.
> 
> I don't see any difference between these results, are there any? Otherwise, the -expected file should just go to platform/mac/http/tests/xmlhttprequest.

Yes, that's what I thought as well. However, when doing that then Chromium picks up that expected file for some reason and the test then fails. This is as of revision 72696.

> +        layoutTestController.queueLoad("http://:password@localhost:8000/xmlhttprequest/resources/basic-auth-nouserpass/basic-auth-nouserpass.php");
> 
> I don't know what queueLoad is - but the explanatory text says "Tests for XMLHttpRequest authentication", so maybe you should actually use XMLHttpRequest?

I had some issues with XMLHttpRequest on mac, it looked like the initial request would not get the username picked up, I had to schedule two of them. With queueLoad() the test passes on all platforms except GTK when just using one request.

> +            layoutTestController.setHandlesAuthenticationChallenges(true);
> +            layoutTestController.setAuthenticationPassword("password");
> 
> Since we're really after empty username, perhaps it would be cleaner to call setAuthenticationUsername(""), too.

Yes, I can do that.

[Jan Erik Hanssen]

(In reply to comment #16)
> (In reply to comment #15)
> > I don't see any difference between these results, are there any? Otherwise, the -expected file should just go to platform/mac/http/tests/xmlhttprequest.
> 
> Yes, that's what I thought as well. However, when doing that then Chromium picks up that expected file for some reason and the test then fails. This is as of revision 72696.

Looks like I can work around this by putting the default expected file into platform/chromium/http/tests/xmlhttprequest/

[Alexey Proskuryakov]

> I had some issues with XMLHttpRequest on mac, it looked like the initial request would not get the username picked up,

Nate (CC'ed) might have a comment about that.

Generally, for a non-trivial test like this, it's best to make it work outside DumpRenderTree as an option, so that we could compare behavior with other browsers.

> Looks like I can work around this by putting the default expected file into platform/chromium/http/tests/xmlhttprequest/

That's the right thing to do when Chromium results don't match Mac.

[Jan Erik Hanssen]

(In reply to comment #18)
> > I had some issues with XMLHttpRequest on mac, it looked like the initial request would not get the username picked up,
> 
> Nate (CC'ed) might have a comment about that.
> 
> Generally, for a non-trivial test like this, it's best to make it work outside DumpRenderTree as an option, so that we could compare behavior with other browsers.

Right. I had a look at this again and I still can't make XMLHttpRequest work the way I want on mac. If I don't call layoutTestController.setAuthenticationUsername() then no requests will get the username picked up, but if I do call that function then the first one still fails but subsequent ones succeeds.

Interrestingly enough the password-only case succeeds on mac, even with no calls to setAuthenticationPassword().

[Adam Barth]

Comment on attachment 74798 [details]
Patch

LGTM.

[WebKit Commit Bot]

Comment on attachment 74798 [details]
Patch

Rejecting attachment 74798 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--no-update', '--non-interactive', 74798]" exit_code: 2
Last 500 characters of output:
xmlhttprequest/basic-auth-nopassword-expected.txt
patching file LayoutTests/platform/qt/Skipped
Hunk #1 succeeded at 275 with fuzz 2 (offset -14 lines).
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/platform/network/qt/ResourceHandleQt.cpp
Hunk #1 succeeded at 124 (offset 1 line).
Hunk #2 succeeded at 186 (offset -6 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7239143

[Jan Erik Hanssen]

Created attachment 77390 [details]
Patch

Fix the patch not applying problem

[Ariya Hidayat]

Manually landed in r74609
http://trac.webkit.org/changeset/74609

[Ariya Hidayat]

Comment on attachment 77390 [details]
Patch

Clearing the r and cq flags.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74609 might have broken GTK Linux 64-bit Debug

[Ariya Hidayat]

GTK bot should be fixed in r74613: http://trac.webkit.org/changeset/74613