From c8ef06b7586a603806cc674b786aa232fd1461e1 Mon Sep 17 00:00:00 2001 From: Alan Lew Date: Wed, 8 Jan 2025 16:12:08 -0500 Subject: [PATCH 1/9] Implement a tool-specific CasC preference for masking credentials in Git URLs; Implement URL credential masking using a system property; Add unit tests --- src/main/java/hudson/plugins/git/GitTool.java | 36 +++++++- .../plugins/gitclient/CliGitAPIImpl.java | 41 ++++++++- .../gitclient/GitToolConfigurator.java | 21 ++++- .../hudson/plugins/git/GitTool/config.jelly | 3 + .../git/GitTool/help-maskUrlCredentials.html | 3 + .../plugins/gitclient/GitClientTest.java | 88 +++++++++++++++++++ .../gitclient/GitToolConfiguratorTest.java | 51 ++++++++++- .../gitclient/configuration-as-code.yaml | 2 + 8 files changed, 237 insertions(+), 8 deletions(-) create mode 100644 src/main/resources/hudson/plugins/git/GitTool/help-maskUrlCredentials.html diff --git a/src/main/java/hudson/plugins/git/GitTool.java b/src/main/java/hudson/plugins/git/GitTool.java index 32b56d1029..6a7c34a403 100644 --- a/src/main/java/hudson/plugins/git/GitTool.java +++ b/src/main/java/hudson/plugins/git/GitTool.java @@ -27,6 +27,7 @@ import net.sf.json.JSONObject; import org.jenkinsci.Symbol; import org.kohsuke.stapler.DataBoundConstructor; +import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.QueryParameter; import org.kohsuke.stapler.StaplerRequest2; import org.kohsuke.stapler.interceptor.RequirePOST; @@ -51,12 +52,39 @@ public GitTool(String name, String home, List> propert super(name, home, properties); } + private static final String MASK_URL_CREDENTIALS_ENV_VAR = "GIT_MASK_URL_CREDENTIALS"; + + @Override + public void buildEnvVars(EnvVars env) { + LOGGER.log(Level.INFO, String.format("+++ Setting env var %s to '%s'", MASK_URL_CREDENTIALS_ENV_VAR, String.valueOf(this.getMaskUrlCredentials()))); + env.put(MASK_URL_CREDENTIALS_ENV_VAR, String.valueOf(this.getMaskUrlCredentials())); + final String envValue = env.get(MASK_URL_CREDENTIALS_ENV_VAR); + LOGGER.log(Level.INFO, String.format("+++ Env var %s is '%s'", MASK_URL_CREDENTIALS_ENV_VAR, (envValue == null ? "NOT_SET" : envValue))); + } + /** Constant DEFAULT="Default" */ public static final transient String DEFAULT = "Default"; @Serial private static final long serialVersionUID = 1; + /** A flag indicating whether to mask URL credentials in the build log */ + private boolean maskUrlCredentials = false; + + /** + * getMaskUrlCredentials. + * + * @return true if credentials in Git URLs should be masked in the build log + */ + public boolean getMaskUrlCredentials() { + return this.maskUrlCredentials; + } + + @DataBoundSetter + public void setMaskUrlCredentials(boolean maskUrlCredentials) { + this.maskUrlCredentials = maskUrlCredentials; + } + /** * getGitExe. * @@ -101,12 +129,16 @@ public static GitTool getDefaultInstallation() { @Override public GitTool forNode(@NonNull Node node, TaskListener log) throws IOException, InterruptedException { - return new GitTool(getName(), translateFor(node, log), Collections.emptyList()); + final GitTool gitTool = new GitTool(getName(), translateFor(node, log), Collections.emptyList()); + gitTool.setMaskUrlCredentials(this.getMaskUrlCredentials()); + return gitTool; } @Override public GitTool forEnvironment(EnvVars environment) { - return new GitTool(getName(), environment.expand(getHome()), Collections.emptyList()); + final GitTool gitTool = new GitTool(getName(), environment.expand(getHome()), Collections.emptyList()); + gitTool.setMaskUrlCredentials(this.getMaskUrlCredentials()); + return gitTool; } @Override diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java index 5f38704bf5..631b9475cb 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java +++ b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java @@ -305,6 +305,22 @@ private void getGitVersion() { return gitVersion >= requestedVersion; } + private static final String MASK_URL_CREDENTIALS_REGEX = "://[/]?[^/@]*@"; + private static final Pattern MASK_URL_CREDENTIALS_PATTERN = Pattern.compile(MASK_URL_CREDENTIALS_REGEX); + private static final String MASK_URL_CREDENTIALS_REPLACE = "://xxxxx@"; + private static final boolean MASK_URL_CREDENTIALS = Boolean.parseBoolean( + System.getProperty(CliGitAPIImpl.class.getName() + ".maskUrlCredentials", "false")); + + /** + * Mask the first occurrence of credentials in the supplied URL. + * + * @param url the URL to mask + * @return the masked URL, or null if null was supplied + */ + /* package */ String maskUrlCredentials(String url) { + return url != null ? MASK_URL_CREDENTIALS_PATTERN.matcher(url).replaceFirst(MASK_URL_CREDENTIALS_REPLACE) : null; + } + /** * Compare the current cli git version with the required version. * Finds if the current cli git version is at-least the required version. @@ -343,6 +359,9 @@ protected CliGitAPIImpl(String gitExe, File workspace, TaskListener listener, En } launcher = new LocalLauncher(IGitAPI.verbose ? listener : TaskListener.NULL); + if (MASK_URL_CREDENTIALS) { + this.listener.getLogger().println("Masking credentials in GIT URLs"); + } } /** {@inheritDoc} */ @@ -568,7 +587,12 @@ public FetchCommand depth(Integer depth) { @Override public void execute() throws GitException, InterruptedException { - listener.getLogger().println("Fetching upstream changes from " + url); + // Mask credentials in the Git URL before writing to the build log, if requested + String displayUrl = url.toString(); + if (MASK_URL_CREDENTIALS) { + displayUrl = maskUrlCredentials(displayUrl); + } + listener.getLogger().println("Fetching upstream changes from " + displayUrl); ArgumentListBuilder args = new ArgumentListBuilder(); args.add("fetch"); @@ -813,7 +837,12 @@ public void execute() throws GitException, InterruptedException { throw new IllegalArgumentException("Invalid repository " + url, e); } - listener.getLogger().println("Cloning repository " + url); + // Mask credentials in the Git URL before writing to the build log, if requested + String displayUrl = urIish.toString(); + if (MASK_URL_CREDENTIALS) { + displayUrl = maskUrlCredentials(displayUrl); + } + listener.getLogger().println("Cloning repository " + displayUrl); try { Util.deleteContentsRecursive(workspace); @@ -2808,7 +2837,13 @@ private String launchCommandIn(ArgumentListBuilder args, File workDir, EnvVars e args.prepend("setsid"); } int usedTimeout = timeout == null ? TIMEOUT : timeout; - listener.getLogger().println(" > " + command + TIMEOUT_LOG_PREFIX + usedTimeout); + + // Mask credentials in the Git URL before writing to the build log, if requested + String displayCommand = command; + if (MASK_URL_CREDENTIALS) { + displayCommand = maskUrlCredentials(displayCommand); + } + listener.getLogger().println(" > " + displayCommand + TIMEOUT_LOG_PREFIX + usedTimeout); Launcher.ProcStarter p = launcher.launch().cmds(args.toCommandArray()).envs(freshEnv); diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java b/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java index 2d0ef18deb..b7dcc7510a 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java +++ b/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java @@ -12,11 +12,13 @@ import io.jenkins.plugins.casc.ConfiguratorException; import io.jenkins.plugins.casc.model.CNode; import io.jenkins.plugins.casc.model.Mapping; +import io.jenkins.plugins.casc.model.Scalar; import io.jenkins.plugins.casc.model.Sequence; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.logging.Level; import java.util.logging.Logger; @Extension(optional = true) @@ -63,8 +65,9 @@ public List> getConfigurators(ConfigurationContext context Attribute name = new Attribute<>("name", String.class); Attribute home = new Attribute<>("home", String.class); Attribute p = new Attribute<>("properties", ToolProperty.class); + Attribute maskUrlCredentials = new Attribute<>("maskUrlCredentials", Boolean.class); p.multiple(true); - return Arrays.asList(name, home, p); + return Arrays.asList(name, home, p, maskUrlCredentials); } @Override @@ -78,18 +81,31 @@ protected GitTool instance(Mapping mapping, @NonNull ConfigurationContext contex if (mapping.remove("home") != null) { // Ignored but could be added, so removing to not fail handleUnknown logger.warning("property `home` is ignored for `" + JGitTool.MAGIC_EXENAME + "`"); } + if (mapping.remove("maskUrlCredentials") != null) { // Ignored but could be added, so removing to not fail handleUnknown + logger.warning("property `maskUrlCredentials` is ignored for `" + JGitTool.MAGIC_EXENAME + "`"); + } return new JGitTool(instantiateProperties(mproperties, context)); } else if (JGitApacheTool.MAGIC_EXENAME.equals(name)) { if (mapping.remove("home") != null) { // Ignored but could be added, so removing to not fail handleUnknown logger.warning("property `home` is ignored for `" + JGitApacheTool.MAGIC_EXENAME + "`"); } + if (mapping.remove("maskUrlCredentials") != null) { // Ignored but could be added, so removing to not fail handleUnknown + logger.warning("property `maskUrlCredentials` is ignored for `" + JGitApacheTool.MAGIC_EXENAME + "`"); + } return new JGitApacheTool(instantiateProperties(mproperties, context)); } else { if (mapping.get("home") == null) { throw new ConfiguratorException(this, "Home required for cli git configuration."); } String home = mapping.getScalarValue("home"); - return new GitTool(name, home, instantiateProperties(mproperties, context)); + Boolean maskUrlCredentials = false; + if (mapping.containsKey("maskUrlCredentials") && mapping.get("maskUrlCredentials") != null) { + maskUrlCredentials = Boolean.valueOf(mapping.getScalarValue("maskUrlCredentials")); + } + final GitTool gitTool = new GitTool(name, home, instantiateProperties(mproperties, context)); + logger.log(Level.INFO, String.format("+++ Set maskUrlCredentials to %b", maskUrlCredentials)); + gitTool.setMaskUrlCredentials(maskUrlCredentials); + return gitTool; } } @@ -122,6 +138,7 @@ public CNode describe(GitTool instance, ConfigurationContext context) throws Exc } else if (instance != null) { mapping.put("name", instance.getName()); mapping.put("home", instance.getHome()); + mapping.put("maskUrlCredentials", new Scalar(instance.getMaskUrlCredentials())); } if (context != null && instance != null diff --git a/src/main/resources/hudson/plugins/git/GitTool/config.jelly b/src/main/resources/hudson/plugins/git/GitTool/config.jelly index 643e8913c1..c9cdad7b31 100644 --- a/src/main/resources/hudson/plugins/git/GitTool/config.jelly +++ b/src/main/resources/hudson/plugins/git/GitTool/config.jelly @@ -8,4 +8,7 @@ + + + diff --git a/src/main/resources/hudson/plugins/git/GitTool/help-maskUrlCredentials.html b/src/main/resources/hudson/plugins/git/GitTool/help-maskUrlCredentials.html new file mode 100644 index 0000000000..413a4fdfaf --- /dev/null +++ b/src/main/resources/hudson/plugins/git/GitTool/help-maskUrlCredentials.html @@ -0,0 +1,3 @@ +
+ Mask credentials in Git URLs. +
\ No newline at end of file diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java index 51997bcdcf..0457667a85 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java +++ b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java @@ -3343,4 +3343,92 @@ public void test_git_branch_with_line_breaks_and_long_strings() throws Exception private boolean isWindows() { return File.pathSeparatorChar == ';'; } + + @Test + public void testMaskUrlCredentialsNoCredentials() throws Exception { + if (!gitImplName.equals("git") || !(gitClient instanceof CliGitAPIImpl)) { + return; + } + CliGitAPIImpl cliGitClient = (CliGitAPIImpl) gitClient; + final String url = "https://dev.baz/git/repo.git"; + final String expected = url; + assertThat(cliGitClient.maskUrlCredentials(url), is(expected)); + } + + @Test + public void testMaskUrlCredentialsNoCredentialSeparator() throws Exception { + if (!gitImplName.equals("git") || !(gitClient instanceof CliGitAPIImpl)) { + return; + } + CliGitAPIImpl cliGitClient = (CliGitAPIImpl) gitClient; + final String url = "https://quux@dev.baz/git/repo.git"; + final String expected = "https://xxxxx@dev.baz/git/repo.git"; + assertThat(cliGitClient.maskUrlCredentials(url), is(expected)); + } + + @Test + public void testMaskUrlCredentialsUsernameAndPassword() throws Exception { + if (!gitImplName.equals("git") || !(gitClient instanceof CliGitAPIImpl)) { + return; + } + CliGitAPIImpl cliGitClient = (CliGitAPIImpl) gitClient; + final String url = "https://foo:bar@dev.baz/git/repo.git"; + final String expected = "https://xxxxx@dev.baz/git/repo.git"; + assertThat(cliGitClient.maskUrlCredentials(url), is(expected)); + } + + @Test + public void testMaskUrlCredentialsUsernameOnly() throws Exception { + if (!gitImplName.equals("git") || !(gitClient instanceof CliGitAPIImpl)) { + return; + } + CliGitAPIImpl cliGitClient = (CliGitAPIImpl) gitClient; + final String url = "https://foo:@dev.baz/git/repo.git"; + final String expected = "https://xxxxx@dev.baz/git/repo.git"; + assertThat(cliGitClient.maskUrlCredentials(url), is(expected)); + } + + @Test + public void testMaskUrlCredentialsPasswordOnly() throws Exception { + if (!gitImplName.equals("git") || !(gitClient instanceof CliGitAPIImpl)) { + return; + } + CliGitAPIImpl cliGitClient = (CliGitAPIImpl) gitClient; + final String url = "https://:bar@dev.baz/git/repo.git"; + final String expected = "https://xxxxx@dev.baz/git/repo.git"; + assertThat(cliGitClient.maskUrlCredentials(url), is(expected)); + } + + @Test + public void testMaskUrlCredentialsNullUrl() throws Exception { + if (!gitImplName.equals("git") || !(gitClient instanceof CliGitAPIImpl)) { + return; + } + CliGitAPIImpl cliGitClient = (CliGitAPIImpl) gitClient; + final String url = null; + final String expected = null; + assertThat(cliGitClient.maskUrlCredentials(url), is(expected)); + } + + @Test + public void testMaskUrlCredentialsCommand() throws Exception { + if (!gitImplName.equals("git") || !(gitClient instanceof CliGitAPIImpl)) { + return; + } + CliGitAPIImpl cliGitClient = (CliGitAPIImpl) gitClient; + final String command = "git fetch --no-tags --force --progress --depth=1 -- https://foo:bar@dev.baz/git/repo.git +refs/heads/main:refs/remotes/origin/main"; + final String expected = "git fetch --no-tags --force --progress --depth=1 -- https://xxxxx@dev.baz/git/repo.git +refs/heads/main:refs/remotes/origin/main"; + assertThat(cliGitClient.maskUrlCredentials(command), is(expected)); + } + + @Test + public void testMaskUrlCredentialsCommandTwoUrls() throws Exception { + if (!gitImplName.equals("git") || !(gitClient instanceof CliGitAPIImpl)) { + return; + } + CliGitAPIImpl cliGitClient = (CliGitAPIImpl) gitClient; + final String command = "git config url.ssh://git@github.com/foobar.insteadof https://baz:qux@github.com/foobar"; + final String expected = "git config url.ssh://xxxxx@github.com/foobar.insteadof https://baz:qux@github.com/foobar"; + assertThat(cliGitClient.maskUrlCredentials(command), is(expected)); + } } diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java index a39d04d709..19486e2d1e 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java +++ b/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java @@ -42,6 +42,8 @@ import io.jenkins.plugins.casc.model.CNode; import io.jenkins.plugins.casc.model.Mapping; import java.util.List; + +import io.jenkins.plugins.casc.model.Scalar; import org.junit.Test; public class GitToolConfiguratorTest { @@ -98,6 +100,7 @@ public void testDescribeJGitTool() throws Exception { assertThat(cNode.getType(), is(CNode.Type.MAPPING)); Mapping cNodeMapping = cNode.asMapping(); assertThat(cNodeMapping.getScalarValue("name"), is(JGitTool.MAGIC_EXENAME)); + assertThat(cNodeMapping.containsKey("maskUrlCredentials"), is(false)); } @Test @@ -108,6 +111,7 @@ public void testDescribeJGitApacheTool() throws Exception { assertThat(cNode.getType(), is(CNode.Type.MAPPING)); Mapping cNodeMapping = cNode.asMapping(); assertThat(cNodeMapping.getScalarValue("name"), is(JGitApacheTool.MAGIC_EXENAME)); + assertThat(cNodeMapping.containsKey("maskUrlCredentials"), is(false)); } @Test @@ -121,6 +125,7 @@ public void testDescribeGitToolWithoutProperties() throws Exception { Mapping cNodeMapping = cNode.asMapping(); assertThat(cNodeMapping.getScalarValue("name"), is(gitName)); assertThat(cNodeMapping.getScalarValue("home"), is(gitHome)); + assertThat(Boolean.valueOf(cNodeMapping.getScalarValue("maskUrlCredentials")), is(false)); } @Test @@ -131,6 +136,7 @@ public void testInstance() throws Exception { assertThat(gitTool, is(instanceOf(GitTool.class))); assertThat(gitTool.getName(), is("Default")); assertThat(gitTool.getHome(), is("")); + assertThat(gitTool.getMaskUrlCredentials(), is(false)); } @Test @@ -154,6 +160,17 @@ public void testInstanceJGitToolWithHome() throws Exception { assertThat(gitTool, is(not(instanceOf(JGitApacheTool.class)))); } + @Test + public void testInstanceJGitToolWithMaskUrlCredentials() throws Exception { + Mapping mapping = new Mapping(); + mapping.put("name", JGitTool.MAGIC_EXENAME); + mapping.put("maskUrlCredentials", new Scalar(true)); // Will log a message + ConfigurationContext context = new ConfigurationContext(null); + GitTool gitTool = gitToolConfigurator.instance(mapping, context); + assertThat(gitTool, is(instanceOf(JGitTool.class))); + assertThat(gitTool, is(not(instanceOf(JGitApacheTool.class)))); + } + @Test public void testInstanceJGitApacheTool() throws Exception { Mapping mapping = new Mapping(); @@ -175,6 +192,17 @@ public void testInstanceJGitApacheToolWithHome() throws Exception { assertThat(gitTool, is(not(instanceOf(JGitTool.class)))); } + @Test + public void testInstanceJGitApacheToolWithMaskUrlCredentials() throws Exception { + Mapping mapping = new Mapping(); + mapping.put("name", JGitApacheTool.MAGIC_EXENAME); + mapping.put("maskUrlCredentials", new Scalar(true)); // Will log a message + ConfigurationContext context = new ConfigurationContext(null); + GitTool gitTool = gitToolConfigurator.instance(mapping, context); + assertThat(gitTool, is(instanceOf(JGitApacheTool.class))); + assertThat(gitTool, is(not(instanceOf(JGitTool.class)))); + } + @Test(expected = ConfiguratorException.class) public void testInstanceGitToolWithoutHome() throws Exception { Mapping mapping = new Mapping(); @@ -183,13 +211,32 @@ public void testInstanceGitToolWithoutHome() throws Exception { gitToolConfigurator.instance(mapping, context); } + @Test + public void testInstanceGitToolWithoutMaskUrlCredentials() throws Exception { + Mapping mapping = new Mapping(); + String gitHome = "testGitHome"; + String gitName = "testGitName"; + mapping.put("home", gitHome); + mapping.put("name", gitName); + ConfigurationContext context = new ConfigurationContext(null); + GitTool gitTool = gitToolConfigurator.instance(mapping, context); + assertThat(gitTool, is(instanceOf(GitTool.class))); + assertThat(gitTool, is(not(instanceOf(JGitTool.class)))); + assertThat(gitTool, is(not(instanceOf(JGitApacheTool.class)))); + assertThat(gitTool.getHome(), is(gitHome)); + assertThat(gitTool.getName(), is(gitName)); + assertThat(gitTool.getMaskUrlCredentials(), is(false)); + } + @Test public void testInstanceGitTool() throws Exception { Mapping mapping = new Mapping(); String gitHome = "testGitHome"; String gitName = "testGitName"; + Boolean maskUrlCredentials = true; mapping.put("home", gitHome); mapping.put("name", gitName); + mapping.put("maskUrlCredentials", new Scalar(maskUrlCredentials)); ConfigurationContext context = new ConfigurationContext(null); GitTool gitTool = gitToolConfigurator.instance(mapping, context); assertThat(gitTool, is(instanceOf(GitTool.class))); @@ -197,6 +244,7 @@ public void testInstanceGitTool() throws Exception { assertThat(gitTool, is(not(instanceOf(JGitApacheTool.class)))); assertThat(gitTool.getHome(), is(gitHome)); assertThat(gitTool.getName(), is(gitName)); + assertThat(gitTool.getMaskUrlCredentials(), is(true)); } @Test @@ -205,6 +253,7 @@ public void testGetAttributes() { Attribute name = new Attribute<>("name", String.class); Attribute home = new Attribute<>("home", String.class); Attribute p = new Attribute<>("properties", ToolProperty.class); - assertThat(gitToolAttributes, containsInAnyOrder(name, home, p)); + Attribute maskUrlCredentials = new Attribute<>("maskUrlCredentials", Boolean.class); + assertThat(gitToolAttributes, containsInAnyOrder(name, home, p, maskUrlCredentials)); } } diff --git a/src/test/resources/org/jenkinsci/plugins/gitclient/configuration-as-code.yaml b/src/test/resources/org/jenkinsci/plugins/gitclient/configuration-as-code.yaml index 6ee606a4fc..7d7df59965 100644 --- a/src/test/resources/org/jenkinsci/plugins/gitclient/configuration-as-code.yaml +++ b/src/test/resources/org/jenkinsci/plugins/gitclient/configuration-as-code.yaml @@ -5,9 +5,11 @@ tool: home: "jgit" - home: "git" name: "Default" + maskUrlCredentials: true - name: "jgitapache" - home: "/opt/git/git" name: "optional" + maskUrlCredentials: false properties: - installSource: installers: From e205c44a735e878a7edd391d75ec13aaa214fa8f Mon Sep 17 00:00:00 2001 From: Alan Lew Date: Wed, 8 Jan 2025 17:00:06 -0500 Subject: [PATCH 2/9] Remove CasC implementation --- src/main/java/hudson/plugins/git/GitTool.java | 36 ++----------------- .../gitclient/GitToolConfigurator.java | 19 ++-------- .../gitclient/GitToolConfiguratorTest.java | 33 +---------------- .../gitclient/configuration-as-code.yaml | 2 -- 4 files changed, 5 insertions(+), 85 deletions(-) diff --git a/src/main/java/hudson/plugins/git/GitTool.java b/src/main/java/hudson/plugins/git/GitTool.java index 6a7c34a403..32b56d1029 100644 --- a/src/main/java/hudson/plugins/git/GitTool.java +++ b/src/main/java/hudson/plugins/git/GitTool.java @@ -27,7 +27,6 @@ import net.sf.json.JSONObject; import org.jenkinsci.Symbol; import org.kohsuke.stapler.DataBoundConstructor; -import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.QueryParameter; import org.kohsuke.stapler.StaplerRequest2; import org.kohsuke.stapler.interceptor.RequirePOST; @@ -52,39 +51,12 @@ public GitTool(String name, String home, List> propert super(name, home, properties); } - private static final String MASK_URL_CREDENTIALS_ENV_VAR = "GIT_MASK_URL_CREDENTIALS"; - - @Override - public void buildEnvVars(EnvVars env) { - LOGGER.log(Level.INFO, String.format("+++ Setting env var %s to '%s'", MASK_URL_CREDENTIALS_ENV_VAR, String.valueOf(this.getMaskUrlCredentials()))); - env.put(MASK_URL_CREDENTIALS_ENV_VAR, String.valueOf(this.getMaskUrlCredentials())); - final String envValue = env.get(MASK_URL_CREDENTIALS_ENV_VAR); - LOGGER.log(Level.INFO, String.format("+++ Env var %s is '%s'", MASK_URL_CREDENTIALS_ENV_VAR, (envValue == null ? "NOT_SET" : envValue))); - } - /** Constant DEFAULT="Default" */ public static final transient String DEFAULT = "Default"; @Serial private static final long serialVersionUID = 1; - /** A flag indicating whether to mask URL credentials in the build log */ - private boolean maskUrlCredentials = false; - - /** - * getMaskUrlCredentials. - * - * @return true if credentials in Git URLs should be masked in the build log - */ - public boolean getMaskUrlCredentials() { - return this.maskUrlCredentials; - } - - @DataBoundSetter - public void setMaskUrlCredentials(boolean maskUrlCredentials) { - this.maskUrlCredentials = maskUrlCredentials; - } - /** * getGitExe. * @@ -129,16 +101,12 @@ public static GitTool getDefaultInstallation() { @Override public GitTool forNode(@NonNull Node node, TaskListener log) throws IOException, InterruptedException { - final GitTool gitTool = new GitTool(getName(), translateFor(node, log), Collections.emptyList()); - gitTool.setMaskUrlCredentials(this.getMaskUrlCredentials()); - return gitTool; + return new GitTool(getName(), translateFor(node, log), Collections.emptyList()); } @Override public GitTool forEnvironment(EnvVars environment) { - final GitTool gitTool = new GitTool(getName(), environment.expand(getHome()), Collections.emptyList()); - gitTool.setMaskUrlCredentials(this.getMaskUrlCredentials()); - return gitTool; + return new GitTool(getName(), environment.expand(getHome()), Collections.emptyList()); } @Override diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java b/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java index b7dcc7510a..e75bb00dbe 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java +++ b/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java @@ -65,9 +65,8 @@ public List> getConfigurators(ConfigurationContext context Attribute name = new Attribute<>("name", String.class); Attribute home = new Attribute<>("home", String.class); Attribute p = new Attribute<>("properties", ToolProperty.class); - Attribute maskUrlCredentials = new Attribute<>("maskUrlCredentials", Boolean.class); p.multiple(true); - return Arrays.asList(name, home, p, maskUrlCredentials); + return Arrays.asList(name, home, p); } @Override @@ -81,31 +80,18 @@ protected GitTool instance(Mapping mapping, @NonNull ConfigurationContext contex if (mapping.remove("home") != null) { // Ignored but could be added, so removing to not fail handleUnknown logger.warning("property `home` is ignored for `" + JGitTool.MAGIC_EXENAME + "`"); } - if (mapping.remove("maskUrlCredentials") != null) { // Ignored but could be added, so removing to not fail handleUnknown - logger.warning("property `maskUrlCredentials` is ignored for `" + JGitTool.MAGIC_EXENAME + "`"); - } return new JGitTool(instantiateProperties(mproperties, context)); } else if (JGitApacheTool.MAGIC_EXENAME.equals(name)) { if (mapping.remove("home") != null) { // Ignored but could be added, so removing to not fail handleUnknown logger.warning("property `home` is ignored for `" + JGitApacheTool.MAGIC_EXENAME + "`"); } - if (mapping.remove("maskUrlCredentials") != null) { // Ignored but could be added, so removing to not fail handleUnknown - logger.warning("property `maskUrlCredentials` is ignored for `" + JGitApacheTool.MAGIC_EXENAME + "`"); - } return new JGitApacheTool(instantiateProperties(mproperties, context)); } else { if (mapping.get("home") == null) { throw new ConfiguratorException(this, "Home required for cli git configuration."); } String home = mapping.getScalarValue("home"); - Boolean maskUrlCredentials = false; - if (mapping.containsKey("maskUrlCredentials") && mapping.get("maskUrlCredentials") != null) { - maskUrlCredentials = Boolean.valueOf(mapping.getScalarValue("maskUrlCredentials")); - } - final GitTool gitTool = new GitTool(name, home, instantiateProperties(mproperties, context)); - logger.log(Level.INFO, String.format("+++ Set maskUrlCredentials to %b", maskUrlCredentials)); - gitTool.setMaskUrlCredentials(maskUrlCredentials); - return gitTool; + return new GitTool(name, home, instantiateProperties(mproperties, context)); } } @@ -138,7 +124,6 @@ public CNode describe(GitTool instance, ConfigurationContext context) throws Exc } else if (instance != null) { mapping.put("name", instance.getName()); mapping.put("home", instance.getHome()); - mapping.put("maskUrlCredentials", new Scalar(instance.getMaskUrlCredentials())); } if (context != null && instance != null diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java index 19486e2d1e..e9d2b6d900 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java +++ b/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java @@ -100,7 +100,6 @@ public void testDescribeJGitTool() throws Exception { assertThat(cNode.getType(), is(CNode.Type.MAPPING)); Mapping cNodeMapping = cNode.asMapping(); assertThat(cNodeMapping.getScalarValue("name"), is(JGitTool.MAGIC_EXENAME)); - assertThat(cNodeMapping.containsKey("maskUrlCredentials"), is(false)); } @Test @@ -111,7 +110,6 @@ public void testDescribeJGitApacheTool() throws Exception { assertThat(cNode.getType(), is(CNode.Type.MAPPING)); Mapping cNodeMapping = cNode.asMapping(); assertThat(cNodeMapping.getScalarValue("name"), is(JGitApacheTool.MAGIC_EXENAME)); - assertThat(cNodeMapping.containsKey("maskUrlCredentials"), is(false)); } @Test @@ -125,7 +123,6 @@ public void testDescribeGitToolWithoutProperties() throws Exception { Mapping cNodeMapping = cNode.asMapping(); assertThat(cNodeMapping.getScalarValue("name"), is(gitName)); assertThat(cNodeMapping.getScalarValue("home"), is(gitHome)); - assertThat(Boolean.valueOf(cNodeMapping.getScalarValue("maskUrlCredentials")), is(false)); } @Test @@ -136,7 +133,6 @@ public void testInstance() throws Exception { assertThat(gitTool, is(instanceOf(GitTool.class))); assertThat(gitTool.getName(), is("Default")); assertThat(gitTool.getHome(), is("")); - assertThat(gitTool.getMaskUrlCredentials(), is(false)); } @Test @@ -160,17 +156,6 @@ public void testInstanceJGitToolWithHome() throws Exception { assertThat(gitTool, is(not(instanceOf(JGitApacheTool.class)))); } - @Test - public void testInstanceJGitToolWithMaskUrlCredentials() throws Exception { - Mapping mapping = new Mapping(); - mapping.put("name", JGitTool.MAGIC_EXENAME); - mapping.put("maskUrlCredentials", new Scalar(true)); // Will log a message - ConfigurationContext context = new ConfigurationContext(null); - GitTool gitTool = gitToolConfigurator.instance(mapping, context); - assertThat(gitTool, is(instanceOf(JGitTool.class))); - assertThat(gitTool, is(not(instanceOf(JGitApacheTool.class)))); - } - @Test public void testInstanceJGitApacheTool() throws Exception { Mapping mapping = new Mapping(); @@ -192,17 +177,6 @@ public void testInstanceJGitApacheToolWithHome() throws Exception { assertThat(gitTool, is(not(instanceOf(JGitTool.class)))); } - @Test - public void testInstanceJGitApacheToolWithMaskUrlCredentials() throws Exception { - Mapping mapping = new Mapping(); - mapping.put("name", JGitApacheTool.MAGIC_EXENAME); - mapping.put("maskUrlCredentials", new Scalar(true)); // Will log a message - ConfigurationContext context = new ConfigurationContext(null); - GitTool gitTool = gitToolConfigurator.instance(mapping, context); - assertThat(gitTool, is(instanceOf(JGitApacheTool.class))); - assertThat(gitTool, is(not(instanceOf(JGitTool.class)))); - } - @Test(expected = ConfiguratorException.class) public void testInstanceGitToolWithoutHome() throws Exception { Mapping mapping = new Mapping(); @@ -225,7 +199,6 @@ public void testInstanceGitToolWithoutMaskUrlCredentials() throws Exception { assertThat(gitTool, is(not(instanceOf(JGitApacheTool.class)))); assertThat(gitTool.getHome(), is(gitHome)); assertThat(gitTool.getName(), is(gitName)); - assertThat(gitTool.getMaskUrlCredentials(), is(false)); } @Test @@ -233,10 +206,8 @@ public void testInstanceGitTool() throws Exception { Mapping mapping = new Mapping(); String gitHome = "testGitHome"; String gitName = "testGitName"; - Boolean maskUrlCredentials = true; mapping.put("home", gitHome); mapping.put("name", gitName); - mapping.put("maskUrlCredentials", new Scalar(maskUrlCredentials)); ConfigurationContext context = new ConfigurationContext(null); GitTool gitTool = gitToolConfigurator.instance(mapping, context); assertThat(gitTool, is(instanceOf(GitTool.class))); @@ -244,7 +215,6 @@ public void testInstanceGitTool() throws Exception { assertThat(gitTool, is(not(instanceOf(JGitApacheTool.class)))); assertThat(gitTool.getHome(), is(gitHome)); assertThat(gitTool.getName(), is(gitName)); - assertThat(gitTool.getMaskUrlCredentials(), is(true)); } @Test @@ -253,7 +223,6 @@ public void testGetAttributes() { Attribute name = new Attribute<>("name", String.class); Attribute home = new Attribute<>("home", String.class); Attribute p = new Attribute<>("properties", ToolProperty.class); - Attribute maskUrlCredentials = new Attribute<>("maskUrlCredentials", Boolean.class); - assertThat(gitToolAttributes, containsInAnyOrder(name, home, p, maskUrlCredentials)); + assertThat(gitToolAttributes, containsInAnyOrder(name, home, p)); } } diff --git a/src/test/resources/org/jenkinsci/plugins/gitclient/configuration-as-code.yaml b/src/test/resources/org/jenkinsci/plugins/gitclient/configuration-as-code.yaml index 7d7df59965..6ee606a4fc 100644 --- a/src/test/resources/org/jenkinsci/plugins/gitclient/configuration-as-code.yaml +++ b/src/test/resources/org/jenkinsci/plugins/gitclient/configuration-as-code.yaml @@ -5,11 +5,9 @@ tool: home: "jgit" - home: "git" name: "Default" - maskUrlCredentials: true - name: "jgitapache" - home: "/opt/git/git" name: "optional" - maskUrlCredentials: false properties: - installSource: installers: From fe36807d1235ccc2397d7afd44d34a3f32644c88 Mon Sep 17 00:00:00 2001 From: Alan Lew Date: Thu, 9 Jan 2025 11:47:40 -0500 Subject: [PATCH 3/9] Remove leftover bits from CasC implementation --- src/main/resources/hudson/plugins/git/GitTool/config.jelly | 3 --- .../hudson/plugins/git/GitTool/help-maskUrlCredentials.html | 3 --- 2 files changed, 6 deletions(-) delete mode 100644 src/main/resources/hudson/plugins/git/GitTool/help-maskUrlCredentials.html diff --git a/src/main/resources/hudson/plugins/git/GitTool/config.jelly b/src/main/resources/hudson/plugins/git/GitTool/config.jelly index c9cdad7b31..643e8913c1 100644 --- a/src/main/resources/hudson/plugins/git/GitTool/config.jelly +++ b/src/main/resources/hudson/plugins/git/GitTool/config.jelly @@ -8,7 +8,4 @@ - - - diff --git a/src/main/resources/hudson/plugins/git/GitTool/help-maskUrlCredentials.html b/src/main/resources/hudson/plugins/git/GitTool/help-maskUrlCredentials.html deleted file mode 100644 index 413a4fdfaf..0000000000 --- a/src/main/resources/hudson/plugins/git/GitTool/help-maskUrlCredentials.html +++ /dev/null @@ -1,3 +0,0 @@ -
- Mask credentials in Git URLs. -
\ No newline at end of file From 29329a5c52578f098fc8e0bf3cc6eab3b483ef27 Mon Sep 17 00:00:00 2001 From: Alan Lew Date: Thu, 9 Jan 2025 11:49:15 -0500 Subject: [PATCH 4/9] Remove unnecessary constants and tighten checks to mask URL credentials --- .../plugins/gitclient/CliGitAPIImpl.java | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java index 631b9475cb..e89b0edb56 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java +++ b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java @@ -305,8 +305,7 @@ private void getGitVersion() { return gitVersion >= requestedVersion; } - private static final String MASK_URL_CREDENTIALS_REGEX = "://[/]?[^/@]*@"; - private static final Pattern MASK_URL_CREDENTIALS_PATTERN = Pattern.compile(MASK_URL_CREDENTIALS_REGEX); + private static final Pattern MASK_URL_CREDENTIALS_PATTERN = Pattern.compile("://[/]?[^/@]*@"); private static final String MASK_URL_CREDENTIALS_REPLACE = "://xxxxx@"; private static final boolean MASK_URL_CREDENTIALS = Boolean.parseBoolean( System.getProperty(CliGitAPIImpl.class.getName() + ".maskUrlCredentials", "false")); @@ -588,10 +587,8 @@ public FetchCommand depth(Integer depth) { @Override public void execute() throws GitException, InterruptedException { // Mask credentials in the Git URL before writing to the build log, if requested - String displayUrl = url.toString(); - if (MASK_URL_CREDENTIALS) { - displayUrl = maskUrlCredentials(displayUrl); - } + final String displayUrl = + MASK_URL_CREDENTIALS ? maskUrlCredentials(url.toString()) : url.toString(); listener.getLogger().println("Fetching upstream changes from " + displayUrl); ArgumentListBuilder args = new ArgumentListBuilder(); @@ -838,10 +835,8 @@ public void execute() throws GitException, InterruptedException { } // Mask credentials in the Git URL before writing to the build log, if requested - String displayUrl = urIish.toString(); - if (MASK_URL_CREDENTIALS) { - displayUrl = maskUrlCredentials(displayUrl); - } + final String displayUrl = + MASK_URL_CREDENTIALS ? maskUrlCredentials(urIish.toString()) : urIish.toString(); listener.getLogger().println("Cloning repository " + displayUrl); try { @@ -2839,10 +2834,8 @@ private String launchCommandIn(ArgumentListBuilder args, File workDir, EnvVars e int usedTimeout = timeout == null ? TIMEOUT : timeout; // Mask credentials in the Git URL before writing to the build log, if requested - String displayCommand = command; - if (MASK_URL_CREDENTIALS) { - displayCommand = maskUrlCredentials(displayCommand); - } + final String displayCommand = + MASK_URL_CREDENTIALS ? maskUrlCredentials(command) : command; listener.getLogger().println(" > " + displayCommand + TIMEOUT_LOG_PREFIX + usedTimeout); Launcher.ProcStarter p = From 9901deff6073a4b5862933b5f0c4f79635642daf Mon Sep 17 00:00:00 2001 From: Alan Lew Date: Thu, 9 Jan 2025 12:09:57 -0500 Subject: [PATCH 5/9] Add JavaDoc to MASK_URL_CREDENTIALS flag; Update README to include the maskUrlCredentials property --- README.adoc | 7 +++++++ .../plugins/gitclient/CliGitAPIImpl.java | 15 +++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/README.adoc b/README.adoc index 6ee678403c..a71303ba59 100644 --- a/README.adoc +++ b/README.adoc @@ -261,6 +261,13 @@ This setting can be helpful with link:https://plugins.jenkins.io/swarm/[Jenkins + Default is `false` so that `setsid` is not used. +maskUrlCredentials:: +When `org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials` is set to `true`, build log messages that contain Git URLs with credentials will have the credentials masked. URLs that appear in error messages are not masked to aid in troubleshooting. ++ +For example, a Git URL of `https://foo:bar@baz.quux/git/my-repo.git` will be masked as `https://xxxxx@baz.quux/git/my-repo.git`. ++ +Default is `false` so that credentials in Git URLs are not masked. + [#changelog] == Changelog in https://github.com/jenkinsci/git-client-plugin/releases[GitHub Releases] diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java index e89b0edb56..9d98715f10 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java +++ b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java @@ -305,11 +305,22 @@ private void getGitVersion() { return gitVersion >= requestedVersion; } - private static final Pattern MASK_URL_CREDENTIALS_PATTERN = Pattern.compile("://[/]?[^/@]*@"); - private static final String MASK_URL_CREDENTIALS_REPLACE = "://xxxxx@"; + + /** + * Constant which enables masking of credentials in Git URLs + * before they are written to the build log. + * + * MASK_URL_CREDENTIALS=Boolean.parseBoolean(System.getProperty(CliGitAPIImpl.class.getName() + ".maskUrlCredentials", "false")). + * + * Use '-Dorg.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials=true' + * to mask credentials in Git URLs. + */ private static final boolean MASK_URL_CREDENTIALS = Boolean.parseBoolean( System.getProperty(CliGitAPIImpl.class.getName() + ".maskUrlCredentials", "false")); + private static final Pattern MASK_URL_CREDENTIALS_PATTERN = Pattern.compile("://[/]?[^/@]*@"); + private static final String MASK_URL_CREDENTIALS_REPLACE = "://xxxxx@"; + /** * Mask the first occurrence of credentials in the supplied URL. * From 22009cf300fd4b530c65925eed4a7c24576c6ea5 Mon Sep 17 00:00:00 2001 From: Alan Lew Date: Thu, 9 Jan 2025 15:32:23 -0500 Subject: [PATCH 6/9] Remove leftover CasC code from GitToolConfigurator and its tests --- .../plugins/gitclient/GitToolConfigurator.java | 2 -- .../gitclient/GitToolConfiguratorTest.java | 18 ------------------ 2 files changed, 20 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java b/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java index e75bb00dbe..2d0ef18deb 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java +++ b/src/main/java/org/jenkinsci/plugins/gitclient/GitToolConfigurator.java @@ -12,13 +12,11 @@ import io.jenkins.plugins.casc.ConfiguratorException; import io.jenkins.plugins.casc.model.CNode; import io.jenkins.plugins.casc.model.Mapping; -import io.jenkins.plugins.casc.model.Scalar; import io.jenkins.plugins.casc.model.Sequence; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.logging.Level; import java.util.logging.Logger; @Extension(optional = true) diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java index e9d2b6d900..a39d04d709 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java +++ b/src/test/java/org/jenkinsci/plugins/gitclient/GitToolConfiguratorTest.java @@ -42,8 +42,6 @@ import io.jenkins.plugins.casc.model.CNode; import io.jenkins.plugins.casc.model.Mapping; import java.util.List; - -import io.jenkins.plugins.casc.model.Scalar; import org.junit.Test; public class GitToolConfiguratorTest { @@ -185,22 +183,6 @@ public void testInstanceGitToolWithoutHome() throws Exception { gitToolConfigurator.instance(mapping, context); } - @Test - public void testInstanceGitToolWithoutMaskUrlCredentials() throws Exception { - Mapping mapping = new Mapping(); - String gitHome = "testGitHome"; - String gitName = "testGitName"; - mapping.put("home", gitHome); - mapping.put("name", gitName); - ConfigurationContext context = new ConfigurationContext(null); - GitTool gitTool = gitToolConfigurator.instance(mapping, context); - assertThat(gitTool, is(instanceOf(GitTool.class))); - assertThat(gitTool, is(not(instanceOf(JGitTool.class)))); - assertThat(gitTool, is(not(instanceOf(JGitApacheTool.class)))); - assertThat(gitTool.getHome(), is(gitHome)); - assertThat(gitTool.getName(), is(gitName)); - } - @Test public void testInstanceGitTool() throws Exception { Mapping mapping = new Mapping(); From aa8eac66730a0293114ad678317e212013823a7f Mon Sep 17 00:00:00 2001 From: Alan Lew Date: Fri, 10 Jan 2025 10:00:20 -0500 Subject: [PATCH 7/9] Format fixes; Documentation fixes --- README.adoc | 6 ++--- .../plugins/gitclient/CliGitAPIImpl.java | 27 +++++++++---------- .../plugins/gitclient/GitClientFetchTest.java | 25 +++++++++++++++++ .../plugins/gitclient/GitClientTest.java | 9 ++++--- 4 files changed, 47 insertions(+), 20 deletions(-) diff --git a/README.adoc b/README.adoc index a71303ba59..0093632588 100644 --- a/README.adoc +++ b/README.adoc @@ -262,11 +262,11 @@ This setting can be helpful with link:https://plugins.jenkins.io/swarm/[Jenkins Default is `false` so that `setsid` is not used. maskUrlCredentials:: -When `org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials` is set to `true`, build log messages that contain Git URLs with credentials will have the credentials masked. URLs that appear in error messages are not masked to aid in troubleshooting. +When `org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials` is set to `true`, build log messages that contain URLs with credentials will have the first occurrence of credentials masked. URLs that appear in error messages are not masked to aid in troubleshooting. + -For example, a Git URL of `https://foo:bar@baz.quux/git/my-repo.git` will be masked as `https://xxxxx@baz.quux/git/my-repo.git`. +For example, a URL of `https://foo:bar@baz.quux/git/my-repo.git` will be masked as `https://xxxxx@baz.quux/git/my-repo.git`. + -Default is `false` so that credentials in Git URLs are not masked. +Default is `false` so that credentials in URLs are not masked. [#changelog] == Changelog in https://github.com/jenkinsci/git-client-plugin/releases[GitHub Releases] diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java index 9d98715f10..77cbb282c6 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java +++ b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java @@ -305,18 +305,17 @@ private void getGitVersion() { return gitVersion >= requestedVersion; } - /** - * Constant which enables masking of credentials in Git URLs - * before they are written to the build log. + * Constant which enables masking of credentials in URLs before + * they are written to the build log. * * MASK_URL_CREDENTIALS=Boolean.parseBoolean(System.getProperty(CliGitAPIImpl.class.getName() + ".maskUrlCredentials", "false")). * * Use '-Dorg.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials=true' - * to mask credentials in Git URLs. + * to mask credentials in URLs. */ - private static final boolean MASK_URL_CREDENTIALS = Boolean.parseBoolean( - System.getProperty(CliGitAPIImpl.class.getName() + ".maskUrlCredentials", "false")); + private static final boolean MASK_URL_CREDENTIALS = + Boolean.parseBoolean(System.getProperty(CliGitAPIImpl.class.getName() + ".maskUrlCredentials", "false")); private static final Pattern MASK_URL_CREDENTIALS_PATTERN = Pattern.compile("://[/]?[^/@]*@"); private static final String MASK_URL_CREDENTIALS_REPLACE = "://xxxxx@"; @@ -328,7 +327,9 @@ private void getGitVersion() { * @return the masked URL, or null if null was supplied */ /* package */ String maskUrlCredentials(String url) { - return url != null ? MASK_URL_CREDENTIALS_PATTERN.matcher(url).replaceFirst(MASK_URL_CREDENTIALS_REPLACE) : null; + return url != null + ? MASK_URL_CREDENTIALS_PATTERN.matcher(url).replaceFirst(MASK_URL_CREDENTIALS_REPLACE) + : null; } /** @@ -597,9 +598,8 @@ public FetchCommand depth(Integer depth) { @Override public void execute() throws GitException, InterruptedException { - // Mask credentials in the Git URL before writing to the build log, if requested - final String displayUrl = - MASK_URL_CREDENTIALS ? maskUrlCredentials(url.toString()) : url.toString(); + // Mask credentials in URLs before writing to the build log, if requested + final String displayUrl = MASK_URL_CREDENTIALS ? maskUrlCredentials(url.toString()) : url.toString(); listener.getLogger().println("Fetching upstream changes from " + displayUrl); ArgumentListBuilder args = new ArgumentListBuilder(); @@ -845,7 +845,7 @@ public void execute() throws GitException, InterruptedException { throw new IllegalArgumentException("Invalid repository " + url, e); } - // Mask credentials in the Git URL before writing to the build log, if requested + // Mask credentials in URLs before writing to the build log, if requested final String displayUrl = MASK_URL_CREDENTIALS ? maskUrlCredentials(urIish.toString()) : urIish.toString(); listener.getLogger().println("Cloning repository " + displayUrl); @@ -2844,9 +2844,8 @@ private String launchCommandIn(ArgumentListBuilder args, File workDir, EnvVars e } int usedTimeout = timeout == null ? TIMEOUT : timeout; - // Mask credentials in the Git URL before writing to the build log, if requested - final String displayCommand = - MASK_URL_CREDENTIALS ? maskUrlCredentials(command) : command; + // Mask credentials in URLs before writing to the build log, if requested + final String displayCommand = MASK_URL_CREDENTIALS ? maskUrlCredentials(command) : command; listener.getLogger().println(" > " + displayCommand + TIMEOUT_LOG_PREFIX + usedTimeout); Launcher.ProcStarter p = diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java index f8631fef72..4c699e2252 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java +++ b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java @@ -594,6 +594,31 @@ public void test_fetch_noTags() throws Exception { assertThat("Tags have been found : " + tags, tags.isEmpty(), is(true)); } + @Test + public void test_fetch_maskUrlCredentials() throws Exception { + if (!gitImplName.equals("git")) { + return; + } + System.setProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials", "true"); + try { + WorkspaceWithRepo newWorkspace = new WorkspaceWithRepo(repo.getRoot(), gitImplName, listener); + GitClient newTestGitClient = newWorkspace.getGitClient(); + newTestGitClient.setRemoteUrl("origin", newWorkspace.localMirror()); + newTestGitClient + .fetch_() + .from( + new URIish("origin"), + Collections.singletonList(new RefSpec("refs/heads/*:refs/remotes/origin/*"))) + .execute(); + check_remote_url(newWorkspace, newWorkspace.getGitClient(), "origin"); + assertBranchesExist(newTestGitClient.getRemoteBranches(), "origin/" + DEFAULT_MIRROR_BRANCH_NAME); + System.out.println("handler contains: " + handler.getMessages()); + assertThat(handler.containsMessageSubstring("Masking"), is(true)); + } finally { + System.clearProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials"); + } + } + /* JENKINS-33258 detected many calls to git rev-parse. This checks * those calls are not being made. The checkoutRandomBranch call * creates a branch with a random name. The later assertion checks that diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java index 0457667a85..314e80738c 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java +++ b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java @@ -3416,8 +3416,10 @@ public void testMaskUrlCredentialsCommand() throws Exception { return; } CliGitAPIImpl cliGitClient = (CliGitAPIImpl) gitClient; - final String command = "git fetch --no-tags --force --progress --depth=1 -- https://foo:bar@dev.baz/git/repo.git +refs/heads/main:refs/remotes/origin/main"; - final String expected = "git fetch --no-tags --force --progress --depth=1 -- https://xxxxx@dev.baz/git/repo.git +refs/heads/main:refs/remotes/origin/main"; + final String command = + "git fetch --no-tags --force --progress --depth=1 -- https://foo:bar@dev.baz/git/repo.git +refs/heads/main:refs/remotes/origin/main"; + final String expected = + "git fetch --no-tags --force --progress --depth=1 -- https://xxxxx@dev.baz/git/repo.git +refs/heads/main:refs/remotes/origin/main"; assertThat(cliGitClient.maskUrlCredentials(command), is(expected)); } @@ -3428,7 +3430,8 @@ public void testMaskUrlCredentialsCommandTwoUrls() throws Exception { } CliGitAPIImpl cliGitClient = (CliGitAPIImpl) gitClient; final String command = "git config url.ssh://git@github.com/foobar.insteadof https://baz:qux@github.com/foobar"; - final String expected = "git config url.ssh://xxxxx@github.com/foobar.insteadof https://baz:qux@github.com/foobar"; + final String expected = + "git config url.ssh://xxxxx@github.com/foobar.insteadof https://baz:qux@github.com/foobar"; assertThat(cliGitClient.maskUrlCredentials(command), is(expected)); } } From 7ed2b9e5ed5b7ead3059e6dea07254dc2a368557 Mon Sep 17 00:00:00 2001 From: Alan Lew Date: Fri, 10 Jan 2025 14:08:55 -0500 Subject: [PATCH 8/9] Fix Spotbugs issues; Reformat source; Add tests for clone and fetch methods --- .../plugins/gitclient/CliGitAPIImpl.java | 33 ++++---- .../plugins/gitclient/GitClientCloneTest.java | 78 +++++++++++++++++-- .../plugins/gitclient/GitClientFetchTest.java | 69 ++++++++++++++-- 3 files changed, 148 insertions(+), 32 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java index 77cbb282c6..5319942bd1 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java +++ b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java @@ -305,18 +305,6 @@ private void getGitVersion() { return gitVersion >= requestedVersion; } - /** - * Constant which enables masking of credentials in URLs before - * they are written to the build log. - * - * MASK_URL_CREDENTIALS=Boolean.parseBoolean(System.getProperty(CliGitAPIImpl.class.getName() + ".maskUrlCredentials", "false")). - * - * Use '-Dorg.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials=true' - * to mask credentials in URLs. - */ - private static final boolean MASK_URL_CREDENTIALS = - Boolean.parseBoolean(System.getProperty(CliGitAPIImpl.class.getName() + ".maskUrlCredentials", "false")); - private static final Pattern MASK_URL_CREDENTIALS_PATTERN = Pattern.compile("://[/]?[^/@]*@"); private static final String MASK_URL_CREDENTIALS_REPLACE = "://xxxxx@"; @@ -326,12 +314,21 @@ private void getGitVersion() { * @param url the URL to mask * @return the masked URL, or null if null was supplied */ - /* package */ String maskUrlCredentials(String url) { + public static String maskUrlCredentials(String url) { return url != null ? MASK_URL_CREDENTIALS_PATTERN.matcher(url).replaceFirst(MASK_URL_CREDENTIALS_REPLACE) : null; } + /** + * Return the value of the org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials system property. + * + * @return true if maskUrlCredentials is enabled, false otherwise + */ + public static boolean getMaskUrlCredentials() { + return Boolean.parseBoolean(System.getProperty(CliGitAPIImpl.class.getName() + ".maskUrlCredentials", "false")); + } + /** * Compare the current cli git version with the required version. * Finds if the current cli git version is at-least the required version. @@ -370,8 +367,8 @@ protected CliGitAPIImpl(String gitExe, File workspace, TaskListener listener, En } launcher = new LocalLauncher(IGitAPI.verbose ? listener : TaskListener.NULL); - if (MASK_URL_CREDENTIALS) { - this.listener.getLogger().println("Masking credentials in GIT URLs"); + if (this.getMaskUrlCredentials()) { + this.listener.getLogger().println("Masking credentials in URLs"); } } @@ -599,7 +596,7 @@ public FetchCommand depth(Integer depth) { @Override public void execute() throws GitException, InterruptedException { // Mask credentials in URLs before writing to the build log, if requested - final String displayUrl = MASK_URL_CREDENTIALS ? maskUrlCredentials(url.toString()) : url.toString(); + final String displayUrl = getMaskUrlCredentials() ? maskUrlCredentials(url.toString()) : url.toString(); listener.getLogger().println("Fetching upstream changes from " + displayUrl); ArgumentListBuilder args = new ArgumentListBuilder(); @@ -847,7 +844,7 @@ public void execute() throws GitException, InterruptedException { // Mask credentials in URLs before writing to the build log, if requested final String displayUrl = - MASK_URL_CREDENTIALS ? maskUrlCredentials(urIish.toString()) : urIish.toString(); + getMaskUrlCredentials() ? maskUrlCredentials(urIish.toString()) : urIish.toString(); listener.getLogger().println("Cloning repository " + displayUrl); try { @@ -2845,7 +2842,7 @@ private String launchCommandIn(ArgumentListBuilder args, File workDir, EnvVars e int usedTimeout = timeout == null ? TIMEOUT : timeout; // Mask credentials in URLs before writing to the build log, if requested - final String displayCommand = MASK_URL_CREDENTIALS ? maskUrlCredentials(command) : command; + final String displayCommand = getMaskUrlCredentials() ? maskUrlCredentials(command) : command; listener.getLogger().println(" > " + displayCommand + TIMEOUT_LOG_PREFIX + usedTimeout); Launcher.ProcStarter p = diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java index 281d867677..30f3f5533c 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java +++ b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java @@ -21,12 +21,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.List; -import java.util.Random; -import java.util.Set; +import java.util.*; import java.util.logging.Level; import java.util.logging.Logger; import org.apache.commons.lang.StringUtils; @@ -429,6 +424,77 @@ public void test_clone_huge_timeout_logging() throws Exception { assertTimeout(testGitClient, "fetch", expectedValue); } + @Test + public void test_clone_maskUrlCredentials_enabled() throws Exception { + if (!gitImplName.equals("git")) { + return; + } + System.setProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials", "true"); + try { + final CliGitAPIImpl newTestGitClient = + new CliGitAPIImpl("git", repo.getRoot(), listener, new hudson.EnvVars()); + assertThat(newTestGitClient.getMaskUrlCredentials(), is(true)); + newTestGitClient + .clone_() + .url("https://foo:bar@localhost/git/my-repo.git") + .repositoryName("origin") + .execute(); + } catch (Exception e) { + System.out.println("(ignored) clone exception: " + e); + } finally { + System.clearProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials"); + } + assertThat(handler.containsMessageSubstring("Masking credentials in URLs"), is(true)); + assertThat(handler.containsMessageSubstring("https://xxxxx@localhost/git/my-repo.git"), is(true)); + } + + @Test + public void test_clone_maskUrlCredentials_disabled() throws Exception { + if (!gitImplName.equals("git")) { + return; + } + System.setProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials", "false"); + try { + final CliGitAPIImpl newTestGitClient = + new CliGitAPIImpl("git", repo.getRoot(), listener, new hudson.EnvVars()); + assertThat(newTestGitClient.getMaskUrlCredentials(), is(false)); + newTestGitClient + .clone_() + .url("https://foo:bar@localhost/git/my-repo.git") + .repositoryName("origin") + .execute(); + } catch (Exception e) { + System.out.println("(ignored) clone exception: " + e); + } finally { + System.clearProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials"); + } + assertThat(handler.containsMessageSubstring("Masking credentials in URLs"), is(false)); + assertThat(handler.containsMessageSubstring("https://xxxxx@localhost/git/my-repo.git"), is(false)); + assertThat(handler.containsMessageSubstring("https://foo:bar@localhost/git/my-repo.git"), is(true)); + } + + @Test + public void test_clone_maskUrlCredentials_unset() throws Exception { + if (!gitImplName.equals("git")) { + return; + } + System.clearProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials"); + final CliGitAPIImpl newTestGitClient = new CliGitAPIImpl("git", repo.getRoot(), listener, new hudson.EnvVars()); + assertThat(newTestGitClient.getMaskUrlCredentials(), is(false)); + try { + newTestGitClient + .clone_() + .url("https://foo:bar@localhost/git/my-repo.git") + .repositoryName("origin") + .execute(); + } catch (Exception e) { + System.out.println("(ignored) clone exception: " + e); + } + assertThat(handler.containsMessageSubstring("Masking credentials in URLs"), is(false)); + assertThat(handler.containsMessageSubstring("https://xxxxx@localhost/git/my-repo.git"), is(false)); + assertThat(handler.containsMessageSubstring("https://foo:bar@localhost/git/my-repo.git"), is(true)); + } + private void assertAlternatesFileExists() { final String alternates = ".git" + File.separator + "objects" + File.separator + "info" + File.separator + "alternates"; diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java index 4c699e2252..353496ee53 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java +++ b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java @@ -595,28 +595,81 @@ public void test_fetch_noTags() throws Exception { } @Test - public void test_fetch_maskUrlCredentials() throws Exception { + public void test_fetch_maskUrlCredentials_enabled() throws Exception { if (!gitImplName.equals("git")) { return; } System.setProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials", "true"); try { - WorkspaceWithRepo newWorkspace = new WorkspaceWithRepo(repo.getRoot(), gitImplName, listener); - GitClient newTestGitClient = newWorkspace.getGitClient(); - newTestGitClient.setRemoteUrl("origin", newWorkspace.localMirror()); + final CliGitAPIImpl newTestGitClient = + new CliGitAPIImpl("git", repo.getRoot(), listener, new hudson.EnvVars()); + assertThat(newTestGitClient.getMaskUrlCredentials(), is(true)); + newTestGitClient.setRemoteUrl("origin", "https://foo:bar@localhost/git/my-repo.git"); newTestGitClient .fetch_() .from( new URIish("origin"), Collections.singletonList(new RefSpec("refs/heads/*:refs/remotes/origin/*"))) .execute(); - check_remote_url(newWorkspace, newWorkspace.getGitClient(), "origin"); - assertBranchesExist(newTestGitClient.getRemoteBranches(), "origin/" + DEFAULT_MIRROR_BRANCH_NAME); - System.out.println("handler contains: " + handler.getMessages()); - assertThat(handler.containsMessageSubstring("Masking"), is(true)); + } catch (Exception e) { + System.out.println("(ignored) fetch exception: " + e); } finally { System.clearProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials"); } + assertThat(handler.containsMessageSubstring("Masking credentials in URLs"), is(true)); + assertThat(handler.containsMessageSubstring("https://xxxxx@localhost/git/my-repo.git"), is(true)); + } + + @Test + public void test_fetch_maskUrlCredentials_disabled() throws Exception { + if (!gitImplName.equals("git")) { + return; + } + System.setProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials", "false"); + try { + final CliGitAPIImpl newTestGitClient = + new CliGitAPIImpl("git", repo.getRoot(), listener, new hudson.EnvVars()); + assertThat(newTestGitClient.getMaskUrlCredentials(), is(false)); + newTestGitClient.setRemoteUrl("origin", "https://foo:bar@localhost/git/my-repo.git"); + newTestGitClient + .fetch_() + .from( + new URIish("origin"), + Collections.singletonList(new RefSpec("refs/heads/*:refs/remotes/origin/*"))) + .execute(); + } catch (Exception e) { + System.out.println("(ignored) fetch exception: " + e); + } finally { + System.clearProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials"); + } + assertThat(handler.containsMessageSubstring("Masking credentials in URLs"), is(false)); + assertThat(handler.containsMessageSubstring("https://xxxxx@localhost/git/my-repo.git"), is(false)); + assertThat(handler.containsMessageSubstring("https://foo:bar@localhost/git/my-repo.git"), is(true)); + } + + @Test + public void test_fetch_maskUrlCredentials_unset() throws Exception { + if (!gitImplName.equals("git")) { + return; + } + System.clearProperty("org.jenkinsci.plugins.gitclient.CliGitAPIImpl.maskUrlCredentials"); + try { + final CliGitAPIImpl newTestGitClient = + new CliGitAPIImpl("git", repo.getRoot(), listener, new hudson.EnvVars()); + assertThat(newTestGitClient.getMaskUrlCredentials(), is(false)); + newTestGitClient.setRemoteUrl("origin", "https://foo:bar@localhost/git/my-repo.git"); + newTestGitClient + .fetch_() + .from( + new URIish("origin"), + Collections.singletonList(new RefSpec("refs/heads/*:refs/remotes/origin/*"))) + .execute(); + } catch (Exception e) { + System.out.println("(ignored) fetch exception: " + e); + } + assertThat(handler.containsMessageSubstring("Masking credentials in URLs"), is(false)); + assertThat(handler.containsMessageSubstring("https://xxxxx@localhost/git/my-repo.git"), is(false)); + assertThat(handler.containsMessageSubstring("https://foo:bar@localhost/git/my-repo.git"), is(true)); } /* JENKINS-33258 detected many calls to git rev-parse. This checks From 1bdbd2a8e5357fe623dbc5099e464d0f57fe4ae8 Mon Sep 17 00:00:00 2001 From: Alan Lew Date: Fri, 10 Jan 2025 14:13:23 -0500 Subject: [PATCH 9/9] Revert unintended imports change --- .../jenkinsci/plugins/gitclient/GitClientCloneTest.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java index 30f3f5533c..d71fa9b0d1 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java +++ b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java @@ -21,7 +21,12 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; -import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.Random; +import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; import org.apache.commons.lang.StringUtils;