• 
      

    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

    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

    Checks run (2 succeeded)

    flake8 passed.
    JSHint passed.