gemnasium safe directory git command overwrites existing entries

Summary

Now that Allow list build dir and gemnasium-dir in Git c... (gitlab-org/security-products/analyzers/gemnasium!1084 - merged) • Oscar Tovar • 18.5 has been merged, the analyzer executes /usr/bin/git -C /gemnasium-db config --global safe.directory /gemnasium-db before initiating a scan, however, this command overwrites any existing entries for the git safe.directory configuration.

For example:

  1. Add a safe.directory entry

    $ docker run -it --rm registry.gitlab.com/security-products/gemnasium:6.1.11 ash
    
    / # git config --global --add safe.directory /var
  2. List safe.directory entries:

    / # git config --global --list | grep -i safe
    safe.directory=/var
  3. Run the safe.directory command from repo.go:

    / # /usr/bin/git -C /gemnasium-db config --global safe.directory /gemnasium-db
  4. List safe.directory entries:

    / # git config --global --list | grep -i safe
    safe.directory=/gemnasium-db

    Notice that the existing safe.directory=/var value has been overwritten.

Impact

The impact of this bug is minimal and only seems to prevent us from implementing new testing behaviour such as Can't run gemnasium integration tests on Mac OS X (#571810) • Adam Cohen • 18.7.

Steps to reproduce

We can also demonstrate this issue by causing git to throw an error when trying to overwrite multiple safe.directory values:

  1. Create multiple safe.directory entries:

    $ docker run -it --rm registry.gitlab.com/security-products/gemnasium:6.1.11 ash
    
    / # git config --global --add safe.directory /var
    / # git config --global --add safe.directory /var
  2. Run the safe.directory command from repo.go:

    / # /usr/bin/git -C /gemnasium-db config --global safe.directory /gemnasium-db
    
    warning: safe.directory has multiple values
    error: cannot overwrite multiple values with a single value
           Use a regexp, --add or --replace-all to change safe.directory.

    Notice an error is thrown by git

What is the current bug behavior?

gemnasium analyzer overwrites existing safe.directory values

What is the expected correct behavior?

gemnasium analyzer does not overwrite existing safe.directory values

Implementation plan

  1. Use --add in the the safe.directory command:

    diff --git a/advisory/repo.go b/advisory/repo.go
    index 97a62d1e..65d50479 100644
    --- a/advisory/repo.go
    +++ b/advisory/repo.go
    @@ -109,7 +109,7 @@ func (r Repo) Update() error {
            // the owner of the gemnasium repository copy doesn't match the user
            // that's operating on it.
            // See issue https://gitlab.com/gitlab-org/gitlab/-/issues/551333
    -       argsList = append(argsList, []string{"config", "--global", "safe.directory", r.Path})
    +       argsList = append(argsList, []string{"config", "--global", "--add", "safe.directory", r.Path})
    
            // add "git remote set-url" command if needed
            if remoteURL := r.RemoteURL; remoteURL != "" {`
  2. Add an integration test to prove the above change works

/cc @hacks4oats @nilieskou

Edited by Adam Cohen