Bug 185224

Created attachment 339360 [details] FIX

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 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 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?

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 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 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 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.

This is the last patch to enable WK2 for WinCairo. What's the status?

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

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.

Created attachment 340680 [details] Patch

Committed r231947: <https://trac.webkit.org/changeset/231947>

<rdar://problem/40358534>