Re: [PATCH 0/5] Multiple hook support

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 24, 2019, 23:26 UTC
Message-ID: <20190424232631.GA186980@google.com>
In-Reply-To: <20190424230744.GL6316@genre.crustytoothpaste.net>

Hi,

brian m. carlson wrote:

> On Tue, Apr 23, 2019 at 07:34:38PM -0700, Jonathan Nieder wrote:
>> brian m. carlson wrote:

Show 11 quoted lines

>>> I've talked with some people about this approach, and they've indicated
>>> they would prefer a configuration-based approach.
>>
>> I would, too, mostly because that reduces the problem of securing
>> hooks to securing configuration.  See
>> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/
>> for more on this subject.
>
> I know this is a common issue, but fixing it is a non-goal for this
> series. Anything we do here is going to have to be backwards compatible,
> so we can't make any changes to the security model.

I think it's worth bringing up because we should have some idea of where we want to head.

I think the backward compatibility part is actually one of the easier aspects of this one. We don't have to change the security model right away because there are similar places to exploit like core.pager. To address that, we need a notion of configuration that individual repos and worktrees can't override, and using such a configuration item, we can provide a way to opt in to the new security model. That provides a smooth path forward.

[...]

Show 13 quoted lines

>> Solving (1) without (2) feels like a bit of a missed opportunity to
>> me.  Ideally, what I would like is
>>
>>    i. A central registry of trustworthy Git hooks that can be upgraded
>>       using the system package manager to address (2).  Perhaps just
>>       git-hook-* commands on the $PATH.
>>
>>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to
>>       run for each event in .git/config.
>
> The problem I had with this when discussing it was that our
> configuration system lacks a good way to control inheritance from outer
> files.

The standard approach in lists defined in Git configuration is for assigning an empty item to clear / restart the list. See http.extraHeader for an example.

Thanks and hope that helps, Jonathan