Linter refactor #78

evqna · 2018-12-20T17:40:05Z

Currently the code that deals with running linters and collecting the results of analysis is scattered around the code base which makes it difficult to change and understand. Additionally, intricacies such as the way gosec interacts with the folder structure have to be accounted in various places which also increases the mental overhead when dealing with simple tasks.

This PR attempts to hide all linter-specific behavior under a common interface Runner that executes the common scanning and reporting logic. The linters folder contains the specifics of each linter runner instance (command line arguments, report parsing logic, ...). I'm not entirely satisfied with the current design (having new Bandit() in javascript is quite ugly IMO), i'm open to suggestions to change it.

Some other notable changes:

I dropped the baseline code as it was specific to Bandit and we weren't using it anymore
The runner only gives the GitHub report back, not the intermediate scan output so I had to modify all tests

TODO:

Switch the main app logic to new runner code
Update all tests to use new mechanism
Document interface and new code
Figure a systematic way to log information
~~Merge report generation code with new linter file?~~ -> Coming in follow up PR

ericwb · 2018-12-21T02:14:25Z

cache.js

-  } else if (fileType === 'python') {
-    writeFileCreateDirs(path.join('cache', repoID.toString(), prID.toString(), branchTag, filePath), data)
-  }
+  const appTag = (fileType === 'go') ? 'gosec' : 'bandit'


Should probably start using a const for 'gosec' and 'bandit'. I've seen the strings more than once. Probably from bandit.js and gosec.js would be appropriate.

I agree but I'd rather tackle that as a follow up.
Opening #103

ericwb · 2018-12-21T02:23:32Z

runner.js

