Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: filepaths with git anywhere in them being erroneously excluded #828

Merged
merged 1 commit into from Jul 6, 2022

Conversation

timcosta
Copy link
Contributor

This fix addresses an issue where the default exclude aimed to exclude .git folders is erroneously excluding any file paths with git in them.

For example, given a path like src/git-client/main.go or client-git/main.go the current implementation fails to scan that directory. This causes gosec to error with No packages found when a folder being scanned only contains .go files in these erroneously matched paths.

I verified this by using regexpal.com and the strings src/git-client/main.go and client-git/main.go mentioned above. The existing exclude .git compiles to ([\\/])?.git([\\/])? which causes the problem as the . isnt escaped. The fixed \\.git/ compiles to ([\\/])?\.git\/([\\/])? which escapes the leading . and adds the protection of the required trailing / indicating that it's a directory. This prevents file paths like src/.git-client/main.go from being excluded while leaving the behavior originally desired in #485 intact.

@timcosta
Copy link
Contributor Author

it may be worth adding a (?:^|\/) to the beginning of the regex as well to enforce that only .git directories should be caught by this. The current regex would exclude a folder like src/something.git/main.go because it still matches the \.git. Curious to hear any thoughts on that as it's an even more restrictive regex but only handles a very specific case whereas the fix as currently implemented handles 9/10 use cases.

@yunwei37
Copy link
Contributor

yunwei37 commented Jun 30, 2022

Hi!

It seems currently gosec is using a helper function to walk through all folders to get all packages path at a given root directory, see

func PackagePaths(root string, excludes []*regexp.Regexp) ([]string, error) {

and

func (gosec *Analyzer) load(pkgPath string, conf *packages.Config) ([]*packages.Package, error) {

Why can't we using packages.Load(cfg, "./...") to load and return all the Go packages in a given project root? For example:

       config := &packages.Config{
		Mode:        LoadMode,
		BuildFlags: buildTags,
		Tests:          gosec.tests,
                Dir:             root,
	}
        pkgs, err := packages.Load(config, "./...")
	if err != nil {
		return nil, err
	}

It seems that we can also use packages.Load to solve the problem described in this PR. For more details, see https://pkg.go.dev/golang.org/x/tools/go/packages

@codecov-commenter
Copy link

Codecov Report

Merging #828 (98a491a) into master (b0f3e78) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master     #828   +/-   ##
=======================================
  Coverage   74.35%   74.35%           
=======================================
  Files          50       50           
  Lines        3124     3124           
=======================================
  Hits         2323     2323           
  Misses        735      735           
  Partials       66       66           
Impacted Files Coverage Δ
cmd/gosec/main.go 0.00% <0.00%> (ø)

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 b0f3e78...98a491a. Read the comment docs.

@ccojocar ccojocar merged commit 9a25f4e into securego:master Jul 6, 2022
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.

None yet

4 participants