From 3727b06cb2d337f6f9f3d3f4936713b17ec1e564 Mon Sep 17 00:00:00 2001 From: Antoine Vigneau Date: Sat, 8 Jun 2024 07:32:41 +0200 Subject: [PATCH] SONAR-22363 Fix SSF-572 --- .../config/GitlabConfigurationServiceIT.java | 30 +++++++++++++++++++ .../org/sonar/server/common/UpdatedValue.java | 8 +++-- .../config/GitlabConfigurationService.java | 8 +++++ .../sonar/server/setting/ws/SetActionIT.java | 10 +++++++ .../sonar/server/setting/ws/SetAction.java | 12 ++++++-- 5 files changed, 64 insertions(+), 4 deletions(-) diff --git a/server/sonar-webserver-common/src/it/java/org/sonar/server/common/gitlab/config/GitlabConfigurationServiceIT.java b/server/sonar-webserver-common/src/it/java/org/sonar/server/common/gitlab/config/GitlabConfigurationServiceIT.java index d71cc0406af..5c01f93e509 100644 --- a/server/sonar-webserver-common/src/it/java/org/sonar/server/common/gitlab/config/GitlabConfigurationServiceIT.java +++ b/server/sonar-webserver-common/src/it/java/org/sonar/server/common/gitlab/config/GitlabConfigurationServiceIT.java @@ -295,6 +295,36 @@ public class GitlabConfigurationServiceIT { verify(managedInstanceService, times(0)).queueSynchronisationTask(); } + @Test + public void updateConfiguration_whenURLChangesWithoutSecret_shouldFail() { + gitlabConfigurationService.createConfiguration(buildGitlabConfiguration(JIT)); + + UpdateGitlabConfigurationRequest updateUrlRequest = builder() + .gitlabConfigurationId(UNIQUE_GITLAB_CONFIGURATION_ID) + .url(withValueOrThrow("http://malicious.url")) + .build(); + + assertThatThrownBy(() -> gitlabConfigurationService.updateConfiguration(updateUrlRequest)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("For security reasons, the URL can't be updated without providing the secret."); + } + + @Test + public void updateConfiguration_whenURLChangesWithAllSecrets_shouldUpdate() { + gitlabConfigurationService.createConfiguration(buildGitlabConfiguration(JIT)); + + UpdateGitlabConfigurationRequest updateUrlRequest = builder() + .gitlabConfigurationId(UNIQUE_GITLAB_CONFIGURATION_ID) + .url(withValueOrThrow("http://new.url")) + .secret(withValueOrThrow("new_secret")) + .build(); + + gitlabConfigurationService.updateConfiguration(updateUrlRequest); + + verifySettingWasSet(GITLAB_AUTH_URL, "http://new.url"); + verifySettingWasSet(GITLAB_AUTH_SECRET, "new_secret"); + } + private static void assertConfigurationFields(GitlabConfiguration configuration) { assertThat(configuration).isNotNull(); assertThat(configuration.id()).isEqualTo("gitlab-configuration"); diff --git a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/UpdatedValue.java b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/UpdatedValue.java index 0b4c6e420bb..c2aa53142e7 100644 --- a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/UpdatedValue.java +++ b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/UpdatedValue.java @@ -19,11 +19,11 @@ */ package org.sonar.server.common; -import java.util.StringJoiner; -import javax.annotation.Nullable; import java.util.Objects; +import java.util.StringJoiner; import java.util.function.Consumer; import java.util.function.Function; +import javax.annotation.Nullable; public class UpdatedValue { final T value; @@ -62,6 +62,10 @@ public class UpdatedValue { return false; } + public boolean isDefined() { + return isDefined; + } + @Override public boolean equals(Object o) { if (this == o) { diff --git a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/gitlab/config/GitlabConfigurationService.java b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/gitlab/config/GitlabConfigurationService.java index 52521d0d765..40126853aa0 100644 --- a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/gitlab/config/GitlabConfigurationService.java +++ b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/gitlab/config/GitlabConfigurationService.java @@ -42,6 +42,7 @@ import static java.lang.String.format; import static org.apache.commons.lang3.StringUtils.isNotBlank; import static org.sonar.alm.client.gitlab.GitlabGlobalSettingsValidator.ValidationMode.AUTH_ONLY; import static org.sonar.alm.client.gitlab.GitlabGlobalSettingsValidator.ValidationMode.COMPLETE; +import static org.sonar.api.utils.Preconditions.checkArgument; import static org.sonar.api.utils.Preconditions.checkState; import static org.sonar.auth.gitlab.GitLabSettings.GITLAB_AUTH_ALLOWED_GROUPS; import static org.sonar.auth.gitlab.GitLabSettings.GITLAB_AUTH_ALLOW_USERS_TO_SIGNUP; @@ -86,6 +87,7 @@ public class GitlabConfigurationService { public GitlabConfiguration updateConfiguration(UpdateGitlabConfigurationRequest updateRequest) { UpdatedValue provisioningEnabled = updateRequest.provisioningType().map(GitlabConfigurationService::shouldEnableAutoProvisioning); + throwIfUrlIsUpdatedWithoutSecrets(updateRequest); try (DbSession dbSession = dbClient.openSession(true)) { throwIfConfigurationDoesntExist(dbSession); GitlabConfiguration currentConfiguration = getConfiguration(updateRequest.gitlabConfigurationId(), dbSession); @@ -114,6 +116,12 @@ public class GitlabConfigurationService { } } + private static void throwIfUrlIsUpdatedWithoutSecrets(UpdateGitlabConfigurationRequest request) { + if (request.url().isDefined()) { + checkArgument(request.secret().isDefined(), "For security reasons, the URL can't be updated without providing the secret."); + } + } + private void setIfDefined(DbSession dbSession, String propertyName, UpdatedValue value) { value .map(definedValue -> new PropertyDto().setKey(propertyName).setValue(definedValue)) diff --git a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/setting/ws/SetActionIT.java b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/setting/ws/SetActionIT.java index 314851cca4f..e61fd9d52eb 100644 --- a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/setting/ws/SetActionIT.java +++ b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/setting/ws/SetActionIT.java @@ -1128,6 +1128,16 @@ public class SetActionIT { .hasMessage(format("Setting '%s' can only be used in sonar.properties", settingKey)); } + @Test + public void fail_when_setting_key_is_forbidden() { + TestRequest testRequest = ws.newRequest() + .setParam("key", "sonar.auth.gitlab.url") + .setParam("value", "http://malicious.url"); + assertThatThrownBy(testRequest::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("For security reasons, the key 'sonar.auth.gitlab.url' cannot be updated using this webservice. Please use the API v2"); + } + @Test public void definition() { WebService.Action definition = ws.getDef(); diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SetAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SetAction.java index 5a87916e21d..034a2478dcd 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SetAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SetAction.java @@ -69,8 +69,9 @@ public class SetAction implements SettingsWsAction { private static final Collector COMMA_JOINER = Collectors.joining(","); private static final String MSG_NO_EMPTY_VALUE = "A non empty value must be provided"; private static final int VALUE_MAXIMUM_LENGTH = 4000; - private static final TypeToken> MAP_TYPE_TOKEN = new TypeToken<>() { - }; + private static final TypeToken> MAP_TYPE_TOKEN = new TypeToken<>() {}; + private static final Set FORBIDDEN_KEYS = Set.of("sonar.auth.gitlab.url"); + private final PropertyDefinitions propertyDefinitions; private final DbClient dbClient; @@ -138,12 +139,19 @@ public class SetAction implements SettingsWsAction { public void handle(Request request, Response response) throws Exception { try (DbSession dbSession = dbClient.openSession(false)) { SetRequest wsRequest = toWsRequest(request); + throwIfForbiddenKey(wsRequest.getKey()); SettingsWsSupport.validateKey(wsRequest.getKey()); doHandle(dbSession, wsRequest); } response.noContent(); } + private static void throwIfForbiddenKey(String key) { + if (FORBIDDEN_KEYS.contains(key)) { + throw new IllegalArgumentException(format("For security reasons, the key '%s' cannot be updated using this webservice. Please use the API v2", key)); + } + } + private void doHandle(DbSession dbSession, SetRequest request) { Optional component = searchEntity(dbSession, request); String projectKey = component.map(EntityDto::getKey).orElse(null); -- 2.39.5