On 12/01, Jeff King wrote:
> On Thu, Dec 01, 2016 at 12:25:59PM -0800, Brandon Williams wrote:
>
> > Add the from_user parameter to the 'is_transport_allowed' function.
> > This allows callers to query if a transport protocol is allowed, given
> > that the caller knows that the protocol is coming from the user (1) or
> > not from the user (0), such as redirects in libcurl. If unknown, a -1
> > should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
> > to determine if the protocol came from the user.
>
> Patches 3 and 4 look good to me (1 and 2 are unchanged, right? They are
> already in 'next' anyway, though I guess we are due for a post-release
> reset of 'next').
>
> > diff --git a/http.c b/http.c
> > index fee128b..e74c0f0 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -725,13 +725,13 @@ static CURL *get_curl_handle(void)
> > curl_easy_setopt(result, CURLOPT_POST301, 1);
> > #endif
> > #if LIBCURL_VERSION_NUM >= 0x071304
> > - if (is_transport_allowed("http"))
> > + if (is_transport_allowed("http", 0))
> > allowed_protocols |= CURLPROTO_HTTP;
> > - if (is_transport_allowed("https"))
> > + if (is_transport_allowed("https", 0))
> > allowed_protocols |= CURLPROTO_HTTPS;
> > - if (is_transport_allowed("ftp"))
> > + if (is_transport_allowed("ftp", 0))
> > allowed_protocols |= CURLPROTO_FTP;
> > - if (is_transport_allowed("ftps"))
> > + if (is_transport_allowed("ftps", 0))
> > allowed_protocols |= CURLPROTO_FTPS;
> > curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
> > #else
>
> This is better, but I think we still need to deal with http-alternates
> on top.
>
> I think we'd need to move this allowed_protocols setup into a function
> like:
>
> int generate_allowed_protocols(int from_user)
> {
> int ret;
> if (is_transport_allowed("http", from_user))
> ret |= CURLPROTO_HTTP;
> ... etc ...
> return ret;
> }
>
> and then create a protocol list for each situation:
>
> allowed_protocols = generate_allowed_protocols(-1);
> allowed_redir_protocols = generate_allowed_protocols(0);
>
> and then we know we can always set up the redir protocols:
>
> curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_redir_protocols);
>
> and which we feed for CURLOPT_PROTOCOLS depends on whether we are
> following an http-alternates redirect or not. But I suspect it will be a
> nasty change to plumb through the idea of "this request is on behalf of
> an http-alternates redirect".
>
> Given how few people probably care, I'm tempted to document it as a
> quirk and direct people to the upcoming http.followRedirects. The newly
> proposed default value of that disables http-alternates entirely anyway.
>
> -Peff
I started taking a look at your http redirect series (I really should
have taking a look at it sooner) and I see exactly what you're talking
about. We can easily move this logic into a function to make it easier
to generate the two whitelists.
--
Brandon Williams