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

8 files changed, 63 insertions, 14 deletions

diff --git a/server/sonar-web/src/main/js/apps/webhooks/components/CreateWebhookForm.tsx b/server/sonar-web/src/main/js/apps/webhooks/components/CreateWebhookForm.tsx
index 7fa06f47afd..f3f1a4fd358 100644
--- a/server/sonar-web/src/main/js/apps/webhooks/components/CreateWebhookForm.tsx
+++ b/server/sonar-web/src/main/js/apps/webhooks/components/CreateWebhookForm.tsx
@@ -49,7 +49,7 @@ export default function CreateWebhookForm({ webhook, onClose, onDone }: Props) {
     } else if (!isWebUri(url)) {
       errors.url = translate('webhooks.url.bad_format');
     }
-    if (secret && secret.length > 200) {
+    if (secret && (secret.length > 200 || secret.length < 16)) {
       errors.secret = translate('webhooks.secret.bad_format');
     }
     return errors;
diff --git a/server/sonar-web/src/main/js/apps/webhooks/components/__tests__/App-it.tsx b/server/sonar-web/src/main/js/apps/webhooks/components/__tests__/App-it.tsx
index afd67cec2ef..0e2bcf81f78 100644
--- a/server/sonar-web/src/main/js/apps/webhooks/components/__tests__/App-it.tsx
+++ b/server/sonar-web/src/main/js/apps/webhooks/components/__tests__/App-it.tsx
@@ -129,7 +129,11 @@ describe('webhook CRUD', () => {
     await ui.waitForWebhooksLoaded();
 
     await ui.clickWebhookRowAction(1, 'Global webhook 2', 'update_verb', 'menuitem');
-    await ui.fillUpdateForm('modified-webhook', 'https://webhook.example.sonarqube.com', 'secret');
+    await ui.fillUpdateForm(
+      'modified-webhook',
+      'https://webhook.example.sonarqube.com',
+      'at_least_16_characters',
+    );
     await ui.submitForm();
 
     ui.checkWebhookRow(1, {
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
diff --git a/sonar-core/src/main/resources/org/sonar/l10n/core.properties b/sonar-core/src/main/resources/org/sonar/l10n/core.properties
index 8bb6653c613..85f3fdb0fed 100644
--- a/sonar-core/src/main/resources/org/sonar/l10n/core.properties
+++ b/sonar-core/src/main/resources/org/sonar/l10n/core.properties
@@ -5389,7 +5389,7 @@ webhooks.no_result=No webhook defined.
 webhooks.update=Update Webhook
 webhooks.secret=Secret
 webhooks.secret_header=Has secret?
-webhooks.secret.bad_format=Secret must have a maximum length of 200 characters
+webhooks.secret.bad_format=Secret length must be between 16 and 200 characters.
 webhooks.secret.description=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.
 webhooks.secret.description.update=If blank, any secret previously configured will be removed. If not set, the secret will remain unchanged.
 webhooks.secret.field_mask.description=Hidden for security reasons: {link}.