On Mon, 2016-08-01 at 01:00 +0200, Daniel Stenberg wrote:
> 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.

And apologies for the further delay in responding; I didn't see it for
a week because I was missing from Cc.

> > 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.

https://github.com/libproxy/libproxy/blob/master/libproxy/proxy.h

> 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.

It's actually really simple to provide a replacement libproxy which
does whatever the hell you want, so using that for testing should be
fine.

Obviously, once we've resolved the question of how to expose this in
the curl executable, that makes it easier to build tests.

> 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?

Anything explicitly specified should be used in favour of the automatic
detection, of course.

We should query libproxy *before* falling back to environment
variables. We use environment variables only if libproxy doesn't return
any results.

I believe that was how I'd implemented it; perhaps I should make the
decision more explicit.

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

Yep.

> 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.

The library should strip it for us. That way it's done *once*,
competently, instead of making all its users reimplement the same logic
with varying levels of quality.

The PacRunner reimplementation of libproxy certainly *does* do so
before letting it near the PAC code. I went to check the "real"
libproxy but.... horrid mess of jumped-the-shark C++ code... $DEITY
knows :)

-- 
dwmw2

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