On 2021-06-20 at 19:51:04, Ævar Arnfjörð Bjarmason wrote:
On Wed, Jun 16 2021, Jonathan Tan wrote:
quoted
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.
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