Skip to content

Commit c5a138a

Browse files
sbrannensbrannen
committed
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
1 parent 1f098e1 commit c5a138a

File tree

3 files changed

+79
-36
lines changed

3 files changed

+79
-36
lines changed

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

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2020 the original author or authors.
2+
* Copyright 2002-2021 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -36,12 +36,11 @@
3636
* Default implementation of {@link PathContainer}.
3737
*
3838
* @author Rossen Stoyanchev
39+
* @author Sam Brannen
3940
* @since 5.0
4041
*/
4142
final class DefaultPathContainer implements PathContainer {
4243

43-
private static final MultiValueMap<String, String> EMPTY_PARAMS = new LinkedMultiValueMap<>();
44-
4544
private static final PathContainer EMPTY_PATH = new DefaultPathContainer("", Collections.emptyList());
4645

4746
private static final Map<Character, DefaultSeparator> SEPARATORS = new HashMap<>(2);
@@ -120,7 +119,7 @@ static PathContainer createFromUrlPath(String path, Options options) {
120119
if (!segment.isEmpty()) {
121120
elements.add(options.shouldDecodeAndParseSegments() ?
122121
decodeAndParsePathSegment(segment) :
123-
new DefaultPathSegment(segment, separatorElement));
122+
DefaultPathSegment.from(segment, separatorElement));
124123
}
125124
if (end == -1) {
126125
break;
@@ -136,13 +135,13 @@ private static PathSegment decodeAndParsePathSegment(String segment) {
136135
int index = segment.indexOf(';');
137136
if (index == -1) {
138137
String valueToMatch = StringUtils.uriDecode(segment, charset);
139-
return new DefaultPathSegment(segment, valueToMatch, EMPTY_PARAMS);
138+
return DefaultPathSegment.from(segment, valueToMatch);
140139
}
141140
else {
142141
String valueToMatch = StringUtils.uriDecode(segment.substring(0, index), charset);
143142
String pathParameterContent = segment.substring(index);
144143
MultiValueMap<String, String> parameters = parsePathParams(pathParameterContent, charset);
145-
return new DefaultPathSegment(segment, valueToMatch, parameters);
144+
return DefaultPathSegment.from(segment, valueToMatch, parameters);
146145
}
147146
}
148147

@@ -226,7 +225,10 @@ public String encodedSequence() {
226225
}
227226

228227

229-
private static class DefaultPathSegment implements PathSegment {
228+
private static final class DefaultPathSegment implements PathSegment {
229+
230+
private static final MultiValueMap<String, String> EMPTY_PARAMS =
231+
CollectionUtils.unmodifiableMultiValueMap(new LinkedMultiValueMap<>());
230232

231233
private final String value;
232234

@@ -236,26 +238,34 @@ private static class DefaultPathSegment implements PathSegment {
236238

237239
private final MultiValueMap<String, String> parameters;
238240

241+
/**
242+
* Factory for segments without decoding and parsing.
243+
*/
244+
static DefaultPathSegment from(String value, DefaultSeparator separator) {
245+
String valueToMatch = value.contains(separator.encodedSequence()) ?
246+
value.replaceAll(separator.encodedSequence(), separator.value()) : value;
247+
return from(value, valueToMatch);
248+
}
239249

240250
/**
241-
* Constructor for decoded and parsed segments.
251+
* Factory for decoded and parsed segments.
242252
*/
243-
DefaultPathSegment(String value, String valueToMatch, MultiValueMap<String, String> params) {
244-
this.value = value;
245-
this.valueToMatch = valueToMatch;
246-
this.valueToMatchAsChars = valueToMatch.toCharArray();
247-
this.parameters = CollectionUtils.unmodifiableMultiValueMap(params);
253+
static DefaultPathSegment from(String value, String valueToMatch) {
254+
return new DefaultPathSegment(value, valueToMatch, EMPTY_PARAMS);
248255
}
249256

250257
/**
251-
* Constructor for segments without decoding and parsing.
258+
* Factory for decoded and parsed segments.
252259
*/
253-
DefaultPathSegment(String value, DefaultSeparator separator) {
260+
static DefaultPathSegment from(String value, String valueToMatch, MultiValueMap<String, String> params) {
261+
return new DefaultPathSegment(value, valueToMatch, CollectionUtils.unmodifiableMultiValueMap(params));
262+
}
263+
264+
private DefaultPathSegment(String value, String valueToMatch, MultiValueMap<String, String> params) {
254265
this.value = value;
255-
this.valueToMatch = value.contains(separator.encodedSequence()) ?
256-
value.replaceAll(separator.encodedSequence(), separator.value()) : value;
257-
this.valueToMatchAsChars = this.valueToMatch.toCharArray();
258-
this.parameters = EMPTY_PARAMS;
266+
this.valueToMatch = valueToMatch;
267+
this.valueToMatchAsChars = valueToMatch.toCharArray();
268+
this.parameters = params;
259269
}
260270

261271

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2019 the original author or authors.
2+
* Copyright 2002-2021 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -124,6 +124,7 @@ interface PathSegment extends Element {
124124

125125
/**
126126
* Path parameters associated with this path segment.
127+
* @return an unmodifiable map containing the parameters
127128
*/
128129
MultiValueMap<String, String> parameters();
129130
}
@@ -136,15 +137,16 @@ interface PathSegment extends Element {
136137
class Options {
137138

138139
/**
139-
* Options for HTTP URL paths:
140-
* <p>Separator '/' with URL decoding and parsing of path params.
140+
* Options for HTTP URL paths.
141+
* <p>Separator '/' with URL decoding and parsing of path parameters.
141142
*/
142143
public final static Options HTTP_PATH = Options.create('/', true);
143144

144145
/**
145-
* Options for a message route:
146-
* <p>Separator '.' without URL decoding nor parsing of params. Escape
147-
* sequences for the separator char in segment values are still decoded.
146+
* Options for a message route.
147+
* <p>Separator '.' with neither URL decoding nor parsing of path parameters.
148+
* Escape sequences for the separator character in segment values are still
149+
* decoded.
148150
*/
149151
public final static Options MESSAGE_ROUTE = Options.create('.', false);
150152

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

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,14 @@
2020

2121
import org.junit.jupiter.api.Test;
2222

23+
import org.springframework.http.server.PathContainer.Element;
24+
import org.springframework.http.server.PathContainer.Options;
2325
import org.springframework.http.server.PathContainer.PathSegment;
2426
import org.springframework.util.LinkedMultiValueMap;
2527
import org.springframework.util.MultiValueMap;
2628

2729
import static org.assertj.core.api.Assertions.assertThat;
30+
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
2831

2932
/**
3033
* Unit tests for {@link DefaultPathContainer}.
@@ -37,42 +40,66 @@ class DefaultPathContainerTests {
3740
@Test
3841
void pathSegment() {
3942
// basic
40-
testPathSegment("cars", "cars", new LinkedMultiValueMap<>());
43+
testPathSegment("cars", "cars", emptyMap());
4144

4245
// empty
43-
testPathSegment("", "", new LinkedMultiValueMap<>());
46+
testPathSegment("", "", emptyMap());
4447

4548
// spaces
46-
testPathSegment("%20%20", " ", new LinkedMultiValueMap<>());
47-
testPathSegment("%20a%20", " a ", new LinkedMultiValueMap<>());
49+
testPathSegment("%20%20", " ", emptyMap());
50+
testPathSegment("%20a%20", " a ", emptyMap());
4851
}
4952

5053
@Test
5154
void pathSegmentParams() {
5255
// basic
53-
LinkedMultiValueMap<String, String> params = new LinkedMultiValueMap<>();
56+
LinkedMultiValueMap<String, String> params = emptyMap();
5457
params.add("colors", "red");
5558
params.add("colors", "blue");
5659
params.add("colors", "green");
5760
params.add("year", "2012");
5861
testPathSegment("cars;colors=red,blue,green;year=2012", "cars", params);
5962

6063
// trailing semicolon
61-
params = new LinkedMultiValueMap<>();
64+
params = emptyMap();
6265
params.add("p", "1");
6366
testPathSegment("path;p=1;", "path", params);
6467

6568
// params with spaces
66-
params = new LinkedMultiValueMap<>();
69+
params = emptyMap();
6770
params.add("param name", "param value");
6871
testPathSegment("path;param%20name=param%20value;%20", "path", params);
6972

7073
// empty params
71-
params = new LinkedMultiValueMap<>();
74+
params = emptyMap();
7275
params.add("p", "1");
7376
testPathSegment("path;;;%20;%20;p=1;%20", "path", params);
7477
}
7578

79+
@Test
80+
void pathSegmentParamsAreImmutable() {
81+
assertPathSegmentParamsAreImmutable("cars", emptyMap(), Options.HTTP_PATH);
82+
83+
LinkedMultiValueMap<String, String> params = emptyMap();
84+
params.add("colors", "red");
85+
params.add("colors", "blue");
86+
params.add("colors", "green");
87+
assertPathSegmentParamsAreImmutable(";colors=red,blue,green", params, Options.HTTP_PATH);
88+
89+
assertPathSegmentParamsAreImmutable(";colors=red,blue,green", emptyMap(), Options.MESSAGE_ROUTE);
90+
}
91+
92+
private void assertPathSegmentParamsAreImmutable(String path, LinkedMultiValueMap<String, String> params, Options options) {
93+
PathContainer container = PathContainer.parsePath(path, options);
94+
assertThat(container.elements()).hasSize(1);
95+
96+
PathSegment segment = (PathSegment) container.elements().get(0);
97+
MultiValueMap<String, String> segmentParams = segment.parameters();
98+
assertThat(segmentParams).isEqualTo(params);
99+
assertThatExceptionOfType(UnsupportedOperationException.class)
100+
.isThrownBy(() -> segmentParams.add("enigma", "boom"));
101+
}
102+
76103
private void testPathSegment(String rawValue, String valueToMatch, MultiValueMap<String, String> params) {
77104
PathContainer container = PathContainer.parsePath(rawValue);
78105

@@ -111,10 +138,10 @@ void path() {
111138
}
112139

113140
private void testPath(String input, String value, String... expectedElements) {
114-
PathContainer path = PathContainer.parsePath(input, PathContainer.Options.HTTP_PATH);
141+
PathContainer path = PathContainer.parsePath(input, Options.HTTP_PATH);
115142

116143
assertThat(path.value()).as("value: '" + input + "'").isEqualTo(value);
117-
assertThat(path.elements()).map(PathContainer.Element::value).as("elements: " + input)
144+
assertThat(path.elements()).map(Element::value).as("elements: " + input)
118145
.containsExactly(expectedElements);
119146
}
120147

@@ -137,7 +164,7 @@ void subPath() {
137164

138165
@Test // gh-23310
139166
void pathWithCustomSeparator() {
140-
PathContainer path = PathContainer.parsePath("a.b%2Eb.c", PathContainer.Options.MESSAGE_ROUTE);
167+
PathContainer path = PathContainer.parsePath("a.b%2Eb.c", Options.MESSAGE_ROUTE);
141168

142169
Stream<String> decodedSegments = path.elements().stream()
143170
.filter(PathSegment.class::isInstance)
@@ -147,4 +174,8 @@ void pathWithCustomSeparator() {
147174
assertThat(decodedSegments).containsExactly("a", "b.b", "c");
148175
}
149176

177+
private static LinkedMultiValueMap<String, String> emptyMap() {
178+
return new LinkedMultiValueMap<>();
179+
}
180+
150181
}

0 commit comments

Comments
 (0)