On 11/04, Jeff King wrote:
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 27069ac..5d845c4 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -2308,6 +2308,31 @@ pretty.<name>::
> > Note that an alias with the same name as a built-in format
> > will be silently ignored.
> >
> > +protocol.allow::
> > + If set, provide a user defined default policy for all protocols which
> > + don't explicitly have a policy (protocol.<name>.allow). By default,
> > + if unset, known-safe protocols (http, https, git, ssh, file) have a
> > + default policy of `always`, known-dangerous protocols (ext) have a
> > + default policy of `never`, and all other protocols have a default policy
> > + of `user`. Supported policies:
> > ++
> > +--
> > +
> > +* `always` - protocol is always able to be used.
> > +
> > +* `never` - protocol is never able to be used.
> > +
> > +* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER`
> > is
> > + either unset or has a value of 1. This policy should be used when you
> > want a
> > + protocol to be usable by the user but don't want it used by commands
> > which
> > + execute clone/fetch/pull commands without user input, e.g. recursive
> > + submodule initialization.
>
> Makes sense. I wonder if it would be good to emphasize _directly_ usable
> here. I.e., "...when you want a protocol to be directly usable by the
> user but don't want...".
>
> Should clone/fetch/pull also include push?
You're right, that should really have been clone/fetch/push.
>
> > +protocol.<name>.allow::
> > + Set a policy to be used by protocol <name> with clone/fetch/pull
> > commands.
> > +
>
> Nice that this matches protocol.allow, so we don't need to re-explain
> that.
>
> Should the list of protocols be here? I know they're covered under
> GIT_ALLOW_PROTOCOL already, but if this is the preferred system, we
> should probably explain them here, and then just have GIT_ALLOW_PROTOCOL
> refer the user.
Right now the list of protocols under GIT_ALLOW_PROTOCOL looks like it
has a bit more documentation with how the colon list works. The
protocols are also mentioned above with their default behaviour.
>
> > diff --git a/Documentation/git.txt b/Documentation/git.txt
> > index ab7215e..ab25580 100644
> > --- a/Documentation/git.txt
> > +++ b/Documentation/git.txt
> > @@ -1150,13 +1150,13 @@ of clones and fetches.
> > cloning a repository to make a backup).
> >
> > `GIT_ALLOW_PROTOCOL`::
> > - If set, provide a colon-separated list of protocols which are
> > - allowed to be used with fetch/push/clone. This is useful to
> > - restrict recursive submodule initialization from an untrusted
> > - repository. Any protocol not mentioned will be disallowed (i.e.,
> > - this is a whitelist, not a blacklist). If the variable is not
> > - set at all, all protocols are enabled. The protocol names
> > - currently used by git are:
> > + The new way to configure allowed protocols is done through the config
> > + interface, though this setting takes precedences. See
> > + linkgit:git-config[1] for more details. If set, provide a
> > + colon-separated list of protocols which are allowed to be used with
> > + fetch/push/clone. Any protocol not mentioned will be disallowed (i.e.,
> > + this is a whitelist, not a blacklist). The protocol names currently
> > + used by git are:
>
> I wonder if we can explain this in terms of the config system. Something
> like:
>
> If set to a colon-separated list of zero or more protocols, behave as
> if `protocol.allow` is set to `never`, and each of the listed
> protocols has `protocol.$protocol.allow` set to `always`.
Yeah that makes sense.
>
> > +`GIT_PROTOCOL_FROM_USER`::
> > + Set to 0 to prevent protocols used by fetch/push/clone which are
> > + configured to the `user` state. This is useful to restrict recursive
> > + submodule initialization from an untrusted repository. See
> > + linkgit:git-config[1] for more details.
>
> Under "this is useful", it may make sense to make it clear that external
> programs can use this, too. Something like:
>
> It may also be useful for programs which feed potentially-untrusted
> URLs to git commands.
I'll put that in too.
>
> > diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
> > index b0917d9..5950fbf 100644
> > --- a/t/lib-proto-disable.sh
> > +++ b/t/lib-proto-disable.sh
> > @@ -1,15 +1,12 @@
> > # Test routines for checking protocol disabling.
> >
> > -# test cloning a particular protocol
> > -# $1 - description of the protocol
> > -# $2 - machine-readable name of the protocol
> > -# $3 - the URL to try cloning
> > -test_proto () {
> > +# Test clone/fetch/push with GIT_ALLOW_PROTOCOL whitelist
> > +test_whitelist () {
> > desc=$1
> > proto=$2
> > url=$3
> >
> > - test_expect_success "clone $1 (enabled)" '
> > + test_expect_success "clone $desc (enabled)" '
>
> Yeah, this should have been $desc all along. It makes the diff really
> noisy, though. Should it be split out into a preparatory change?
I'll pull it out to make the patch a bit cleaner.
--
Brandon Williams
