Re: [PATCH] hooks: propose repository owner configured hooks

On 2021-03-18 at 22:03:35, Albert Cui via GitGitGadget wrote:

This sounds like you're proposing pre-commit hooks for this purpose. Let me quote you this passage from the FAQ that I wrote that explains my position perfectly:

  [S]ome advanced users find `pre-commit` hooks to be an impediment to
  workflows that use temporary commits to stage work in progress or that create
  fixup commits, so it's better to push these kinds of checks to the server
  anyway.

It also outlines that pre-commit hooks are not an effective control and for effective implementation, this must live on the server.

I can never imagine myself wanting to use a pre-commit hook for these reasons. It would interrupt my typical workflow which uses many, many fixup and squash commits and near constant rebases.

Similarly, a pre-push hook prevents me from pushing up in-progress code to a remote so my colleagues and other interested parties can follow my progress, contribute design feedback, and, if I need to be out of the office for a while, pick up my work and continue it.

This is also not an effective control. Any user can bypass this trivially, so there's little point in shipping a hook that does this.

Unfortunately, in most cases, what hooks are to be used and whether they should be used is a developer choice, and developers should be able to use or not use hooks much the same way as they choose an editor. Thus, a proposal that a project maintainer should specify a set of hooks to be used on a developer machine is much like that maintainer proposing that a developer use a specific editor.

While there are situations where that's done, in general, it's not a best practice to do so, so I'm generally negative on the idea of automatically deploying hooks to developer machines. I think doing so is a fundamental mistake in the developer process in most circumstances. Certainly there are exceptions, but I think they are so few that the burden of running a script is very minor in comparison.

I think this assumes that developers need these hooks to be productive, and I'm not in agreement with that assumption. For the reasons I've outlined above, I think that's a misunderstanding about best practices and how codebases should be structured.

I don't think we can securely automatically update hooks from a remote source. That sounds like a terrible idea from a security perspective.

If users really want this, they can add a post-checkout hook to do so.

This sounds like you're proposing automatically installing hooks, which would of course be a security vulnerability. If that's not what you're proposing, then perhaps you'll want to rephrase this point.

Git supports other platforms as well.

As I explain further below, shipping config is a security problem.

I don't want to be prompted to install hooks, because I never want to use hooks from a repository like this.

Moreover, how could a user possibly make an educated decision? They can't actually see the hooks at this point because no checkout has occurred, so the user must make a leap of faith based on the purported reputation of the repository owner.

In similar situations, such as piping curl to bash, the recommendation is always to download the script manually, inspect it for any problems or vulnerabilities, and only then execute it. This proposal doesn't allow that.

"Yes (always)" is almost certainly a security problem.

I don't think any sort of automatic configuration is going to be secure. Git has had numerous vulnerabilities with the .gitmodules file and it is highly restricted in its contents. So I'm pretty much completely opposed to this since I think it's likely to result in Git becoming a CVE magnet.

Even if the entire rest of the project disagreed with me and adopted this, the config would need to be restricted to an allowlist, not a denylist, because we have good and sufficient evidence that the latter is not secure. Even things as simple as changing diff colors can cause a security problem (e.g., hiding malicious code in a code review), so I'm having trouble imagining how we could come up with more than a tiny set of options that could be legitimately shipped securely, at which point the extra maintenance and development burden becomes excessive.

There are already a variety of proposals for this, such as https://github.com/github/scripts-to-rule-them-all. I don't believe Git needs to be in the business of doing this, and all of my concerns about security are appropriate here as well.

In general, I have grave concerns about this proposal. It makes assumptions that users should or will want to use the repository owner's hooks, which I disagree with. I think prompting users to install hooks will encourage them to be much more likely to do so, especially for less skilled users, which is likely to lead to social engineering attacks. And overall, this proposal sounds like it does not give enough consideration to the giant security risks it introduces. I don't think this proposal as it stands should be accepted, and I am having a hard time thinking of any similar proposal I could support.

I appreciate that this proposal does make things slightly easier for a subset of users and environments, but I think picking a standard set of scripts for those users and environments and letting users execute them if and when they want is only slightly more difficult and much more secure. I've used that approach at work and it works great, so I feel confident it can work in most other environments as well.