Skip to content

Commit

Permalink
Test with remote URL checking enabled and disabled
Browse files Browse the repository at this point in the history 
Randomize remote check test, test a subset for speed.

Don't assert expected message when testing with remote URL checks
disabled.  The assertion messages come from command line git and vary
depending on the version of git installed on the computer.  Not reliable
across multiple git versions.

Ignore marker file existence in some tests

If a test has remote URL checking disabled, then it is expected that
some cases will allow the marker file to be created.  Only check for
the marker file when running with remote URL checking enabled.
  • Loading branch information
@MarkEWaite
MarkEWaite committed Sep 7, 2019
1 parent 899123f commit 701c12c
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 21 deletions.
3 changes: 2 additions & 1 deletion src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ public List<IndexEntry> getSubmodules( String treeIsh ) throws GitException, Int
return submodules;
}

// Package protected for testing
/**
* Constant which disables safety check of remote URL.
*
Expand All @@ -380,7 +381,7 @@ public List<IndexEntry> getSubmodules( String treeIsh ) throws GitException, Int
* Use '-Dorg.jenkinsci.plugins.gitclient.CliGitAPIImpl.checkRemoteURL=false'
* to prevent check of remote URL.
*/
private static final boolean CHECK_REMOTE_URL = Boolean.valueOf(System.getProperty(CliGitAPIImpl.class.getName() + ".checkRemoteURL", "true"));
static boolean CHECK_REMOTE_URL = Boolean.valueOf(System.getProperty(CliGitAPIImpl.class.getName() + ".checkRemoteURL", "true"));

/**
* SECURITY-1534 found that arguments
Expand Down
86 changes: 66 additions & 20 deletions src/test/java/org/jenkinsci/plugins/gitclient/GitClientSecurityTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.eclipse.jgit.transport.URIish;

import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Rule;
Expand All @@ -42,6 +43,8 @@ public class GitClientSecurityTest {

/* Test parameter - remote URL that is expected to fail with known exception */
private final String badRemoteUrl;
/* Test parameter - should remote URL check be enabled */
private final boolean enableRemoteCheckUrl;

/* Git client plugin repository directory. */
private static final File SRC_REPO_DIR = new File(".git");
Expand All @@ -65,8 +68,9 @@ public class GitClientSecurityTest {
public TemporaryFolder tempFolder = new TemporaryFolder();
private File repoRoot = null;

public GitClientSecurityTest(final String badRemoteUrl) throws IOException, InterruptedException {
public GitClientSecurityTest(final String badRemoteUrl, final boolean enableRemoteCheckUrl) throws IOException, InterruptedException {
this.badRemoteUrl = badRemoteUrl;
this.enableRemoteCheckUrl = enableRemoteCheckUrl;
this.srcGitClient = Git.with(TaskListener.NULL, new EnvVars()).in(SRC_REPO_DIR).using("git").getClient();

CliGitAPIImpl cliGitClient;
Expand All @@ -78,29 +82,38 @@ public GitClientSecurityTest(final String badRemoteUrl) throws IOException, Inte
CLI_GIT_SUPPORTS_SYMREF = cliGitClient.isAtLeastVersion(2, 8, 0, 0);
}

@Parameterized.Parameters(name = "{0}")
@Parameterized.Parameters(name = "{1},{0}")
public static Collection testConfiguration() {
final Random configRandom = new Random();
markerFileName = String.format(markerFileName, configRandom.nextInt()); // Unique enough file name
List<Object[]> arguments = new ArrayList<>();
for (String prefix : BAD_REMOTE_URL_PREFIXES) {
String formattedPrefix = String.format(prefix, markerFileName);
/* insert markerFileName into test data */
int remoteIndex = configRandom.nextInt(VALID_REMOTES.length);
/* Choose a random remote URL */
String formattedPrefix = String.format(prefix, markerFileName);

/* Random remote URL with prefix */
String firstChar = configRandom.nextBoolean() ? " " : "";
String middleChar = configRandom.nextBoolean() ? " " : "";
String lastChar = configRandom.nextBoolean() ? " " : "";
int remoteIndex = configRandom.nextInt(VALID_REMOTES.length);
String remoteUrl = firstChar + formattedPrefix + middleChar + VALID_REMOTES[remoteIndex] + lastChar;
remoteIndex = configRandom.nextInt(VALID_REMOTES.length);
Object[] remoteUrlItem = {remoteUrl};
Object[] remoteUrlItem = {remoteUrl, configRandom.nextBoolean()};
arguments.add(remoteUrlItem);
Object[] remoteUrlItemOneSpace = {formattedPrefix + " " + VALID_REMOTES[remoteIndex]};

/* Random remote URL with prefix separated by a space */
remoteIndex = configRandom.nextInt(VALID_REMOTES.length);
remoteUrl = formattedPrefix + " " + VALID_REMOTES[remoteIndex];
Object[] remoteUrlItemOneSpace = {remoteUrl, configRandom.nextBoolean()};
arguments.add(remoteUrlItemOneSpace);
Object[] noSpaceItem = {formattedPrefix + VALID_REMOTES[remoteIndex]};

/* Random remote URL with prefix and no separator */
remoteIndex = configRandom.nextInt(VALID_REMOTES.length);
remoteUrl = formattedPrefix + VALID_REMOTES[remoteIndex];
Object[] noSpaceItem = {remoteUrl, configRandom.nextBoolean()};
arguments.add(noSpaceItem);
Object[] prefixItem = {formattedPrefix};

/* Remote URL with only the prefix */
Object[] prefixItem = {formattedPrefix, configRandom.nextBoolean()};
arguments.add(prefixItem);
}
Collections.shuffle(arguments);
Expand All @@ -114,6 +127,16 @@ public static void setCliGitDefaults() throws Exception {
gitCmd.setDefaults();
}

@AfterClass
public static void resetRemoteCheckUrl() {
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.CHECK_REMOTE_URL = true;
}

@Before
public void setRemoteCheckUrl() {
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.CHECK_REMOTE_URL = enableRemoteCheckUrl;
}

@Before
public void setGitClient() throws IOException, InterruptedException {
repoRoot = tempFolder.newFolder();
Expand All @@ -128,7 +151,6 @@ public void setGitClient() throws IOException, InterruptedException {
private final Random random = new Random();

private static final String[] BAD_REMOTE_URL_PREFIXES = {
"--get-url",
"--sort version:refname",
"--upload-pack=/usr/bin/id",
"--upload-pack=`touch %s`",
Expand Down Expand Up @@ -185,23 +207,33 @@ public void removeMarkerFile() throws Exception {

@After
public void checkMarkerFile() {
File markerFile = new File(markerFileName);
assertThat(markerFileName + " unexpectedly detected after test", markerFile.exists(), is(false));
if (enableRemoteCheckUrl) { /* If remote checking is disabled, marker file is expected in several cases */
File markerFile = new File(markerFileName);
assertFalse("Marker file '" + markerFileName + "' detected after test", markerFile.exists());
}
}

@Test
@Issue("SECURITY-1534")
public void testGetHeadRev_String_SECURITY_1534() throws Exception {
thrown.expect(GitException.class);
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl));
if (enableRemoteCheckUrl) {
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl));
} else {
thrown.expectMessage(containsString(badRemoteUrl.trim()));
}
gitClient.getHeadRev(badRemoteUrl);
}

@Test
@Issue("SECURITY-1534")
public void testGetHeadRev_String_String_SECURITY_1534() throws Exception {
thrown.expect(GitException.class);
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl));
if (enableRemoteCheckUrl) {
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl));
} else {
thrown.expectMessage(containsString(badRemoteUrl.trim()));
}
gitClient.getHeadRev(badRemoteUrl, "master");
}

