Re: bw/transport-protocol-policy

From: Jeff King <peff@peff.net>
Date: Dec 1, 2016, 19:46 UTC
Message-ID: <20161201194621.dd3dbjv25ul6qgu5@sigill.intra.peff.net>
In-Reply-To: <20161201193524.GC54082@google.com>

On Thu, Dec 01, 2016 at 11:35:24AM -0800, Brandon Williams wrote:

Show 15 quoted lines

> > I wouldn't expect anyone to ever set GIT_PROTOCOL_FROM_USER=1, but it
> > does behave in a funny way here, overriding the "redirect" flag. I think
> > we'd want something more like:
> > 
> >   if (redirect < 0)
> > 	redirect = git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
> > 
> > and then pass in "-1" from transport_check_allowed().
> 
> I don't think I quite follow your solution but I came up with this:
> 
>   case PROTOCOL_ALLOW_USER_ONLY:
>     return redirect ? 0 : git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
> 
> Which should address the same issue.

I think mine was confused a bit by using the word "redirect". It was really meant to be "from_user", and could take three values: definitely yes, definitely no, and unknown (-1). And in the unknown case, we pull the value from the environment.

Yours combines "definitely no" and "unknown" into a single value ("1" in your case, but that is because "redirect" and "from_user" have inverted logic from each other).

I think that is OK, as there isn't any case where a caller would want to say "definitely no". The most they would say is "_I_ am not doing anything to make you think this value is not from the user", but we would still want to check the environment to see that nobody _else_ had put in such a restriction.

-Peff