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

From: Brandon Williams <[email-masked]>
Date: Thu, 1 Dec 2016 15:26:56 -0800

On 12/01, Brandon Williams wrote:
> 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.

Thinking about this some more...I was told that having http redirect to
file:// could be scary.  The way the new protocol configuration is setup
we have file:// as a default known-safe protocol.  Do we need to worry
about this or can we leave this be since this can be overridden by the
user?

-- 
Brandon Williams