From d2fc0d5366b44667933aeded92a13340060daee2 Mon Sep 17 00:00:00 2001 From: Johnny Lim Date: Thu, 20 Feb 2020 14:09:50 +0900 Subject: [PATCH 1/3] Polish doc fixes gh-1557 --- docs/src/main/asciidoc/quickstart.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/main/asciidoc/quickstart.adoc b/docs/src/main/asciidoc/quickstart.adoc index 3c42b5cf..c0449227 100644 --- a/docs/src/main/asciidoc/quickstart.adoc +++ b/docs/src/main/asciidoc/quickstart.adoc @@ -147,6 +147,6 @@ $ curl localhost:8080/env } ---- -A property source called ```configService:/` contains the `foo` property with a value of `bar` and is highest priority. +A property source called `configService:/` contains the `foo` property with a value of `bar` and is the highest priority. NOTE: The URL in the property source name is the git repository, not the config server URL. From 4d6cf75452612ee05cb95c8fcb935a19b5ce7bf1 Mon Sep 17 00:00:00 2001 From: FWinkler79 <52044081+FWinkler79@users.noreply.github.com> Date: Wed, 15 Jan 2020 19:06:25 +0100 Subject: [PATCH 2/3] Allows overriding of configTokenProvider. Made sure configTokenProvider default bean will be overridden by more specific versions (e.g. vault configTokenProvider, etc.) Fixes gh-1485 Fixes gh-1537 --- .../EnvironmentRepositoryConfiguration.java | 19 ++---- ...vironmentRepositoryConfigurationTests.java | 64 +++++++++++++++++++ 2 files changed, 71 insertions(+), 12 deletions(-) create mode 100644 spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/config/EnvironmentRepositoryConfigurationTests.java diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/config/EnvironmentRepositoryConfiguration.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/config/EnvironmentRepositoryConfiguration.java index f67836c9..38262fa0 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/config/EnvironmentRepositoryConfiguration.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/config/EnvironmentRepositoryConfiguration.java @@ -125,6 +125,13 @@ public class EnvironmentRepositoryConfiguration { return new MultipleJGitEnvironmentProperties(); } + @Bean + @ConditionalOnMissingBean(ConfigTokenProvider.class) + public ConfigTokenProvider defaultConfigTokenProvider( + ObjectProvider httpRequest) { + return new HttpRequestConfigTokenProvider(httpRequest); + } + @Configuration(proxyBeanMethods = false) @ConditionalOnProperty("spring.cloud.config.server.consul.watch.enabled") protected static class ConsulEnvironmentWatchConfiguration { @@ -147,18 +154,6 @@ public class EnvironmentRepositoryConfiguration { } - @Configuration(proxyBeanMethods = false) - @ConditionalOnMissingBean(ConfigTokenProvider.class) - protected static class DefaultConfigTokenProvider { - - @Bean - public ConfigTokenProvider defaultConfigTokenProvider( - ObjectProvider httpRequest) { - return new HttpRequestConfigTokenProvider(httpRequest); - } - - } - @Configuration(proxyBeanMethods = false) @ConditionalOnClass(TransportConfigCallback.class) static class JGitFactoryConfig { diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/config/EnvironmentRepositoryConfigurationTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/config/EnvironmentRepositoryConfigurationTests.java new file mode 100644 index 00000000..f9555273 --- /dev/null +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/config/EnvironmentRepositoryConfigurationTests.java @@ -0,0 +1,64 @@ +/* + * Copyright 2018-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.config.server.config; + +import org.junit.Test; + +import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.test.context.TestConfiguration; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.cloud.config.server.environment.ConfigTokenProvider; +import org.springframework.cloud.config.server.environment.EnvironmentConfigTokenProvider; +import org.springframework.context.annotation.Bean; + +import static org.assertj.core.api.Assertions.assertThat; + +public class EnvironmentRepositoryConfigurationTests { + + @Test + public void configTokenProviderCanBeOverridden() { + new ApplicationContextRunner() + .withConfiguration(AutoConfigurations + .of(EnvironmentRepositoryConfiguration.class, TestBeans.class)) + .withPropertyValues("spring.profiles.active=composite", + "spring.cloud.config.server.vault.authentication=TOKEN", + "spring.cloud.config.server.vault.token=testTokenValue", + "spring.cloud.config.server.composite[0].type=vault", + "spring.cloud.config.server.composite[1].type=git", + "spring.cloud.config.server.composite[1].uri=https://test.com/Some-Test-Repo.git") + .run((context) -> { + assertThat(context.getBean(ConfigTokenProvider.class)).isNotNull(); + assertThat(context.getBean(ConfigTokenProvider.class)) + .isInstanceOf(EnvironmentConfigTokenProvider.class); + EnvironmentConfigTokenProvider tokenProvider = context + .getBean(EnvironmentConfigTokenProvider.class); + assertThat(tokenProvider.getToken()).isEqualTo("testTokenValue"); + }); + } + + @TestConfiguration + public static class TestBeans { + + @Bean + public ConfigServerProperties vaultConfigServerProperties() { + ConfigServerProperties configServerProperties = new ConfigServerProperties(); + return configServerProperties; + } + + } + +} From 651f458919c40ef9a5e93e7d76bf98575910fad0 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Wed, 26 Feb 2020 12:38:01 -0500 Subject: [PATCH 3/3] Consolidates normalization of names and labels. fixes gh-1561 --- .../ConfigServicePropertySourceLocator.java | 5 +-- .../cloud/config/environment/Environment.java | 31 ++++++++++++++ .../config/environment/EnvironmentTests.java | 42 +++++++++++++++++++ .../environment/EnvironmentController.java | 12 +----- .../resource/GenericResourceRepository.java | 38 +++++++++++++++++ .../server/resource/ResourceController.java | 26 ++---------- .../GenericResourceRepositoryTests.java | 24 ++++++++++- 7 files changed, 142 insertions(+), 36 deletions(-) create mode 100644 spring-cloud-config-client/src/test/java/org/springframework/cloud/config/environment/EnvironmentTests.java diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java index 8942086a..2e3d5061 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java @@ -233,9 +233,8 @@ public class ConfigServicePropertySourceLocator implements PropertySourceLocator Object[] args = new String[] { name, profile }; if (StringUtils.hasText(label)) { - if (label.contains("/")) { - label = label.replace("/", "(_)"); - } + // workaround for Spring MVC matching / in paths + label = Environment.denormalize(label); args = new String[] { name, profile, label }; path = path + "/{label}"; } diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/environment/Environment.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/environment/Environment.java index c711a191..070f0d24 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/environment/Environment.java +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/environment/Environment.java @@ -34,6 +34,11 @@ import com.fasterxml.jackson.annotation.JsonProperty; */ public class Environment { + /** + * "(_)" is uncommon in a git repo name, but "/" cannot be matched by Spring MVC. + */ + public static final String SLASH_PLACEHOLDER = "(_)"; + private String name; private String[] profiles = new String[0]; @@ -72,6 +77,32 @@ public class Environment { this.state = state; } + /** + * Utility method for normalizing names and labels. + * @param s String to normalize. + * @return if s contains (_), replace with slash. + */ + public static String normalize(String s) { + if (s != null && s.contains(SLASH_PLACEHOLDER)) { + // "(_)" is uncommon in a git repo name, but "/" cannot be matched + // by Spring MVC + return s.replace(SLASH_PLACEHOLDER, "/"); + } + return s; + } + + /** + * Utility method for denormalizing names and labels. + * @param s String to denormalize. + * @return if s contains slash, replace with (_). + */ + public static String denormalize(String s) { + if (s != null && s.contains("/")) { + return s.replace("/", SLASH_PLACEHOLDER); + } + return s; + } + public void add(PropertySource propertySource) { this.propertySources.add(propertySource); } diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/environment/EnvironmentTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/environment/EnvironmentTests.java new file mode 100644 index 00000000..e72b7453 --- /dev/null +++ b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/environment/EnvironmentTests.java @@ -0,0 +1,42 @@ +/* + * Copyright 2013-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.config.environment; + +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +public class EnvironmentTests { + + @Test + public void normalizeWorks() { + assertThat(Environment.normalize("abc123")).isEqualTo("abc123"); + assertThat(Environment.normalize("abc(_)123")).isEqualTo("abc/123"); + assertThat(Environment.normalize("abc(%5F)123")).isEqualTo("abc(%5F)123"); + assertThat(Environment.normalize("abc%28_%29123")).isEqualTo("abc%28_%29123"); + assertThat(Environment.normalize("abc%28%5F%29123")).isEqualTo("abc%28%5F%29123"); + } + + @Test + public void denormalizeWorks() { + assertThat(Environment.denormalize("abc123")).isEqualTo("abc123"); + assertThat(Environment.denormalize("abc/123")).isEqualTo("abc(_)123"); + assertThat(Environment.denormalize("abc%2F123")).isEqualTo("abc%2F123"); + assertThat(Environment.denormalize("abc%25F123")).isEqualTo("abc%25F123"); + } + +} diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/EnvironmentController.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/EnvironmentController.java index f5958a6c..1800c5c1 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/EnvironmentController.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/EnvironmentController.java @@ -131,16 +131,8 @@ public class EnvironmentController { public Environment getEnvironment(String name, String profiles, String label, boolean includeOrigin) { - if (name != null && name.contains("(_)")) { - // "(_)" is uncommon in a git repo name, but "/" cannot be matched - // by Spring MVC - name = name.replace("(_)", "/"); - } - if (label != null && label.contains("(_)")) { - // "(_)" is uncommon in a git branch name, but "/" cannot be matched - // by Spring MVC - label = label.replace("(_)", "/"); - } + name = Environment.normalize(name); + label = Environment.normalize(label); Environment environment = this.repository.findOne(name, profiles, label, includeOrigin); if (!this.acceptEmpty diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/GenericResourceRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/GenericResourceRepository.java index 47c035ad..747e824e 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/GenericResourceRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/GenericResourceRepository.java @@ -66,6 +66,9 @@ public class GenericResourceRepository try { for (int i = locations.length; i-- > 0;) { String location = locations[i]; + if (isInvalidEncodedLocation(location)) { + continue; + } for (String local : getProfilePaths(profile, path)) { if (!isInvalidPath(local) && !isInvalidEncodedPath(local)) { Resource file = this.resourceLoader.getResource(location) @@ -85,6 +88,41 @@ public class GenericResourceRepository throw new NoSuchResourceException("Not found: " + path); } + /** + * Check whether the given location contains invalid escape sequences. + * @param location the location to validate + * @return {@code true} if the path is invalid, {@code false} otherwise + */ + private boolean isInvalidEncodedLocation(String location) { + if (location.contains("%")) { + try { + // Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 + // chars + String decodedPath = URLDecoder.decode(location, "UTF-8"); + if (isInvalidLocation(decodedPath)) { + return true; + } + decodedPath = processPath(decodedPath); + if (isInvalidLocation(decodedPath)) { + return true; + } + } + catch (IllegalArgumentException | UnsupportedEncodingException ex) { + // Should never happen... + } + } + return isInvalidLocation(location); + } + + private boolean isInvalidLocation(String location) { + boolean isInvalid = location.contains(".."); + + if (isInvalid && logger.isWarnEnabled()) { + logger.warn("Location contains \"..\""); + } + return isInvalid; + } + private Collection getProfilePaths(String profiles, String path) { Set paths = new LinkedHashSet<>(); for (String profile : StringUtils.commaDelimitedListToSet(profiles)) { diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/ResourceController.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/ResourceController.java index d228bf44..986f36d7 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/ResourceController.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/resource/ResourceController.java @@ -135,8 +135,8 @@ public class ResourceController { synchronized String retrieve(ServletWebRequest request, String name, String profile, String label, String path, boolean resolvePlaceholders) throws IOException { - name = resolveName(name); - label = resolveLabel(label); + name = Environment.normalize(name); + label = Environment.normalize(label); Resource resource = this.resourceRepository.findOne(name, profile, label, path); if (checkNotModified(request, resource)) { // Content was not modified. Just return. @@ -199,8 +199,8 @@ public class ResourceController { private synchronized byte[] binary(ServletWebRequest request, String name, String profile, String label, String path) throws IOException { - name = resolveName(name); - label = resolveLabel(label); + name = Environment.normalize(name); + label = Environment.normalize(label); Resource resource = this.resourceRepository.findOne(name, profile, label, path); if (checkNotModified(request, resource)) { // Content was not modified. Just return. @@ -223,24 +223,6 @@ public class ResourceController { return false; } - private String resolveName(String name) { - if (name != null && name.contains("(_)")) { - // "(_)" is uncommon in a git repo name, but "/" cannot be matched - // by Spring MVC - name = name.replace("(_)", "/"); - } - return name; - } - - private String resolveLabel(String label) { - if (label != null && label.contains("(_)")) { - // "(_)" is uncommon in a git branch name, but "/" cannot be matched - // by Spring MVC - label = label.replace("(_)", "/"); - } - return label; - } - @ExceptionHandler(NoSuchResourceException.class) @ResponseStatus(HttpStatus.NOT_FOUND) public void notFound(NoSuchResourceException e) { diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/resource/GenericResourceRepositoryTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/resource/GenericResourceRepositoryTests.java index eba1bfc3..f333c0d7 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/resource/GenericResourceRepositoryTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/resource/GenericResourceRepositoryTests.java @@ -100,9 +100,31 @@ public class GenericResourceRepositoryTests { this.exception.expect(NoSuchResourceException.class); this.nativeRepository .setSearchLocations("file:./src/test/resources/test/{profile}"); - this.repository.findOne("blah", "local", "master", "..%2F..%2Fdata-jdbc.sql"); this.output.expect(containsString( "Path contains \"../\" after call to StringUtils#cleanPath")); + this.repository.findOne("blah", "local", "master", "..%2F..%2Fdata-jdbc.sql"); + } + + @Test + public void invalidPathWithPreviousDirectory() { + testInvalidPath("../"); + } + + @Test + public void invalidPathWithPreviousDirectoryEncodedSlash() { + testInvalidPath("..%2F"); + } + + @Test + public void invalidPathWithPreviousDirectoryAllEncoded() { + testInvalidPath("%2E%2E%2F"); + } + + private void testInvalidPath(String label) { + this.exception.expect(NoSuchResourceException.class); + this.nativeRepository.setSearchLocations("file:./src/test/resources/test/local"); + this.output.expect(containsString("Location contains \"..\"")); + this.repository.findOne("blah", "local", label, "foo.properties"); } }