[webkit-reviews] review granted: [Bug 31494] Add unauthenticated proxy support to SocketStreamHandleCFNet : [Attachment 43205] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 31494: Add unauthenticated proxy support to SocketStreamHandleCFNet
https://bugs.webkit.org/show_bug.cgi?id=31494

Attachment 43205: proposed patch
https://bugs.webkit.org/attachment.cgi?id=43205&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>  void SocketStreamHandle::chooseProxy()
>  {
> -    // FIXME: Retrieve proxy information.
> +#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)
> +    RetainPtr<CFDictionaryRef> proxyDictionary(AdoptCF,
CFNetworkCopySystemProxySettings());
> +#else
> +    // We don't need proxy information often, so there is no need to set up
a permanent dynamic store session.
> +    RetainPtr<CFDictionaryRef> proxyDictionary(AdoptCF,
SCDynamicStoreCopyProxies(0));
> +#endif
> +
> +    // SOCKS or HTTPS (AKA CONNECT) proxies are supported.
> +    // WebSocket protocol relies on handshake being transferred unchanged,
so we need a proxy that will not modify headers.
> +    // Since HTTP proxies must add Via headers, they are highly unlikely to
work.
> +    // Many CONNECT proxies limit connectivity to port 443, so we prefer
SOCKS, if configured.
> +
> +    if (!proxyDictionary) {
> +	   m_connectionType = Direct;
> +	   return;
> +    }
> +
> +#ifndef BUILDING_ON_TIGER
> +    // CFNetworkCopyProxiesForURL doesn't know about WebSocket schemes, so
pretend to use http.
> +    // Always use "https" to get HTTPS proxies in result - we'll try to use
those for ws:. even though many are configured to reject connections to ports
other than 443.
> +    KURL httpsURL(KURL(), "https://" + m_url.host());
> +    RetainPtr<CFURLRef> httpsURLCF(AdoptCF, httpsURL.createCFURL());
> +
> +    RetainPtr<CFArrayRef> proxyArray(AdoptCF,
CFNetworkCopyProxiesForURL(httpsURLCF.get(), proxyDictionary.get()));
> +    CFIndex proxyArrayCount = CFArrayGetCount(proxyArray.get());
> +
> +    // FIXME: Support PAC files (always the preferred entry).
> +
> +    CFDictionaryRef chosenProxy = 0;
> +    for (CFIndex i = 0; i < proxyArrayCount; ++i) {
> +	   CFDictionaryRef proxyInfo =
static_cast<CFDictionaryRef>(CFArrayGetValueAtIndex(proxyArray.get(), i));
> +	   CFTypeRef proxyType = CFDictionaryGetValue(proxyInfo,
kCFProxyTypeKey);
> +	   if (proxyType && CFGetTypeID(proxyType) == CFStringGetTypeID()) {
> +	       if (CFEqual(proxyType, kCFProxyTypeSOCKS)) {
> +		   m_connectionType = SOCKSProxy;
> +		   chosenProxy = proxyInfo;
> +		   break;
> +	       }
> +	       if (CFEqual(proxyType, kCFProxyTypeHTTPS)) {
> +		   m_connectionType = CONNECTProxy;
> +		   chosenProxy = proxyInfo;
> +		   // Keep looking for proxies, as a SOCKS one is preferable.
> +	       }
> +	   }
> +    }
> +
> +    if (chosenProxy) {
> +	   ASSERT(m_connectionType != Unknown);
> +	   ASSERT(m_connectionType != Direct);
> +
> +	   CFTypeRef proxyHost = CFDictionaryGetValue(chosenProxy,
kCFProxyHostNameKey);
> +	   CFTypeRef proxyPort = CFDictionaryGetValue(chosenProxy,
kCFProxyPortNumberKey);
> +
> +	   if (proxyHost && CFGetTypeID(proxyHost) == CFStringGetTypeID() &&
proxyPort && CFGetTypeID(proxyPort) == CFNumberGetTypeID()) {
> +	       m_proxyHost = static_cast<CFStringRef>(proxyHost);
> +	       m_proxyPort = static_cast<CFNumberRef>(proxyPort);
> +	       return;
> +	   }
> +    }
> +#else // BUILDING_ON_TIGER
> +    // FIXME: check proxy bypass list and ExcludeSimpleHostnames.
> +    // FIXME: Support PAC files.
> +
> +    CFTypeRef socksEnableCF = CFDictionaryGetValue(proxyDictionary.get(),
kSCPropNetProxiesSOCKSEnable);
> +    int socksEnable;
> +    if (socksEnableCF && CFGetTypeID(socksEnableCF) == CFNumberGetTypeID()
&& CFNumberGetValue(static_cast<CFNumberRef>(socksEnableCF), kCFNumberIntType,
&socksEnable) && socksEnable) {
> +	   CFTypeRef proxyHost = CFDictionaryGetValue(proxyDictionary.get(),
kSCPropNetProxiesSOCKSProxy);
> +	   CFTypeRef proxyPort = CFDictionaryGetValue(proxyDictionary.get(),
kSCPropNetProxiesSOCKSPort);
> +	   if (proxyHost && CFGetTypeID(proxyHost) == CFStringGetTypeID() &&
proxyPort && CFGetTypeID(proxyPort) == CFNumberGetTypeID()) {
> +	       m_proxyHost = static_cast<CFStringRef>(proxyHost);
> +	       m_proxyPort = static_cast<CFNumberRef>(proxyPort);
> +	       m_connectionType = SOCKSProxy;
> +	       return;
> +	   }
> +    }
> +
> +    CFTypeRef httpsEnableCF = CFDictionaryGetValue(proxyDictionary.get(),
kSCPropNetProxiesHTTPSEnable);
> +    int httpsEnable;
> +    if (httpsEnableCF && CFGetTypeID(httpsEnableCF) == CFNumberGetTypeID()
&& CFNumberGetValue(static_cast<CFNumberRef>(httpsEnableCF), kCFNumberIntType,
&httpsEnable) && httpsEnable) {
> +	   CFTypeRef proxyHost = CFDictionaryGetValue(proxyDictionary.get(),
kSCPropNetProxiesHTTPSProxy);
> +	   CFTypeRef proxyPort = CFDictionaryGetValue(proxyDictionary.get(),
kSCPropNetProxiesHTTPSPort);
> +
> +	   if (proxyHost && CFGetTypeID(proxyHost) == CFStringGetTypeID() &&
proxyPort && CFGetTypeID(proxyPort) == CFNumberGetTypeID()) {
> +	       m_proxyHost = static_cast<CFStringRef>(proxyHost);
> +	       m_proxyPort = static_cast<CFNumberRef>(proxyPort);
> +	       m_connectionType = CONNECTProxy;
> +	       return;
> +	   }
> +    }
> +#endif
> +
>      m_connectionType = Direct;
>  }

So much of this is #ifdef'd that it seems maybe we should have an entirely
separate copy of the function for Tiger.

> +    case SOCKSProxy: {
> +	   // FIXME: SOCKS5 doesn't do challenge-response, should we try to
apply credentials from Keychain right away?
> +	   // But SOCKS5 credentials don't work at the time of this writing
anyway, see <rdar://6776698>.
> +	   const void* proxyKeys[] = { kCFStreamPropertySOCKSProxyHost,
kCFStreamPropertySOCKSProxyPort };
> +	   const void* proxyValues[] = { m_proxyHost.get(), m_proxyPort.get()
};
> +	   RetainPtr<CFDictionaryRef> connectDictionary(AdoptCF,
CFDictionaryCreate(0, proxyKeys, proxyValues, sizeof(proxyKeys) /
sizeof(*proxyKeys), &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
> +	   CFReadStreamSetProperty(m_readStream.get(),
kCFStreamPropertySOCKSProxy, connectDictionary.get());
> +	   break;
> +	   }
> +    case CONNECTProxy: {
> +	   const void* proxyKeys[] = { kCFStreamPropertyCONNECTProxyHost,
kCFStreamPropertyCONNECTProxyPort };
> +	   const void* proxyValues[] = { m_proxyHost.get(), m_proxyPort.get()
};
> +	   RetainPtr<CFDictionaryRef> connectDictionary(AdoptCF,
CFDictionaryCreate(0, proxyKeys, proxyValues, sizeof(proxyKeys) /
sizeof(*proxyKeys), &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
> +	   CFReadStreamSetProperty(m_readStream.get(),
kCFStreamPropertyCONNECTProxy, connectDictionary.get());
> +	   break;
> +	   }
> +    }

I think the braces are not right here. Either we need to indent the code
another level, or move the ending brace one level to the left.

I think this shows why some were pushing to indent cases one level inside a
switch statement. I think I was one of those people. Too bad we didn't discuss
this aspect of it. We should probably briefly discuss on webkit-dev, although I
am sick of coding style discussions!

r=me