Expand All @@ -211,7 +243,11 @@ public void testGetRemoteReferences_SECURITY_1534() throws Exception {
boolean headsOnly = random.nextBoolean();
boolean tagsOnly = random.nextBoolean();
thrown.expect(GitException.class);
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl));
if (enableRemoteCheckUrl) {
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl));
} else {
thrown.expectMessage(containsString(badRemoteUrl.trim()));
}
gitClient.getRemoteReferences(badRemoteUrl, "*master", headsOnly, tagsOnly);
}

Expand All @@ -220,7 +256,11 @@ public void testGetRemoteReferences_SECURITY_1534() throws Exception {
public void testGetRemoteSymbolicReferences_SECURITY_1534() throws Exception {
assumeTrue(CLI_GIT_SUPPORTS_SYMREF);
thrown.expect(GitException.class);
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl));
if (enableRemoteCheckUrl) {
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl));
} else {
thrown.expectMessage(containsString(badRemoteUrl.trim()));
}
gitClient.getRemoteSymbolicReferences(badRemoteUrl, "master");
}

Expand All @@ -233,7 +273,9 @@ public void testFetch_URIish_SECURITY_1534() throws Exception {
refSpecs.add(refSpec);
URIish badRepoUrl = new URIish(badRemoteUrl.trim());
thrown.expect(GitException.class);
thrown.expectMessage(containsString("Invalid remote URL: " + badRepoUrl.toPrivateASCIIString()));
if (enableRemoteCheckUrl) {
thrown.expectMessage(containsString("Invalid remote URL: " + badRepoUrl.toPrivateASCIIString()));
}
gitClient.fetch_().from(badRepoUrl, refSpecs).execute();
}

Expand All @@ -243,7 +285,9 @@ public void testFetch_String_SECURITY_1534() throws Exception {
RefSpec refSpec = new RefSpec("+refs/heads/*:refs/remotes/origin/*");
gitClient.setRemoteUrl("origin", badRemoteUrl);
thrown.expect(GitException.class);
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl.trim()));
if (enableRemoteCheckUrl) {
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl.trim()));
}
gitClient.fetch("origin", refSpec);
}

Expand All @@ -253,7 +297,9 @@ public void testFetch_String_RefSpec_SECURITY_1534() throws Exception {
RefSpec refSpec = new RefSpec("+refs/heads/*:refs/remotes/origin/*");
gitClient.setRemoteUrl("origin", badRemoteUrl);
thrown.expect(GitException.class);
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl.trim()));
if (enableRemoteCheckUrl) {
thrown.expectMessage(containsString("Invalid remote URL: " + badRemoteUrl.trim()));
}
gitClient.fetch("origin", refSpec, refSpec, refSpec);
}
}

0 comments on commit 701c12c

Please sign in to comment.