Skip to content

Add Git pre-commit hook to abort commit with unencrypted files (#31) #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 17, 2020

Conversation

jmurty
Copy link
Collaborator

@jmurty jmurty commented Apr 14, 2020

On init, add a pre-commit Git hook script to check Transcrypt-managed files and abort a commit if there is an un-encrypted file staged in the index that would otherwise be committed in plaintext. See related issue #31

This is a safety mechanism to prevent accidental commits of plain text files that have been staged by tools that do not respect or run the .gitattribute filters that Transcrypt needs to do its job.

On commit failure, the error message says how to re-stage the file using Git on the command line:

Transcrypt managed file is not encrypted in the Git index: secret_file

You probably staged this file using a tool that does not apply .gitattribute filters as required by Transcrypt.

Fix this by re-staging the file with a compatible tool or with Git on the command line:

    git reset -- secret_file
    git add secret_file

Because Git hooks work with single scripts only it is difficult to cleanly install and uninstall hook scripts, especially if the user already has a pre-commit hook script in place.

To handle this situation cleanly if naively

  • always install a pre-commit-crypt copy of the hook script, which isn't active because it doesn't have the pre-commit file name
  • on init, check if the user already has a pre-commit script and instead of clobbering it, print a warning telling the user to manually install the script from pre-commit-crypt
  • on uninstall, only delete the pre-commit if it exactly matches pre-commit-crypt file, which would indicate the user had manually edited the pre-commit file.

@jmurty jmurty requested a review from elasticdog April 14, 2020 14:04
@jmurty jmurty self-assigned this Apr 14, 2020
@jmurty
Copy link
Collaborator Author

jmurty commented Apr 14, 2020

This branch is a mess of merges and spurious commits sorry, I should have cherry-picked instead of merging the original v1 changes.

If/when this is merged to master the whole lot should be squashed.

Copy link
Owner

@elasticdog elasticdog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a reasonable implementation for that safety check...thank you for submitting it. Let me know once the merge conflicts have been handled, and I'd be glad to give it another look over.

plain text files staged by tools that do not respect the .gitattribute
filters Transcrypt needs to do its job.

- Add functional tests.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same suggestion as the other PR, but I think we could use a legit CHANGELOG.

jmurty added 3 commits April 17, 2020 22:45
On transcrypt init, add a pre-commit Git hook script to check crypt
files and abort a commit if there is an un-encrypted file staged in
the index that would otherwise be committed in plaintext.

This is a safety mechanism to prevent accidental commits of plain
text files that have been staged by tools that do not respect or
run the .gitattribute filters that Transcrypt needs to do its job.

A pre-commit-crypt file is now always installed in the Git hooks
directory, but is only "activated" if there is no existing
pre-commit hook file.

On uninstall the pre-commit file is removed only if it is definitely
safe to do so because the file exactly matches the hook file put in
place by Transcrypt, proving the user has made no manual changes.
@jmurty jmurty force-pushed the 31-add-pre-commit-hook-safety-check branch from c44d473 to 1e4aecf Compare April 17, 2020 12:46
@jmurty
Copy link
Collaborator Author

jmurty commented Apr 17, 2020

Hi @elasticdog I have rebased and cleaned up the commits in this PR to make more sense, I think this is ready for review.

Regarding the change log I agree a CHANGELOG file would be best. This is probably something to tackle after dealing with this PR and #76 since the new changeling file should probably include all changes, not just changes specific to these PRs.

Copy link
Owner

@elasticdog elasticdog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@elasticdog elasticdog merged commit f3a9914 into master Apr 17, 2020
@elasticdog elasticdog deleted the 31-add-pre-commit-hook-safety-check branch April 17, 2020 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants