talisman icon indicating copy to clipboard operation
talisman copied to clipboard

Published 20 hours ago • verified publisher icon thoughtworks

bug: talisman allows push when user adds a commit to delete the added secret

Open jacksingleton opened this issue 8 years ago • 5 comments

touch secret.pem git commit -am "whoops" git push talisman: booo rm secret.pem git commit -am "all safe now" git push talisman: :+1:

jacksingleton avatar May 27 '16 02:05 jacksingleton

Shouldn't talisman be a pre-commit hook instead of a pre-push hook?

giovaneliberato avatar Jun 24 '16 18:06 giovaneliberato

It works with the latest Talisman. Tested. @jacksingleton Please test and close the ticket if it looks ok

harinee avatar Apr 25 '19 09:04 harinee

Closing this ticket, due to no further feedback. Please do let us know if you would like to re-open this.

harinee avatar May 20 '19 07:05 harinee

Sorry, closed in error. This issue is actually still reproducible. Re-opening.

This is because on pre-push hook, talisman checks the complete file, and not the diff

harinee avatar Aug 02 '19 13:08 harinee

I just stumbled on this bug as well 😅 I think this behavior should be highlighted in https://github.com/thoughtworks/talisman#talisman-in-action to prevent anyone from getting bitten by this.

It is mentioned that a pre-commit hook is preferred but its not clear why.

In case you have installed Talisman as a pre-push hook, it will scan the complete file in which changes are made. As mentioned above, it is recommended that you use Talisman as a pre-commit hook.

Especially

...as a pre-push hook, it will scan the complete file in which changes are made.

What's happening is that the pre-push hook will get the filenames of files that were added, copied or modified via git diff ShaOfLatestCommitOnRemote..ShaOfNotLatestLocalCommit --name-only --diff-filter=ACM in the range of commits that are about to be pushed. It will then only get and scan the contents of the files as they are in the last commit. This explains the behavior described at https://github.com/thoughtworks/talisman/issues/16#issue-157115065

Here is the path in the code:

  • https://github.com/thoughtworks/talisman/blob/201dd2c9078ba46450d77485f8743eac1374cb4f/pre_push_hook.go#L75
  • https://github.com/thoughtworks/talisman/blob/201dd2c9078ba46450d77485f8743eac1374cb4f/gitrepo/gitrepo.go#L78-L83

This is pretty confusing as there are at least 3 different scanning behaviors I can find. The --scan, pre-commit, pre-push (maybe--ignoreHistory is a 4th one).

Hope this helps the person picking this up :)

teleivo avatar Apr 19 '21 15:04 teleivo