Bug 185224

Summary: [Curl] Remove unused SystemProxyWin.cpp
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: WebKit2Assignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, Basuke.Suzuki, Hironori.Fujii, koivisto, pvollan, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 174003    
Attachments:
Description Flags
FIX
none
FIX
none
Patch koivisto: review+

Description Basuke Suzuki 2018-05-02 15:45:16 PDT
Curl network layer has a new proxy setting interface since r230973: <https://trac.webkit.org/changeset/230973>/
Comment 1 Basuke Suzuki 2018-05-02 15:51:56 PDT
Created attachment 339360 [details]
FIX
Comment 2 Per Arne Vollan 2018-05-03 13:30:29 PDT
Comment on attachment 339360 [details]
FIX

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

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:68
> +    WebCore::CurlProxySettings settings(WebCore::URL(WebCore::ParsedURLString, String::format("http://%s:%d/", proxy, port)), String());

Is this guaranteed to be a well-formed url?
Comment 3 Basuke Suzuki 2018-05-03 14:35:35 PDT
Comment on attachment 339360 [details]
FIX

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

Thanks for the review.

>> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:68
>> +    WebCore::CurlProxySettings settings(WebCore::URL(WebCore::ParsedURLString, String::format("http://%s:%d/", proxy, port)), String());
> 
> Is this guaranteed to be a well-formed url?

With our internal interface, `proxy` is passed as a host name. From here, url is parsed in the CurlProxySettings class and host name is extracted from the parsed result. I don't think there's security issue hidden here.
Comment 4 Per Arne Vollan 2018-05-04 15:55:19 PDT
Comment on attachment 339360 [details]
FIX

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

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:66
>  void WindowsSystemProxy::setCurlHttpProxy(char* proxy, int port)

I think we could pass the first parameter as a String reference instead of a char pointer. Also, how about renaming it to host or hostname?
Comment 5 Basuke Suzuki 2018-05-07 16:09:51 PDT
Created attachment 339763 [details]
FIX

Add host string normalization. Also rewrote to modernize other static methods in the file to read Windows registry information.
Comment 6 Basuke Suzuki 2018-05-07 16:28:55 PDT
Comment on attachment 339763 [details]
FIX

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

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:43
> +static String normalizedHost(const String& host)

Use only host part of original string even if the string like `xxx:yyy` if passed.

> Source/WebKit/NetworkProcess/win/SystemProxyWin.h:-34
> -    static bool getSystemHttpsProxy(char* buffer, int bufferLen, int* port);

https and ftp version was not defined. Deleted.

> Source/WebKit/NetworkProcess/win/SystemProxyWin.h:34
> +    static std::optional<std::pair<String, unsigned>> getSystemHttpProxy();

Instead of bool + out variable, use optional and pair.

> Source/WebKit/NetworkProcess/win/SystemProxyWin.h:36
> +    static void setCurlHttpProxy(const String& host, unsigned port);

use unsigned to match with WebCore Url's port type.

> Source/WebKit/NetworkProcess/win/SystemProxyWin.h:41
> +    static std::optional<Vector<TCHAR>> readRegistryValue(const TCHAR* path, const TCHAR* valueName);

Extract registry value reading code.
Comment 7 Per Arne Vollan 2018-05-09 08:01:50 PDT
Comment on attachment 339763 [details]
FIX

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

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:78
> +std::optional<std::pair<String, unsigned>> WindowsSystemProxy::parseProxyString(Vector<TCHAR>&& buffer)

Is it possible to use the URL or URLParser class to parse the proxy string?
Comment 8 Basuke Suzuki 2018-05-09 11:07:01 PDT
Comment on attachment 339763 [details]
FIX

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

>> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:78
>> +std::optional<std::pair<String, unsigned>> WindowsSystemProxy::parseProxyString(Vector<TCHAR>&& buffer)
> 
> Is it possible to use the URL or URLParser class to parse the proxy string?

It seems neither URL nor URLParser can handle HOST:PORT string. Also I have to update my code to handle advanced format of the registry. I'll send another patch soon.
Comment 9 Fujii Hironori 2018-05-17 18:08:12 PDT
This is the last patch to enable WK2 for WinCairo. What's the status?
Comment 10 youenn fablet 2018-05-17 18:58:38 PDT
Comment on attachment 339763 [details]
FIX

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

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:50
> +    WebCore::URL url { WebCore::URL(), buffer.toString() };

You can probably write it as return WebCore:URL { WebCore::URL(), makeString("http://", host) };
But maybe it would be simpler to just search for ':' and create a substring if needed?

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:61
> +    buffer.appendNumber(port);

Might be able to use makeString also with StringConcatenateNumbers.h
Comment 11 Fujii Hironori 2018-05-17 22:25:50 PDT
Thank you for the review, youenn.
I had a discussion with Basuke.
We agreed that this bug should be split into two parts, the build fix and the improvement.

At the moment, SystemProxyWin is not used at all even though it is compiled.
So, I will just remove SystemProxyWin.cpp in this bug.

Basuke will work on the improvement part.
Comment 12 Fujii Hironori 2018-05-17 22:32:10 PDT
Created attachment 340680 [details]
Patch
Comment 13 Fujii Hironori 2018-05-18 00:41:29 PDT
Committed r231947: <https://trac.webkit.org/changeset/231947>
Comment 14 Radar WebKit Bug Importer 2018-05-18 00:42:20 PDT
<rdar://problem/40358534>