Update the git pre-receive hook to ensure that code hasn't changed before being pushed.

Review Request #13282 — Created Oct. 6, 2023 and updated

Information

RBTools
release-5.x

Reviewers

There's currentyly a code security concern with Review Board: code can be
approved in Review Board, but then altered and pushed to a remote repository,
without any way to block this.

This concern can be addressed using repository hooks. This change updates our
Git pre-receive script to ensure that the files and diffs on the user's
machine matches the files and diffs posted on the review request. To do this,
we compare the local diff to the diff posted on the review request, and
compare the local patched file's SHA with the SHA stored on the Review
Board server. For binary files, only the diffs get compared since we don't
store patched file SHAs for them.

The script already checks for review request approval, so this ensures that
only approved code can be pushed.

This only solves the concern for Git repositories. In the future we can
apply the same idea for our Perforce script.

  • Tested running the script locally and set up as a server side hook script
    for a GitLab repository for the following cases:
  • With a review request containing multiple commits where files match,
    and where at least one file doesn't match.
  • With a review request containing one commit where files match,
    and where at least one file doesn't match.
  • When unable to generate a local diff.
  • When unable to fetch the diff from the Review Board server.
  • When unable to load the file diff's patched_sha256 and/or patched_sha1
    file.
  • Tested with text-based files that end with newlines, and ones that don't end
    with newlines.
  • Tested with binary files (image files).
  • Tested with git blob SHAs in a diff not matching the ones posted on the
    review request, and was able to push the changes for that review request.
Summary ID
Update the git pre-receive hook to ensure a diff hasn't changed before being pushed.
ca0c9198e558ea2e176a87bbea40c5e1367b049c
Description From Last Updated

Instead of skipping binary files, should we read the contents of its patched file from the DiffFileAttachmentResource? And then compare …

maubinmaubin

undefined name 'DiffResource' Column: 6 Error code: F821

reviewbotreviewbot
maubin
  1. 
      
  2. contrib/tools/git-hook-check-approval (Diff revision 1)
     
     
     
     
    Show all issues

    Instead of skipping binary files, should we read the contents of its patched file from the DiffFileAttachmentResource? And then compare that to the local content of the file?

    1. We can't get the patched contents because we don't upload anything related to the binary file. This is something we'd like to address in the future, but for now that won't work.

      However, it seems like we might be able to do something about not having the patched SHA. If we store that somehow, then we can just compare the SHA against the blob ID of that file in the current commit to verify that the binary file matches. That could (should?) definitely be a separate change, though.

    2. Got it. I'll leave this issue open for now so that Christian can see the discussion.

  3. 
      
david
  1. Ship It!
  2. 
      
maubin
Review request changed

Change Summary:

  • Uses run_process instead of execute.

Commits:

Summary ID
Update the git pre-receive hook to ensure a diff hasn't changed before being pushed.
42c0f47ca205aebbe139dd82aa1e5129bedcf358
Update the git pre-receive hook to ensure a diff hasn't changed before being pushed.
2c623d25816a1826313d0109493ee1a47dd104e2

Depends On:

+13426 - Switch to using run_process() instead of execute() in rbtools.hooks

Diff:

Revision 2 (+670 -28)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
Review request changed

Commits:

Summary ID
Update the git pre-receive hook to ensure a diff hasn't changed before being pushed.
2c623d25816a1826313d0109493ee1a47dd104e2
Update the git pre-receive hook to ensure a diff hasn't changed before being pushed.
ca0c9198e558ea2e176a87bbea40c5e1367b049c

Diff:

Revision 3 (+678 -30)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...