Bug 38956 – [Gtk] http/tests/xmlhttprequest/basic-auth-default.html fails

• Component: Page Loading
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

This means that loader doesn't preemptively send Basic credentials to directories that are known to be in a certain protection space.

This is not a bug per RFC 2616 - mostly a performance issue.

[Sergio Villar Senin]

Created attachment 69924 [details]
Fix basic authentication in WebKitGtk+

libsoup does support sending credentials to directories under known protection spaces. The problem was basically located in soup_uri_to_string() function used to create the request. That function never returns the password, that's why authentication fails. This patch provides a wrapper over that function that makes this test work properly.

[Martin Robinson]

Comment on attachment 69924 [details]
Fix basic authentication in WebKitGtk+

View in context: https://bugs.webkit.org/attachment.cgi?id=69924&action=review

Great patch. I have a few suggestions, but I like the way this fix is structured.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:755
> +    ResourceHandleInternal* d = handle->getInternal();
> +
> +    d->m_context = context;

I don't understand how this change is related. Was it included by accident?

> WebCore/platform/network/soup/ResourceRequestSoup.cpp:28
> +#include "UriSoup.h"

I think I'd prefer this include to be called SoupURI or SoupURIUtils to match the type name.

> WebCore/platform/network/soup/UriSoup.cpp:27
> +gchar*
> +uriSoupToStringWithPassword(SoupURI* soupUri)

No newline needed after the return type and the parameter should be soupURI. IMO, it should also be soupURITo<something> (see below).

> WebCore/platform/network/soup/UriSoup.cpp:42
> +    gchar* url = soup_uri_to_string(soupUri, FALSE);
> +
> +    if (!soupUri->password)
> +        return url;
> +
> +    GString* stringUrl = g_string_sized_new(strlen(url) + strlen(soupUri->password) + 1);
> +    const gchar* at = strchr(url, '@');
> +
> +    stringUrl = g_string_append(stringUrl, url);
> +    stringUrl = g_string_insert_c(stringUrl, at - url, ':');
> +    stringUrl = g_string_insert(stringUrl, at - url + 1, soupUri->password);
> +    g_free(url);
> +
> +    return g_string_free(stringUrl, FALSE);

Perhaps this method should just return a KURL. That might future-proof it in case soup ever starts including the password. It could just be something like this:

GOwnPtr<gchar> urlString(soup_uri_toString(soupURI));
KURL url(KURL(), String::fromUTF8(urlString.get()));
if (!soupURI->password)
    return url;
url->setPass(String::fromUTF8(soupURI->password);
return url;

You should also put a comment here with a link to the bug and a little text describing the issue.

> WebCore/platform/network/soup/UriSoup.h:3
> + * Copyright (C) 2008 Xan Lopez <[e-mail]>
> + * Copyright (C) 2008 Apple Inc. All rights reserved.

There should just be an Igalia line here and 2010. :)

> WebCore/platform/network/soup/UriSoup.h:30
> +#include <libsoup/soup.h>

I'd prefer a forward declaration here instead of a libsoup include.

[Sergio Villar Senin]

(In reply to comment #2)
> (From update of attachment 69924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69924&action=review
> 
> Great patch. I have a few suggestions, but I like the way this fix is structured.
> 
> > WebCore/platform/network/soup/ResourceHandleSoup.cpp:755
> > +    ResourceHandleInternal* d = handle->getInternal();
> > +
> > +    d->m_context = context;
> 
> I don't understand how this change is related. Was it included by accident?

Actually it's not related but I think it's missing anyway.
 
> > WebCore/platform/network/soup/ResourceRequestSoup.cpp:28
> > +#include "UriSoup.h"
> 
> I think I'd prefer this include to be called SoupURI or SoupURIUtils to match the type name.

Not strong feelings about that, but take into account that we currently have CookieJarSoup or GOwnPtrSoup. That was the reason why I chose uri before Soup.
 
> > WebCore/platform/network/soup/UriSoup.cpp:27
> > +gchar*
> > +uriSoupToStringWithPassword(SoupURI* soupUri)
> 
> No newline needed after the return type and the parameter should be soupURI. IMO, it should also be soupURITo<something> (see below).
> 
> > WebCore/platform/network/soup/UriSoup.cpp:42
> > +    gchar* url = soup_uri_to_string(soupUri, FALSE);
> > +
> > +    if (!soupUri->password)
> > +        return url;
> > +
> > +    GString* stringUrl = g_string_sized_new(strlen(url) + strlen(soupUri->password) + 1);
> > +    const gchar* at = strchr(url, '@');
> > +
> > +    stringUrl = g_string_append(stringUrl, url);
> > +    stringUrl = g_string_insert_c(stringUrl, at - url, ':');
> > +    stringUrl = g_string_insert(stringUrl, at - url + 1, soupUri->password);
> > +    g_free(url);
> > +
> > +    return g_string_free(stringUrl, FALSE);
> 
> Perhaps this method should just return a KURL. That might future-proof it in case soup ever starts including the password. It could just be something like this:
> 
> GOwnPtr<gchar> urlString(soup_uri_toString(soupURI));
> KURL url(KURL(), String::fromUTF8(urlString.get()));
> if (!soupURI->password)
>     return url;
> url->setPass(String::fromUTF8(soupURI->password);
> return url;

I like your approach.
 
> You should also put a comment here with a link to the bug and a little text describing the issue.
> 
> > WebCore/platform/network/soup/UriSoup.h:3
> > + * Copyright (C) 2008 Xan Lopez <[e-mail]>
> > + * Copyright (C) 2008 Apple Inc. All rights reserved.
> 
> There should just be an Igalia line here and 2010. :)

Ups, copy&paste badness I guess :)
 
> > WebCore/platform/network/soup/UriSoup.h:30
> > +#include <libsoup/soup.h>
> 
> I'd prefer a forward declaration here instead of a libsoup include.

Ok

[Sergio Villar Senin]

Created attachment 71921 [details]
Fix basic authentication in WebKitGtk+

[WebKit Review Bot]

Attachment 71921 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/network/soup/SoupURIUtils.h:26:  #ifndef header guard has wrong style, please use: SoupURIUtils_h  [build/header_guard] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 71921 [details]
Fix basic authentication in WebKitGtk+

View in context: https://bugs.webkit.org/attachment.cgi?id=71921&action=review

This looks great! There are just a couple small issues.

> WebCore/ChangeLog:11
> +        Added a new utility function that turns SoupURI's into
> +        KURL's. That addresses some issues with SoupURI's like for example
> +        soup_uri_to_string ignoring the URI password. Basic authentication
> +        using URL credentials should work now.

I think all the SoupURI's and KURL's should be SoupURIs and KURLs.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:759
> +    ResourceHandleInternal* d = handle->getInternal();
> +
> +    d->m_context = context;
>      handle->start(context);

How does this relate to the rest of the patch? It's very unclear to me. Perhaps a note in the ChangeLog at line 16.

> WebCore/platform/network/soup/SoupURIUtils.h:27
> +#ifndef UriSoup_h
> +#define UriSoup_h

You should update the header guard.

[Sergio Villar Senin]

Created attachment 71996 [details]
Fix basic authentication in WebKitGtk+

[Martin Robinson]

Comment on attachment 71996 [details]
Fix basic authentication in WebKitGtk+

Thanks! I'll land this one.

[Martin Robinson]

Committed r70651: <http://trac.webkit.org/changeset/70651>