SONAR-22274 set minimum length for webhook secret to 16 to improve security

5 files changed, 56 insertions, 11 deletions

diff --git a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/webhook/ws/CreateActionIT.java b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/webhook/ws/CreateActionIT.java
index 1b105778963..d6bcb1a716e 100644
--- a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/webhook/ws/CreateActionIT.java
+++ b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/webhook/ws/CreateActionIT.java
@@ -58,6 +58,8 @@ import static org.sonar.server.ws.KeyExamples.URL_WEBHOOK_EXAMPLE_001;
 
 public class CreateActionIT {
 
+  private static final String DEFAULT_COMPLIANT_SECRET = "at_least_16_characters";
+
   @Rule
   public UserSessionRule userSession = standalone();
 
@@ -90,7 +92,6 @@ public class CreateActionIT {
         tuple("name", true),
         tuple("url", true),
         tuple("secret", false));
-
   }
 
   @Test
@@ -104,7 +105,7 @@ public class CreateActionIT {
       .setParam("project", longProjectKey)
       .setParam("name", NAME_WEBHOOK_EXAMPLE_001)
       .setParam("url", URL_WEBHOOK_EXAMPLE_001)
-      .setParam("secret", "a_secret")
+      .setParam("secret", DEFAULT_COMPLIANT_SECRET)
       .executeProtobuf(CreateWsResponse.class);
 
     assertThat(response.getWebhook()).isNotNull();
@@ -121,7 +122,7 @@ public class CreateActionIT {
     CreateWsResponse response = wsActionTester.newRequest()
       .setParam("name", NAME_WEBHOOK_EXAMPLE_001)
       .setParam("url", URL_WEBHOOK_EXAMPLE_001)
-      .setParam("secret", "a_secret")
+      .setParam("secret", DEFAULT_COMPLIANT_SECRET)
       .executeProtobuf(CreateWsResponse.class);
 
     assertThat(response.getWebhook()).isNotNull();
@@ -136,7 +137,7 @@ public class CreateActionIT {
         assertThat(reloaded.getName()).isEqualTo(NAME_WEBHOOK_EXAMPLE_001);
         assertThat(reloaded.getUrl()).isEqualTo(URL_WEBHOOK_EXAMPLE_001);
         assertThat(reloaded.getProjectUuid()).isNull();
-        assertThat(reloaded.getSecret()).isEqualTo("a_secret");
+        assertThat(reloaded.getSecret()).isEqualTo(DEFAULT_COMPLIANT_SECRET);
       });
   }
 
@@ -232,6 +233,18 @@ public class CreateActionIT {
   }
 
   @Test
