Ensure DefaultPathSegment does not allow parameters to be mutated

Prior to this commit, if a PathContainer was created using
Options.MESSAGE_ROUTE, DefaultPathSegment#parameters() returned a
mutable map which would allow the user to modify the contents of the
static, shared EMPTY_PARAMS map in DefaultPathContainer.

This commit prevents corruption of the shared EMPTY_PARAMS map by
ensuring that parameters stored in DefaultPathSegment are always
immutable.

Closes gh-27064

spring-web/src/main/java/org/springframework/http/server/DefaultPathContainer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2020 the original author or authors.
+ * Copyright 2002-2021 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.
@@ -36,12 +36,11 @@ import org.springframework.util.StringUtils;
  * Default implementation of {@link PathContainer}.
  *
  * @author Rossen Stoyanchev
+ * @author Sam Brannen
  * @since 5.0
  */
 final class DefaultPathContainer implements PathContainer {
 
-	private static final MultiValueMap<String, String> EMPTY_PARAMS = new LinkedMultiValueMap<>();
-
 	private static final PathContainer EMPTY_PATH = new DefaultPathContainer("", Collections.emptyList());
 
 	private static final Map<Character, DefaultSeparator> SEPARATORS = new HashMap<>(2);
@@ -120,7 +119,7 @@ final class DefaultPathContainer implements PathContainer {
 			if (!segment.isEmpty()) {
 				elements.add(options.shouldDecodeAndParseSegments() ?
 						decodeAndParsePathSegment(segment) :
-						new DefaultPathSegment(segment, separatorElement));
+						DefaultPathSegment.from(segment, separatorElement));
 			}
 			if (end == -1) {
 				break;
@@ -136,13 +135,13 @@ final class DefaultPathContainer implements PathContainer {
 		int index = segment.indexOf(';');
 		if (index == -1) {
 			String valueToMatch = StringUtils.uriDecode(segment, charset);
-			return new DefaultPathSegment(segment, valueToMatch, EMPTY_PARAMS);
+			return DefaultPathSegment.from(segment, valueToMatch);
 		}
 		else {
 			String valueToMatch = StringUtils.uriDecode(segment.substring(0, index), charset);
 			String pathParameterContent = segment.substring(index);
 			MultiValueMap<String, String> parameters = parsePathParams(pathParameterContent, charset);
-			return new DefaultPathSegment(segment, valueToMatch, parameters);
+			return DefaultPathSegment.from(segment, valueToMatch, parameters);
 		}
 	}
 
@@ -226,7 +225,10 @@ final class DefaultPathContainer implements PathContainer {
 	}
 
 
-	private static class DefaultPathSegment implements PathSegment {
+	private static final class DefaultPathSegment implements PathSegment {
+
+		private static final MultiValueMap<String, String> EMPTY_PARAMS =
+				CollectionUtils.unmodifiableMultiValueMap(new LinkedMultiValueMap<>());
 
 		private final String value;
 
@@ -234,24 +236,33 @@ final class DefaultPathContainer implements PathContainer {
 
 		private final MultiValueMap<String, String> parameters;
 
-
 		/**
-		 * Constructor for decoded and parsed segments.
+		 * Factory for segments without decoding and parsing.
 		 */
-		DefaultPathSegment(String value, String valueToMatch, MultiValueMap<String, String> params) {
+		static DefaultPathSegment from(String value, DefaultSeparator separator) {
-			this.value = value;
+			String valueToMatch = value.contains(separator.encodedSequence()) ?
-			this.valueToMatch = valueToMatch;
+					value.replaceAll(separator.encodedSequence(), separator.value()) : value;
-			this.parameters = CollectionUtils.unmodifiableMultiValueMap(params);
+			return from(value, valueToMatch);
 		}
 
 		/**
-		 * Constructor for segments without decoding and parsing.
+		 * Factory for decoded and parsed segments.
 		 */
-		DefaultPathSegment(String value, DefaultSeparator separator) {
+		static DefaultPathSegment from(String value, String valueToMatch) {
+			return new DefaultPathSegment(value, valueToMatch, EMPTY_PARAMS);
+		}
+
+		/**
+		 * Factory for decoded and parsed segments.
+		 */
+		static DefaultPathSegment from(String value, String valueToMatch, MultiValueMap<String, String> params) {
+			return new DefaultPathSegment(value, valueToMatch, CollectionUtils.unmodifiableMultiValueMap(params));
+		}
+
+		private DefaultPathSegment(String value, String valueToMatch, MultiValueMap<String, String> params) {
 			this.value = value;
-			this.valueToMatch = value.contains(separator.encodedSequence()) ?
+			this.valueToMatch = valueToMatch;
-					value.replaceAll(separator.encodedSequence(), separator.value()) : value;
+			this.parameters = params;
-			this.parameters = EMPTY_PARAMS;
 		}
 
 

spring-web/src/main/java/org/springframework/http/server/PathContainer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2021 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.
@@ -124,6 +124,7 @@ public interface PathContainer {
 
 		/**
 		 * Path parameters associated with this path segment.
+		 * @return an unmodifiable map containing the parameters
 		 */
 		MultiValueMap<String, String> parameters();
 	}
@@ -136,15 +137,16 @@ public interface PathContainer {
 	class Options {
 
 		/**
-		 * Options for HTTP URL paths:
+		 * Options for HTTP URL paths.
-		 * <p>Separator '/' with URL decoding and parsing of path params.
+		 * <p>Separator '/' with URL decoding and parsing of path parameters.
 		 */
 		public final static Options HTTP_PATH = Options.create('/', true);
 
 		/**
-		 * Options for a message route:
+		 * Options for a message route.
-		 * <p>Separator '.' without URL decoding nor parsing of params. Escape
+		 * <p>Separator '.' with neither URL decoding nor parsing of path parameters.
-		 * sequences for the separator char in segment values are still decoded.
+		 * Escape sequences for the separator character in segment values are still
+		 * decoded.
 		 */
 		public final static Options MESSAGE_ROUTE = Options.create('.', false);
 

spring-web/src/test/java/org/springframework/http/server/DefaultPathContainerTests.java

@@ -20,11 +20,14 @@ import java.util.stream.Stream;
 
 import org.junit.jupiter.api.Test;
 
+import org.springframework.http.server.PathContainer.Element;
+import org.springframework.http.server.PathContainer.Options;
 import org.springframework.http.server.PathContainer.PathSegment;
 import org.springframework.util.LinkedMultiValueMap;
 import org.springframework.util.MultiValueMap;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 
 /**
  * Unit tests for {@link DefaultPathContainer}.
@@ -37,20 +40,20 @@ class DefaultPathContainerTests {
 	@Test
 	void pathSegment() {
 		// basic
-		testPathSegment("cars", "cars", new LinkedMultiValueMap<>());
+		testPathSegment("cars", "cars", emptyMap());
 
 		// empty
-		testPathSegment("", "", new LinkedMultiValueMap<>());
+		testPathSegment("", "", emptyMap());
 
 		// spaces
-		testPathSegment("%20%20", "  ", new LinkedMultiValueMap<>());
+		testPathSegment("%20%20", "  ", emptyMap());
-		testPathSegment("%20a%20", " a ", new LinkedMultiValueMap<>());
+		testPathSegment("%20a%20", " a ", emptyMap());
 	}
 
 	@Test
 	void pathSegmentParams() {
 		// basic
-		LinkedMultiValueMap<String, String> params = new LinkedMultiValueMap<>();
+		LinkedMultiValueMap<String, String> params = emptyMap();
 		params.add("colors", "red");
 		params.add("colors", "blue");
 		params.add("colors", "green");
@@ -58,21 +61,45 @@ class DefaultPathContainerTests {
 		testPathSegment("cars;colors=red,blue,green;year=2012", "cars", params);
 
 		// trailing semicolon
-		params = new LinkedMultiValueMap<>();
+		params = emptyMap();
 		params.add("p", "1");
 		testPathSegment("path;p=1;", "path", params);
 
 		// params with spaces
-		params = new LinkedMultiValueMap<>();
+		params = emptyMap();
 		params.add("param name", "param value");
 		testPathSegment("path;param%20name=param%20value;%20", "path", params);
 
 		// empty params
-		params = new LinkedMultiValueMap<>();
+		params = emptyMap();
 		params.add("p", "1");
 		testPathSegment("path;;;%20;%20;p=1;%20", "path", params);
 	}
 
+	@Test
+	void pathSegmentParamsAreImmutable() {
+		assertPathSegmentParamsAreImmutable("cars", emptyMap(), Options.HTTP_PATH);
+
+		LinkedMultiValueMap<String, String> params = emptyMap();
+		params.add("colors", "red");
+		params.add("colors", "blue");
+		params.add("colors", "green");
+		assertPathSegmentParamsAreImmutable(";colors=red,blue,green", params, Options.HTTP_PATH);
+
+		assertPathSegmentParamsAreImmutable(";colors=red,blue,green", emptyMap(), Options.MESSAGE_ROUTE);
+	}
+
+	private void assertPathSegmentParamsAreImmutable(String path, LinkedMultiValueMap<String, String> params, Options options) {
+		PathContainer container = PathContainer.parsePath(path, options);
+		assertThat(container.elements()).hasSize(1);
+
+		PathSegment segment = (PathSegment) container.elements().get(0);
+		MultiValueMap<String, String> segmentParams = segment.parameters();
+		assertThat(segmentParams).isEqualTo(params);
+		assertThatExceptionOfType(UnsupportedOperationException.class)
+			.isThrownBy(() -> segmentParams.add("enigma", "boom"));
+	}
+
 	private void testPathSegment(String rawValue, String valueToMatch, MultiValueMap<String, String> params) {
 		PathContainer container = PathContainer.parsePath(rawValue);
 
@@ -111,10 +138,10 @@ class DefaultPathContainerTests {
 	}
 
 	private void testPath(String input, String value, String... expectedElements) {
-		PathContainer path = PathContainer.parsePath(input, PathContainer.Options.HTTP_PATH);
+		PathContainer path = PathContainer.parsePath(input, Options.HTTP_PATH);
 
 		assertThat(path.value()).as("value: '" + input + "'").isEqualTo(value);
-		assertThat(path.elements()).map(PathContainer.Element::value).as("elements: " + input)
+		assertThat(path.elements()).map(Element::value).as("elements: " + input)
 				.containsExactly(expectedElements);
 	}
 
@@ -137,7 +164,7 @@ class DefaultPathContainerTests {
 
 	@Test // gh-23310
 	void pathWithCustomSeparator() {
-		PathContainer path = PathContainer.parsePath("a.b%2Eb.c", PathContainer.Options.MESSAGE_ROUTE);
+		PathContainer path = PathContainer.parsePath("a.b%2Eb.c", Options.MESSAGE_ROUTE);
 
 		Stream<String> decodedSegments = path.elements().stream()
 				.filter(PathSegment.class::isInstance)
@@ -147,4 +174,8 @@ class DefaultPathContainerTests {
 		assertThat(decodedSegments).containsExactly("a", "b.b", "c");
 	}
 
+	private static LinkedMultiValueMap<String, String> emptyMap() {
+		return new LinkedMultiValueMap<>();
+	}
+
 }