Skip to content

Commit

Permalink
brakeman:chore - improve tests asserts and code cleaning
Browse files Browse the repository at this point in the history 
Previously the tests was not asserting that we was correctly parsing the
output from brakeman and we was only checking that vulnerabilities was
being added on analysis without checking if their contents was
correctly.

This commit add some new checks to assert that all vulnerabilities
founded by brakeman is parsed correctly.

Updates #718

Signed-off-by: Matheus Alcantara <matheus.alcantara@zup.com.br>
  • Loading branch information
@matheusalcantarazup
matheusalcantarazup committed Dec 16, 2021
1 parent 2937c65 commit d7f1161
Show file tree
Hide file tree
Showing 5 changed files with 158 additions and 125 deletions.
19 changes: 0 additions & 19 deletions internal/services/formatters/ruby/brakeman/entities/output.go
View file

This file was deleted.

56 changes: 29 additions & 27 deletions internal/services/formatters/ruby/brakeman/formatter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,28 +17,36 @@ package brakeman
import (
"encoding/json"
"errors"
"path/filepath"
"strings"

"github.com/ZupIT/horusec-devkit/pkg/entities/vulnerability"
"github.com/ZupIT/horusec-devkit/pkg/enums/languages"
"github.com/ZupIT/horusec-devkit/pkg/enums/tools"
"github.com/ZupIT/horusec-devkit/pkg/utils/logger"

dockerEntities "github.com/ZupIT/horusec/internal/entities/docker"
"github.com/ZupIT/horusec/internal/entities/docker"
"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/brakeman/entities"
vulnhash "github.com/ZupIT/horusec/internal/utils/vuln_hash"
)

const (
defaultOutputMaxCharacters = 45

// notFoundError is the error returned by brakeman when the path
// analyzed is not a Rails project.
notFoundError = "please supply the path to a rails application"
)

var ErrNotFoundRailsProject = errors.New("brakeman only works on Ruby On Rails project")

type Formatter struct {
formatters.IService
}