+  public void execute_whenSecretIsTooShort_fail() {
+    userSession.logIn().addPermission(GlobalPermission.ADMINISTER);
+    TestRequest request = wsActionTester.newRequest()
+      .setParam(NAME_PARAM, NAME_WEBHOOK_EXAMPLE_001)
+      .setParam(URL_PARAM, URL_WEBHOOK_EXAMPLE_001)
+      .setParam("secret", "short");
+
+    assertThatThrownBy(request::execute)
+      .isInstanceOf(IllegalArgumentException.class);
+  }
+
+  @Test
   public void fail_if_credential_in_url_is_have_a_wrong_format() {
     userSession.logIn().addPermission(GlobalPermission.ADMINISTER);
     TestRequest request = wsActionTester.newRequest()
@@ -286,7 +299,7 @@ public class CreateActionIT {
       .setParam("project", longProjectKey)
       .setParam("name", NAME_WEBHOOK_EXAMPLE_001)
       .setParam("url", URL_WEBHOOK_EXAMPLE_001)
-      .setParam("secret", "a_secret");
+      .setParam("secret", DEFAULT_COMPLIANT_SECRET);
 
     assertThatThrownBy(() -> request.executeProtobuf(CreateWsResponse.class))
       .isInstanceOf(IllegalArgumentException.class)
diff --git a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/webhook/ws/UpdateActionIT.java b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/webhook/ws/UpdateActionIT.java
index af82e4b82ae..b6ef5454bde 100644
--- a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/webhook/ws/UpdateActionIT.java
+++ b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/webhook/ws/UpdateActionIT.java
@@ -54,6 +54,8 @@ import static org.sonar.server.ws.KeyExamples.URL_WEBHOOK_EXAMPLE_001;
 
 public class UpdateActionIT {
 
+  private static final String DEFAULT_COMPLIANT_SECRET = "at_least_16_characters";
+
   @Rule
   public UserSessionRule userSession = standalone();
 
@@ -126,7 +128,7 @@ public class UpdateActionIT {
     assertThat(reloaded.get().getName()).isEqualTo(NAME_WEBHOOK_EXAMPLE_001);
     assertThat(reloaded.get().getUrl()).isEqualTo(URL_WEBHOOK_EXAMPLE_001);
     assertThat(reloaded.get().getProjectUuid()).isEqualTo(dto.getProjectUuid());
-    assertThat(reloaded.get().getSecret()).isEqualTo(null);
+    assertThat(reloaded.get().getSecret()).isNull();
   }
 
   @Test
@@ -139,7 +141,7 @@ public class UpdateActionIT {
       .setParam("webhook", dto.getUuid())
       .setParam("name", NAME_WEBHOOK_EXAMPLE_001)
       .setParam("url", URL_WEBHOOK_EXAMPLE_001)
-      .setParam("secret", "a_new_secret")
+      .setParam("secret", DEFAULT_COMPLIANT_SECRET)
       .execute();
 
     assertThat(response.getStatus()).isEqualTo(HTTP_NO_CONTENT);
@@ -148,7 +150,7 @@ public class UpdateActionIT {
     assertThat(reloaded.get().getName()).isEqualTo(NAME_WEBHOOK_EXAMPLE_001);
     assertThat(reloaded.get().getUrl()).isEqualTo(URL_WEBHOOK_EXAMPLE_001);
     assertThat(reloaded.get().getProjectUuid()).isEqualTo(dto.getProjectUuid());
-    assertThat(reloaded.get().getSecret()).isEqualTo("a_new_secret");
+    assertThat(reloaded.get().getSecret()).isEqualTo(DEFAULT_COMPLIANT_SECRET);
   }
 
   @Test
@@ -160,7 +162,7 @@ public class UpdateActionIT {
       .setParam("webhook", dto.getUuid())
       .setParam("name", NAME_WEBHOOK_EXAMPLE_001)
       .setParam("url", URL_WEBHOOK_EXAMPLE_001)
-      .setParam("secret", "a_new_secret")
+      .setParam("secret", DEFAULT_COMPLIANT_SECRET)
       .execute();
 
     assertThat(response.getStatus()).isEqualTo(HTTP_NO_CONTENT);
@@ -169,7 +171,7 @@ public class UpdateActionIT {
     assertThat(reloaded.get().getName()).isEqualTo(NAME_WEBHOOK_EXAMPLE_001);
     assertThat(reloaded.get().getUrl()).isEqualTo(URL_WEBHOOK_EXAMPLE_001);
     assertThat(reloaded.get().getProjectUuid()).isNull();
-    assertThat(reloaded.get().getSecret()).isEqualTo("a_new_secret");
+    assertThat(reloaded.get().getSecret()).isEqualTo(DEFAULT_COMPLIANT_SECRET);
   }
 
   @Test
@@ -242,6 +244,21 @@ public class UpdateActionIT {
   }
 
   @Test
+  public void handle_whenSecretIsTooShort_fail() {
+    ProjectDto project = componentDbTester.insertPrivateProject().getProjectDto();
+    WebhookDto dto = webhookDbTester.insertWebhook(project);
+    userSession.logIn().addProjectPermission(UserRole.ADMIN, project);
+    TestRequest request = wsActionTester.newRequest()
+      .setParam("webhook", dto.getUuid())
+      .setParam("name", NAME_WEBHOOK_EXAMPLE_001)
+      .setParam("url", URL_WEBHOOK_EXAMPLE_001)
+      .setParam("secret", "short");
+
+    assertThatThrownBy(request::execute)
+      .isInstanceOf(IllegalArgumentException.class);
+  }
+
+  @Test
   public void fail_if_credential_in_url_is_have_a_wrong_format() {
     ProjectDto project = componentDbTester.insertPrivateProject().getProjectDto();
     WebhookDto dto = webhookDbTester.insertWebhook(project);
diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/CreateAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/CreateAction.java
index c987f014370..0866066c829 100644
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/CreateAction.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/CreateAction.java
@@ -20,6 +20,7 @@
 package org.sonar.server.webhook.ws;
 
 import javax.annotation.Nullable;
+import org.sonar.api.server.ws.Change;
 import org.sonar.api.server.ws.Request;
 import org.sonar.api.server.ws.Response;
 import org.sonar.api.server.ws.WebService;
@@ -40,6 +41,7 @@ import static org.sonar.server.webhook.ws.WebhooksWsParameters.PROJECT_KEY_PARAM
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.PROJECT_KEY_PARAM_MAXIMUM_LENGTH;
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.SECRET_PARAM;
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.SECRET_PARAM_MAXIMUM_LENGTH;
+import static org.sonar.server.webhook.ws.WebhooksWsParameters.SECRET_PARAM_MINIMUM_LENGTH;
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.URL_PARAM;
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.URL_PARAM_MAXIMUM_LENGTH;
 import static org.sonar.server.ws.KeyExamples.KEY_PROJECT_EXAMPLE_001;
@@ -74,6 +76,7 @@ public class CreateAction implements WebhooksWsAction {
       .setDescription("Create a Webhook.<br>" +
         "Requires 'Administer' permission on the specified project, or global 'Administer' permission.")
       .setSince("7.1")
+      .setChangelog(new Change("10.6", "The minimum length of parameter '" + SECRET_PARAM + "' increased to 16."))
       .setResponseExample(getClass().getResource("example-webhook-create.json"))
       .setHandler(this);
 
@@ -99,7 +102,7 @@ public class CreateAction implements WebhooksWsAction {
 
     action.createParam(SECRET_PARAM)
       .setRequired(false)
-      .setMinimumLength(1)
+      .setMinimumLength(SECRET_PARAM_MINIMUM_LENGTH)
       .setMaximumLength(SECRET_PARAM_MAXIMUM_LENGTH)
       .setDescription("If provided, secret will be used as the key to generate the HMAC hex (lowercase) digest value in the 'X-Sonar-Webhook-HMAC-SHA256' header")
       .setExampleValue("your_secret")
diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/UpdateAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/UpdateAction.java
index b146c73fa3a..0776af931b1 100644
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/UpdateAction.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/UpdateAction.java
@@ -39,12 +39,14 @@ import static org.sonar.server.webhook.ws.WebhooksWsParameters.NAME_PARAM;
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.NAME_PARAM_MAXIMUM_LENGTH;
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.SECRET_PARAM;
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.SECRET_PARAM_MAXIMUM_LENGTH;
+import static org.sonar.server.webhook.ws.WebhooksWsParameters.SECRET_PARAM_MINIMUM_LENGTH;
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.UPDATE_ACTION;
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.URL_PARAM;
 import static org.sonar.server.webhook.ws.WebhooksWsParameters.URL_PARAM_MAXIMUM_LENGTH;
 import static org.sonar.server.ws.KeyExamples.KEY_PROJECT_EXAMPLE_001;
 import static org.sonar.server.ws.KeyExamples.NAME_WEBHOOK_EXAMPLE_001;
 import static org.sonar.server.ws.KeyExamples.URL_WEBHOOK_EXAMPLE_001;
+import static org.sonarqube.ws.WsUtils.checkArgument;
 
 public class UpdateAction implements WebhooksWsAction {
 
@@ -106,6 +108,8 @@ public class UpdateAction implements WebhooksWsAction {
     String url = request.mandatoryParam(URL_PARAM);
     String secret = request.param(SECRET_PARAM);
 
+    validateSecretLength(secret);
+
     webhookSupport.checkUrlPattern(url, "Url parameter with value '%s' is not a valid url", url);
 
     try (DbSession dbSession = dbClient.openSession(false)) {
@@ -129,6 +133,13 @@ public class UpdateAction implements WebhooksWsAction {
     response.noContent();
   }
 
+  private static void validateSecretLength(@Nullable String secret) {
+    if (secret != null && !secret.isEmpty()) {
+      checkArgument(secret.length() >= SECRET_PARAM_MINIMUM_LENGTH && secret.length() <= SECRET_PARAM_MAXIMUM_LENGTH,
+        "Secret length must between %s and %s characters", SECRET_PARAM_MINIMUM_LENGTH, SECRET_PARAM_MAXIMUM_LENGTH);
+    }
+  }
+
   private void updateWebhook(DbSession dbSession, WebhookDto dto, String name, String url, @Nullable String secret,
     @Nullable String projectKey, @Nullable String projectName) {
     dto
diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/WebhooksWsParameters.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/WebhooksWsParameters.java
index 14f240bb23b..461f0535870 100644
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/WebhooksWsParameters.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/WebhooksWsParameters.java
@@ -37,6 +37,7 @@ class WebhooksWsParameters {
   static final int KEY_PARAM_MAXIMUM_LENGTH = 40;
   static final String SECRET_PARAM = "secret";
   static final int SECRET_PARAM_MAXIMUM_LENGTH = 200;
+  static final int SECRET_PARAM_MINIMUM_LENGTH = 16;
 
   private WebhooksWsParameters() {
     // prevent instantiation