thoughtworks#454 Fix - Malformed talismanrc should not be ignored and…

… raise error

cmd/acceptance_test.go

@@ -42,6 +42,13 @@ fileignoreconfig:
   checksum: 1db800b79e6e9695adc451f77be974dc47bcd84d42873560d7767bfca30db8b1
   ignore_detectors: []
 `
+const invalidTalismanRC = `
+fileignoreconfig:
+- filename: 
+private.pem
+  checksum: checksum_value
+  ignore_detectors: []
+  `
 
 func init() {
 	git_testing.Logger = logrus.WithField("Environment", "Debug")
@@ -297,6 +304,64 @@ func TestIgnoreHistoryDetectsExistingIssuesOnHead(t *testing.T) {
 	})
 }
 
+func TestTalismanFailsIfTalismanrcIsInvalidYamlInPrePushMode(t *testing.T) {
+	withNewTmpGitRepo(func(git *git_testing.GitTesting) {
+		git.SetupBaselineFiles("simple-file")
+		git.CreateFileWithContents(".talismanrc", invalidTalismanRC)
+		git.AddAndcommit("*", "Incorrect Talismanrc commit")
+
+		assert.Equal(t, 1, runTalismanInPrePushMode(git), "Expected run() to return 1 and fails as talismanrc is invalid")
+	})
+}
+
+func TestTalismanFailsIfTalismanrcIsInvalidYamlInPreCommitMode(t *testing.T) {
+	withNewTmpGitRepo(func(git *git_testing.GitTesting) {
+		options.Debug = true
+		options.GitHook = PreCommit
+		git.SetupBaselineFiles("simple-file")
+		git.CreateFileWithContents(".talismanrc", invalidTalismanRC)
+		git.AddAndcommit("*", "Incorrect Talismanrc commit")
+
+		assert.Equal(t, 1, runTalisman(git), "Expected run() to return 0 and pass as pem file was ignored")
+	})
+}
+
+func TestTalismanFailsIfTalismanrcIsInvalidYamlInScanMode(t *testing.T) {
+	withNewTmpGitRepo(func(git *git_testing.GitTesting) {
+		options.Debug = true
+		options.Scan = true
+		git.SetupBaselineFiles("simple-file")
+		git.CreateFileWithContents(".talismanrc", invalidTalismanRC)
+		git.AddAndcommit("*", "Incorrect Talismanrc commit")
+
+		assert.Equal(t, 1, runTalisman(git), "Expected run() to return 0 and pass as pem file was ignored")
+	})
+}
+
+func TestTalismanFailsIfTalismanrcIsInvalidYamlInScanWithHTMLMode(t *testing.T) {
+	withNewTmpGitRepo(func(git *git_testing.GitTesting) {
+		options.Debug = true
+		options.ScanWithHtml = true
+		git.SetupBaselineFiles("simple-file")
+		git.CreateFileWithContents(".talismanrc", invalidTalismanRC)
+		git.AddAndcommit("*", "Incorrect Talismanrc commit")
+
+		assert.Equal(t, 1, runTalisman(git), "Expected run() to return 0 and pass as pem file was ignored")
+	})
+}
+
+func TestTalismanFailsIfTalismanrcIsInvalidYamlInPatternMode(t *testing.T) {
+	withNewTmpGitRepo(func(git *git_testing.GitTesting) {
+		options.Debug = false
+		options.Pattern = "./*.*"
+
+		git.SetupBaselineFiles("simple-file")
+		git.CreateFileWithContents(".talismanrc", invalidTalismanRC)
+
+		assert.Equal(t, 1, runTalisman(git), "Expected run() to return 1 and fail as pem file was present in the repo")
+	})
+}
+
 func runTalismanInPrePushMode(git *git_testing.GitTesting) int {
 	options.Debug = true
 	options.GitHook = PrePush

cmd/talisman.go

@@ -143,25 +143,45 @@ func run(promptContext prompt.PromptContext) (returnCode int) {
 	fields := make(map[string]interface{})
 	_ = json.Unmarshal(optionsBytes, &fields)
 	log.WithFields(fields).Debug("Talisman execution environment")
-	defer  utility.DestroyHashers()
+	defer utility.DestroyHashers()
 	if options.Checksum != "" {
 		log.Infof("Running %s patterns against checksum calculator", options.Checksum)
 		return NewChecksumCmd(strings.Fields(options.Checksum)).Run()
 	} else if options.Scan {
 		log.Infof("Running scanner")
-		return NewScannerCmd(options.IgnoreHistory, options.ReportDirectory).Run(talismanrc.ForScan(options.IgnoreHistory))
+		talismanrcForScan, err := talismanrc.ForScan(options.IgnoreHistory)
+		if err != nil {
+			return EXIT_FAILURE
+		}
+		return NewScannerCmd(options.IgnoreHistory, options.ReportDirectory).Run(talismanrcForScan)
 	} else if options.ScanWithHtml {
 		log.Infof("Running scanner with html report")
-		return NewScannerCmd(options.IgnoreHistory, "talisman_html_report").Run(talismanrc.ForScan(options.IgnoreHistory))
+		talismanrcForScan, err := talismanrc.ForScan(options.IgnoreHistory)
+		if err != nil {
+			return EXIT_FAILURE
+		}
+		return NewScannerCmd(options.IgnoreHistory, "talisman_html_report").Run(talismanrcForScan)
 	} else if options.Pattern != "" {
 		log.Infof("Running scan for %s", options.Pattern)
-		return NewPatternCmd(options.Pattern).Run(talismanrc.For(talismanrc.HookMode), promptContext)
+		talismanrcForScan, err := talismanrc.For(talismanrc.HookMode)
+		if err != nil {
+			return EXIT_FAILURE
+		}
+		return NewPatternCmd(options.Pattern).Run(talismanrcForScan, promptContext)
 	} else if options.GitHook == PreCommit {
 		log.Infof("Running %s hook", options.GitHook)
-		return NewPreCommitHook().Run(talismanrc.For(talismanrc.HookMode), promptContext)
+		talismanrcForScan, err := talismanrc.For(talismanrc.HookMode)
+		if err != nil {
+			return EXIT_FAILURE
+		}
+		return NewPreCommitHook().Run(talismanrcForScan, promptContext)
 	} else {
 		log.Infof("Running %s hook", options.GitHook)
-		return NewPrePushHook(talismanInput).Run(talismanrc.For(talismanrc.HookMode), promptContext)
+		talismanrcForScan, err := talismanrc.For(talismanrc.HookMode)
+		if err != nil {
+			return EXIT_FAILURE
+		}
+		return NewPrePushHook(talismanInput).Run(talismanrcForScan, promptContext)
 	}
 }
 

detector/filename/filename_detector_test.go

@@ -169,7 +169,7 @@ func shouldNotFailWithDefaultDetectorAndIgnores(fileName, ignore string, thresho
 	fileIgnoreConfig := &talismanrc.FileIgnoreConfig{}
 	fileIgnoreConfig.FileName = ignore
 	fileIgnoreConfig.IgnoreDetectors = []string{"filename"}
-	talismanRC := talismanrc.For(talismanrc.HookMode)
+	talismanRC, _ := talismanrc.For(talismanrc.HookMode)
 	talismanRC.IgnoreConfigs = []talismanrc.IgnoreConfig{fileIgnoreConfig}
 
 	DefaultFileNameDetector(threshold).

detector/helpers/detection_results.go

@@ -45,10 +45,10 @@ type ResultsSummary struct {
 	Types FailureTypes `json:"types"`
 }
 
-//DetectionResults represents all interesting information collected during a detection run.
-//It serves as a collecting parameter for the tests performed by the various Detectors in the DetectorChain
-//Currently, it keeps track of failures and ignored files.
-//The results are grouped by FilePath for easy reporting of all detected problems with individual files.
+// DetectionResults represents all interesting information collected during a detection run.
+// It serves as a collecting parameter for the tests performed by the various Detectors in the DetectorChain
+// Currently, it keeps track of failures and ignored files.
+// The results are grouped by FilePath for easy reporting of all detected problems with individual files.
 type DetectionResults struct {
 	mode    talismanrc.Mode
 	Summary ResultsSummary   `json:"summary"`
@@ -64,7 +64,7 @@ func (r *DetectionResults) getResultDetailsForFilePath(fileName gitrepo.FilePath
 	return nil
 }
 
-//NewDetectionResults is a new DetectionResults struct. It represents the pre-run state of a Detection run.
+// NewDetectionResults is a new DetectionResults struct. It represents the pre-run state of a Detection run.
 func NewDetectionResults(mode talismanrc.Mode) *DetectionResults {
 	return &DetectionResults{
 		mode,
@@ -76,9 +76,9 @@ func NewDetectionResults(mode talismanrc.Mode) *DetectionResults {
 
 }
 
-//Fail is used to mark the supplied FilePath as failing a detection for a supplied reason.
-//Detectors are encouraged to provide context sensitive messages so that fixing the errors is made simple for the end user
-//Fail may be called multiple times for each FilePath and the calls accumulate the provided reasons
+// Fail is used to mark the supplied FilePath as failing a detection for a supplied reason.
+// Detectors are encouraged to provide context sensitive messages so that fixing the errors is made simple for the end user
+// Fail may be called multiple times for each FilePath and the calls accumulate the provided reasons
 func (r *DetectionResults) Fail(filePath gitrepo.FilePath, category string, message string, commits []string, severity severity.Severity) {
 	isFilePresentInResults := false
 	for resultIndex := 0; resultIndex < len(r.Results); resultIndex++ {
@@ -131,8 +131,8 @@ func (r *DetectionResults) Warn(filePath gitrepo.FilePath, category string, mess
 	r.Summary.Types.Warnings++
 }
 
-//Ignore is used to mark the supplied FilePath as being ignored.
-//The most common reason for this is that the FilePath is Denied by the Ignores supplied to the Detector, however, Detectors may use more sophisticated reasons to ignore files.
+// Ignore is used to mark the supplied FilePath as being ignored.
+// The most common reason for this is that the FilePath is Denied by the Ignores supplied to the Detector, however, Detectors may use more sophisticated reasons to ignore files.
 func (r *DetectionResults) Ignore(filePath gitrepo.FilePath, category string) {
 
 	isFilePresentInResults := false
@@ -176,12 +176,12 @@ func (r *DetectionResults) updateResultsSummary(category string, decr bool) {
 	}
 }
 
-//HasFailures answers if any Failures were detected for any FilePath in the current run
+// HasFailures answers if any Failures were detected for any FilePath in the current run
 func (r *DetectionResults) HasFailures() bool {
 	return r.Summary.Types.Filesize > 0 || r.Summary.Types.Filename > 0 || r.Summary.Types.Filecontent > 0
 }
 
-//HasIgnores answers if any FilePaths were ignored in the current run
+// HasIgnores answers if any FilePaths were ignored in the current run
 func (r *DetectionResults) HasIgnores() bool {
 	return r.Summary.Types.Ignores > 0
 }
@@ -194,12 +194,12 @@ func (r *DetectionResults) HasDetectionMessages() bool {
 	return r.HasWarnings() || r.HasFailures() || r.HasIgnores()
 }
 
-//Successful answers if no detector was able to find any possible result to fail the run
+// Successful answers if no detector was able to find any possible result to fail the run
 func (r *DetectionResults) Successful() bool {
 	return !r.HasFailures()
 }
 
-//GetFailures returns the various reasons that a given FilePath was marked as failing by all the detectors in the current run
+// GetFailures returns the various reasons that a given FilePath was marked as failing by all the detectors in the current run
 func (r *DetectionResults) GetFailures(fileName gitrepo.FilePath) []Details {
 	results := r.getResultDetailsForFilePath(fileName)
 	if results == nil {
@@ -232,7 +232,7 @@ func (r *DetectionResults) ReportWarnings() string {
 	return results.String()
 }
 
-//Report returns a string documenting the various failures and ignored files for the current run
+// Report returns a string documenting the various failures and ignored files for the current run
 func (r *DetectionResults) Report(promptContext prompt.PromptContext, mode string) string {
 	var result string
 	var filePathsForFailures []string
@@ -273,7 +273,8 @@ func (r *DetectionResults) suggestTalismanRC(filePaths []string, promptContext p
 
 	if promptContext.Interactive && runtime.GOOS != "windows" {
 		confirmedEntries := getUserConfirmation(entriesToAdd, promptContext)
-		talismanrc.ConfigFromFile().AddIgnores(r.mode, confirmedEntries)
+		talismanrcConfig, _ := talismanrc.ConfigFromFile()
+		talismanrcConfig.AddIgnores(r.mode, confirmedEntries)
 
 		for _, confirmedEntry := range confirmedEntries {
 			resultsDetails := r.getResultDetailsForFilePath(gitrepo.FilePath(confirmedEntry.GetFileName()))
@@ -329,7 +330,7 @@ func confirm(config talismanrc.IgnoreConfig, promptContext prompt.PromptContext)
 	return promptContext.Prompt.Confirm(confirmationString)
 }
 
-//ReportFileFailures adds a string to table documenting the various failures detected on the supplied FilePath by all detectors in the current run
+// ReportFileFailures adds a string to table documenting the various failures detected on the supplied FilePath by all detectors in the current run
 func (r *DetectionResults) ReportFileFailures(filePath gitrepo.FilePath) [][]string {
 	failureList := r.getResultDetailsForFilePath(filePath).FailureList
 	var data [][]string

talismanrc/entry-point.go

@@ -1,6 +1,7 @@
 package talismanrc
 
 import (
+	"fmt"
 	"regexp"
 	"talisman/utility"
 
@@ -16,25 +17,26 @@ var (
 	currentRCFileName  = DefaultRCFileName
 )
 
-func ReadConfigFromRCFile(fileReader func(string) ([]byte, error)) *persistedRC {
+func ReadConfigFromRCFile(fileReader func(string) ([]byte, error)) (*persistedRC, error) {
 	fileContents, err := fileReader(currentRCFileName)
 	if err != nil {
 		panic(err)
 	}
 	return newPersistedRC(fileContents)
 }
 
-func newPersistedRC(fileContents []byte) *persistedRC {
+func newPersistedRC(fileContents []byte) (*persistedRC, error) {
 	talismanRCFromFile := persistedRC{}
 	err := yaml.Unmarshal(fileContents, &talismanRCFromFile)
 	if err != nil {
 		logr.Errorf("Unable to parse .talismanrc : %v", err)
-		return &persistedRC{}
+		fmt.Println(fmt.Errorf("\n\x1b[1m\x1b[31mUnable to parse .talismanrc %s. Please ensure it is following the right YAML structure\x1b[0m\x1b[0m\n", err))
+		return &persistedRC{}, err
 	}
 	if talismanRCFromFile.Version == "" {
 		talismanRCFromFile.Version = DefaultRCVersion
 	}
-	return &talismanRCFromFile
+	return &talismanRCFromFile, nil
 }
 
 const (
@@ -68,7 +70,7 @@ func setRepoFileReader(rfr RepoFileReader) {
 	repoFileReader = func() RepoFileReader { return rfr }
 }
 
-func ConfigFromFile() *persistedRC {
+func ConfigFromFile() (*persistedRC, error) {
 	return ReadConfigFromRCFile(repoFileReader())
 }
 

talismanrc/talismanrc.go

@@ -44,7 +44,7 @@ type persistedRC struct {
 	Version          string                          `default:"2.0" yaml:"version"`
 }
 
-//SuggestRCFor returns the talismanRC file content corresponding to input ignore configs
+// SuggestRCFor returns the talismanRC file content corresponding to input ignore configs
 func SuggestRCFor(configs []IgnoreConfig) string {
 	fileIgnoreConfigs := []FileIgnoreConfig{}
 	for _, config := range configs {
@@ -61,17 +61,17 @@ func SuggestRCFor(configs []IgnoreConfig) string {
 	return string(result)
 }
 
-//AcceptsAll returns true if there are no rules specified
+// AcceptsAll returns true if there are no rules specified
 func (tRC *TalismanRC) AcceptsAll() bool {
 	return len(tRC.effectiveRules("any-detector")) == 0
 }
 
-//Accept answers true if the Addition.Path is configured to be checked by the detectors
+// Accept answers true if the Addition.Path is configured to be checked by the detectors
 func (tRC *TalismanRC) Accept(addition gitrepo.Addition, detectorName string) bool {
 	return !tRC.Deny(addition, detectorName)
 }
 
-//FilterAdditions removes scope files from additions
+// FilterAdditions removes scope files from additions
 func (tRC *TalismanRC) FilterAdditions(additions []gitrepo.Addition) []gitrepo.Addition {
 	var applicableScopeFileNames []string
 	if tRC.ScopeConfig != nil {
@@ -100,7 +100,7 @@ func (tRC *TalismanRC) FilterAdditions(additions []gitrepo.Addition) []gitrepo.A
 func (tRC *persistedRC) AddIgnores(mode Mode, entriesToAdd []IgnoreConfig) {
 	if len(entriesToAdd) > 0 {
 		logr.Debugf("Adding entries: %v", entriesToAdd)
-		talismanRCConfig := ConfigFromFile()
+		talismanRCConfig, _ := ConfigFromFile()
 		if mode == HookMode {
 			fileIgnoreEntries := make([]FileIgnoreConfig, len(entriesToAdd))
 			for idx, entry := range entriesToAdd {
@@ -154,7 +154,7 @@ func combineFileIgnores(exsiting, incoming []FileIgnoreConfig) []FileIgnoreConfi
 	return result
 }
 
-//Deny answers true if the Addition.Path is configured to be ignored and not checked by the detectors
+// Deny answers true if the Addition.Path is configured to be ignored and not checked by the detectors
 func (tRC *TalismanRC) Deny(addition gitrepo.Addition, detectorName string) bool {
 	for _, pattern := range tRC.effectiveRules(detectorName) {
 		if addition.Matches(pattern) {
@@ -164,8 +164,8 @@ func (tRC *TalismanRC) Deny(addition gitrepo.Addition, detectorName string) bool
 	return false
 }
 
-//Strip git addition 
-func(tRC *TalismanRC) FilterAllowedPatternsFromAddition(addition gitrepo.Addition) string {
+// Strip git addition
+func (tRC *TalismanRC) FilterAllowedPatternsFromAddition(addition gitrepo.Addition) string {
 	additionPathAsString := string(addition.Path)
 	// Processing global allowed patterns
 	for _, pattern := range tRC.AllowedPatterns {
@@ -222,13 +222,13 @@ func fromPersistedRC(configFromTalismanRCFile *persistedRC, mode Mode) *Talisman
 	return &tRC
 }
 
-func For(mode Mode) *TalismanRC {
-	configFromTalismanRCFile := ConfigFromFile()
+func For(mode Mode) (*TalismanRC, error) {
+	configFromTalismanRCFile, err := ConfigFromFile()
 	talismanRC := fromPersistedRC(configFromTalismanRCFile, mode)
-	return talismanRC
+	return talismanRC, err
 }
 
-func ForScan(ignoreHistory bool) *TalismanRC {
+func ForScan(ignoreHistory bool) (*TalismanRC, error) {
 	if ignoreHistory {
 		return For(HookMode)
 	}