+ * @param {string} prID
+ */
+async function runLinters (files, repoID, prID) {
+  // TODO: Sync with file download location resolution


Also in the future, it would be nicer if the linters were pluggable and not hardcoded here. I imagine a linter registration model, that the runner would iterate through the loaded linters.

I thought the same yes. That would be really cool.

Yes, this is a rough first draft but it would be nice to have an array of objects and just iterate through it. I'll see what I can do.

linters/index.js now exports a list of code linters, we might add a utility function to register linters dynamically later

Note that this requires the report generation rewrite, coming in a followup PR

linters/bandit.js

runner.js

ericwb · 2018-12-21T02:31:03Z

linters/bandit.js

+   * @param {any} results Linter results
+   * @returns GitHub checks report
+   */
+  generateReport (results) {


Looks like both bandit.js and gosec.js have this method which just calls another. Do we anticipate future linters doing other work here?

For now the report generation logic lives in a separate file for bandit and gosec but it may not be necessary as most of it is similar and could be factored away.
This PR is already large enough so right now I'm taking the simplest approach.

Also bringing everything in the same file might make it more difficult to test correctly, though I am open to suggestions.
Of course we can export every helper function but then there is not much point in getting rid of the helper files.

linters/bandit.js

index.js

test/gosec.spawn.test.js

MVrachev

I don't see where do you use bandit/bandit.js and gosec/gosec.js files.
Also there is something important if you ask me in gosec/gosec.js file which is missing when you spawn Gosec:

The reason this is important is that when Gosec fails it throws an exception and the exception usually comes from the fact that the report file doesn't exists and fs.readFileSync throws that exception.
But if you have the logs how Gosec had started it's much more easier for debug as I explain here:
#61

test/gosec.spawn.test.js

evqna · 2018-12-21T18:08:41Z

@MVrachev Yes I didn't put the logging code back in yet because I wanted to try to use Bunyan (the probot logger) instead of console.log but all things considered this is outside the scope of this PR. I'll copy your changes and we can decide on the logging situation later.

The baseline feature is now irrelevant since we rely on GitHub to correctly filter the annotations and only display relevant ones. This is split off from #78 to help trim it down.

evqna · 2019-01-10T22:58:36Z

Ready for review.

linters/bandit.js

MVrachev · 2019-01-11T08:48:36Z

linters/gosec.js

+   * relative to the working directory
+   */
+  get reportPath () {
+    return '../gosec.json'


Same here as I mentioned before in the linters/bandit.js file

Again I expected change here.
If you have different opinion I will be glad to hear it.

test/gosec.spawn.test.js

linters/bandit.js

MVrachev · 2019-01-15T16:04:28Z

linters/gosec.js

+   * relative to the working directory
+   */
+  get reportPath () {
+    return '../gosec.json'


Again I expected change here.
If you have different opinion I will be glad to hear it.

linters/index.js

test/gosec.spawn.test.js

Signed-off-by: Antoine Salon <[email protected]>

Use 'safe' and 'vulnerable' to describe all instances of test files

Signed-off-by: Antoine Salon <[email protected]>

MVrachev

Now it looks good to me.

evqna requested a review from a team December 20, 2018 17:40

vmwclabot added the cla-not-required label Dec 20, 2018

ericwb suggested changes Dec 21, 2018

View reviewed changes

MVrachev suggested changes Dec 21, 2018

View reviewed changes

test/gosec.spawn.test.js Outdated Show resolved Hide resolved

MVrachev suggested changes Dec 21, 2018

View reviewed changes

test/gosec.spawn.test.js Show resolved Hide resolved

test/gosec.spawn.test.js Outdated Show resolved Hide resolved

test/gosec.spawn.test.js Show resolved Hide resolved

evqna added the WIP label Dec 21, 2018

evqna mentioned this pull request Jan 9, 2019

Remove all baseline related code #92

Merged

evqna removed the WIP label Jan 10, 2019

MVrachev suggested changes Jan 11, 2019

View reviewed changes

MVrachev suggested changes Jan 15, 2019

View reviewed changes

Antoine Salon added 11 commits January 15, 2019 13:38

Adding new linter interface and runner

b36509e

Signed-off-by: Antoine Salon <[email protected]>

Switch code and tests to new linter runner

664a137

Signed-off-by: Antoine Salon <[email protected]>

Added documentation

20150c1

Signed-off-by: Antoine Salon <[email protected]>

Linter fixes

0402b8a

Signed-off-by: Antoine Salon <[email protected]>

Log linter results

b2af24a

Signed-off-by: Antoine Salon <[email protected]>

Remove obsolete files

c6ba61e

Signed-off-by: Antoine Salon <[email protected]>

runner: return default report when there are no files to analyze

3ae0d45

Signed-off-by: Antoine Salon <[email protected]>

travis: add cache dir to GOPATH

a6ccc0d

Signed-off-by: Antoine Salon <[email protected]>

baseline: remove leftovers

e7c9467

Signed-off-by: Antoine Salon <[email protected]>

Naming consistency pass

677c6d6

Use 'safe' and 'vulnerable' to describe all instances of test files

refactor: handle array of linters instead of individual ones

b71fd62

Signed-off-by: Antoine Salon <[email protected]>

evqna mentioned this pull request Jan 15, 2019

Report generation refactoring #104

Merged

Move report path resolution to runner

b681814

Signed-off-by: Antoine Salon <[email protected]>

MVrachev approved these changes Jan 17, 2019

View reviewed changes

MVrachev mentioned this pull request Jan 17, 2019

Do a initial versioned release #96

Open

ericwb approved these changes Jan 17, 2019

View reviewed changes

ericwb merged commit 0dc05ad into securesauce:master Jan 17, 2019

evqna deleted the linter_refactor branch January 18, 2019 00:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linter refactor #78

Linter refactor #78

evqna commented Dec 20, 2018 •

edited by ericwb

Loading

ericwb Dec 21, 2018

evqna Jan 15, 2019

ericwb Dec 21, 2018

MVrachev Dec 21, 2018

evqna Dec 21, 2018

evqna Jan 15, 2019

evqna Jan 15, 2019

ericwb Dec 21, 2018

evqna Jan 10, 2019

evqna Jan 15, 2019

MVrachev left a comment

evqna commented Dec 21, 2018

evqna commented Jan 10, 2019

MVrachev Jan 11, 2019

MVrachev Jan 15, 2019

evqna Jan 15, 2019

MVrachev Jan 15, 2019

MVrachev left a comment

Linter refactor #78

Linter refactor #78

Conversation

evqna commented Dec 20, 2018 • edited by ericwb Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev left a comment

Choose a reason for hiding this comment

evqna commented Dec 21, 2018

evqna commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev left a comment

Choose a reason for hiding this comment

evqna commented Dec 20, 2018 •

edited by ericwb

Loading