Skip to content

Commit

Permalink
bundler:chore - improve tests and code cleaning
Browse files Browse the repository at this point in the history 
This commit add some new asserts on successful parsing bundler results
to verify that all fields of Vulnerability was filled.

Some code organization was also made, and the entities and enum packages
was removed and the bundler schema output was moved to bundler package.

This commit also fix a bug when parsing invalid output from Bundler.
The `strings.Split(output, "Name:")` on `parseOutput` return a list with
one element when the split fails, so when Bundler return an output that
is not expected we still try to parse this invalid output which results
invalid vulnerabilities. To fix this a validation was added before the
split to check if output contains the `Name:` field.

Updates #718

Signed-off-by: Matheus Alcantara <[email protected]>
  • Loading branch information
@matheusalcantarazup
matheusalcantarazup committed Jan 14, 2022
1 parent bfb07e6 commit cc057c3
Show file tree
Hide file tree
Showing 4 changed files with 123 additions and 117 deletions.
20 changes: 10 additions & 10 deletions ...uby/bundler/entities/enums/criticality.go → ...es/formatters/ruby/bundler/criticality.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,27 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package enums
package bundler

type CriticalityType string
type bundlerCriticalityType string

const (
High CriticalityType = "High"
Medium CriticalityType = "Medium"
Low CriticalityType = "Low"
High bundlerCriticalityType = "High"
Medium bundlerCriticalityType = "Medium"
Low bundlerCriticalityType = "Low"
)

