Skip to content

Conversation

rafaveira3
Copy link
Contributor

@rafaveira3 rafaveira3 commented Dec 27, 2019

Closes #425

This PR aims to add an OR statement when checking a specific line of code for both default and alternative flag. If any of them is found, the line should be considered as a false positive.

Changes

  • analyze.go: Added an OR logic to check for both tags and updated the function description.
  • analyze_test.go: Updated one function description and adjusted another test as the default tag should also be considered when the alternative is set.
  • Makefile: Added a few aliases to avoid errors when working outside the GOPATH

@ccojocar
Copy link
Member

ccojocar commented Jan 2, 2020

@rafaveira3 Thanks for this contribution! It seems that one test is failing:

Analyzer when processing a package 
  should not ignore vulnerabilities
  /home/travis/gopath/src/github.com/securego/gosec/analyzer_test.go:292
• Failure [0.238 seconds]
Analyzer
/home/travis/gopath/src/github.com/securego/gosec/analyzer_test.go:19
  when processing a package
  /home/travis/gopath/src/github.com/securego/gosec/analyzer_test.go:32
    should not ignore vulnerabilities [It]
    /home/travis/gopath/src/github.com/securego/gosec/analyzer_test.go:292
    Expected
        <[]*gosec.Issue | len:0, cap:16>: []
    to have length 1
    /home/travis/gopath/src/github.com/securego/gosec/analyzer_test.go:312

Copy link
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good! Please could you fix the test? Thanks!

@codecov-io
Copy link

codecov-io commented Jan 4, 2020

Codecov Report

Merging #426 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #426      +/-   ##
==========================================
+ Coverage   71.38%   71.52%   +0.14%     
==========================================
  Files           9        9              
  Lines         601      604       +3     
==========================================
+ Hits          429      432       +3     
  Misses        154      154              
  Partials       18       18
Impacted Files Coverage Δ
analyzer.go 92.48% <100%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79fbf3a...21d827a. Read the comment docs.

@rafaveira3
Copy link
Contributor Author

Sure, @ccojocar! Sorry about that. I have adjusted the failing test to now pass as the default #nosec tag is being used at b00176a.

I have also proposed at 694b37d changes in the Makefile to add a few aliases to avoid errors when working outside the GOPATH.

@ccojocar ccojocar merged commit f43a957 into securego:master Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check for both tags when handling false positives
3 participants