Bug 185266 – [Curl] Enable Proxy Authentication.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Basuke Suzuki]

Add support for proxy authentication to curl backend.

[Basuke Suzuki]

Created attachment 339467 [details]
PATCH

[Don Olmstead]

Pinging potential reviewers since this has been hanging out for over a month now.

[Alex Christensen]

Comment on attachment 339467 [details]
PATCH

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

This code is in general a little hard to follow.

> Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:67
> +int AuthenticationChallenge::getPort(const URL& url)

This should have a better name.  It's not just getting the port.

> Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:80
> +    auto protocol = url.protocol().toString();
> +    if (protocol == "socks4" || protocol == "socks4a" || protocol == "socks5" || protocol == "socks5h")

We shouldn't need to make a String just to compare it.

> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:75
> +    if (authMethod & CURLAUTH_NTLM)

Should these be else ifs?

[Basuke Suzuki]

Comment on attachment 339467 [details]
PATCH

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

>> Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:67
>> +int AuthenticationChallenge::getPort(const URL& url)
> 
> This should have a better name.  It's not just getting the port.

How about determinPortForProxy()?

>> Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:80
>> +    if (protocol == "socks4" || protocol == "socks4a" || protocol == "socks5" || protocol == "socks5h")
> 
> We shouldn't need to make a String just to compare it.

Of course.

>> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:75
>> +    if (authMethod & CURLAUTH_NTLM)
> 
> Should these be else ifs?

Right.

[Basuke Suzuki]

Created attachment 343088 [details]
FIX

Fixed reviewed points. Also rebased to HEAD.

[Alex Christensen]

Comment on attachment 343088 [details]
FIX

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

> Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:68
> +int AuthenticationChallenge::determineProxyPort(const URL& url)

This should return a std::optional<uint16_t>

[Basuke Suzuki]

Created attachment 343116 [details]
FIX

Thanks for r+ > Alex

[WebKit Commit Bot]

Comment on attachment 343116 [details]
FIX

Clearing flags on attachment: 343116

Committed r232992: <https://trac.webkit.org/changeset/232992>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41269224>