From c66e8437d84d23e31688ae8220475b2d1301e91d Mon Sep 17 00:00:00 2001 From: Deepthi Date: Fri, 19 Jul 2024 07:10:32 +0530 Subject: [PATCH] #454 Fix - Malformed talismanrc should not be ignored and raise error (#458) --- cmd/acceptance_test.go | 65 +++++++++++++++++++++ cmd/talisman.go | 32 ++++++++-- detector/filename/filename_detector_test.go | 2 +- detector/helpers/detection_results.go | 35 +++++------ talismanrc/entry-point.go | 12 ++-- talismanrc/talismanrc.go | 24 ++++---- talismanrc/talismanrc_test.go | 40 +++++++------ 7 files changed, 151 insertions(+), 59 deletions(-) diff --git a/cmd/acceptance_test.go b/cmd/acceptance_test.go index ea0b04cc..22de4eaf 100644 --- a/cmd/acceptance_test.go +++ b/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 diff --git a/cmd/talisman.go b/cmd/talisman.go index 7add50ac..ea27c297 100644 --- a/cmd/talisman.go +++ b/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) } } diff --git a/detector/filename/filename_detector_test.go b/detector/filename/filename_detector_test.go index 0f31243f..f398ffbe 100644 --- a/detector/filename/filename_detector_test.go +++ b/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). diff --git a/detector/helpers/detection_results.go b/detector/helpers/detection_results.go index a3317367..a265ab40 100644 --- a/detector/helpers/detection_results.go +++ b/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 diff --git a/talismanrc/entry-point.go b/talismanrc/entry-point.go index a081ef85..7cd0211a 100644 --- a/talismanrc/entry-point.go +++ b/talismanrc/entry-point.go @@ -1,6 +1,7 @@ package talismanrc import ( + "fmt" "regexp" "talisman/utility" @@ -16,7 +17,7 @@ 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) @@ -24,17 +25,18 @@ func ReadConfigFromRCFile(fileReader func(string) ([]byte, error)) *persistedRC 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()) } diff --git a/talismanrc/talismanrc.go b/talismanrc/talismanrc.go index 8b8ba6bc..ba58b40e 100644 --- a/talismanrc/talismanrc.go +++ b/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 { @@ -223,13 +223,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) } diff --git a/talismanrc/talismanrc_test.go b/talismanrc/talismanrc_test.go index 96240912..fdf7d233 100644 --- a/talismanrc/talismanrc_test.go +++ b/talismanrc/talismanrc_test.go @@ -18,19 +18,22 @@ func init() { logr.SetOutput(ioutil.Discard) } -func TestShouldIgnoreEmptyLinesInTheFile(t *testing.T) { - defaultRepoFileReader := repoFileReader() - for _, s := range []string{"", " ", " ", "\t", " \t", "\t\t \t"} { - setRepoFileReader(func(string) ([]byte, error) { - return []byte(s), nil - }) +func TestForTalismanFileStructure(t *testing.T) { + var repoFileReader = func(string) ([]byte, error) { + return []byte(`fileignoreconfig: +- filename: testfile_1.yml + checksum: file1_checksum - talismanRC := For(HookMode) - assert.True(t, talismanRC.AcceptsAll(), "Expected '%s' to result in no ignore patterns.", s) - talismanRC = For(ScanMode) - assert.True(t, talismanRC.AcceptsAll(), "Expected '%s' to result in no ignore patterns.", s) + +custom_patterns: +- 'pwd_[a-z]{8,20}'`), nil } - setRepoFileReader(defaultRepoFileReader) + t.Run("talismanrc should not fail as long as the yaml structure is correct", func(t *testing.T) { + setRepoFileReader(repoFileReader) + rc, _ := For(HookMode) + assert.Equal(t, 1, len(rc.IgnoreConfigs)) + assert.Equal(t, 1, len(rc.CustomPatterns)) + }) } func TestShouldIgnoreUnformattedFiles(t *testing.T) { @@ -40,9 +43,9 @@ func TestShouldIgnoreUnformattedFiles(t *testing.T) { return []byte(s), nil }) - talismanRC := For(HookMode) + talismanRC, _ := For(HookMode) assert.True(t, talismanRC.AcceptsAll(), "Expected commented line '%s' to result in no ignore patterns", s) - talismanRC = For(ScanMode) + talismanRC, _ = For(ScanMode) assert.True(t, talismanRC.AcceptsAll(), "Expected commented line '%s' to result in no ignore patterns", s) } setRepoFileReader(defaultRepoFileReader) @@ -75,7 +78,8 @@ func TestShouldFilterAllowedPatternsFromAdditionBasedOnFileConfig(t *testing.T) func TestShouldConvertThresholdToValue(t *testing.T) { talismanRCContents := []byte("threshold: high") - assert.Equal(t, newPersistedRC(talismanRCContents).Threshold, severity.High) + persistedTalismanrc, _ := newPersistedRC(talismanRCContents) + assert.Equal(t, persistedTalismanrc.Threshold, severity.High) } func TestObeysCustomSeverityLevelsAndThreshold(t *testing.T) { @@ -173,7 +177,7 @@ func TestAddIgnoreFilesInHookMode(t *testing.T) { os.Remove(DefaultRCFileName) talismanRCConfig := createTalismanRCWithScopeIgnores([]string{}) talismanRCConfig.base.AddIgnores(HookMode, []IgnoreConfig{ignoreConfig}) - talismanRCConfigFromFile := ConfigFromFile() + talismanRCConfigFromFile, _ := ConfigFromFile() assert.Equal(t, 1, len(talismanRCConfigFromFile.FileIgnoreConfig)) os.Remove(DefaultRCFileName) } @@ -303,7 +307,7 @@ func TestFor(t *testing.T) { } t.Run("talismanrc.For(mode) should read multiple entries in rc file correctly", func(t *testing.T) { setRepoFileReader(repoFileReader) - rc := For(HookMode) + rc, _ := For(HookMode) assert.Equal(t, 3, len(rc.IgnoreConfigs)) assert.Equal(t, rc.IgnoreConfigs[0].GetFileName(), "testfile_1.yml") @@ -329,7 +333,7 @@ func TestForScan(t *testing.T) { } t.Run("talismanrc.ForScan(ignoreHistory) should populate talismanrc for scan mode with ignore history", func(t *testing.T) { setRepoFileReader(repoFileReader) - rc := ForScan(true) + rc, _ := ForScan(true) assert.Equal(t, 3, len(rc.IgnoreConfigs)) @@ -337,7 +341,7 @@ func TestForScan(t *testing.T) { t.Run("talismanrc.ForScan(ignoreHistory) should populate talismanrc for scan mode without ignore history", func(t *testing.T) { setRepoFileReader(repoFileReader) - rc := ForScan(false) + rc, _ := ForScan(false) assert.Equal(t, 0, len(rc.IgnoreConfigs))