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