func (c CriticalityType) ToString() string {
func (c bundlerCriticalityType) String() string {
return string(c)
}

func GetCriticalityTypeByString(criticalityType string) CriticalityType {
func getCriticalityTypeByString(criticalityType string) bundlerCriticalityType {
switch criticalityType {
case High.ToString():
case High.String():
return High
case Medium.ToString():
case Medium.String():
return Medium
case Low.ToString():
case Low.String():
return Low
}
return Low
Expand Down
44 changes: 19 additions & 25 deletions internal/services/formatters/ruby/bundler/formatter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (
"github.com/ZupIT/horusec/internal/enums/images"
"github.com/ZupIT/horusec/internal/helpers/messages"
"github.com/ZupIT/horusec/internal/services/formatters"
"github.com/ZupIT/horusec/internal/services/formatters/ruby/bundler/entities"
"github.com/ZupIT/horusec/internal/utils/file"
vulnhash "github.com/ZupIT/horusec/internal/utils/vuln_hash"
)
Expand Down Expand Up @@ -111,49 +110,44 @@ func (f *Formatter) removeOutputEsc(output string) string {
}

func (f *Formatter) parseOutput(output, projectSubPath string) {
if strings.Contains(output, "No vulnerabilities found") {
if strings.Contains(output, "No vulnerabilities found") || !strings.Contains(output, "Name:") {
return
}

for _, outputSplit := range strings.Split(output, "Name:") {
f.setOutput(outputSplit, projectSubPath)
f.processOutput(outputSplit, projectSubPath)
}
}

func (f *Formatter) setOutput(outputSplit, projectSubPath string) {
if outputSplit == "" {
func (f *Formatter) processOutput(outputData, projectSubPath string) {
if outputData == "" {
return
}

output := &entities.Output{}
for _, value := range strings.Split(outputSplit, "\r\n") {
var output bundlerOutput
for _, value := range strings.Split(outputData, "\r\n") {
if value == "" || value == "Vulnerabilities found!" {
continue
}

output.SetOutputData(output, value)
output.setOutputData(&output, value)
}

f.AddNewVulnerabilityIntoAnalysis(f.setVulnerabilityData(output, projectSubPath))
f.AddNewVulnerabilityIntoAnalysis(f.newVulnerability(&output, projectSubPath))
}

func (f *Formatter) setVulnerabilityData(output *entities.Output, projectSubPath string) *vulnerability.Vulnerability {
vuln := f.getDefaultVulnerabilitySeverity()
vuln.Confidence = confidence.Low
vuln.Severity = output.GetSeverity()
vuln.Details = output.GetDetails()
vuln.File = f.GetFilepathFromFilename("Gemfile.lock", projectSubPath)
vuln.Code = f.GetCodeWithMaxCharacters(output.Name, 0)
func (f *Formatter) newVulnerability(output *bundlerOutput, projectSubPath string) *vulnerability.Vulnerability {
vuln := &vulnerability.Vulnerability{
SecurityTool: tools.BundlerAudit,
Language: languages.Ruby,
Confidence: confidence.Low,
Severity: output.getSeverity(),
Details: output.getDetails(),
File: f.GetFilepathFromFilename("Gemfile.lock", projectSubPath),
Code: f.GetCodeWithMaxCharacters(output.Name, 0),
}
vuln.Line = f.getVulnerabilityLineByName(vuln.Code, vuln.File)
vuln = vulnhash.Bind(vuln)
return f.SetCommitAuthor(vuln)
}

func (f *Formatter) getDefaultVulnerabilitySeverity() *vulnerability.Vulnerability {
vulnerabilitySeverity := &vulnerability.Vulnerability{}
vulnerabilitySeverity.SecurityTool = tools.BundlerAudit
vulnerabilitySeverity.Language = languages.Ruby
return vulnerabilitySeverity
return f.SetCommitAuthor(vulnhash.Bind(vuln))
}

func (f *Formatter) getVulnerabilityLineByName(module, fileName string) string {
Expand Down
130 changes: 72 additions & 58 deletions internal/services/formatters/ruby/bundler/formatter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,124 +18,138 @@ import (
"errors"
"testing"

entitiesAnalysis "github.com/ZupIT/horusec-devkit/pkg/entities/analysis"
"github.com/ZupIT/horusec-devkit/pkg/entities/analysis"
"github.com/ZupIT/horusec-devkit/pkg/enums/confidence"
"github.com/ZupIT/horusec-devkit/pkg/enums/languages"
"github.com/ZupIT/horusec-devkit/pkg/enums/tools"
"github.com/stretchr/testify/assert"

cliConfig "github.com/ZupIT/horusec/config"
"github.com/ZupIT/horusec/config"
"github.com/ZupIT/horusec/internal/entities/toolsconfig"
"github.com/ZupIT/horusec/internal/entities/workdir"
"github.com/ZupIT/horusec/internal/services/formatters"
"github.com/ZupIT/horusec/internal/utils/testutil"
)

func TestParseOutput(t *testing.T) {
t.Run("Should success parse output to analysis", func(t *testing.T) {
analysis := &entitiesAnalysis.Analysis{}

config := &cliConfig.Config{}
config.WorkDir = &workdir.WorkDir{}
t.Run("should add 2 vulnerabilities on analysis with no errors", func(t *testing.T) {
analysis := new(analysis.Analysis)

dockerAPIControllerMock := testutil.NewDockerMock()
dockerAPIControllerMock.On("SetAnalysisID")

output := "\u001B[31mName: \u001B[0mactionpack\n\u001B[31mVersion: \u001B[0m6.0.0\n\u001B[31mAdvisory: \u001B[0mCVE-2020-8164\n\u001B[31mCriticality: \u001B[0mUnknown\n\u001B[31mURL: \u001B[0mhttps://groups.google.com/forum/#!topic/rubyonrails-security/f6ioe4sdpbY\n\u001B[31mTitle: \u001B[0mPossible Strong Parameters Bypass in ActionPack\n\u001B[31mSolution: upgrade to \u001B[0m~> 5.2.4.3, >= 6.0.3.1\n\n\u001B[31mName: \u001B[0mactionpack\n\u001B[31mVersion: \u001B[0m6.0.0\n\u001B[31mAdvisory: \u001B[0mCVE-2020-8166\n\u001B[31mCriticality: \u001B[0mUnknown\n\u001B[31mURL: \u001B[0mhttps://groups.google.com/forum/#!topic/rubyonrails-security/NOjKiGeXUgw\n\u001B[31mTitle: \u001B[0mAbility to forge per-form CSRF tokens given a global CSRF token\n\u001B[31mSolution: upgrade to \u001B[0m~> 5.2.4.3, >= 6.0.3.1\n"

dockerAPIControllerMock.On("CreateLanguageAnalysisContainer").Return(output, nil)

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, config)

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, newTestConfig(t, analysis))
formatter := NewFormatter(service)

assert.NotPanics(t, func() {
formatter.StartAnalysis("")
})
formatter.StartAnalysis("")

assert.Len(t, analysis.AnalysisVulnerabilities, 2)
for _, v := range analysis.AnalysisVulnerabilities {
vuln := v.Vulnerability
assert.Equal(t, tools.BundlerAudit, vuln.SecurityTool)
assert.Equal(t, languages.Ruby, vuln.Language)
assert.Equal(t, confidence.Low, vuln.Confidence)
assert.NotEmpty(t, vuln.Details, "Exepcted not empty details")
assert.NotContains(t, vuln.Details, "()")
assert.NotEmpty(t, vuln.File, "Expected not empty file name")
assert.NotEmpty(t, vuln.Line, "Expected not empty line")
assert.NotEmpty(t, vuln.Severity, "Expected not empty severity")
}
})

t.Run("Should return error when parsing invalid output", func(t *testing.T) {
analysis := &entitiesAnalysis.Analysis{}

config := &cliConfig.Config{}
config.WorkDir = &workdir.WorkDir{}
t.Run("should add no error and no vulnerabilities on analysis when parse invalid output", func(t *testing.T) {
analysis := new(analysis.Analysis)

dockerAPIControllerMock := testutil.NewDockerMock()
dockerAPIControllerMock.On("SetAnalysisID")
dockerAPIControllerMock.On("CreateLanguageAnalysisContainer").Return("invalid output", nil)

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, config)

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, newTestConfig(t, analysis))
formatter := NewFormatter(service)

formatter.StartAnalysis("")
})

t.Run("Should return error when gem lock not found", func(t *testing.T) {
analysis := &entitiesAnalysis.Analysis{}
assert.False(t, analysis.HasErrors(), "Expected no errors on analysis")
assert.Len(t, analysis.AnalysisVulnerabilities, 0)
})

config := &cliConfig.Config{}
config.WorkDir = &workdir.WorkDir{}
t.Run("Should add error on analysis when Gemfile.lock not found", func(t *testing.T) {
analysis := new(analysis.Analysis)

dockerAPIControllerMock := testutil.NewDockerMock()
dockerAPIControllerMock.On("SetAnalysisID")
dockerAPIControllerMock.On("CreateLanguageAnalysisContainer").
Return("No such file or directory Errno::ENOENT", nil)

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, config)
dockerAPIControllerMock.On("CreateLanguageAnalysisContainer").Return(`Could not find "Gemfile.lock"`, nil)

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, newTestConfig(t, analysis))
formatter := NewFormatter(service)

formatter.StartAnalysis("")
})

t.Run("Should return no vulnerabilities", func(t *testing.T) {
analysis := &entitiesAnalysis.Analysis{}
assert.True(t, analysis.HasErrors(), "Expected errors on analysis")
})

config := &cliConfig.Config{}
config.WorkDir = &workdir.WorkDir{}
t.Run("Should add no vulnerabilities on analysis when empty", func(t *testing.T) {
analysis := new(analysis.Analysis)

dockerAPIControllerMock := testutil.NewDockerMock()
dockerAPIControllerMock.On("SetAnalysisID")
dockerAPIControllerMock.On("CreateLanguageAnalysisContainer").
Return("No vulnerabilities found", nil)

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, config)
dockerAPIControllerMock.On("CreateLanguageAnalysisContainer").Return("No vulnerabilities found", nil)

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, newTestConfig(t, analysis))
formatter := NewFormatter(service)

formatter.StartAnalysis("")

assert.False(t, analysis.HasErrors(), "Expected no errors on analysis")
assert.Len(t, analysis.AnalysisVulnerabilities, 0)
})

t.Run("Should return error when something went wrong in container", func(t *testing.T) {
analysis := &entitiesAnalysis.Analysis{}
t.Run("Should add error on analysis when something went wrong in container", func(t *testing.T) {
analysis := new(analysis.Analysis)

dockerAPIControllerMock := testutil.NewDockerMock()
dockerAPIControllerMock.On("SetAnalysisID")
dockerAPIControllerMock.On("CreateLanguageAnalysisContainer").Return("", errors.New("test"))

config := &cliConfig.Config{}
config.WorkDir = &workdir.WorkDir{}

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, config)

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, newTestConfig(t, analysis))
formatter := NewFormatter(service)

formatter.StartAnalysis("")

assert.True(t, analysis.HasErrors(), "Expected errors on analysis")
})

t.Run("Should not execute tool because it's ignored", func(t *testing.T) {
analysis := &entitiesAnalysis.Analysis{}
analysis := new(analysis.Analysis)

dockerAPIControllerMock := testutil.NewDockerMock()
config := &cliConfig.Config{}
config.ToolsConfig = toolsconfig.ToolsConfig{
cfg := config.New()
cfg.ToolsConfig = toolsconfig.ToolsConfig{
tools.BundlerAudit: toolsconfig.Config{
IsToIgnore: true,
},
}

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, config)
service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, cfg)
formatter := NewFormatter(service)

formatter.StartAnalysis("")
})
}

func newTestConfig(t *testing.T, analysiss *analysis.Analysis) *config.Config {
cfg := config.New()
cfg.ProjectPath = testutil.CreateHorusecAnalysisDirectory(t, analysiss, testutil.RubyExample1)
return cfg
}

// output contains the expected output from bundler
//
// Note, output from bundler is colored, and the formatter remove this colors
// before the parsing, so the expected output.
//
// This output has the following schema:
//
// Name: actionpack
// Version: 6.0.0
// Advisory: CVE-2020-8164
// Criticality: Unknown
// URL: https://groups.google.com/forum/#!topic/rubyonrails-security/f6ioe4sdpbY
// Title: Possible Strong Parameters Bypass in ActionPack
// Solution: upgrade to ~> 5.2.4.3, >= 6.0.3.1
//
const output = "\u001B[31mName: \u001B[0mactionpack\n\u001B[31mVersion: \u001B[0m6.0.0\n\u001B[31mAdvisory: \u001B[0mCVE-2020-8164\n\u001B[31mCriticality: \u001B[0mUnknown\n\u001B[31mURL: \u001B[0mhttps://groups.google.com/forum/#!topic/rubyonrails-security/f6ioe4sdpbY\n\u001B[31mTitle: \u001B[0mPossible Strong Parameters Bypass in ActionPack\n\u001B[31mSolution: upgrade to \u001B[0m~> 5.2.4.3, >= 6.0.3.1\n\n\u001B[31mName: \u001B[0mactionpack\n\u001B[31mVersion: \u001B[0m6.0.0\n\u001B[31mAdvisory: \u001B[0mCVE-2020-8166\n\u001B[31mCriticality: \u001B[0mUnknown\n\u001B[31mURL: \u001B[0mhttps://groups.google.com/forum/#!topic/rubyonrails-security/NOjKiGeXUgw\n\u001B[31mTitle: \u001B[0mAbility to forge per-form CSRF tokens given a global CSRF token\n\u001B[31mSolution: upgrade to \u001B[0m~> 5.2.4.3, >= 6.0.3.1\n"
Loading

0 comments on commit cc057c3

Please sign in to comment.