Subject: Re: [PATCH v8 00/37] config-based hooks

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 25 Mar 2021 13:41:28 +0100

On Fri, Mar 12 2021, Ævar Arnfjörð Bjarmason wrote:

A small correction to one of my comments:

> On Thu, Mar 11 2021, Emily Shaffer wrote:

>  2. You're sticking full paths in the git config key, which is
>     case-insensitive, and a feature of this format is being able to
>     configure/override previously configured hooks.
>
>     So the behavior of this feature depends on git's interaction with
>     the case-sensitivity of filesystems, and not just one fs, any fs
>     we're walking in our various config sources, and where the hook
>     itself lives.
>
>     As recent CVEs have shown that's a big can of worms, particularly
>     for something whose goal is to address the security aspect of
>     running hooks from other config.
>
>     Arguably the case-sensitivity issue is just confusing since we
>     canonicalize it anyway. But once you add in FS path canonicalization
>     it becomes a real big can of worms. See the .gitmodules fsck code.
>
>     Even if it wasn't for that it's relatively nastier to edit/maintain
>     full paths and the appropriate escaping in the double-quoted key in
>     the config file v.s. having it as an optionally quoted value.

So the "case-insensitive" part of that *mostly* doesn't apply.

I'd forgotten that we don't consider the "LeVeL" part of
"ThReE.LeVeL.KeY" to be case-insensitive, but the other two components
are, as discussed in git-config(1)'s docs.

I say "mostly" because that's tolower()'s idea of case normalization,
which may or may not match the FS's, but anyway, I think that's probably
splitting hairs, but I worry more about the path normalization aspect
noted in the last two paragraphs there.

>  3. We're left with this "*.command = cmd", and "*.skip = true"
>     special-case syntax. I can't see any reason for why it's needed over
>     simply having "*.command = true" clobber earlier hooks as noted in
>     the proposed docs above.
>
>     And that doesn't require any magic to support, just like our
>     existing "core.pager=cat" case.
>
>     I mean, I suppose it's magical in that we might otherwise error on
>     non-consumed stdin (do we?), anyway, documenting it as a synonym for
>     "cat >/dev/null" would get around that :)
>
>  4. It makes the common case of having the same hooks for N commands
>     needlessly verbose, if you can just support "type" (or whatever we
>     should call it) you can add that N times...
>
>  5. At the end of this series we're left with the start of the docs
>     saying:
>
>       You can list and run configured hooks with this command. Later,
>       you will be able to add and modify hooks with this command.
>
>     But those patches have yet to land, and looking at the design
>     document I'm skeptical of that being a good addition v.s. just
>     adding the same thing to "git config".
>
>     As just one exmaple; surely "git config edit <name>" would need to
>     run around and find config files to edit, then open them in a loop
>     for you, no?
>
>     Which we'd eventually want for "git config" in general with an
>     --edit-regexp option or whatever, which brings us (well, at least
>     me) back to "then let's just add it to git-config?".
>
>  6. The whole 'git hook' config special-casing doesn't help other
>     commands or the security issue that seemed to have prompted (at
>     least some of) its existence
>
>     In the design doc we mention the "core.pager = rm -rf /" case for a
>     .git/config.
>
>     This series doesn't implement, but the design docs note a future
>     want for solving that issue for the hooks.
>
>     To me that's another case where we should just have general config
>     syntax, not something hook-specific, e.g. if I could do this in my
>     ~/.gitconfig:
>
>        ;; We consider 'config.ignore' in reverse order, so e.g setting
>        ;; it in. ~/.gitconfig will ignore any such keys for repo-level
>        ;; config
>        [config "ignore"]
>        key = core.pager
>        keyRegexp = "^hook\."
>
>     We'd address both any hook security concerns, as well as core.pager
>     etc. We could then just have e.g. some syntax sugar of:
>
>        [include]
>        path = built-in://gimme-safe-config
>
>     Which would just be a thin layer of magit to include
>     <path-to-git-prefix>/config-templates/gimme-safe-config or whatever.
>
>     We'd thus address the issue for all config types without
>     hook-specific magic.
>
> Anyway. I'm very willing to be convinced otherwise. I just think that
> for a first-draft implementation leaving aside 'hook.<command>.command'
> and the whole 'list' thing makes sense.
>
> We can consider the core code changes relatively separately from any
> future aspirations, particularly with a 40-some patch series, and the
> end-state of *this series* IMO not really justifying, that part of the
> implementation, and thus requiring reviewers to look ahead beyond the
> 40-some patches.

Emily: *Bump* on being interesed in what you think about the rest of
this though.