Review Request: Proxy overhaul Part 4: More proxy changes and fixes for KProtocolManager

[Andreas Hartmetz]

> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > I'd actually be interested to hear which testing you did.
> > 
> > The "ResolveHostNamesBeforeProxyCheck" option seems strange. In which situations is this supposed to be set / not set?
> 
> Dawit Alemayehu wrote:
>     The "ResolveHostNamesBeforeProxyCheck" option is used to give the user the ability to turn DNS lookups of request URLs on or off before checking them against the "No Proxy For" list. This makes it possible for us to let people enter IP address ranges, e.g. "192.168.0.1/24" in the "NoProxyFor" list while at the same time protecting those people that do not want to have any form of name resolution to happen at the client application level. The default behavior is what it currently is, no lookup, since we do not support IP address ranges right now.
>     
>     As far as testing goes, I created a basic SOCKS server using ssh, ssh -D 9999 127.0.0.1, and a very basic PAC script file that contains the following:
>     
>     function FindProxyForURL( url, host )
>     {
>         var resolved_ip = dnsResolve(host);
>         
>         if (isInNet(resolved_ip, "127.0.0.1", "255.255.255.0"))
>             return "DIRECT";
>     
>         return "SOCKS 127.0.0.1:9999; DIRECT";
>     }
>     
>     I am also in the process of testing all these changes agains a real proxy sever. I am going to test against bother privoxy and squid. To test this however, you also need the next patch in the series, https://git.reviewboard.kde.org/r/102696/.

OK, I see what ResolveHostNamesBeforeProxyCheck does now. Thanks.


> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > kio/kio/kprotocolmanager.cpp, line 471
> > <http://git.reviewboard.kde.org/r/102691/diff/1/?file=37173#file37173line471>
> >
> >     This looks more like
> >     "if no proxy scheme is given, assume SOCKS"
> 
> Dawit Alemayehu wrote:
>     Ahh... Did you mean, "if no proxy could be found for the scheme of the given url, assume SOCKS" ? Even then that comment belongs to the if statement above the one where this comment currently resides. Perhaps I should move the comment down inside the if statement.

I am now more confused than before.
What I (still) think the code is doing is: if there is a proxy URL given with no scheme, prepend "socks://" to the proxy URL.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102691/#review6796
-----------------------------------------------------------


On Sept. 25, 2011, 4:15 p.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102691/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2011, 4:15 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> This patch is the 4th in the serious of patches designed to resolve bugs and missing functionality in KDE's proxy manager. The changes made with this patch are as follows:
> 
>     * Add code that resolves a request url's hostname before attempting to match
>       it against the no proxy for list so long as the "ResolveHostNamesBeforeProxyCheck"
>       option is set.
>     
>     * Allow "DIRECT" as a special keyword in the list of proxy server addresses
>       returned in slaveProtocol(const QString& protocol, QStringList& proxy).
>     
>     * Change KProtocolManager::proxyFor to properly handle the changes in the new
>       proxy management dialog (KDE 4.8) where the proxy server port, in the
>       manual proxy configuration mode, will be saved separated from the address with
>       a white space.
>     
>     * Move the code that accounts for SOCKS proxy from KProtocolManager::proxyFor
>       to KProtocolManager::proxyForUrl where it belongs. The current implementation
>       only works correctly under one circumstance while breaking the previous behavior
>       of the function.
>     
>     * Fix KProtocoManager::proxiesForUrl so that it accounts for the proxy
>       exception list.
>     
>     * Update API documentation to reflect the changes above.
> 
> 
> Diffs
> -----
> 
>   kio/kio/kprotocolmanager.h 11e43fe 
>   kio/kio/kprotocolmanager.cpp 50ebb6e 
> 
> Diff: http://git.reviewboard.kde.org/r/102691/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110929/9c86477c/attachment.htm>