On Tue, 26 Jul 2016, David Woodhouse wrote:

Hello!

Thanks a lot for your contribution. Sorry for the slight delay in responding
to this patch.

> The original libproxy has evolved into a C++ monstrosity which potentially
> loads a JavaScript interpreter into the address space of anything which uses
> it... but the API is clean and simple.

Is the API documentation available online? I couldn't find it.

> Thus the expectation will be that asking libproxy should *consistently* give
> correct answers. Once that's true, I hope to change Linux distribution
> packaging guidelines to suggest that packaged applications *should* be using
> libproxy's results (or at least PacRunner's results) by default.

I can certainly see how that would be really useful for all those users behind
proxies, and especially ones using very creative PAC scripts.

I looked at your patch and it seems generally fine but I'm missing some
things:

1. Tests. Can we provide some input to fake libproxy to respond something or
    is there any other support to make tests with libproxy? We can of course
    wrap/inject something ourselves otherwise. We need tests to make sure
    things work as expected. Especially in combination with other
    options/environment variables etc.

2. Clarity. This adds an alternative way to figure out proxy. What happens to
    the other options if you enable/disable this? If libproxy detection fails,
    should we consider the other options then? Should we fail the connection?

3. Version. We should probably add "libproxy/$version" to the version string
    similar to how we output other dependencies and their versions.

4. Security. I see that you pass the full URL to
    px_proxy_factory_get_proxies() and the libproxy code I found for running
    pac is rather old. If this passes the full URL to the PAC javascript then
    it is a security problem - this was recently addressed in browsers to make
    sure maliciously injected PAC-scripts can't be made to leak full
    destination URLs.

-- 
  / daniel.haxx.se
-------------------------------------------------------------------
List admin: https://cool.haxx.se/list/listinfo/curl-library
Etiquette:  https://curl.haxx.se/mail/etiquette.html