Bug 127421 - [curl] Improve realm string parsing in WWW-Authenticate headers
Summary: [curl] Improve realm string parsing in WWW-Authenticate headers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-22 03:24 PST by Szabolcs David
Modified: 2014-01-30 10:57 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch (1.82 KB, patch)
2014-01-22 03:26 PST, Szabolcs David
no flags Details | Formatted Diff | Diff
Proposed patch II. (2.31 KB, patch)
2014-01-23 02:36 PST, Szabolcs David
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.