From d633541ecc5cafbf5660e0bf57ecbd96bfb77da0 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Wed, 21 Feb 2024 20:23:00 +0000 Subject: [PATCH] Vendor a copy of Gerrit's commit-msg Git hook Gerrit wants each commit message to include a unique identifier string in a special footer line, so provides a commit-msg hook to randomly generate and insert one. Traditionally, this file is served directly from each Gerrit server and users retrieve it via SCP or HTTPS to install a local copy in their clone of every repository. Retrieving this file over the network has historically presented a number of challenges: modern OpenSSH has deprecated the SCP protocol while the mina-sshd library Gerrit uses hasn't implemented compatible SFTP support, authentication failures can shadow some clearer error handling later in git-review's workflow leading to confusing error messages, and then there are the security concerns with needing to trust the Gerrit server to supply a script which will end up running locally on the developer's machine. In order to address these problems, making git-review more robust and secure, we embed a copy of the Gerrit upstream project's commit-msg hook in the client itself and write that to disk by default rather than pulling a remote copy. This approach does mean that the user will end up with a frozen version of the script contemporary with the git-review release they've installed (but its function is simple and the implementation has changed very infrequently). It may also break workflows for sites which rely on users retrieving a customized commit-msg hook. For those reasons, a command-line option is provided to restore the prior behavior. Change-Id: Ia26abc781a281817115cb1cafcd5e7b78b383e39 --- git-review.1 | 3 + git_review/cmd.py | 20 ++- git_review/hooks.py | 133 ++++++++++++++++++ git_review/tests/test_git_review.py | 20 ++- ...mbed-commit-msg-hook-87509c496b561f97.yaml | 10 ++ 5 files changed, 183 insertions(+), 3 deletions(-) create mode 100644 git_review/hooks.py create mode 100644 releasenotes/notes/emmbed-commit-msg-hook-87509c496b561f97.yaml diff --git a/git-review.1 b/git-review.1 index 15a4bd83..d821c6e1 100644 --- a/git-review.1 +++ b/git-review.1 @@ -147,6 +147,9 @@ Don\(aqt actually perform any commands that have direct effects. Print them instead. .It Fl r Ar remote , Fl \-remote= Ns Ar remote Git remote to use for Gerrit. +.It Fl \-remote\-hook +When installing the commit-msg hook, fetch it from the remote rather than using +the built-in version. .It Fl s , Fl \-setup Just run the repo setup commands but don\(aqt submit anything. .It Fl t Ar topic , Fl \-topic= Ns Ar topic diff --git a/git_review/cmd.py b/git_review/cmd.py index fc600dbe..85cb4c40 100644 --- a/git_review/cmd.py +++ b/git_review/cmd.py @@ -16,6 +16,8 @@ from __future__ import print_function +from . import hooks + import argparse import configparser import datetime @@ -385,7 +387,10 @@ def set_hooks_commit_msg(remote, target_file): if not os.path.exists(target_file) or UPDATE: remote_url = get_remote_url(remote) - if (remote_url.startswith('http://') or + if not remote_url: + with open(target_file, 'w') as f: + f.write(hooks.COMMIT_MSG) + elif (remote_url.startswith('http://') or remote_url.startswith('https://')): hook_url = urljoin(remote_url, '/tools/hooks/commit-msg') if VERBOSE: @@ -575,6 +580,11 @@ def get_remote_url(remote): URL rather than the fetch URL. """ + # Short-circuit to a false value if the remote passed in is falsey, used + # for determining commit-msg hook retrieval behavior + if not remote: + return remote + push_url = git_config_get_value('remote.%s' % remote, 'pushurl') if push_url is not None: # Git rewrites pushurl using insteadOf but not pushInsteadOf. @@ -1564,6 +1574,9 @@ additional information: help="Regenerate Change-id before submitting") parser.add_argument("-r", "--remote", dest="remote", help="git remote to use for gerrit") + parser.add_argument("--remote-hook", dest="remote_hook", + action="store_true", + help="Fetch the remote version of the commit-msg hook") parser.add_argument("--use-pushurl", dest="usepushurl", action="store_true", help="Use remote push-url logic instead of separate" @@ -1798,7 +1811,10 @@ additional information: have_hook = os.path.exists(hook_file) and os.access(hook_file, os.X_OK) if not have_hook: - set_hooks_commit_msg(remote, hook_file) + if options.remote_hook: + set_hooks_commit_msg(remote, hook_file) + else: + set_hooks_commit_msg(None, hook_file) if options.setup: if options.finish and not options.dry: diff --git a/git_review/hooks.py b/git_review/hooks.py new file mode 100644 index 00000000..1c088d25 --- /dev/null +++ b/git_review/hooks.py @@ -0,0 +1,133 @@ +# Copyright (C) 2009 The Android Open Source Project +# Copyright OpenDev Contributors +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +# Because wheels are unable to embed and install data files, we store +# our vendored copy of the Gerrit commit-msg hook script in a string and +# write it into place at runtime. + +# Taken from https://gerrit.googlesource.com/gerrit/+/refs/heads/master +# /resources/com/google/gerrit/server/tools/root/hooks/commit-msg as of +# commit 05d6470 on 2024-02-21. + +COMMIT_MSG = r"""#!/bin/sh +# +# Part of Gerrit Code Review (https://www.gerritcodereview.com/) +# +# Copyright (C) 2009 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -u + +# avoid [[ which is not POSIX sh. +if test "$#" != 1 ; then + echo "$0 requires an argument." + exit 1 +fi + +if test ! -f "$1" ; then + echo "file does not exist: $1" + exit 1 +fi + +# Do not create a change id if requested +case "$(git config --get gerrit.createChangeId)" in + false) + exit 0 + ;; + always) + ;; + *) + # Do not create a change id for squash/fixup commits. + if head -n1 "$1" | LC_ALL=C grep -q '^[a-z][a-z]*! '; then + exit 0 + fi + ;; +esac + + +if git rev-parse --verify HEAD >/dev/null 2>&1; then + refhash="$(git rev-parse HEAD)" +else + refhash="$(git hash-object -t tree /dev/null)" +fi + +random=$({ git var GIT_COMMITTER_IDENT ; echo "$refhash" ; cat "$1"; } | git hash-object --stdin) +dest="$1.tmp.${random}" + +trap 'rm -f "$dest" "$dest-2"' EXIT + +if ! cat "$1" | sed -e '/>8/q' | git stripspace --strip-comments > "${dest}" ; then + echo "cannot strip comments from $1" + exit 1 +fi + +if test ! -s "${dest}" ; then + echo "file is empty: $1" + exit 1 +fi + +reviewurl="$(git config --get gerrit.reviewUrl)" +if test -n "${reviewurl}" ; then + token="Link" + value="${reviewurl%/}/id/I$random" + pattern=".*/id/I[0-9a-f]\{40\}" +else + token="Change-Id" + value="I$random" + pattern=".*" +fi + +if git interpret-trailers --parse < "$1" | grep -q "^$token: $pattern$" ; then + exit 0 +fi + +# There must be a Signed-off-by trailer for the code below to work. Insert a +# sentinel at the end to make sure there is one. +# Avoid the --in-place option which only appeared in Git 2.8 +if ! git interpret-trailers \ + --trailer "Signed-off-by: SENTINEL" < "$1" > "$dest-2" ; then + echo "cannot insert Signed-off-by sentinel line in $1" + exit 1 +fi + +# Make sure the trailer appears before any Signed-off-by trailers by inserting +# it as if it was a Signed-off-by trailer and then use sed to remove the +# Signed-off-by prefix and the Signed-off-by sentinel line. +# Avoid the --in-place option which only appeared in Git 2.8 +# Avoid the --where option which only appeared in Git 2.15 +if ! git -c trailer.where=before interpret-trailers \ + --trailer "Signed-off-by: $token: $value" < "$dest-2" | + sed -e "s/^Signed-off-by: \($token: \)/\1/" \ + -e "/^Signed-off-by: SENTINEL/d" > "$dest" ; then + echo "cannot insert $token line in $1" + exit 1 +fi + +if ! mv "${dest}" "$1" ; then + echo "cannot mv ${dest} to $1" + exit 1 +fi +""" # noqa diff --git a/git_review/tests/test_git_review.py b/git_review/tests/test_git_review.py index b7965dbf..155d3425 100644 --- a/git_review/tests/test_git_review.py +++ b/git_review/tests/test_git_review.py @@ -105,6 +105,24 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase): self._run_git_review( '-s', chdir=os.path.join(self.test_dir, 'subdirectory')) + def test_install_embedded_hook(self): + """Test whether git-review -s correctly creates the commit-msg hook + from the embedded copy with appropriate permissions and content. + """ + hooks_subdir = ".git/hooks" + hook_file = os.path.join(self.test_dir, hooks_subdir, 'commit-msg') + self.reset_remote() + + self._run_git_review('-s') + self.assertTrue(os.path.exists(hook_file)) + self.assertTrue(os.access(hook_file, os.W_OK)) + self.assertTrue(os.access(hook_file, os.X_OK)) + with open(hook_file) as f: + content = f.read() + self.assertTrue(content.startswith('#!/')) + # This line should not appear in the embedded version + self.assertNotIn('# From Gerrit Code Review', content) + def test_install_remote_hook(self): """Test whether git-review -s correctly creates the commit-msg hook from the Gerrit server with appropriate permissions and content. @@ -113,7 +131,7 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase): hook_file = os.path.join(self.test_dir, hooks_subdir, 'commit-msg') self.reset_remote() - self._run_git_review('-s') + self._run_git_review('-s', '--remote-hook') self.assertTrue(os.path.exists(hook_file)) self.assertTrue(os.access(hook_file, os.W_OK)) self.assertTrue(os.access(hook_file, os.X_OK)) diff --git a/releasenotes/notes/emmbed-commit-msg-hook-87509c496b561f97.yaml b/releasenotes/notes/emmbed-commit-msg-hook-87509c496b561f97.yaml new file mode 100644 index 00000000..88d473a7 --- /dev/null +++ b/releasenotes/notes/emmbed-commit-msg-hook-87509c496b561f97.yaml @@ -0,0 +1,10 @@ +--- +features: + - | + A copy of the Gerrit upstream project's commit-msg hook is now included + directly and written to disk by default rather than pulling a remote copy. + This approach is safer and more robust, but may break workflows for sites + which rely on users retrieving a customized commit-msg hook. For this + reason, a command-line override is provided so that users can, for example, + execute `git review -s --remote-hook` to get the old behavior when setting + up a new clone.