Bug 127421

Summary: [curl] Improve realm string parsing in WWW-Authenticate headers
Product: WebKit Reporter: Szabolcs David <davidsz>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, galpeter
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
none
Proposed patch II. none

Description Szabolcs David 2014-01-22 03:24:13 PST
The realm string contains quotes at the beginning and end - this is the opposite of the libsoup implementation. Furthermore, if the header is concatenated from two or more another headers, it contains more incorrect part.

For example, if the header is:
WWW-Authenticate: Basic realm="First realm", Basic realm="Second realm"

realm string will be:
"First realm", Basic realm="Second realm"
Comment 1 Szabolcs David 2014-01-22 03:26:58 PST
Created attachment 221852 [details]
Proposed patch
Comment 2 Brent Fulgham 2014-01-22 09:54:16 PST
Comment on attachment 221852 [details]
Proposed patch

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

Looks good. I have a couple of minor comments for your consideration.

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:351
>      String authHeader = response.httpHeaderField("WWW-Authenticate");

I just noticed that this could probably be const, since we aren't modifying it.

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:358
> +        if (realm.startsWith('"') && realm.endsWith('"') && realm.length() > 1)

What happens if we get the input ""? Are we supposed to create a protection space for the "" realm? Or should we be bailing out early?

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:359
> +            realm = realm.substring(1, realm.length()-2);

This might be clearer if it were wrapped up as a little function:

static void removeLeadingAndTrailingQuotes(String& value) ...
Comment 3 Szabolcs David 2014-01-23 02:36:06 PST
Created attachment 221966 [details]
Proposed patch II.

Thanks for your comments!

> What happens if we get the input ""? Are we supposed to create a protection space for the "" realm? Or should we be bailing out early?

I think we should accept the empty string as realm, because the most popular browsers accept it too.
Comment 4 Brent Fulgham 2014-01-30 10:30:09 PST
Comment on attachment 221966 [details]
Proposed patch II.

r=me
Comment 5 WebKit Commit Bot 2014-01-30 10:57:46 PST
Comment on attachment 221966 [details]
Proposed patch II.

Clearing flags on attachment: 221966

Committed r163091: <http://trac.webkit.org/changeset/163091>
Comment 6 WebKit Commit Bot 2014-01-30 10:57:48 PST
All reviewed patches have been landed.  Closing bug.