Bug 180408 – WebDriver: implement proxy support

• Component: WebDriver
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

https://w3c.github.io/webdriver/webdriver-spec.html#proxy

[Blaze Burg]

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.

[Carlos Garcia Campos]

Created attachment 383117 [details]
Patch

[EWS Watchlist]

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

[Carlos Alberto Lopez Perez]

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://

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 383257 [details]
Patch

[Carlos Alberto Lopez Perez]

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.

[Carlos Garcia Campos]

I forgot there's wincairo implementation of WebDriver

[Carlos Garcia Campos]

Committed r252323: <https://trac.webkit.org/changeset/252323>

[Radar WebKit Bug Importer]

<rdar://problem/57099330>