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

On 12/01, Brandon Williams wrote:

On 12/01, Jeff King wrote:

quoted

On Thu, Dec 01, 2016 at 12:25:59PM -0800, Brandon Williams wrote:

quoted

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').

quoted

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