On Thu, 23 Jan 2014, Maciej Puzio wrote:

> 1. The patch includes modifications to curl library, but not to the command
> line curl tool. I attach the patch that adds --proxy-header command line
> option.

Thanks!

> 3. When --proxy-header option is absent, but --header is present, curl sends
> headers specified in --header to both proxy and the origin server (i.e.
> final destination of the request). In my opinion this is an incorrect and
> potentially dangerous behavior (please see my bug report for details).
> However, Vijay Panghal comments in his code that this is done for backward
> compatibility, and my testing confirms that without --proxy-header the
> behavior of curl matches the previous version.

I think this is a tricky question and I'd love to get more feedback on this.
The existing (before this patch) features affect headers to both the server
and the proxy, and since it does that we can't really know which of the
features existing applications want to preserve for the future.

  A - they add/remove/change headers for the origin server so the proxy should
      not get them. Like server auth or cookies.

  B - they add/remove/change headers for the proxy, so the server should not
      get them. Like proxy auth or similar

  C - they add/remove/change headers for _both_ the proxy and the server. Like
      user-agent or similar.

You're clearly focusing on case (A) and I suppose we have reason to suspect
that is the most common use case. Is that a valid reason for us to say that
(B) and (C) applications need to be modified for them to do right in the
release with this change? I'm hesitating. Perhaps it is.

> 4. Option --proxy-header is only used for CONNECT requests, i.e. when https
> URL is requested, or when --proxytunnel option is explicitly used. When
> proxying with GET/POST (typical for unencrypted requests), --proxy-header is
> ignored. While this is technically correct (only one set of headers is sent
> in such a case), this behavior may be confusing to the user. I suggest
> issuing a warning when --proxy-header is used for a request that is not
> tunneled.

Yeah that's indeed a good idea, at least to the extent that the command
line tool knows about it.

It could also be a reason to reconsider the name. Perhaps a different one
would make it clearer to users what the option actually does? I would assume
that a typical user won't really see the difference when switching from
http:// to https:// or vice versa.

> 5. A very minor issue that I noticed in the code: in file lib/http.c the
> second (newly added) argument to function Curl_add_custom_headers is named
> 'is_proxy', while in file lib/http.h it is named 'is_connect'.

Traces from the origin of that patch when they were called connect-headers...
=)

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