Merge experimental / refactor #146

gcmurphy · 2017-12-13T06:50:56Z

I've had a number of changes that I've been sitting on for a while that I think need to get merged to move forward.

The most notable:

gas root repository acts more like a library, with the cmd being moved off to cmd/gas directory.
gas will only process valid go packages rather than individual files. This significantly simplifies the file include/exclude file logic as it works as the compiler does. To recursively evaluate packages you can still run: gas $GOPATH/src/github.com/GoASTScanner/gas/... for example. The vendor directory and any test files will be excluded by default.
Moved to using ginkgo for writing tests.

There is still much to do but this change is getting too big, and am getting pinged with issues & PR's that won't be relevant under this model.

Initial commit to change GAS to process packages rather than standalone files. This is to address issues with type resolution for external dependencies. Uses golang.org/x/tools/go/loader to prepare analyzer input rather than finding the individual files.

The analyzer now only handles packages rather than one off files. This simplifies the CLI functionality significantly.

- Get rid of 'core' and move CLI to cmd/gas directory - Migrate (most) tests to use Ginkgo and testutils framework - GAS now expects package to reside in $GOPATH - GAS now can resolve dependencies for better type checking (if package on GOPATH) - Simplified public API

output/formatter: Issue.Line was already a string

houndci-bot · 2017-12-13T06:51:17Z

cmd/gas/main.go

+}
+
+func loadRules(include, exclude string) rules.RuleList {
+	filters := make([]rules.RuleFilter, 0)


can probably use "var filters []rules.RuleFilter" instead

houndci-bot · 2017-12-13T06:51:17Z

import_tracker.go

+	}
+}
+
+func (t *ImportTracker) TrackImport(n ast.Node) {


exported method ImportTracker.TrackImport should have comment or be unexported

houndci-bot · 2017-12-13T06:51:18Z

import_tracker.go

+	}
+}
+
+func (t *ImportTracker) TrackPackages(pkgs ...*types.Package) {


exported method ImportTracker.TrackPackages should have comment or be unexported

houndci-bot · 2017-12-13T06:51:18Z

import_tracker.go

+	InitOnly map[string]bool
+}
+
+func NewImportTracker() *ImportTracker {


exported function NewImportTracker should have comment or be unexported

houndci-bot · 2017-12-13T06:51:18Z

import_tracker.go

+	"strings"
+)
+
+type ImportTracker struct {


exported type ImportTracker should have comment or be unexported

houndci-bot · 2017-12-13T06:51:18Z

rules/tls.go

@@ -162,12 +169,12 @@ func NewIntermediateTlsCheck(conf map[string]interface{}) (gas.Rule, []ast.Node)
 	}, []ast.Node{(*ast.CompositeLit)(nil)}
 }

