CVE-2021-43286: Improper Neutralization of Special Elements used in a Command ('Command Injection') (CWE-77)
value: An issue was discovered in ThoughtWorks GoCD before 21.3.0. An attacker with privileges to create a new pipeline on a GoCD server can abuse a command-line injection in the Git URL "Test Connection" feature to execute arbitrary code.
CWE description: The product constructs all or part of a command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended command when it is sent to a downstream component.
CVSS v3 base score: 8.8
Published: 2022-04-14T13:15Z
Repository: https://github.com/gocd/gocd
Language: Java
Files changed: config/config-api/src/main/java/com/thoughtworks/go/config/materials/ScmMaterialConfig.java, config/config-api/src/main/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfig.java, config/config-api/src/main/java/com/thoughtworks/go/util/command/UrlArgument.java, config/config-api/src/test/java/com/thoughtworks/go/config/materials/ScmMaterialConfigTest.java, config/config-api/src/test/java/com/thoughtworks/go/config/materials/git/GitMaterialConfigTest.java, config/config-api/src/test/java/com/thoughtworks/go/config/materials/mercurial/HgMaterialConfigTest.java, config/config-api/src/test/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfigTest.java, config/config-api/src/test/java/com/thoughtworks/go/config/materials/svn/SvnMaterialConfigTest.java, config/config-api/src/test/java/com/thoughtworks/go/config/materials/tfs/TfsMaterialConfigTest.java, config/config-api/src/test/java/com/thoughtworks/go/helper/MaterialConfigsMother.java, config/config-api/src/test/java/com/thoughtworks/go/util/command/UrlArgumentTest.java, domain/src/main/java/com/thoughtworks/go/domain/materials/git/GitCommand.java
Commit date: 2021-10-23 12:18:28 +0100
Commit message: #000 - Validate URLs provided

- Disallow URLs which are obviously not URLs.

