talisman
talisman copied to clipboard
thoughtworks
Talisman pre-commit hook
I am running talisman as pre-commit hook. I have added a text file with password in it but during commit it did not raise any alert.
Please let me know whether it checks password or not. I ram talisman on Linux instance PFA is the log.
[XXXXXXXXXXXXX gittest]# git add Test.txt [XXXXXXXXXXXXX gittest]# git commit Test.txt -m "test" [master faf918b] test 1 file changed, 1 insertion(+), 1 deletion(-) [XXXXXXXXXXXXX gittest]# ls -lrt total 8 drwxr-xr-x 3 root root 18 Oct 29 10:40 talisman_reports drwxr-xr-x 2 root root 6 Oct 29 10:42 talisman_html_report -rw-r--r-- 1 root root 15 Oct 29 10:44 Test.txt [XXXXXXXXXXXXX gittest]# cat Test.txt passwords=1234 [XXXXXXXXXXXXX gittest]#
I can confirm this. Currently it only detects when the filename is password. Think there is some bug in the pattern detector.
In filename_detector.go, there is a pattern that literally says password, therefore any filename that contains the word password will match.
In pattern_detector.go, there are extra rules for what should surround the word password (as of commit dac0d04):
pattern_detector.go: regexp.MustCompile("(?i)(['\"_]?password['\"]? *[:=][^,;\n]{8,})"),
pattern_detector.go: regexp.MustCompile("(<[^(><.)]?password[^(><.)]*?>[^(><.)]+</[^(><.)]?password[^(><.)]*?>)"),
such as ', " optionally as prefix or suffix, but if they're not present, only password (note the space) will match for the first pattern, or password> for the second one.
@mrngm @jpninanjohn : So you are saying it will check only the filename with password and not inside the file?
As far as I can tell, it does check inside the file, but the patterns for filenames and contents are different.
Perhaps try it with a test2.txt containing:
password=hunter123
[XXXXXXXXXXXXX puneet]# git add Test.txt [XXXXXXXXXXXXX puneet]# git status On branch master
No commits yet
Changes to be committed:
(use "git rm --cached
new file: Test.txt
[XXXXXXXXXXXXX puneet]# git commit Test.txt -m "Test file" [master (root-commit) 34d79a7] Test file 1 file changed, 2 insertions(+) create mode 100644 Test.txt [XXXXXXXXXXXXX puneet]# cat Test.txt password=1234 Passwords=3456 [XXXXXXXXXXXXX puneet]# cd .git/hooks/ [XXXXXXXXXXXXX hooks]# l s-l -bash: l: command not found [XXXXXXXXXXXXX hooks]# ls -l total 0 lrwxrwxrwx 1 root root 40 Oct 30 05:56 pre-commit -> /root/.talisman/bin/talisman_hook_script [XXXXXXXXXXXXX hooks]#
As far as I can tell, it does check inside the file, but the patterns for filenames and contents are different.
Perhaps try it with a test2.txt containing:
password=hunter123
I tried this. It does not work. Currently I could only get it to work if the file was named password.
@mrngm @jpninanjohn : So you are saying it will check only the filename with password and not inside the file?
It does check. There is a bug somewhere I think.
There is another thing hidden in the pattern, it will only match when there are at least 8 characters after = and they must not be ,, ; or a newline.
As to why a default configuration would not scan files, I have no idea.
@jpninanjohn @mrngm : It should check inside the file. Password will be inside the file
There is another thing hidden in the pattern, it will only match when there are at least 8 characters after
=and they must not be,,;or a newline.As to why a default configuration would not scan files, I have no idea.
You are right. It works for text larger than 8 letters. Why is the length of the password considered?
@Puneet1726 Could you run a new test, but using TALISMAN_DEBUG="some-non-empty-value" git commit -m "your msg" file.txt when committing?
That should give some more information when running the hook. If you see no output, check the permissions of your /root/.talisman/bin/talisman_hook_script. They should have the executable bit set.
There is another thing hidden in the pattern, it will only match when there are at least 8 characters after
=and they must not be,,;or a newline. As to why a default configuration would not scan files, I have no idea.You are right. It works for text larger than 8 letters. Why is the length of the password considered?
Perhaps @harinee, @jacksingleton, or the original committer @prachisr can comment on that.
I do think this would be a bug if it is not looking into the content of a file.
There is another thing hidden in the pattern, it will only match when there are at least 8 characters after
=and they must not be,,;or a newline. As to why a default configuration would not scan files, I have no idea.You are right. It works for text larger than 8 letters. Why is the length of the password considered?
Perhaps @harinee, @jacksingleton, or the original committer @prachisr can comment on that.
This was because password policies in production systems typically impose a check of minimum 8 characters. The worry with introducing 'password' as a check in Talisman was because of the numerous places where it would cry out false positives in the code. So some constraints were put in place to try and detect if it would really be a password entered. Open to hear feedback on that though. (FYI, we have got some feedback already that even the current check throws out too many false positives. But no one raised any issues on github officially I think)
If its intended that passwords with less than 8 characters should not be detected, then this is not a bug. User was using a password with 4 characters in it.
I think the length of a password does not matter. At least for passwords, even if its a false positive, the user can ignore it explicitly.
Can it be added as a warning maybe?