Re: [PATCH] transport: add core.allowProtocol config option

From: Brandon Williams <bmwill@google.com>
Date: Nov 2, 2016, 22:47 UTC
Message-ID: <20161102224736.GA183367@google.com>
In-Reply-To: <CAGZ79kbytopD1UgvU6R0X18Ge6JsJ46K2KQ-SFLh+hu5tprTmA@mail.gmail.com>

On 11/02, Stefan Beller wrote:

Show 7 quoted lines

> > This is useful to restrict recursive
> > +       submodule initialization from an untrusted repository.
> 
> ok. Though as a user submodules may not spring to mind immediately here.
> I think this is generally useful, too. e.g. an admin could put this in
> the system wide
> config to prevent certain protocols from being used.

Oh I pretty much copied the description from what exists for `GIT_ALLOW_PROTOCOL` which included this bit about submodules.

Show 6 quoted lines

> > Any protocol not
> > +       mentioned will be disallowed
> 
> For the regular fetch/clone/pull case. For the submodule case we still
> fall back to
> the hardcoded list of known good things?

Yep! This is done by explicitly setting GIT_ALLOW_PROTOCOL to the hardcoded list if the user hasn't supplied a whitelist.

Show 7 quoted lines

> 
> > (i.e., this is a whitelist, not a
> > +       blacklist).
> 
> That is very explicit, I'd drop it. However this inspires bike
> shedding on the name:
> What about core.protocolWhitelist instead?

Simply to keep the name similar to the env variable that already exists for this functionality.

> So the env var is of higher priority than this config.

Show 8 quoted lines

> > -: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
> > +config_whitelist=$(git config core.allowProtocol)
> 
> So first we lookup the configured protocols.
> 
> > +: ${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}
> 
> Then if they are not configured use the current hard coded white list.

The lookup of the configured whitelist is done first but wont be used unless GIT_ALLOW_PROTOCOL is unset. If neither is set it will fallback to the hardcoded list.

> Do we have any tests for this that could be extended? (Otherwise we'd
> maybe want to add a test for both the regular case as well as a forbidden
> submodule?)
>

I can write a couple tests for a v2 of the patch.

-- 
Brandon Williams