Skip to content
Permalink
Browse files Browse the repository at this point in the history
#000 - Validate URLs provided
- Disallow URLs which are obviously not URLs.
  • Loading branch information
arvindsv committed Oct 23, 2021
1 parent 46c7284 commit 6fa9fb7
Show file tree
Hide file tree
Showing 12 changed files with 134 additions and 12 deletions.
Expand Up @@ -36,7 +36,6 @@
* @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;
Expand Down Expand Up @@ -392,6 +391,10 @@ protected void validateMaterialUrl(UrlArgument url) {
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() {
Expand Down
Expand Up @@ -149,7 +149,12 @@ public void validateConcreteScmMaterial(ValidationContext validationContext) {
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
Expand Down
Expand Up @@ -21,12 +21,16 @@

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) {
Expand Down Expand Up @@ -123,4 +127,8 @@ public String withoutCredentials() {
return url;
}
}

public boolean isValidURLOrLocalPath() {
return pattern.matcher(url).matches();
}
}
Expand Up @@ -29,8 +29,7 @@
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 {
Expand Down
Expand Up @@ -268,6 +268,15 @@ void shouldBeValidWhenCredentialsAreProvidedOnlyAsAttributes() {
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;
Expand Down
Expand Up @@ -31,8 +31,11 @@
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 {
Expand Down Expand Up @@ -289,6 +292,20 @@ void shouldEnsureBranchIsNotProvidedInBothUrlAsWellAsAttributes() {

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
Expand Down
Expand Up @@ -20,7 +20,6 @@
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;
Expand All @@ -33,6 +32,8 @@

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 {
Expand Down Expand Up @@ -111,17 +112,17 @@ void setConfigAttributes_shouldUpdatePasswordWhenPasswordChangedBooleanChanged()
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();
Expand All @@ -141,6 +142,23 @@ void shouldNotSetUseTicketsIfNotInConfigAttributesMap() {
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
Expand Down
Expand Up @@ -34,6 +34,8 @@

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 {
Expand Down Expand Up @@ -158,6 +160,20 @@ void shouldEnsureDestFilePathIsValid() {

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
Expand Down
Expand Up @@ -38,6 +38,8 @@
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 {
Expand Down Expand Up @@ -166,6 +168,20 @@ void shouldEnsureUrlIsNotNull() {

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
Expand Down
Expand Up @@ -168,6 +168,10 @@ public static TfsMaterialConfig tfs(GoCipher goCipher, String url, String userNa
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);
Expand Down
Expand Up @@ -279,4 +279,31 @@ void shouldReplaceAllThePasswordsInSvnInfo() {
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);
}
}
}
Expand Up @@ -215,7 +215,7 @@ public UrlArgument workingRepositoryUrl() {

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)) {
Expand All @@ -232,7 +232,7 @@ public GitVersion version() {

@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);
}
Expand Down Expand Up @@ -299,7 +299,7 @@ public List<String> submoduleFolders() {

@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"};

Expand Down

0 comments on commit 6fa9fb7

Please sign in to comment.