-func NewCompatTlsCheck(conf map[string]interface{}) (gas.Rule, []ast.Node) {
+func NewCompatTlsCheck(conf gas.Config) (gas.Rule, []ast.Node) {


exported function NewCompatTlsCheck should have comment or be unexported
func NewCompatTlsCheck should be NewCompatTLSCheck

houndci-bot · 2017-12-13T06:51:18Z

rules/tls.go

@@ -136,12 +143,12 @@ func NewModernTlsCheck(conf map[string]interface{}) (gas.Rule, []ast.Node) {
 	}, []ast.Node{(*ast.CompositeLit)(nil)}
 }

-func NewIntermediateTlsCheck(conf map[string]interface{}) (gas.Rule, []ast.Node) {
+func NewIntermediateTlsCheck(conf gas.Config) (gas.Rule, []ast.Node) {


exported function NewIntermediateTlsCheck should have comment or be unexported
func NewIntermediateTlsCheck should be NewIntermediateTLSCheck

houndci-bot · 2017-12-13T06:51:19Z

rules/tls.go

@@ -121,12 +128,12 @@ func (t *InsecureConfigTLS) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, er
 	return
 }

-func NewModernTlsCheck(conf map[string]interface{}) (gas.Rule, []ast.Node) {
+func NewModernTlsCheck(conf gas.Config) (gas.Rule, []ast.Node) {


exported function NewModernTlsCheck should have comment or be unexported
func NewModernTlsCheck should be NewModernTLSCheck

houndci-bot · 2017-12-13T06:51:19Z

rules/templates.go

@@ -37,9 +36,10 @@ func (t *TemplateCheck) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err er
 	return nil, nil
 }

-func NewTemplateCheck(conf map[string]interface{}) (gas.Rule, []ast.Node) {
+func NewTemplateCheck(conf gas.Config) (gas.Rule, []ast.Node) {


exported function NewTemplateCheck should have comment or be unexported

houndci-bot · 2017-12-13T06:51:19Z

rules/templates.go

 }

-func (t *TemplateCheck) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) {
-	if node := gas.MatchCall(n, t.call); node != nil {
+func (t *TemplateCheck) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {


exported method TemplateCheck.Match should have comment or be unexported

houndci-bot · 2017-12-13T06:51:19Z

cmd/gas/main.go

+}
+
+func loadRules(include, exclude string) rules.RuleList {
+	filters := make([]rules.RuleFilter, 0)


can probably use "var filters []rules.RuleFilter" instead

houndci-bot · 2017-12-13T06:51:19Z

import_tracker.go

+	}
+}
+
+func (t *ImportTracker) TrackImport(n ast.Node) {


exported method ImportTracker.TrackImport should have comment or be unexported

houndci-bot · 2017-12-13T06:51:19Z

import_tracker.go

+	}
+}
+
+func (t *ImportTracker) TrackPackages(pkgs ...*types.Package) {


exported method ImportTracker.TrackPackages should have comment or be unexported

houndci-bot · 2017-12-13T06:51:20Z

import_tracker.go

+	InitOnly map[string]bool
+}
+
+func NewImportTracker() *ImportTracker {


exported function NewImportTracker should have comment or be unexported

houndci-bot · 2017-12-13T06:51:20Z

import_tracker.go

+	"strings"
+)
+
+type ImportTracker struct {


exported type ImportTracker should have comment or be unexported

houndci-bot · 2017-12-13T06:51:20Z

rules/tls.go

@@ -162,12 +169,12 @@ func NewIntermediateTlsCheck(conf map[string]interface{}) (gas.Rule, []ast.Node)
 	}, []ast.Node{(*ast.CompositeLit)(nil)}
 }

-func NewCompatTlsCheck(conf map[string]interface{}) (gas.Rule, []ast.Node) {
+func NewCompatTlsCheck(conf gas.Config) (gas.Rule, []ast.Node) {


exported function NewCompatTlsCheck should have comment or be unexported
func NewCompatTlsCheck should be NewCompatTLSCheck

houndci-bot · 2017-12-13T06:51:20Z

rules/tls.go

@@ -136,12 +143,12 @@ func NewModernTlsCheck(conf map[string]interface{}) (gas.Rule, []ast.Node) {
 	}, []ast.Node{(*ast.CompositeLit)(nil)}
 }

-func NewIntermediateTlsCheck(conf map[string]interface{}) (gas.Rule, []ast.Node) {
+func NewIntermediateTlsCheck(conf gas.Config) (gas.Rule, []ast.Node) {


exported function NewIntermediateTlsCheck should have comment or be unexported
func NewIntermediateTlsCheck should be NewIntermediateTLSCheck

houndci-bot · 2017-12-13T06:51:20Z

rules/tls.go

@@ -121,12 +128,12 @@ func (t *InsecureConfigTLS) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, er
 	return
 }

-func NewModernTlsCheck(conf map[string]interface{}) (gas.Rule, []ast.Node) {
+func NewModernTlsCheck(conf gas.Config) (gas.Rule, []ast.Node) {


exported function NewModernTlsCheck should have comment or be unexported
func NewModernTlsCheck should be NewModernTLSCheck

houndci-bot · 2017-12-13T06:51:20Z

rules/templates.go

@@ -37,9 +36,10 @@ func (t *TemplateCheck) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err er
 	return nil, nil
 }

-func NewTemplateCheck(conf map[string]interface{}) (gas.Rule, []ast.Node) {
+func NewTemplateCheck(conf gas.Config) (gas.Rule, []ast.Node) {


exported function NewTemplateCheck should have comment or be unexported

houndci-bot · 2017-12-13T06:51:20Z

rules/templates.go

 }

-func (t *TemplateCheck) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) {
-	if node := gas.MatchCall(n, t.call); node != nil {
+func (t *TemplateCheck) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {


exported method TemplateCheck.Match should have comment or be unexported

ccojocar

Great work! I would love to see this merge.

I would like to help you out to complete this. What are the top work items which you would like to get done?

ccojocar · 2017-12-13T09:25:23Z

analyzer.go

+		gas.logger.Println("Checking package:", pkg.String())
+		for _, file := range pkg.Files {
+			gas.logger.Println("Checking file:", builtPackage.Fset.File(file.Pos()).Name())
+			gas.context.FileSet = builtPackage.Fset


It would be nice in future to build the context locally instead of providing it into a global shared state. In this way the processing can be done in parallel.

I think this is something that we can look at, although some of the context like type information is relevant between files in a package I think. Parallel processing is something I'd like to work towards.

ccojocar · 2017-12-13T09:36:35Z

analyzer_test.go

+	})
+
+	Context("when processing a package", func() {
+


I would add a test for a package consisting of multiple files.

Agreed. Done.

ccojocar · 2017-12-13T09:49:20Z

cmd/gas/main.go

+	flag.PrintDefaults()
+	fmt.Fprint(os.Stderr, "\n\nRULES:\n\n")
+
+	// sorted rule list for eas of reading


ccojocar · 2017-12-13T10:00:36Z

cmd/gas/main.go

+	}
+
+	// Load enabled rule definitions
+	ruleDefinitions := loadRules(*flagRulesInclude, *flagRulesExclude)


Are all rules included if both flags are empty? If all are included, I would print a info messages here. If all are excluded, I would print an error.

Agreed. Done.

ccojocar · 2017-12-13T10:02:48Z

cmd/gas/main.go

+	analyzer := gas.NewAnalyzer(config, logger)
+	analyzer.LoadRules(ruleDefinitions.Builders()...)
+
+	vendor := regexp.MustCompile(`[\\/]vendor([\\/]|$)`)


Maybe is worth make it this configurable. I think we can get a list of folders which could be potentially excluded. I would add vendor folder by defualt to this list.

The file inclusion / exclusion logic in the past has been a major source of bugs and confusion. The aim here was to basically make gas work as simply as possible.

However I agree it would make sense to be able to configure package directories in the recursive case. Do you think we should fix this as a separate PR or try to sneak it in with change?

In my opinion the vendor folder exclusion is enough. A more complex logic can be added after the merge.

ccojocar · 2017-12-13T10:06:44Z

cmd/gas/main.go

+		abspath, _ := filepath.Abs(pkg)
+		logger.Println("Searching directory:", abspath)
+		if err := analyzer.Process(pkg); err != nil {
+			logger.Fatal(err)


Maybe it is worth continuing the processing if only one package fails.

I think the causes of errors here are:

It is a valid go package but it doesn't compile

It is not a valid go package

I think I was trying to mimic the compiler behaviour here. For the purposes of a mass scan I
could see how this would be useful to continue processing.

However I would typically do something like to scan everything in my workspace:

# (mac osx) $ cd $GOPATH/src && go list ./... | xargs -I {} gas $GOPATH/src/{}/...

Agreed. The workaround is fair enough.

ccojocar · 2017-12-13T10:11:17Z

config.go

+
+// GetGlobal returns value associated with global configuration option
+func (c Config) GetGlobal(option string) (string, error) {
+	if globals, ok := c["global"]; ok {


I would extract this string into a constant.

ccojocar · 2017-12-13T10:11:30Z

config.go

+
+// SetGlobal associates a value with a global configuration ooption
+func (c Config) SetGlobal(option, value string) {
+	if globals, ok := c["global"]; ok {


The same here.

ccojocar · 2017-12-13T10:40:11Z

rules/rulelist.go

+
+func NewRuleFilter(action bool, ruleIDs ...string) RuleFilter {
+	rulelist := make(map[string]bool)
+	for _, rule := range ruleIDs {


I would validate if the ruleID is really defined in the list. If not, I will skip it.

ccojocar · 2017-12-13T10:54:51Z

testutils/source.go

+	Errors int
+}
+
+var (


Do you plan to add more code samples here to cover all the rules? I am asking this because the tests of each rule were removed.

Yes. This is something I need to finish doing. There are also some placeholder tests that I need to get finished also (currently skipping).

houndci-bot · 2017-12-13T12:36:28Z

analyzer.go

+	}
+
+	packageConfig := loader.Config{Build: &build.Default, ParserMode: parser.ParseComments}
+	packageFiles := make([]string, 0)


can probably use "var packageFiles []string" instead

houndci-bot · 2017-12-28T06:55:59Z

testutils/source.go

+	}
+}`, 1}}
+
+	// SampleCode502 - Blacklisted import DES


comment on exported var SampleCodeG502 should be of the form "SampleCodeG502 ..."

houndci-bot · 2017-12-28T06:55:59Z

testutils/source.go

+	println(i)
+}`, 0}}
+
+	// SampleCode501 - Blacklisted import MD5


comment on exported var SampleCodeG501 should be of the form "SampleCodeG501 ..."

The tests are running extremely slow at the moment, and these extra options add to the problem.

cedturk · 2021-07-18T03:19:02Z

helpers_test.go

@@ -2,13 +2,13 @@ package gas_test

 import (


ben hiç bir şey anlamadım işe geç kalacağım yardım

yardım edin biraz

gcmurphy and others added 19 commits April 25, 2017 16:01

Fix to reporting to use output formats

8df48f9

Restructure to focus on lib rather than cli

cacf21f

Restructure and introduce a standalone config

bf78d02

Isolate import tracking functionality

50bbc53

Move rule definitions into own file

5160048

Hack to address circular dependency in rulelist

65b18da

Simplify analyzer and command line interface

026fe4c

The analyzer now only handles packages rather than one off files. This simplifies the CLI functionality significantly.

Use glide to manage vendored dependencies

f4b705a

Add test cases

3caf7c3

Issue.Line is already a string

9c959ca

remove commited binary

5a11336

Merge pull request #136 from lanzafame/experimental

27b2fd9

output/formatter: Issue.Line was already a string

use godep instead of glide

67dc432

make it clear that these tests have not been implemented yet

d4311c9

actually skip tests until implementation exists

02901b9

update readme to provide info regarding package level scans

e3b6fd9

update travis-ci to use ginkgo tests

97cde35

houndci-bot reviewed Dec 13, 2017

View reviewed changes

This was referenced Dec 13, 2017

Add support for #excluding specific rules #142

Merged

Use regular expressions for file exclusions #139

Closed

fix hound-ci errors

cfa4327

ccojocar reviewed Dec 13, 2017

View reviewed changes

fix golint errors picked up by hound-ci

af25ac1

houndci-bot reviewed Dec 13, 2017

View reviewed changes

gcmurphy added 3 commits December 14, 2017 10:04

address review comments

25d74c6

Migrated old test cases.

e925d3c

move utils to separate executable

4c49716

houndci-bot reviewed Dec 28, 2017

View reviewed changes

gcmurphy added 2 commits January 5, 2018 21:55

Fix ginko invocation

d452dcb

The tests are running extremely slow at the moment, and these extra options add to the problem.

Fix lint issues

867d300

gcmurphy merged commit 3520a5a into master Jan 5, 2018

This was referenced Jan 5, 2018

gas does not work properly if gopkg.in/yaml.v2 is installed in vendor folder - there may be other packages affected. #144

Closed

[BUG] Unexpected error "undeclared name" #113

Closed

Embedded structs #141

Closed

cedturk reviewed Jul 18, 2021

View reviewed changes

Merge experimental / refactor #146

Merge experimental / refactor #146

Conversation

gcmurphy commented Dec 13, 2017

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

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

ccojocar left a 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

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

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