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 8942086aa..2e3d50611 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 @@ private Environment getRemoteEnvironment(RestTemplate restTemplate, 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 c711a1912..070f0d242 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 @@ */ 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 Environment(@JsonProperty("name") String name, 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 000000000..e72b7453b --- /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 f5958a6ce..1800c5c12 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 Environment labelledIncludeOrigin(@PathVariable String name, 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 47c035add..747e824ee 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 synchronized Resource findOne(String application, String profile, String 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 synchronized Resource findOne(String application, String profile, String 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 d228bf440..986f36d77 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 @@ private String getFilePath(ServletWebRequest request, String name, String profil 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 @@ byte[] binary(String name, String profile, String label, String path) 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 @@ private boolean checkNotModified(ServletWebRequest request, Resource resource) { 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 eba1bfc3f..f333c0d7e 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 void invalidPath() { 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"); } }