Diff: diff --git a/config/config-api/src/main/java/com/thoughtworks/go/config/materials/ScmMaterialConfig.java b/config/config-api/src/main/java/com/thoughtworks/go/config/materials/ScmMaterialConfig.java
index 42495e067e..9b623a8e03 100644
--- a/config/config-api/src/main/java/com/thoughtworks/go/config/materials/ScmMaterialConfig.java
+++ b/config/config-api/src/main/java/com/thoughtworks/go/config/materials/ScmMaterialConfig.java
@@ -34,11 +34,10 @@ import static org.apache.commons.lang3.StringUtils.*;
 
 /**
  * @understands a source control repository and its configuration
  */
 public abstract class ScmMaterialConfig extends AbstractMaterialConfig implements ParamsAttributeAware {
-
     public static final String URL = "url";
     public static final String USERNAME = "username";
     protected GoCipher goCipher;
 
     @ConfigSubtag
@@ -390,10 +389,14 @@ public abstract class ScmMaterialConfig extends AbstractMaterialConfig implement
     protected void validateMaterialUrl(UrlArgument url) {
         if (url == null || isBlank(url.forDisplay())) {
             errors().add(URL, "URL cannot be blank");
             return;
         }
+
+        if (System.getProperty("gocd.verify.url.correctness", "y").equalsIgnoreCase("y") && !url.isValidURLOrLocalPath()) {
+            errors().add(URL, "URL does not seem to be valid.");
+        }
     }
 
     protected void validateCredentials() {
         try {
             if (isBlank(getUrl()) || isAllBlank(userName, getPassword())) {
diff --git a/config/config-api/src/main/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfig.java b/config/config-api/src/main/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfig.java
index a08f5c7b89..c90fe04cff 100644
--- a/config/config-api/src/main/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfig.java
+++ b/config/config-api/src/main/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfig.java
@@ -147,11 +147,16 @@ public class P4MaterialConfig extends ScmMaterialConfig implements ParamsAttribu
             errors.add(VIEW, "P4 view cannot be empty.");
         }
         if (StringUtils.isBlank(getServerAndPort())) {
             errors.add(SERVER_AND_PORT, "P4 port cannot be empty.");
         }
-        validateEncryptedPassword();
+
+        if (!new UrlArgument(getUrl()).isValidURLOrLocalPath()) {
+            errors.add(SERVER_AND_PORT, "P4 port seems to be invalid.");
+        }
+
+        view.validate(validationContext);
     }
 
     @Override
     protected String getLocation() {
         return getServerAndPort();
diff --git a/config/config-api/src/main/java/com/thoughtworks/go/util/command/UrlArgument.java b/config/config-api/src/main/java/com/thoughtworks/go/util/command/UrlArgument.java
index e7ac224418..bc077f4cef 100644
--- a/config/config-api/src/main/java/com/thoughtworks/go/util/command/UrlArgument.java
+++ b/config/config-api/src/main/java/com/thoughtworks/go/util/command/UrlArgument.java
@@ -19,16 +19,20 @@ import com.thoughtworks.go.config.ConfigAttributeValue;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.http.client.utils.URIBuilder;
 
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.regex.Pattern;
 
 import static com.thoughtworks.go.util.ExceptionUtils.bombIfNull;
 import static org.apache.commons.lang3.StringUtils.isBlank;
 
 @ConfigAttributeValue(fieldName = "url")
 public class UrlArgument extends CommandArgument {
+    private static final String URL_DUMB_VALIDATION_REGEX = "^[a-zA-Z0-9/].*";
+    private static final Pattern pattern = Pattern.compile(URL_DUMB_VALIDATION_REGEX);
+
     protected String url;
 
     public UrlArgument(String url) {
         bombIfNull(url, "Url cannot be null.");
         this.url = url;
@@ -121,6 +125,10 @@ public class UrlArgument extends CommandArgument {
             return new URIBuilder(this.sanitizeUrl()).setUserInfo(null).build().toString();
         } catch (URISyntaxException e) {
             return url;
         }
     }
+
+    public boolean isValidURLOrLocalPath() {
+        return pattern.matcher(url).matches();
+    }
 }
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/ScmMaterialConfigTest.java b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/ScmMaterialConfigTest.java
index c607a2961e..84481a7f36 100644
--- a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/ScmMaterialConfigTest.java
+++ b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/ScmMaterialConfigTest.java
@@ -27,12 +27,11 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Collections;
 import java.util.HashMap;
 
-import static com.thoughtworks.go.config.materials.ScmMaterialConfig.AUTO_UPDATE;
-import static com.thoughtworks.go.config.materials.ScmMaterialConfig.FOLDER;
+import static com.thoughtworks.go.config.materials.ScmMaterialConfig.*;
 import static org.assertj.core.api.Assertions.assertThat;
 
 class ScmMaterialConfigTest {
     private DummyMaterialConfig material;
 
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/git/GitMaterialConfigTest.java b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/git/GitMaterialConfigTest.java
index b4e07765a5..83e8b9c840 100644
--- a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/git/GitMaterialConfigTest.java
+++ b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/git/GitMaterialConfigTest.java
@@ -266,10 +266,19 @@ class GitMaterialConfigTest {
             gitMaterialConfig.setPassword("badger");
 
             assertFalse(validating(gitMaterialConfig).errors().containsKey(GitMaterialConfig.URL));
         }
 
+        @Test
+        void rejectsObviouslyWrongURL() {
+            assertTrue(validating(git("-url-not-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));
+            assertTrue(validating(git("_url-not-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));
+            assertTrue(validating(git("@url-not-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));
+
+            assertFalse(validating(git("url-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));
+        }
+
         private GitMaterialConfig validating(GitMaterialConfig git) {
             git.validate(new ConfigSaveValidationContext(null));
             return git;
         }
     }
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/mercurial/HgMaterialConfigTest.java b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/mercurial/HgMaterialConfigTest.java
index 477e37ce83..be0b88726e 100644
--- a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/mercurial/HgMaterialConfigTest.java
+++ b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/mercurial/HgMaterialConfigTest.java
@@ -29,12 +29,15 @@ import java.util.HashMap;
 import java.util.Map;
 
 import static com.thoughtworks.go.config.materials.AbstractMaterialConfig.MATERIAL_NAME;
 import static com.thoughtworks.go.config.materials.ScmMaterialConfig.FOLDER;
 import static com.thoughtworks.go.config.materials.ScmMaterialConfig.URL;
+import static com.thoughtworks.go.helper.MaterialConfigsMother.git;
 import static com.thoughtworks.go.helper.MaterialConfigsMother.hg;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.*;
 
 class HgMaterialConfigTest {
     private HgMaterialConfig hgMaterialConfig;
 
@@ -287,10 +290,24 @@ class HgMaterialConfigTest {
 
             hgMaterialConfig.validate(new ConfigSaveValidationContext(null));
 
             assertThat(hgMaterialConfig.errors().on(HgMaterialConfig.URL)).isEqualTo("Ambiguous branch, must be provided either in URL or as an attribute.");
         }
+
+        @Test
+        void rejectsObviouslyWrongURL() {
+            assertTrue(validating(hg("-url-not-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));
+            assertTrue(validating(hg("_url-not-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));
+            assertTrue(validating(hg("@url-not-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));
+
+            assertFalse(validating(hg("url-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));
+        }
+
+        private HgMaterialConfig validating(HgMaterialConfig hg) {
+            hg.validate(new ConfigSaveValidationContext(null));
+            return hg;
+        }
     }
 
     @Nested
     class ValidateTree {
         @Test
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfigTest.java b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfigTest.java
index 0157203a90..7847f6fd59 100644
--- a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfigTest.java
+++ b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfigTest.java
@@ -18,11 +18,10 @@ package com.thoughtworks.go.config.materials.perforce;
 import com.thoughtworks.go.config.*;
 import com.thoughtworks.go.config.materials.AbstractMaterialConfig;
 import com.thoughtworks.go.config.materials.Filter;
 import com.thoughtworks.go.config.materials.IgnoredFiles;
 import com.thoughtworks.go.config.materials.ScmMaterialConfig;
-import com.thoughtworks.go.config.materials.svn.SvnMaterialConfig;
 import com.thoughtworks.go.domain.materials.MaterialConfig;
 import com.thoughtworks.go.security.GoCipher;
 import com.thoughtworks.go.util.ReflectionUtil;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Nested;
@@ -31,10 +30,12 @@ import org.junit.jupiter.api.Test;
 import java.util.HashMap;
 import java.util.Map;
 
 import static com.thoughtworks.go.helper.MaterialConfigsMother.p4;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.*;
 
 class P4MaterialConfigTest {
     private P4MaterialConfig p4MaterialConfig;
 
@@ -109,21 +110,21 @@ class P4MaterialConfigTest {
         assertThat(ReflectionUtil.getField(materialConfig, "password")).isNull();
         assertThat(materialConfig.getPassword()).isEqualTo("secret");
         assertThat(materialConfig.getEncryptedPassword()).isEqualTo(new GoCipher().encrypt("secret"));
 
         //Dont change
-        map.put(SvnMaterialConfig.PASSWORD, "Hehehe");
-        map.put(SvnMaterialConfig.PASSWORD_CHANGED, "0");
+        map.put(P4MaterialConfig.PASSWORD, "Hehehe");
+        map.put(P4MaterialConfig.PASSWORD_CHANGED, "0");
         materialConfig.setConfigAttributes(map);
 
         assertThat(ReflectionUtil.getField(materialConfig, "password")).isNull();
         assertThat(materialConfig.getPassword()).isEqualTo("secret");
         assertThat(materialConfig.getEncryptedPassword()).isEqualTo(new GoCipher().encrypt("secret"));
 
         //Dont change
-        map.put(SvnMaterialConfig.PASSWORD, "");
-        map.put(SvnMaterialConfig.PASSWORD_CHANGED, "1");
+        map.put(P4MaterialConfig.PASSWORD, "");
+        map.put(P4MaterialConfig.PASSWORD_CHANGED, "1");
         materialConfig.setConfigAttributes(map);
 
         assertThat(materialConfig.getPassword()).isNull();
         assertThat(materialConfig.getEncryptedPassword()).isNull();
     }
@@ -139,10 +140,27 @@ class P4MaterialConfigTest {
 
         p4MaterialConfig.setConfigAttributes(new HashMap());
         assertThat(p4MaterialConfig.getUseTickets()).isFalse();
     }
 
+    @Nested
+    class Validate {
+        @Test
+        void rejectsObviouslyWrongURL() {
+            assertTrue(validating(p4("-url-not-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));
+            assertTrue(validating(p4("_url-not-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));
+            assertTrue(validating(p4("@url-not-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));
+
+            assertFalse(validating(p4("url-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));
+        }
+
+        private P4MaterialConfig validating(P4MaterialConfig p4) {
+            p4.validate(new ConfigSaveValidationContext(null));
+            return p4;
+        }
+
+    }
     @Nested
     class ValidateTree {
         @Test
         void shouldCallValidate() {
             final MaterialConfig materialConfig = spy(new P4MaterialConfig());
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/svn/SvnMaterialConfigTest.java b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/svn/SvnMaterialConfigTest.java
index 6dc5f02bf8..182d98814e 100644
--- a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/svn/SvnMaterialConfigTest.java
+++ b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/svn/SvnMaterialConfigTest.java
@@ -32,10 +32,12 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
 import static com.thoughtworks.go.helper.MaterialConfigsMother.svn;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.*;
 
 class SvnMaterialConfigTest {
     private SvnMaterialConfig svnMaterialConfig;
 
@@ -156,10 +158,24 @@ class SvnMaterialConfigTest {
             svnMaterialConfig.setConfigAttributes(Collections.singletonMap(ScmMaterialConfig.FOLDER, "../a"));
             svnMaterialConfig.validate(new ConfigSaveValidationContext(null));
 
             assertThat(svnMaterialConfig.errors().on(SvnMaterialConfig.FOLDER)).isEqualTo("Dest folder '../a' is not valid. It must be a sub-directory of the working folder.");
         }
+
+        @Test
+        void rejectsObviouslyWrongURL() {
+            assertTrue(validating(svn("-url-not-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));
+            assertTrue(validating(svn("_url-not-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));
+            assertTrue(validating(svn("@url-not-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));
+
+            assertFalse(validating(svn("url-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));
+        }
+
+        private SvnMaterialConfig validating(SvnMaterialConfig svn) {
+            svn.validate(new ConfigSaveValidationContext(null));
+            return svn;
+        }
     }
 
     @Nested
     class ValidateTree {
         @BeforeEach
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/tfs/TfsMaterialConfigTest.java b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/tfs/TfsMaterialConfigTest.java
index 809aef7367..eab7cb5da5 100644
--- a/config/config-api/src/test/java/com/thoughtworks/go/config/materials/tfs/TfsMaterialConfigTest.java
+++ b/config/config-api/src/test/java/com/thoughtworks/go/config/materials/tfs/TfsMaterialConfigTest.java
@@ -36,10 +36,12 @@ import static com.thoughtworks.go.config.materials.AbstractMaterialConfig.MATERI
 import static com.thoughtworks.go.config.materials.ScmMaterialConfig.FOLDER;
 import static com.thoughtworks.go.config.materials.ScmMaterialConfig.URL;
 import static com.thoughtworks.go.helper.MaterialConfigsMother.tfs;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.fail;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.*;
 
 class TfsMaterialConfigTest {
     private TfsMaterialConfig tfsMaterialConfig;
 
@@ -164,10 +166,24 @@ class TfsMaterialConfigTest {
 
             tfsMaterialConfig.validate(new ConfigSaveValidationContext(null));
 
             assertThat(tfsMaterialConfig.errors().on(URL)).isEqualTo("URL cannot be blank");
         }
+
+        @Test
+        void rejectsObviouslyWrongURL() {
+            assertTrue(validating(tfs("-url-not-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));
+            assertTrue(validating(tfs("_url-not-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));
+            assertTrue(validating(tfs("@url-not-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));
+
+            assertFalse(validating(tfs("url-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));
+        }
+
+        private TfsMaterialConfig validating(TfsMaterialConfig tfs) {
+            tfs.validate(new ConfigSaveValidationContext(null));
+            return tfs;
+        }
     }
 
     @Test
     void shouldEncryptTfsPasswordAndMarkPasswordAsNull() throws Exception {
         TfsMaterialConfig materialConfig = tfs(null, new UrlArgument("http://10.4.4.101:8080/tfs/Sample"), "loser", "CORPORATE", "password", "walk_this_path");
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/helper/MaterialConfigsMother.java b/config/config-api/src/test/java/com/thoughtworks/go/helper/MaterialConfigsMother.java
index 2e5044a094..c322740a50 100644
--- a/config/config-api/src/test/java/com/thoughtworks/go/helper/MaterialConfigsMother.java
+++ b/config/config-api/src/test/java/com/thoughtworks/go/helper/MaterialConfigsMother.java
@@ -166,10 +166,14 @@ public class MaterialConfigsMother {
 
     public static TfsMaterialConfig tfs(GoCipher goCipher, String url, String userName, String domain, String projectPath) {
         return tfs(goCipher, new UrlArgument(url), userName, domain, null, projectPath);
     }
 
+    public static TfsMaterialConfig tfs(String url) {
+        return tfs(new GoCipher(), new UrlArgument(url), null, null, "password");
+    }
+
     public static TfsMaterialConfig tfs(UrlArgument urlArgument, String password, String encryptedPassword, GoCipher goCipher) {
         TfsMaterialConfig tfsMaterialConfig = tfs(goCipher, urlArgument, null, null, password);
         tfsMaterialConfig.setEncryptedPassword(encryptedPassword);
         return tfsMaterialConfig;
     }
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/util/command/UrlArgumentTest.java b/config/config-api/src/test/java/com/thoughtworks/go/util/command/UrlArgumentTest.java
index 8a74be2b79..1fd0ef2291 100644
--- a/config/config-api/src/test/java/com/thoughtworks/go/util/command/UrlArgumentTest.java
+++ b/config/config-api/src/test/java/com/thoughtworks/go/util/command/UrlArgumentTest.java
@@ -277,6 +277,33 @@ class UrlArgumentTest {
             assertThat(result).contains("<url>http://cce:******@10.18.3.171:8080/svn/connect4/trunk</url>");
             assertThat(result).contains("<root>http://cce:******@10.18.3.171:8080/svn/connect4</root>");
             assertThat(result).doesNotContain("cce:password");
         }
     }
+
+    @Nested
+    @TestInstance(PER_CLASS)
+    class isValidURL {
+        Stream<Arguments> urls() {
+            return Stream.of(
+                    Arguments.of("http://my-site.com/abc", true),
+                    Arguments.of("svn+ssh://my-site.com/def", true),
+                    Arguments.of("file://my-site.com/def", true),
+                    Arguments.of("/path/in/file/system", true),
+                    Arguments.of("git@github.com:org/repo.git", true),
+                    Arguments.of("user@my-site.com:org/repo.git", true),
+                    Arguments.of("1a2b3c://abc", true),
+                    Arguments.of("-xyz", false),
+                    Arguments.of("_xyz", false),
+                    Arguments.of("@xyz", false)
+            );
+        }
+
+        @ParameterizedTest
+        @MethodSource("urls")
+        void shouldValidateURLs(String input, boolean expectedValidity) {
+            final UrlArgument url = new UrlArgument(input);
+
+            assertThat(url.isValidURLOrLocalPath()).as("Verify validity of '%s' as a URL", url.originalArgument()).isEqualTo(expectedValidity);
+        }
+    }
 }
\ No newline at end of file
diff --git a/domain/src/main/java/com/thoughtworks/go/domain/materials/git/GitCommand.java b/domain/src/main/java/com/thoughtworks/go/domain/materials/git/GitCommand.java
index 8c36ecb47b..97a5737053 100644
--- a/domain/src/main/java/com/thoughtworks/go/domain/materials/git/GitCommand.java
+++ b/domain/src/main/java/com/thoughtworks/go/domain/materials/git/GitCommand.java
@@ -213,11 +213,11 @@ public class GitCommand extends SCMCommand {
         return new UrlArgument(runOrBomb(gitConfig).outputForDisplay().get(0));
     }
 
     public void checkConnection(UrlArgument repoUrl) {
         final String ref = fullUpstreamRef();
-        final CommandLine commandLine = git().withArgs("ls-remote").withArg(repoUrl).withArg(ref);
+        final CommandLine commandLine = git().withArgs("ls-remote", "--").withArg(repoUrl).withArg(ref);
         final ConsoleResult result = commandLine.runOrBomb(new NamedProcessTag(repoUrl.forDisplay()));
 
         if (!hasExactlyOneMatchingBranch(result)) {
             throw new CommandLineException(format("The ref %s could not be found.", ref));
         }
@@ -230,11 +230,11 @@ public class GitCommand extends SCMCommand {
         return GitVersion.parse(gitVersionString);
     }
 
     @TestOnly
     public void add(File fileToAdd) {
-        String[] args = new String[]{"add", fileToAdd.getName()};
+        String[] args = new String[]{"add", "--", fileToAdd.getName()};
         CommandLine gitAdd = gitWd().withArgs(args);
         runOrBomb(gitAdd);
     }
 
     @TestOnly
@@ -297,11 +297,11 @@ public class GitCommand extends SCMCommand {
         return submoduleFolders(result.output());
     }
 
     @TestOnly
     public void submoduleAdd(String repoUrl, String submoduleNameToPutInGitSubmodules, String folder) {
-        String[] addSubmoduleWithSameNameArgs = new String[]{"submodule", "add", repoUrl, folder};
+        String[] addSubmoduleWithSameNameArgs = new String[]{"submodule", "add", "--", repoUrl, folder};
         String[] changeSubmoduleNameInGitModules = new String[]{"config", "--file", ".gitmodules", "--rename-section", "submodule." + folder, "submodule." + submoduleNameToPutInGitSubmodules};
         String[] addGitModules = new String[]{"add", ".gitmodules"};
 
         runOrBomb(gitWd().withArgs(addSubmoduleWithSameNameArgs));
         runOrBomb(gitWd().withArgs(changeSubmoduleNameInGitModules));

Vulnerable code: 
        validateEncryptedPassword();
import static com.thoughtworks.go.config.materials.ScmMaterialConfig.AUTO_UPDATE;
import static com.thoughtworks.go.config.materials.ScmMaterialConfig.FOLDER;
import com.thoughtworks.go.config.materials.svn.SvnMaterialConfig;
        map.put(SvnMaterialConfig.PASSWORD, "Hehehe");
        map.put(SvnMaterialConfig.PASSWORD_CHANGED, "0");
        map.put(SvnMaterialConfig.PASSWORD, "");
        map.put(SvnMaterialConfig.PASSWORD_CHANGED, "1");
        final CommandLine commandLine = git().withArgs("ls-remote").withArg(repoUrl).withArg(ref);
        String[] args = new String[]{"add", fileToAdd.getName()};
        String[] addSubmoduleWithSameNameArgs = new String[]{"submodule", "add", repoUrl, folder};
Fixed code: 
        if (System.getProperty("gocd.verify.url.correctness", "y").equalsIgnoreCase("y") && !url.isValidURLOrLocalPath()) {
            errors().add(URL, "URL does not seem to be valid.");
        }

        if (!new UrlArgument(getUrl()).isValidURLOrLocalPath()) {
            errors.add(SERVER_AND_PORT, "P4 port seems to be invalid.");
        }

        view.validate(validationContext);
import java.util.regex.Pattern;
    private static final String URL_DUMB_VALIDATION_REGEX = "^[a-zA-Z0-9/].*";
    private static final Pattern pattern = Pattern.compile(URL_DUMB_VALIDATION_REGEX);


    public boolean isValidURLOrLocalPath() {
        return pattern.matcher(url).matches();
    }
import static com.thoughtworks.go.config.materials.ScmMaterialConfig.*;
        @Test
        void rejectsObviouslyWrongURL() {
            assertTrue(validating(git("-url-not-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));
            assertTrue(validating(git("_url-not-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));
            assertTrue(validating(git("@url-not-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));

            assertFalse(validating(git("url-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));
        }

import static com.thoughtworks.go.helper.MaterialConfigsMother.git;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

        @Test
        void rejectsObviouslyWrongURL() {
            assertTrue(validating(hg("-url-not-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));
            assertTrue(validating(hg("_url-not-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));
            assertTrue(validating(hg("@url-not-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));

            assertFalse(validating(hg("url-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));
        }

        private HgMaterialConfig validating(HgMaterialConfig hg) {
            hg.validate(new ConfigSaveValidationContext(null));
            return hg;
        }
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
        map.put(P4MaterialConfig.PASSWORD, "Hehehe");
        map.put(P4MaterialConfig.PASSWORD_CHANGED, "0");
        map.put(P4MaterialConfig.PASSWORD, "");
        map.put(P4MaterialConfig.PASSWORD_CHANGED, "1");
    @Nested
    class Validate {
        @Test
        void rejectsObviouslyWrongURL() {
            assertTrue(validating(p4("-url-not-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));
            assertTrue(validating(p4("_url-not-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));
            assertTrue(validating(p4("@url-not-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));

            assertFalse(validating(p4("url-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));
        }

        private P4MaterialConfig validating(P4MaterialConfig p4) {
            p4.validate(new ConfigSaveValidationContext(null));
            return p4;
        }

    }
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

        @Test
        void rejectsObviouslyWrongURL() {
            assertTrue(validating(svn("-url-not-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));
            assertTrue(validating(svn("_url-not-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));
            assertTrue(validating(svn("@url-not-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));

            assertFalse(validating(svn("url-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));
        }

        private SvnMaterialConfig validating(SvnMaterialConfig svn) {
            svn.validate(new ConfigSaveValidationContext(null));
            return svn;
        }
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

        @Test
        void rejectsObviouslyWrongURL() {
            assertTrue(validating(tfs("-url-not-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));
            assertTrue(validating(tfs("_url-not-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));
            assertTrue(validating(tfs("@url-not-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));

            assertFalse(validating(tfs("url-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));
        }

        private TfsMaterialConfig validating(TfsMaterialConfig tfs) {
            tfs.validate(new ConfigSaveValidationContext(null));
            return tfs;
        }
    public static TfsMaterialConfig tfs(String url) {
        return tfs(new GoCipher(), new UrlArgument(url), null, null, "password");
    }


    @Nested
    @TestInstance(PER_CLASS)
    class isValidURL {
        Stream<Arguments> urls() {
            return Stream.of(
                    Arguments.of("http://my-site.com/abc", true),
                    Arguments.of("svn+ssh://my-site.com/def", true),
                    Arguments.of("file://my-site.com/def", true),
                    Arguments.of("/path/in/file/system", true),
                    Arguments.of("git@github.com:org/repo.git", true),
                    Arguments.of("user@my-site.com:org/repo.git", true),
                    Arguments.of("1a2b3c://abc", true),
                    Arguments.of("-xyz", false),
                    Arguments.of("_xyz", false),
                    Arguments.of("@xyz", false)
            );
        }

        @ParameterizedTest
        @MethodSource("urls")
        void shouldValidateURLs(String input, boolean expectedValidity) {
            final UrlArgument url = new UrlArgument(input);

            assertThat(url.isValidURLOrLocalPath()).as("Verify validity of '%s' as a URL", url.originalArgument()).isEqualTo(expectedValidity);
        }
    }
        final CommandLine commandLine = git().withArgs("ls-remote", "--").withArg(repoUrl).withArg(ref);
        String[] args = new String[]{"add", "--", fileToAdd.getName()};
        String[] addSubmoduleWithSameNameArgs = new String[]{"submodule", "add", "--", repoUrl, folder};