Redact secrets from error messages

TWChennai · Jul 4, 2023 · 4e4ca37 · 4e4ca37
1 parent 54170d9
commit 4e4ca37
Show file tree

Hide file tree

Showing 17 changed files with 143 additions and 46 deletions.
diff --git a/src/main/java/com/thoughtworks/go/scm/plugin/git/GitConfig.java b/src/main/java/com/thoughtworks/go/scm/plugin/git/GitConfig.java
@@ -1,9 +1,12 @@
 package com.thoughtworks.go.scm.plugin.git;
 
-import com.thoughtworks.go.scm.plugin.util.StringUtil;
+import org.apache.commons.lang3.StringUtils;
 
+import java.util.Collection;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 public class GitConfig {
     private String url;
@@ -37,7 +40,7 @@ public boolean isRemoteUrl() {
     }
 
     public boolean hasCredentials() {
-        return !StringUtil.isBlank(url) && !StringUtil.isBlank(password);
+        return StringUtils.isNotBlank(url) && StringUtils.isNotBlank(password);
     }
 
     public String getEffectiveUrl() {
@@ -81,7 +84,7 @@ public String getRemoteBranch() {
     }
 
     public String getEffectiveBranch() {
-        return StringUtil.isBlank(branch) ? "master" : branch;
+        return StringUtils.isBlank(branch) ? "master" : branch;
     }
 
     public String getBranch() {
@@ -147,4 +150,8 @@ public boolean equals(Object o) {
     public int hashCode() {
         return Objects.hash(url, username, password, branch, subModule, recursiveSubModuleUpdate, noCheckout, shallowClone);
     }
+
+    public Collection<String> redactableTokens() {
+        return Stream.of(username, password).filter(StringUtils::isNotBlank).collect(Collectors.toSet());
+    }
 }
diff --git a/src/main/java/com/thoughtworks/go/scm/plugin/git/GitHelper.java b/src/main/java/com/thoughtworks/go/scm/plugin/git/GitHelper.java
@@ -4,9 +4,9 @@
 import com.thoughtworks.go.scm.plugin.git.cmd.ConsoleResult;
 import com.thoughtworks.go.scm.plugin.git.cmd.InMemoryConsumer;
 import com.thoughtworks.go.scm.plugin.git.cmd.ProcessOutputStreamConsumer;
-import com.thoughtworks.go.scm.plugin.util.StringUtil;
 import org.apache.commons.exec.CommandLine;
 import org.apache.commons.io.FileUtils;
+import org.apache.commons.lang3.StringUtils;
 
 import java.io.File;
 import java.io.IOException;
@@ -218,7 +218,7 @@ public void pull() {
     public void fetch(String refSpec) {
         stdOut.consumeLine("[GIT] Fetching changes");
         List<String> args = new ArrayList<>(Arrays.asList("fetch", "origin", "--prune", "--recurse-submodules=no"));
-        if (!StringUtil.isBlank(refSpec)) {
+        if (!StringUtils.isBlank(refSpec)) {
             args.add(refSpec);
         }
         runOrBomb(Console.createCommand(args.toArray(new String[0])));
@@ -438,7 +438,7 @@ private ConsoleResult runAndGetOutput(CommandLine gitCmd, File workingDir) {
     }
 
     private ConsoleResult runAndGetOutput(CommandLine gitCmd, File workingDir, ProcessOutputStreamConsumer stdOut, ProcessOutputStreamConsumer stdErr) {
-        return Console.runOrBomb(gitCmd, workingDir, stdOut, stdErr);
+        return Console.runOrBomb(gitCmd, workingDir, stdOut, stdErr, gitConfig == null ? Set.of() : gitConfig.redactableTokens());
     }
 
     public void cloneOrFetch() {

diff --git a/src/main/java/com/thoughtworks/go/scm/plugin/git/cmd/Console.java b/src/main/java/com/thoughtworks/go/scm/plugin/git/cmd/Console.java
@@ -1,11 +1,13 @@
 package com.thoughtworks.go.scm.plugin.git.cmd;
 
+import com.thoughtworks.go.scm.plugin.util.StringUtil;
 import org.apache.commons.exec.CommandLine;
 import org.apache.commons.exec.DefaultExecutor;
 import org.apache.commons.exec.Executor;
 import org.apache.commons.exec.PumpStreamHandler;
 
 import java.io.File;
+import java.util.Collection;
 
 public class Console {
     public static CommandLine createCommand(String... args) {
@@ -14,7 +16,7 @@ public static CommandLine createCommand(String... args) {
         return gitCmd;
     }
 
-    public static ConsoleResult runOrBomb(CommandLine commandLine, File workingDir, ProcessOutputStreamConsumer stdOut, ProcessOutputStreamConsumer stdErr) {
+    public static ConsoleResult runOrBomb(CommandLine commandLine, File workingDir, ProcessOutputStreamConsumer stdOut, ProcessOutputStreamConsumer stdErr, Collection<String> redactables) {
         Executor executor = new DefaultExecutor();
         executor.setStreamHandler(new PumpStreamHandler(stdOut, stdErr));
         if (workingDir != null) {
@@ -25,11 +27,10 @@ public static ConsoleResult runOrBomb(CommandLine commandLine, File workingDir,
             int exitCode = executor.execute(commandLine);
             return new ConsoleResult(exitCode, stdOut.output(), stdErr.output());
         } catch (Exception e) {
-            throw new RuntimeException(getMessage(String.format("Exception (%s)", e.getMessage()), commandLine, workingDir), e);
+            throw new RuntimeException(String.format("Exception (%s) Occurred: %s - %s",
+                    StringUtil.replaceSecretText(e.getMessage(), redactables),
+                    StringUtil.replaceSecretText(commandLine.toString(), redactables),
+                    workingDir), e);
         }
     }
-
-    private static String getMessage(String type, CommandLine commandLine, File workingDir) {
-        return String.format("%s Occurred: %s - %s", type, commandLine.toString(), workingDir);
-    }
 }
diff --git a/...ain/java/com/thoughtworks/go/scm/plugin/model/requestHandlers/CheckoutRequestHandler.java b/...ain/java/com/thoughtworks/go/scm/plugin/model/requestHandlers/CheckoutRequestHandler.java
@@ -11,9 +11,7 @@
 import com.thoughtworks.go.scm.plugin.util.JsonUtils;
 
 import java.io.File;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 public class CheckoutRequestHandler implements RequestHandler {
     private static final Logger LOGGER = Logger.getLoggerFor(CheckoutRequestHandler.class);
@@ -45,7 +43,7 @@ public GoPluginApiResponse handle(GoPluginApiRequest apiRequest) {
                     "messages", messages)
             );
         } catch (Throwable t) {
-            return JsonUtils.renderErrorApiResponse(apiRequest, t);
+            return JsonUtils.renderErrorApiResponse(apiRequest, t, Arrays.asList(gitConfig.getUsername(), gitConfig.getPassword()));
         }
     }
 }
diff --git a/...com/thoughtworks/go/scm/plugin/model/requestHandlers/GetLatestRevisionRequestHandler.java b/...com/thoughtworks/go/scm/plugin/model/requestHandlers/GetLatestRevisionRequestHandler.java
@@ -11,9 +11,7 @@
 import com.thoughtworks.go.scm.plugin.util.Validator;
 
 import java.io.File;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 public class GetLatestRevisionRequestHandler implements RequestHandler {
     private static final Logger LOGGER = Logger.getLoggerFor(GetLatestRevisionRequestHandler.class);
@@ -47,7 +45,7 @@ public GoPluginApiResponse handle(GoPluginApiRequest apiRequest) {
                 return JsonUtils.renderSuccessApiResponse(Map.of("revision", RevisionUtil.toMap(revision)));
             }
         } catch (Throwable t) {
-            return JsonUtils.renderErrorApiResponse(apiRequest, t);
+            return JsonUtils.renderErrorApiResponse(apiRequest, t, Arrays.asList(gitConfig.getUsername(), gitConfig.getPassword()));
         }
     }
 }
diff --git a/...m/thoughtworks/go/scm/plugin/model/requestHandlers/LatestRevisionSinceRequestHandler.java b/...m/thoughtworks/go/scm/plugin/model/requestHandlers/LatestRevisionSinceRequestHandler.java
@@ -11,9 +11,7 @@
 import com.thoughtworks.go.scm.plugin.util.Validator;
 
 import java.io.File;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 import static java.util.stream.Collectors.toList;
 
@@ -36,7 +34,7 @@ public GoPluginApiResponse handle(GoPluginApiRequest apiRequest) {
         Validator.validateUrl(gitConfig, fieldMap);
         if (!fieldMap.isEmpty()) {
             String message = (String) fieldMap.get("message");
-            LOGGER.error(String.format("Invalid url: %s", message));
+            LOGGER.error(String.format("Invalid url: %s", message)); // FIXME
             return JsonUtils.renderErrorApiResponse(message);
         }
 
@@ -60,7 +58,7 @@ public GoPluginApiResponse handle(GoPluginApiRequest apiRequest) {
                                         .collect(toList())));
             }
         } catch (Throwable t) {
-            return JsonUtils.renderErrorApiResponse(apiRequest, t);
+            return JsonUtils.renderErrorApiResponse(apiRequest, t, Arrays.asList(gitConfig.getUsername(), gitConfig.getPassword()));
         }
     }
 }
diff --git a/...om/thoughtworks/go/scm/plugin/model/requestHandlers/SCMCheckConnectionRequestHandler.java b/...om/thoughtworks/go/scm/plugin/model/requestHandlers/SCMCheckConnectionRequestHandler.java
@@ -7,8 +7,8 @@
 import com.thoughtworks.go.scm.plugin.git.GitHelper;
 import com.thoughtworks.go.scm.plugin.git.HelperFactory;
 import com.thoughtworks.go.scm.plugin.util.JsonUtils;
-import com.thoughtworks.go.scm.plugin.util.StringUtil;
 import com.thoughtworks.go.scm.plugin.util.Validator;
+import org.apache.commons.lang3.StringUtils;
 
 import java.io.File;
 import java.util.ArrayList;
@@ -40,7 +40,7 @@ public GoPluginApiResponse handle(GoPluginApiRequest goPluginApiRequest) {
     private void checkConnection(GitConfig gitConfig, Map<String, Object> response, ArrayList<String> messages) {
         LOGGER.debug("SCMCheckConnectionRequestHandler In handle");
         try {
-            if (StringUtil.isBlank(gitConfig.getUrl())) {
+            if (StringUtils.isBlank(gitConfig.getUrl())) {
                 response.put("status", "failure");
                 messages.add("URL is empty");
             } else if (gitConfig.getUrl().startsWith("/")) {
@@ -67,11 +67,7 @@ private void checkConnection(GitConfig gitConfig, Map<String, Object> response,
             }
         } catch (Exception e) {
             response.put("status", "failure");
-            if (e.getMessage() != null) {
-                messages.add(e.getMessage());
-            } else {
-                messages.add(e.getClass().getCanonicalName());
-            }
+            messages.add(e.getMessage() != null ? e.getMessage() : e.getClass().getCanonicalName());
         }
     }
 }
diff --git a/...main/java/com/thoughtworks/go/scm/plugin/model/requestHandlers/UnknownRequestHandler.java b/...main/java/com/thoughtworks/go/scm/plugin/model/requestHandlers/UnknownRequestHandler.java
@@ -8,6 +8,6 @@ public class UnknownRequestHandler implements RequestHandler {
 
     @Override
     public GoPluginApiResponse handle(GoPluginApiRequest apiRequest) {
-        return JsonUtils.renderErrorApiResponse(apiRequest, null);
+        return JsonUtils.renderErrorApiResponse(apiRequest, null, null);
     }
 }
diff --git a/src/main/java/com/thoughtworks/go/scm/plugin/util/JsonUtils.java b/src/main/java/com/thoughtworks/go/scm/plugin/util/JsonUtils.java
@@ -11,10 +11,7 @@
 import org.apache.commons.lang3.exception.ExceptionUtils;
 
 import java.io.IOException;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -32,13 +29,13 @@ public static GoPluginApiResponse renderErrorApiResponse(Object response) {
         return renderJSON(INTERNAL_ERROR_RESPONSE_CODE, response);
     }
 
-    public static GoPluginApiResponse renderErrorApiResponse(GoPluginApiRequest apiRequest, Throwable t) {
+    public static GoPluginApiResponse renderErrorApiResponse(GoPluginApiRequest apiRequest, Throwable t, Collection<String> secrets) {
         LOGGER.error(apiRequest.requestName() + " failed", t);
         return renderJSON(INTERNAL_ERROR_RESPONSE_CODE,
                 String.format("%s failed due to [%s], rootCause [%s]",
                         apiRequest.requestName(),
-                        ExceptionUtils.getMessage(t),
-                        ExceptionUtils.getRootCauseMessage(t)));
+                        StringUtil.replaceSecretText(ExceptionUtils.getMessage(t), secrets),
+                        StringUtil.replaceSecretText(ExceptionUtils.getRootCauseMessage(t), secrets)));
     }
 
     public static Map<String, String> parseScmConfiguration(GoPluginApiRequest apiRequest) {

diff --git a/src/main/java/com/thoughtworks/go/scm/plugin/util/StringUtil.java b/src/main/java/com/thoughtworks/go/scm/plugin/util/StringUtil.java
@@ -1,7 +1,18 @@
 package com.thoughtworks.go.scm.plugin.util;
 
+import org.apache.commons.lang3.StringUtils;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Optional;
+
 public class StringUtil {
-    public static boolean isBlank(String str) {
-        return str == null || str.isBlank();
+    private static final String REDACT_REPLACEMENT = "******";
+
+    public static String replaceSecretText(String redactableText, Collection<String> secrets) {
+        return Optional.ofNullable(secrets).orElse(Collections.emptySet()).stream()
+                .filter(StringUtils::isNotBlank)
+                .map(String::trim)
+                .reduce(redactableText, (partiallyRedacted, secret) -> partiallyRedacted.replaceAll(secret, REDACT_REPLACEMENT));
     }
 }
diff --git a/src/main/java/com/thoughtworks/go/scm/plugin/util/Validator.java b/src/main/java/com/thoughtworks/go/scm/plugin/util/Validator.java
@@ -1,6 +1,7 @@
 package com.thoughtworks.go.scm.plugin.util;
 
 import com.thoughtworks.go.scm.plugin.git.GitConfig;
+import org.apache.commons.lang3.StringUtils;
 
 import java.io.File;
 import java.util.Map;
@@ -16,7 +17,7 @@ public static boolean isValidURL(String url) {
     }
 
     public static void validateUrl(GitConfig gitConfig, Map<String, Object> fieldMap) {
-        if (StringUtil.isBlank(gitConfig.getUrl())) {
+        if (StringUtils.isBlank(gitConfig.getUrl())) {
             fieldMap.put("key", "url");
             fieldMap.put("message", "URL is a required field");
         } else {

diff --git a/src/test/java/com/thoughtworks/go/scm/plugin/git/GitConfigTest.java b/src/test/java/com/thoughtworks/go/scm/plugin/git/GitConfigTest.java
@@ -141,4 +141,19 @@ public void getBranch() {
 
         assertThat(gitConfig.getBranch()).isEqualTo("branch");
     }
+
+    @Test
+    public void usernameAndPasswordShouldBeRedactable() {
+        assertThat(new GitConfig("http://url.test", "username", "password", "branch").redactableTokens())
+                .containsExactlyInAnyOrder("username", "password");
+
+        assertThat(new GitConfig("http://url.test", "password", "password", "branch").redactableTokens())
+                .containsExactlyInAnyOrder("password");
+
+        assertThat(new GitConfig("http://url.test", " ", "password", "branch").redactableTokens())
+                .containsExactlyInAnyOrder("password");
+
+        assertThat(new GitConfig("http://url.test", "", " ", "branch").redactableTokens())
+                .isEmpty();
+    }
 }
diff --git a/src/test/java/com/thoughtworks/go/scm/plugin/git/cmd/ConsoleTest.java b/src/test/java/com/thoughtworks/go/scm/plugin/git/cmd/ConsoleTest.java
@@ -0,0 +1,41 @@
+package com.thoughtworks.go.scm.plugin.git.cmd;
+
+import org.apache.commons.exec.CommandLine;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Set;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+class ConsoleTest {
+
+    @TempDir
+    File tempDir;
+
+    @Test
+    public void shouldRedactExceptionMessagesFromCommandLine() {
+        CommandLine command = CommandLine.parse("badbinary https://secret:thing/here");
+        Set<String> secrets = Set.of("secret", "thing");
+        assertThatThrownBy(() -> Console.runOrBomb(command, tempDir, null, null, secrets))
+                .isExactlyInstanceOf(RuntimeException.class)
+                .hasMessageContaining("Exception (Cannot run program \"badbinary\"")
+                .hasMessageContaining("https://******:******/here")
+                .hasMessageNotContainingAny(secrets.toArray(String[]::new))
+                .hasCauseExactlyInstanceOf(IOException.class);
+    }
+
+    @Test
+    public void shouldRedactExceptionMessagesFromException() {
+        CommandLine command = CommandLine.parse("secretthing and some args secret");
+        Set<String> secrets = Set.of("secret", "thing");
+        assertThatThrownBy(() -> Console.runOrBomb(command, tempDir, null, null, secrets))
+                .isExactlyInstanceOf(RuntimeException.class)
+                .hasMessageContaining("Exception (Cannot run program \"************\"")
+                .hasMessageContaining("************, and, some, args, ******")
+                .hasMessageNotContainingAny(secrets.toArray(String[]::new))
+                .hasCauseExactlyInstanceOf(IOException.class);
+    }
+}
diff --git a/...java/com/thoughtworks/go/scm/plugin/model/requestHandlers/CheckoutRequestHandlerTest.java b/...java/com/thoughtworks/go/scm/plugin/model/requestHandlers/CheckoutRequestHandlerTest.java
@@ -73,7 +73,7 @@ public void shouldHandleApiRequestAndRenderErrorApiResponse() {
             Exception cause = new IllegalArgumentException("git@github.com:lifealike/gocd-config.git: UnknownHostKey: github.com. RSA key fingerprint is 16:27:ac:a5:76:28:2d:36:63:1b:56:4d:eb:df:a6:48");
             RuntimeException runtimeException = new RuntimeException("clone failed", cause);
             doThrow(runtimeException).when(gitHelperMock).cloneOrFetch();
-            when(JsonUtils.renderErrorApiResponse(eq(pluginApiRequestMock), errorCaptor.capture())).thenReturn(mock(GoPluginApiResponse.class));
+            when(JsonUtils.renderErrorApiResponse(eq(pluginApiRequestMock), errorCaptor.capture(), any())).thenReturn(mock(GoPluginApiResponse.class));
 
             checkoutRequestHandler.handle(pluginApiRequestMock);
             assertThat(errorCaptor.getValue()).isEqualTo(runtimeException);

diff --git a/...thoughtworks/go/scm/plugin/model/requestHandlers/GetLatestRevisionRequestHandlerTest.java b/...thoughtworks/go/scm/plugin/model/requestHandlers/GetLatestRevisionRequestHandlerTest.java
@@ -97,7 +97,7 @@ public void shouldHandleApiRequestAndRenderErrorApiResponseWhenCloneFailed() {
             Exception cause = new IllegalArgumentException("git@github.com:lifealike/gocd-config.git: UnknownHostKey: github.com. RSA key fingerprint is 16:27:ac:a5:76:28:2d:36:63:1b:56:4d:eb:df:a6:48");
             RuntimeException runtimeException = new RuntimeException("clone failed", cause);
 
-            when(JsonUtils.renderErrorApiResponse(eq(pluginApiRequestMock), errorCaptor.capture())).thenReturn(mock(GoPluginApiResponse.class));
+            when(JsonUtils.renderErrorApiResponse(eq(pluginApiRequestMock), errorCaptor.capture(), any())).thenReturn(mock(GoPluginApiResponse.class));
             when(gitConfigMock.getUrl()).thenReturn("https://github.com/TWChennai/gocd-git-path-material-plugin.git");
             doThrow(runtimeException).when(gitHelperMock).cloneOrFetch();
 

diff --git a/src/test/java/com/thoughtworks/go/scm/plugin/util/JsonUtilsTest.java b/src/test/java/com/thoughtworks/go/scm/plugin/util/JsonUtilsTest.java
@@ -10,6 +10,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
@@ -68,7 +69,7 @@ public void shouldReturnGoPluginApiResponseFromThrowable() throws IOException {
 
         Throwable throwable = new IllegalArgumentException("bad args", new IOException("connection failure"));
 
-        GoPluginApiResponse apiResponse = JsonUtils.renderErrorApiResponse(request, throwable);
+        GoPluginApiResponse apiResponse = JsonUtils.renderErrorApiResponse(request, throwable, null);
 
         assertThat(apiResponse.responseCode()).isEqualTo(500);
         assertThat(apiResponse.responseHeaders()).isNull();
@@ -130,4 +131,10 @@ public void shouldSplitPath() {
         assertThat(JsonUtils.splitPaths("a/b, c/d")).isEqualTo(List.of("a/b", "c/d"));
     }
 
+    @Test
+    public void shouldRedactExceptions() {
+        Throwable throwable = new RuntimeException("hello supersecret world", new RuntimeException("root supersecret"));
+        assertThat(JsonUtils.renderErrorApiResponse(mock(GoPluginApiRequest.class), throwable, Set.of("supersecret")).responseBody())
+                .isEqualTo("\"null failed due to [RuntimeException: hello ****** world], rootCause [RuntimeException: root ******]\"");
+    }
 }