Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Jun 22, 2021, 00:40 UTC
Message-ID: <YNExhalSLWvmky55@camp.crustytoothpaste.net>
In-Reply-To: <87fsxc47le.fsf@evledraar.gmail.com>

On 2021-06-20 at 19:51:04, Ævar Arnfjörð Bjarmason wrote:

Show 43 quoted lines

> 
> On Wed, Jun 16 2021, Jonathan Tan wrote:
> 
> > This but does not use any features from es/config-based-hooks but is
> > implemented on that branch anyway because firstly, I need an existing
> > command to attach the "autoupdate" subcommand (and "git hook" works),
> > and secondly, when we test this at $DAYJOB, we will be testing it
> > together with the aforementioned branch.
> >
> > I have had to make several design choices (which I will discuss later),
> > but now with this implementation, the following workflow is possible:
> >
> >  1. The remote repo administrator creates a new branch
> >     "refs/heads/suggested-hooks" pointing to a commit that has all the
> >     hooks that the administrator wants to suggest. The hooks are
> >     directly referenced by the commit tree (i.e. they are in the "/"
> >     directory).
> >
> >  2. When a user clones, Git notices that
> >     "refs/remotes/origin/suggested-hooks" is present and prints out a
> >     message about a command that can be run.
> >
> >  3. If the user runs that command, Git will install the hooks pointed to
> >     by that ref, and set hook.autoupdate to true. This config variable
> >     is checked whenever "git fetch" is run: whenever it notices that
> >     "refs/remotes/origin/suggested-hooks" changes, it will reinstall the
> >     hooks.
> >
> >  4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks
> >     will remain.
> >
> > Design choices:
> >
> >  1. Where should the suggested hooks be in the remote repo? A branch,
> >     a non-branch ref, a config? I think that a branch is best - it is
> >     relatively well-understood and any hooks there can be
> >     version-controlled (and its history is independent of the other
> >     branches).
> 
> First, unlike brian I don't (I hope I'm fairly summarizing his view
> here) disagree mostly or entirely with the existence of such a feature
> at all. I mean, I get the viewpoint that git shouldn't bless what
> amounts to an active RCE from the remote.

It's accurate that I'm generally opposed to such a feature. I feel that suggesting people install hooks is likely to lead to social engineering attacks, and it's also likely to lead to bad practices such as the expectation that all developers will install hooks or the use of hooks instead of CI or other effective controls.

If we do add this feature (which, as I said, I'm opposed to) and we decide to store it in a ref, that ref should not be a normal branch by default (it should be a special one-level ref, like refs/stash or such), and the ref name should be configurable. Not all developers use English as their working language and we should respect that.

In addition, there should be an advice.* option that allows people to turn this off once and for all, and it should be clearly documented. Ideally it should be off by default.

Show 10 quoted lines

> I think I get why you want to do it that way, I just don't get why, as
> mostly noted in those earlier rounds why it wouldn't be a better
> approach / more straightforward / more git-y to:
> 
> 1. Work on getting hooks driven by config <this is happening with
>    Emily's series / my split-out "base" topic>
> 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety
>    valves etc. around this, I suggested starting with a whitelist of the
>    N least dangerous config options, e.g. some diff viewing options, or
>    a suggested sendemail.to or whatever.

This also makes me deeply nervous for much of the same reasons. There are situations where e.g. ignoring whitespace can lead to security problems in code review (think Python), and in general it's hard to reason about all the ways people can do malicious things. Typically adding untrusted config ends poorly (think of all the modeline vulnerabilities in Vim).

I'd definitely want support for this to be off with no prompting by default.

-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA