Issue 53 hide password (#203)

* Update Velocity Templating Engine from 1.7 to 2.3
* Bump jetty-webapp in /tcwebhooks-core (#195)
* Format secure parameters correctly so that TeamCity obscures them

Previously, the value was prefixed with `secure:`. This was incorrect.
It is supposed to be a parameters name that is prefixed with `secure:`.

This fix changes that, so that TeamCity correctly swaps out the values
with tokens transparently.

* Track secure value access and hide URL from UI

Keep a flag to determine when a secure: value is accessed. If so, hide
the full URL in the webUI so that secure values in the URL are not
leaked.

It's not possible to determine why the value was accessed. It could have
been to build the payload, or the URL. Therefore, if either of these
have resolved a secure value, we hide that URL.

* Add support for hide-secure-values in the WebHookConfig

Adds ability to store/retreive the value of hide-secure-values.
This will be used as a flag to determine whether the UI and logs should
container secure values.

* Add support for hiding values from log and UI
* Show/Hide URL in UI when secure values checkbox is toggled.
* Populate external and internal project IDs when executing test
* Don't log Parameter values. They might contain a secure value.

sonar-project.properties

@@ -18,4 +18,10 @@ sonar.jacoco.reportPath=target/jacoco.exec
 
 sonar.java.coveragePlugin=jacoco
 sonar.dynamicAnalysis=reuseReports
-sonar.surefire.reportsPath=target/surefire-reports
+sonar.surefire.reportsPath=target/surefire-reports
+
+sonar.issue.ignore.multicriteria=e1
+
+# Console usage - ignore a single class
+sonar.issue.ignore.multicriteria.e1.ruleKey=java:S5411
+sonar.issue.ignore.multicriteria.e1.resourceKey=**/*.java

tcwebhooks-core/pom.xml

@@ -148,7 +148,7 @@
      	<dependency>
 	         <groupId>org.eclipse.jetty</groupId>
 	         <artifactId>jetty-webapp</artifactId>
-			<version>9.4.43.v20210629</version>
+			<version>9.4.44.v20210927</version>
 			<scope>test</scope>
 	     </dependency>
     	 <!--Jetty dependencies end here -->
@@ -190,10 +190,17 @@
 
 		<dependency>
 			<groupId>org.apache.velocity</groupId>
-			<artifactId>velocity</artifactId>
-			<version>1.7</version>
+			<artifactId>velocity-engine-core</artifactId>
+			<version>2.3</version>
 		</dependency>
-
+
+		<!--  Use same version of slf4j as velocity 2.3 uses -->
+		<dependency>
+			<groupId>org.slf4j</groupId>
+			<artifactId>slf4j-simple</artifactId>
+			<version>1.7.30</version>
+		</dependency>
+
 		<dependency>
 			<groupId>commons-lang</groupId>
 			<artifactId>commons-lang</artifactId>

tcwebhooks-core/src/main/java/webhook/WebHook.java

@@ -124,6 +124,11 @@ public interface WebHook {
 
 	public abstract void setVariableResolverFactory(VariableResolverFactory variableResolverFactory);
 
+	public abstract boolean shouldHideSecureData();
+
+	public abstract boolean isHideSecureValues();
+	public abstract void setHideSecureValues(boolean hideSecureValues);
+
 
 
 }

tcwebhooks-core/src/main/java/webhook/WebHookExecutionStats.java

@@ -28,6 +28,7 @@ public class WebHookExecutionStats {
 	boolean errored = false;
 	boolean enabled = true;
 	BuildStateEnum buildState;
+	boolean secureValueAccessed = false;
 
 	public WebHookExecutionStats(String url) {
 		this.url = url;

tcwebhooks-core/src/main/java/webhook/WebHookImpl.java

@@ -71,6 +71,7 @@ public class WebHookImpl implements WebHook {
 	@Getter
 	private UUID requestId = UUID.randomUUID();
 	private VariableResolverFactory variableResolverFactory;
+	private boolean hideSecureValues;
 
 
 	public WebHookImpl (String url, WebHookProxyConfig proxyConfig, HttpClient client){
@@ -481,5 +482,21 @@ public VariableResolverFactory getVariableResolverFactory() {
 	public void setVariableResolverFactory(VariableResolverFactory variableResolverFactory) {
 		this.variableResolverFactory = variableResolverFactory;		
 	}
+
+	@Override
+	public boolean shouldHideSecureData() {
+		return this.hideSecureValues && this.getExecutionStats().isSecureValueAccessed();
+	}
+
+	@Override
+	public boolean isHideSecureValues() {
+		return this.hideSecureValues;
+	}
+
+	@Override
+	public void setHideSecureValues(boolean hideSecureValues) {
+		this.hideSecureValues = hideSecureValues;
+
+	}
 
 }

tcwebhooks-core/src/main/java/webhook/teamcity/WebHookContentBuilder.java

@@ -75,6 +75,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc, SQueuedBuild s
 				wh.setUrl(builder.build(whc.getUrl()));
 				wh.checkFilters(builder);
 				wh.resolveHeaders(builder);
+				wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 			}
 		} else if (state.equals(BuildStateEnum.BUILD_REMOVED_FROM_QUEUE) ){
 			wh.setEnabledForBuildState(state, overrideIsEnabled || (whc.isEnabledForBuildType(sBuild.getBuildType()) && wh.getBuildStates().enabled(state)));
@@ -90,6 +91,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc, SQueuedBuild s
 				wh.setUrl(builder.build(whc.getUrl()));
 				wh.checkFilters(builder);
 				wh.resolveHeaders(builder);
+				wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 			}
 		}
 		return wh;
@@ -135,6 +137,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc, SBuild sBuild,
 				wh.setUrl(builder.build(whc.getUrl()));
 				wh.checkFilters(builder);
 				wh.resolveHeaders(builder);
+				wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 			}
 		} else if (state.equals(BuildStateEnum.CHANGES_LOADED)){
 			wh.setEnabledForBuildState(BuildStateEnum.CHANGES_LOADED, isOverrideEnabled || (whc.isEnabledForBuildType(sBuild.getBuildType()) && wh.getBuildStates().enabled(BuildStateEnum.CHANGES_LOADED)));
@@ -151,6 +154,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc, SBuild sBuild,
 				wh.setUrl(builder.build(whc.getUrl()));
 				wh.checkFilters(builder);
 				wh.resolveHeaders(builder);
+				wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 			}
 		} else if (state.equals(BuildStateEnum.SERVICE_MESSAGE_RECEIVED)){
 			wh.setEnabledForBuildState(BuildStateEnum.SERVICE_MESSAGE_RECEIVED, isOverrideEnabled || (whc.isEnabledForBuildType(sBuild.getBuildType()) && wh.getBuildStates().enabled(BuildStateEnum.SERVICE_MESSAGE_RECEIVED)));
@@ -167,6 +171,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc, SBuild sBuild,
 				wh.setUrl(builder.build(whc.getUrl()));
 				wh.checkFilters(builder);
 				wh.resolveHeaders(builder);
+				wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 			}
 		} else if (state.equals(BuildStateEnum.BUILD_INTERRUPTED)){
 			wh.setEnabledForBuildState(BuildStateEnum.BUILD_INTERRUPTED, isOverrideEnabled || (whc.isEnabledForBuildType(sBuild.getBuildType()) && wh.getBuildStates().enabled(BuildStateEnum.BUILD_INTERRUPTED)));
@@ -183,6 +188,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc, SBuild sBuild,
 				wh.setUrl(builder.build(whc.getUrl()));
 				wh.checkFilters(builder);
 				wh.resolveHeaders(builder);
+				wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 			}
 		} else if (state.equals(BuildStateEnum.BEFORE_BUILD_FINISHED)){
 			wh.setEnabledForBuildState(BuildStateEnum.BEFORE_BUILD_FINISHED, isOverrideEnabled || (whc.isEnabledForBuildType(sBuild.getBuildType()) && wh.getBuildStates().enabled(BuildStateEnum.BEFORE_BUILD_FINISHED)));
@@ -199,6 +205,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc, SBuild sBuild,
 				wh.setUrl(builder.build(whc.getUrl()));
 				wh.checkFilters(builder);
 				wh.resolveHeaders(builder);
+				wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 			}
 		} else if (state.equals(BuildStateEnum.BUILD_FINISHED) || state.equals(BuildStateEnum.BUILD_SUCCESSFUL) || state.equals(BuildStateEnum.BUILD_FAILED) || state.equals(BuildStateEnum.BUILD_FIXED) || state.equals(BuildStateEnum.BUILD_BROKEN)){
 			wh.setEnabledForBuildState(BuildStateEnum.BUILD_FINISHED, isOverrideEnabled || (whc.isEnabledForBuildType(sBuild.getBuildType()) && wh.getBuildStates().enabled(
@@ -219,6 +226,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc, SBuild sBuild,
 				wh.setUrl(builder.build(whc.getUrl()));
 				wh.checkFilters(builder);
 				wh.resolveHeaders(builder);
+				wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 			}
 		} else if (state.equals(BuildStateEnum.BUILD_PINNED)) {
 			wh.setEnabledForBuildState(state, isOverrideEnabled || (whc.isEnabledForBuildType(sBuild.getBuildType()) && wh.getBuildStates().enabled(state)));
@@ -235,6 +243,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc, SBuild sBuild,
 				wh.setUrl(builder.build(whc.getUrl()));
 				wh.checkFilters(builder);
 				wh.resolveHeaders(builder);
+				wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 			}
 		} else if (state.equals(BuildStateEnum.BUILD_UNPINNED)) {
 			wh.setEnabledForBuildState(state, isOverrideEnabled || (whc.isEnabledForBuildType(sBuild.getBuildType()) && wh.getBuildStates().enabled(state)));
@@ -251,6 +260,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc, SBuild sBuild,
 				wh.setUrl(builder.build(whc.getUrl()));
 				wh.checkFilters(builder);
 				wh.resolveHeaders(builder);
+				wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 			}
 		}
 		return wh;
@@ -278,6 +288,7 @@ public WebHook buildWebHookContent(WebHook wh, WebHookConfig whc,
 			wh.setUrl(builder.build(whc.getUrl()));
 			wh.checkFilters(builder);
 			wh.resolveHeaders(builder);
+			wh.getExecutionStats().setSecureValueAccessed(extraParameters.wasSecureValueAccessed());
 		}
 		return wh;
 	}

tcwebhooks-core/src/main/java/webhook/teamcity/WebHookExecutionException.java

@@ -18,6 +18,7 @@ public class WebHookExecutionException extends RuntimeException {
 	public static final int    WEBHOOK_PAYLOAD_CONTENT_ASSEMBLY_EXCEPTION_ERROR_CODE = 904;
 	public static final int    WEBHOOK_CONFIGURATION_NOT_FOUND_EXCEPTION_ERROR_CODE = 905;
 	public static final int    WEBHOOK_VARIABLE_RESOLVER_NOT_FOUND_EXCEPTION_ERROR_CODE = 906;
+	public static final int    WEBHOOK_TEMPLATE_PARSING_EXCEPTION_ERROR_CODE = 907;
 
 	public static final String WEBHOOK_UNEXPECTED_EXCEPTION_MESSAGE = "Unexpected exception. Please log a bug on GitHub tcplugins/tcWebHooks. Exception was: ";
 	public static final int    WEBHOOK_UNEXPECTED_EXCEPTION_ERROR_CODE = 999;

tcwebhooks-core/src/main/java/webhook/teamcity/WebHookFactoryImpl.java

@@ -29,6 +29,7 @@ public WebHook getWebHook(WebHookConfig webHookConfig, WebHookProxyConfig proxyC
 		WebHook webHook = new WebHookImpl(webHookConfig.getUrl(), proxyConfig, myWebHookHttpClientFactory.getHttpClient());
 		webHook.setUrl(webHookConfig.getUrl());
 		webHook.setEnabled(webHookConfig.getEnabled());
+		webHook.setHideSecureValues(webHookConfig.isHideSecureValues());
 		if (!webHookConfig.getEnabled()) {
 			webHook.getExecutionStats().setStatusReason(WebHookExecutionException.WEBHOOK_DISABLED_INFO_MESSAGE);
 			webHook.getExecutionStats().setStatusCode(WebHookExecutionException.WEBHOOK_DISABLED_INFO_CODE);

tcwebhooks-core/src/main/java/webhook/teamcity/WebHookTemplateParsingException.java

@@ -0,0 +1,18 @@
+package webhook.teamcity;
+
+import lombok.Getter;
+
+@Getter
+public class WebHookTemplateParsingException extends WebHookContentResolutionException {
+
+	private static final long serialVersionUID = -3373028723068101280L;
+
+	public WebHookTemplateParsingException(String message) {
+		super(message, WEBHOOK_TEMPLATE_PARSING_EXCEPTION_ERROR_CODE);
+	}
+
+	public WebHookTemplateParsingException(String message, int errorCode) {
+		super(message, errorCode);
+	}
+
+}

tcwebhooks-core/src/main/java/webhook/teamcity/executor/AbstractWebHookExecutor.java

@@ -72,7 +72,15 @@ public void run() {
 		} catch (WebHookExecutionException ex){
 			webhook.getExecutionStats().setErrored(true);
 			webhook.getExecutionStats().setRequestCompleted(ex.getErrorCode(), ex.getMessage());
-			Loggers.SERVER.error(CLASS_NAME + webhook.getExecutionStats().getTrackingIdAsString() + " :: " + ex.getMessage());
+			Loggers.SERVER.error(
+					String.format("%s trackingId: %s :: projectId: %s :: webhookId: %s :: templateId: %s, errorCode: %s, errorMessage: %s", 
+							CLASS_NAME, 
+							webhook.getExecutionStats().getTrackingIdAsString(),
+							whc.getProjectExternalId(),
+							whc.getUniqueKey(),
+							whc.getPayloadTemplate(),
+							ex.getErrorCode(),
+							ex.getMessage()));
 			Loggers.SERVER.debug(CLASS_NAME + webhook.getExecutionStats().getTrackingIdAsString() + " :: URL: " + webhook.getUrl(), ex);
 			this.webHookHistoryItem = buildWebHookHistoryItem(new WebHookErrorStatus(ex, ex.getMessage(), ex.getErrorCode()));
 			webHookHistoryRepository.addHistoryItem(webHookHistoryItem);
@@ -81,8 +89,15 @@ public void run() {
 		} catch (Exception ex){
 			webhook.getExecutionStats().setErrored(true);
 			webhook.getExecutionStats().setRequestCompleted(WebHookExecutionException.WEBHOOK_UNEXPECTED_EXCEPTION_ERROR_CODE, WebHookExecutionException.WEBHOOK_UNEXPECTED_EXCEPTION_MESSAGE + ex.getMessage());
-			Loggers.SERVER.error(CLASS_NAME + webhook.getExecutionStats().getTrackingIdAsString() + " :: " + ex.getMessage());
-			Loggers.SERVER.debug(CLASS_NAME + webhook.getExecutionStats().getTrackingIdAsString() + " :: URL: " + webhook.getUrl(), ex);
+			Loggers.SERVER.error(
+					String.format("%s trackingId: %s :: projectId: %s :: webhookId: %s :: templateId: %s, errorCode: %s, errorMessage: %s", 
+							CLASS_NAME, 
+							webhook.getExecutionStats().getTrackingIdAsString(),
+							whc.getProjectExternalId(),
+							whc.getUniqueKey(),
+							whc.getPayloadTemplate(),
+							WebHookExecutionException.WEBHOOK_UNEXPECTED_EXCEPTION_ERROR_CODE,
+							ex.getMessage()));			Loggers.SERVER.debug(CLASS_NAME + webhook.getExecutionStats().getTrackingIdAsString() + " :: URL: " + webhook.getUrl(), ex);
 			this.webHookHistoryItem = buildWebHookHistoryItem(new WebHookErrorStatus(ex, ex.getMessage(), 
 					WebHookExecutionException.WEBHOOK_UNEXPECTED_EXCEPTION_ERROR_CODE));
 			webHookHistoryRepository.addHistoryItem(this.webHookHistoryItem);
@@ -102,41 +117,54 @@ public void run() {
 	 * @param payloadTemplate
 	 */
 	public static void doPost(WebHook wh, String payloadTemplate) {
+		boolean shouldHideSecureData = wh.shouldHideSecureData();
 		try {
 			if (Boolean.TRUE.equals(wh.isEnabled())){
 				wh.post();
 				Loggers.SERVER.info(CLASS_NAME + " :: WebHook triggered : " 
-						+ wh.getUrl() + " using template " + payloadTemplate 
+						+ determineSecureUrl(wh, shouldHideSecureData) + " using template " + payloadTemplate 
 						+ " returned " + wh.getStatus() 
-						+ " " + wh.getErrorReason());	
-				Loggers.SERVER.debug(CLASS_NAME + ":doPost :: content dump: " + wh.getPayload());
-				if (Loggers.SERVER.isDebugEnabled()) Loggers.SERVER.debug("WebHook execution stats: " + wh.getExecutionStats().toString());
+						+ " " + wh.getErrorReason());
+				if (Loggers.SERVER.isDebugEnabled()) {
+					if (shouldHideSecureData) {
+						Loggers.SERVER.debug(CLASS_NAME + ":doPost :: Hiding content payload because it may contain secured values. To log content to this log file uncheck 'Secure Values' in the WebHook edit dialog.");
+					} else if (wh.getExecutionStats().isSecureValueAccessed()) {
+						Loggers.SERVER.debug(CLASS_NAME + ":doPost :: Logging content payload even though it may contain secured values. To hide content in this log file check 'Secure Values' in the WebHook edit dialog.\n--- begin webhook payload ---\n" + wh.getPayload() + "\n--- end webhook payload ---");
+						Loggers.SERVER.debug("WebHook execution stats: " + wh.getExecutionStats().toString());
+					} else {
+						Loggers.SERVER.debug(CLASS_NAME + ":doPost :: Logging content payload because it contains no secured values.\n--- begin webhook payload ---\n" + wh.getPayload() + "\n--- end webhook payload ---");
+					}
+				}
 				if (Boolean.TRUE.equals(wh.isErrored())){
 					Loggers.SERVER.error(wh.getErrorReason());
 				}
 				if (wh.getStatus() == null) {
-					Loggers.SERVER.warn(CLASS_NAME + wh.getParam("projectId") + " WebHook (url: " + wh.getUrl() + " proxy: " + wh.getProxyHost() + ":" + wh.getProxyPort()+") returned HTTP status " + wh.getStatus().toString());
+					Loggers.SERVER.warn(CLASS_NAME + wh.getParam("projectId") + " WebHook (url: " + determineSecureUrl(wh, shouldHideSecureData) + " proxy: " + wh.getProxyHost() + ":" + wh.getProxyPort()+") returned HTTP status " + wh.getStatus().toString());
 					throw new WebHookHttpExecutionException("WebHook endpoint returned null response code");
 				} else if (wh.getStatus() < HttpStatus.SC_OK || wh.getStatus() >= HttpStatus.SC_MULTIPLE_CHOICES) {
-					Loggers.SERVER.warn(CLASS_NAME + wh.getParam("projectId") + " WebHook (url: " + wh.getUrl() + " proxy: " + wh.getProxyHost() + ":" + wh.getProxyPort()+") returned HTTP status " + wh.getStatus().toString());
+					Loggers.SERVER.warn(CLASS_NAME + wh.getParam("projectId") + " WebHook (url: " + determineSecureUrl(wh, shouldHideSecureData) + " proxy: " + wh.getProxyHost() + ":" + wh.getProxyPort()+") returned HTTP status " + wh.getStatus().toString());
 					throw new WebHookHttpResponseException("WebHook endpoint returned non-2xx response (" + EnglishReasonPhraseCatalog.INSTANCE.getReason(wh.getStatus(), null) +")", wh.getStatus());
 				}
 			} else {
-				if (Loggers.SERVER.isDebugEnabled()) Loggers.SERVER.debug("WebHook NOT triggered: " + wh.getDisabledReason() + " " +  wh.getParam("buildStatus") + " " + wh.getUrl());	
+				if (Loggers.SERVER.isDebugEnabled()) Loggers.SERVER.debug("WebHook NOT triggered: " + wh.getDisabledReason() + " " +  wh.getParam("buildStatus") + " " + determineSecureUrl(wh, shouldHideSecureData));	
 			}
 		} catch (FileNotFoundException e) {
 			Loggers.SERVER.warn(CLASS_NAME + ":doPost :: " 
-					+ "A FileNotFoundException occurred while attempting to execute WebHook (" + wh.getUrl() + "). See the following debug stacktrace");
+					+ "A FileNotFoundException occurred while attempting to execute WebHook (" + determineSecureUrl(wh, shouldHideSecureData) + "). See the following debug stacktrace");
 			Loggers.SERVER.debug(e);
-			throw new WebHookHttpExecutionException("A FileNotFoundException occurred while attempting to execute WebHook (" + wh.getUrl() + ")", e);
+			throw new WebHookHttpExecutionException("A FileNotFoundException occurred while attempting to execute WebHook (" + determineSecureUrl(wh, shouldHideSecureData) + ")", e);
 		} catch (IOException e) {
 			Loggers.SERVER.warn(CLASS_NAME + ":doPost :: " 
-					+ "An IOException occurred while attempting to execute WebHook (" + wh.getUrl() + "). See the following debug stacktrace");
+					+ "An IOException occurred while attempting to execute WebHook (" + determineSecureUrl(wh, shouldHideSecureData) + "). See the following debug stacktrace");
 			Loggers.SERVER.debug(e);
 			throw new WebHookHttpExecutionException("Error " + e.getMessage() + " occurred while attempting to execute WebHook.", e);
 		}
 
 	}
+
+	private static String determineSecureUrl(WebHook wh, boolean shouldHideSecureData) {
+		return shouldHideSecureData ? "********" : wh.getUrl();
+	}
 	protected abstract WebHook getWebHookContent();
 	protected abstract WebHookHistoryItem buildWebHookHistoryItem(WebHookErrorStatus errorStatus);
 

tcwebhooks-core/src/main/java/webhook/teamcity/history/WebHookHistoryItemFactoryImpl.java

@@ -90,8 +90,13 @@ public WebHookHistoryItem getWebHookHistoryTestItem(WebHookConfig whc, WebHookEx
 
 	private void addGeneralisedWebAddress(WebHookConfig whc, WebHookHistoryItem item) {
 		try {
-			URL url = new URL(whc.getUrl());
-			item.setGeneralisedWebAddress(myWebAddressTransformer.getGeneralisedHostName(url));
+			if (item.getWebHookExecutionStats().getUrl() != null) {
+				URL url = new URL(item.getWebHookExecutionStats().getUrl());
+				item.setGeneralisedWebAddress(myWebAddressTransformer.getGeneralisedHostName(url));
+			} else {
+				URL url = new URL(whc.getUrl());
+				item.setGeneralisedWebAddress(myWebAddressTransformer.getGeneralisedHostName(url));
+			}
 		} catch (MalformedURLException ex) {
 			item.setGeneralisedWebAddress(null);
 		}