Summary: | WebDriver: implement proxy support | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | WebDriver | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bburg, berto, clopez, ews-watchlist, gustavo, hi, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2017-12-05 02:54:29 PST
safaridriver work tracked by <rdar://problem/36854985>. For safaridriver, we'd need WKWebView support for proxy settings that hook into the NSURLSession. I suppose other network stacks have similar settings to propagate. Created attachment 383117 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Comment on attachment 383117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383117&action=review > Source/WebDriver/WebDriverService.cpp:367 > + if (proxy.type == "pac") { > + String autoconfigURL; > + if (!proxyObject.getString("proxyAutoconfigUrl"_s, autoconfigURL)) > + return WTF::nullopt; > + > + proxy.autoconfigURL = autoconfigURL; > + return proxy; > + } This type of proxy doesn't seem to be handled. Maybe returning an error is more according to the spec than silently accepting it and ignoring it? > Source/WebDriver/WebDriverService.cpp:415 > + if (proxyObject.getValue("socksProxy", value)) { > + String socksProxy; > + if (!value->asString(socksProxy)) > + return WTF::nullopt; > + > + proxy.socksURL = URL({ }, makeString("socks://", socksProxy)); > + if (!proxy.socksURL->isValid()) > + return WTF::nullopt; > + > + RefPtr<JSON::Value> socksVersionValue; > + if (!proxyObject.getValue("socksVersion", socksVersionValue)) > + return WTF::nullopt; > + > + auto socksVersion = unsignedValue(*socksVersionValue); > + if (!socksVersion || socksVersion.value() > 255) > + return WTF::nullopt; > + proxy.socksVersion = socksVersion.value(); > + } socksVersionValue seems to be ignored when constructing socksURL. It is true that webkit_network_proxy_settings_add_proxy_for_scheme() works with the three types of sock proxies if not a specific version is specified, but I think its better to be specific here is the user/application has requested a specific type of proxy. This is my suggestion: * proxy.socksURL should start with: - if socksVersionValue is invalid/missing or anything other than 4 or 5 -> socks:// - if socksVersionValue is 4 and socksURL is an IP Adresss -> socks4:// - if socksVersionValue is 4 and socksURL is a domain name -> socks4a:// - if socksVersionValue is 5 -> socks5:// Comment on attachment 383117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383117&action=review >> Source/WebDriver/WebDriverService.cpp:367 >> + } > > This type of proxy doesn't seem to be handled. > Maybe returning an error is more according to the spec than silently accepting it and ignoring it? Yes, I thought about it, I'll add a way to ask platform is a particular proxy types is supported. >> Source/WebDriver/WebDriverService.cpp:415 >> + } > > socksVersionValue seems to be ignored when constructing socksURL. > It is true that webkit_network_proxy_settings_add_proxy_for_scheme() works with the three types of sock proxies if not a specific version is specified, but I think its better to be specific here is the user/application has requested a specific type of proxy. > > This is my suggestion: > * proxy.socksURL should start with: > - if socksVersionValue is invalid/missing or anything other than 4 or 5 -> socks:// > - if socksVersionValue is 4 and socksURL is an IP Adresss -> socks4:// > - if socksVersionValue is 4 and socksURL is a domain name -> socks4a:// > - if socksVersionValue is 5 -> socks5:// I had no idea how to specify the socks version, I'll do what you suggest here. Created attachment 383257 [details]
Patch
Comment on attachment 383257 [details]
Patch
r=me. Thanks!
But before landing check build on wincairo EWS: It broke because platformSupportProxyType() is not defined there.
I forgot there's wincairo implementation of WebDriver Committed r252323: <https://trac.webkit.org/changeset/252323> |