Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed

[Brandon Williams <bmwill@google.com>]

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