func NewFormatter(service formatters.IService) formatters.IFormatter {
func NewFormatter(service formatters.IService) *Formatter {
return &Formatter{
service,
}
Expand Down Expand Up @@ -78,42 +86,38 @@ func (f *Formatter) parseOutput(containerOutput, projectSubPath string) error {

for _, warning := range brakemanOutput.Warnings {
value := warning
f.AddNewVulnerabilityIntoAnalysis(f.setVulnerabilityData(&value, projectSubPath))
f.AddNewVulnerabilityIntoAnalysis(f.newVulnerability(&value, projectSubPath))
}

return nil
}

func (f *Formatter) newContainerOutputFromString(containerOutput string) (output entities.Output, err error) {
func (f *Formatter) newContainerOutputFromString(containerOutput string) (output brakemanOutput, err error) {
if f.isNotFoundRailsProject(containerOutput) {
return entities.Output{}, ErrNotFoundRailsProject
return brakemanOutput{}, ErrNotFoundRailsProject
}

err = json.Unmarshal([]byte(containerOutput), &output)
return output, err
}

func (f *Formatter) setVulnerabilityData(output *entities.Warning, projectSubPath string) *vulnerability.Vulnerability {
data := f.getDefaultVulnerabilitySeverity()
data.Severity = output.GetSeverity()
data.Confidence = output.GetConfidence()
data.Details = output.GetDetails()
data.Line = output.GetLine()
data.File = f.GetFilepathFromFilename(output.File, projectSubPath)
data.Code = f.GetCodeWithMaxCharacters(output.Code, 0)
data = vulnhash.Bind(data)
return f.SetCommitAuthor(data)
}
func (f *Formatter) newVulnerability(output *warning, projectSubPath string) *vulnerability.Vulnerability {
vuln := &vulnerability.Vulnerability{
SecurityTool: tools.Brakeman,
Language: languages.Ruby,
Severity: output.getSeverity(),
Confidence: output.getConfidence(),
Details: output.getDetails(),
Line: output.getLine(),
File: f.GetFilepathFromFilename(filepath.FromSlash(output.File), projectSubPath),
Code: f.GetCodeWithMaxCharacters(output.Code, 0),
}

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

func (f *Formatter) getDockerConfig(projectSubPath string) *dockerEntities.AnalysisData {
analysisData := &dockerEntities.AnalysisData{
func (f *Formatter) getDockerConfig(projectSubPath string) *docker.AnalysisData {
analysisData := &docker.AnalysisData{
CMD: f.AddWorkDirInCmd(CMD, projectSubPath, tools.Brakeman),
Language: languages.Ruby,
}
Expand All @@ -122,10 +126,8 @@ func (f *Formatter) getDockerConfig(projectSubPath string) *dockerEntities.Analy
}

func (f *Formatter) isNotFoundRailsProject(output string) bool {
const DefaultOutputMaxCharacters = 45
lowerOutput := strings.ToLower(output)
notFoundError := strings.ToLower("Please supply the path to a Rails application")
if len(lowerOutput) >= DefaultOutputMaxCharacters {
if len(lowerOutput) >= defaultOutputMaxCharacters {
return strings.Contains(lowerOutput[:45], notFoundError)
}
return false
Expand Down
160 changes: 103 additions & 57 deletions internal/services/formatters/ruby/brakeman/formatter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,71 +18,66 @@ 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/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) {
func TestParseBrakemanOutput(t *testing.T) {
t.Run("Should success parse output to analysis", func(t *testing.T) {
analysis := &entitiesAnalysis.Analysis{}
analysis := new(analysis.Analysis)

config := &cliConfig.Config{}
config.WorkDir = &workdir.WorkDir{}
cfg := config.New()
cfg.ProjectPath = testutil.CreateHorusecAnalysisDirectory(t, analysis, testutil.RubyExample1)

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

output := "{\"warnings\":[{\"warning_type\":\"Command Injection\",\"warning_code\":14,\"check_name\":\"Execute\",\"message\":\"Possible command injection\",\"file\":\"app/controllers/application_controller.rb\",\"line\":4,\"code\":\"system(\\\"ls #{options}\\\")\",\"render_path\":null,\"user_input\":\"options\",\"confidence\":\"Low\"},{\"warning_type\":\"Command Injection\",\"warning_code\":14,\"check_name\":\"Execute\",\"message\":\"Possible command injection\",\"file\":\"app/controllers/application_controller.rb\",\"line\":4,\"code\":\"system(\\\"ls #{options}\\\")\",\"render_path\":null,\"user_input\":\"options\",\"confidence\":\"Medium\"},{\"warning_type\":\"Command Injection\",\"warning_code\":14,\"check_name\":\"Execute\",\"message\":\"Possible command injection\",\"file\":\"app/controllers/application_controller.rb\",\"line\":4,\"code\":\"system(\\\"ls #{options}\\\")\",\"render_path\":null,\"user_input\":\"options\",\"confidence\":\"High\"},{\"warning_type\":\"Command Injection\",\"warning_code\":14,\"check_name\":\"Execute\",\"message\":\"Possible command injection\",\"file\":\"app/controllers/application_controller.rb\",\"line\":4,\"code\":\"system(\\\"ls #{options}\\\")\",\"render_path\":null,\"user_input\":\"options\",\"confidence\":\"Test\"}]}"
service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, cfg)

dockerAPIControllerMock.On("CreateLanguageAnalysisContainer").Return(output, nil)
formatter := NewFormatter(service)
formatter.StartAnalysis("")

service := formatters.NewFormatterService(analysis, dockerAPIControllerMock, config)
assert.Len(t, analysis.AnalysisVulnerabilities, 4)

formatter := NewFormatter(service)
for _, v := range analysis.AnalysisVulnerabilities {
vuln := v.Vulnerability

assert.NotPanics(t, func() {
formatter.StartAnalysis("")
})
assert.Equal(t, tools.Brakeman, vuln.SecurityTool)
assert.Equal(t, languages.Ruby, vuln.Language)
assert.NotEmpty(t, vuln.Details, "Expected not empty details")
assert.NotEmpty(t, vuln.Code, "Expected not empty code")
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")

assert.Len(t, analysis.AnalysisVulnerabilities, 4)
}
})

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

config := &cliConfig.Config{}
config.WorkDir = &workdir.WorkDir{}
analysis := new(analysis.Analysis)

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

output := ""

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

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

formatter := NewFormatter(service)

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

assert.Len(t, analysis.AnalysisVulnerabilities, 0)
})

t.Run("Should error rails not found when parse output to analysis", func(t *testing.T) {
analysis := &entitiesAnalysis.Analysis{}

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

dockerAPIControllerMock := testutil.NewDockerMock()
dockerAPIControllerMock.On("SetAnalysisID")
Expand All @@ -91,63 +86,114 @@ func TestParseOutput(t *testing.T) {

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

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

formatter := NewFormatter(service)

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

assert.NotEmpty(t, analysis.Errors)
})

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 error on analysis when parsing 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, config.New())

formatter := NewFormatter(service)

formatter.StartAnalysis("")

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

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, config.New())

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.Brakeman: toolsconfig.Config{
IsToIgnore: true,
},
}

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

formatter := NewFormatter(service)
formatter.StartAnalysis("")
})
}

const outputMock = `
{
"warnings": [
{
"warning_type": "Command Injection",
"warning_code": 14,
"check_name": "Execute",
"message": "Possible command injection",
"file": "app/controllers/application_controller.rb",
"line": 4,
"code": "system(\"ls #{options}\")",
"render_path": null,
"user_input": "options",
"confidence": "Low"
},
{
"warning_type": "Command Injection",
"warning_code": 14,
"check_name": "Execute",
"message": "Possible command injection",
"file": "app/controllers/application_controller.rb",
"line": 4,
"code": "system(\"ls #{options}\")",
"render_path": null,
"user_input": "options",
"confidence": "Medium"
},
{
"warning_type": "Command Injection",
"warning_code": 14,
"check_name": "Execute",
"message": "Possible command injection",
"file": "app/controllers/application_controller.rb",
"line": 4,
"code": "system(\"ls #{options}\")",
"render_path": null,
"user_input": "options",
"confidence": "High"
},
{
"warning_type": "Command Injection",
"warning_code": 14,
"check_name": "Execute",
"message": "Possible command injection",
"file": "app/controllers/application_controller.rb",
"line": 4,
"code": "system(\"ls #{options}\")",
"render_path": null,
"user_input": "options",
"confidence": "Test"
}
]
}
`
Loading

0 comments on commit d7f1161

Please sign in to comment.