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

[jacksingleton]

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

[giovaneliberato]

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

[harinee]

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

[harinee]

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

[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

[teleivo]

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 #16 (comment)

Here is the path in the code:

• talisman/pre_push_hook.go

  Line 75 in 201dd2c

  return repo.AdditionsWithinRange(oldCommit, newCommit)

• talisman/gitrepo/gitrepo.go

  Lines 78 to 83 in 201dd2c

  	files := repo.outgoingNonDeletedFiles(oldCommit, newCommit)
  	result := make([]Addition, len(files))
  	for i, file := range files {
  	data, _ := repo.ReadRepoFile(file)
  	result[i] = NewAddition(file, data)
  	}

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 :)