Bug 185266

Summary: [Curl] Enable Proxy Authentication.
Product: WebKit Reporter: Basuke Suzuki <basuke>
Component: WebCore Misc.Assignee: Basuke Suzuki <basuke>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, basuke, bfulgham, commit-queue, don.olmstead, ews-watchlist, galpeter, pvollan, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=188998
Attachments:
Description Flags
PATCH
none
FIX
achristensen: review+, achristensen: commit-queue-
FIX none

Basuke Suzuki
Reported 2018-05-03 14:04:38 PDT
Add support for proxy authentication to curl backend.
Attachments
PATCH (17.88 KB, patch)
2018-05-03 14:28 PDT, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
FIX (18.60 KB, patch)
2018-06-19 12:55 PDT, Basuke Suzuki 		achristensen: review+
achristensen: commit-queue-
DetailsFormatted DiffDiff
FIX (18.65 KB, patch)
2018-06-19 15:40 PDT, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Basuke Suzuki
Comment 1 2018-05-03 14:28:22 PDT
Created attachment 339467 [details] PATCH
Don Olmstead
Comment 2 2018-06-13 13:02:34 PDT
Pinging potential reviewers since this has been hanging out for over a month now.
Alex Christensen
Comment 3 2018-06-13 13:23:56 PDT
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 4 2018-06-14 16:44:53 PDT
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
Comment 5 2018-06-19 12:55:52 PDT
Created attachment 343088 [details] FIX Fixed reviewed points. Also rebased to HEAD.
Alex Christensen
Comment 6 2018-06-19 13:57:25 PDT
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
Comment 7 2018-06-19 15:40:35 PDT
Created attachment 343116 [details] FIX Thanks for r+ > Alex
WebKit Commit Bot
Comment 8 2018-06-19 16:27:04 PDT
Comment on attachment 343116 [details] FIX Clearing flags on attachment: 343116 Committed r232992: <https://trac.webkit.org/changeset/232992>
WebKit Commit Bot
Comment 9 2018-06-19 16:27:06 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 10 2018-06-19 16:28:18 PDT
<rdar://problem/41269224>
Note You need to log in before you can comment on or make changes to this bug.