Skip to content

Commit

Permalink
Ensure DefaultPathSegment does not allow parameters to be mutated
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@sbrannen
sbrannen committed Jun 15, 2021
1 parent bcb0580 commit 3676084
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 34 deletions.
45 changes: 28 additions & 17 deletions spring-web/src/main/java/org/springframework/http/server/DefaultPathContainer.java
View file
@@ -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.
Expand Down Expand Up @@ -36,12 +36,11 @@
* 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);
Expand Down Expand Up @@ -120,7 +119,7 @@ static PathContainer createFromUrlPath(String path, Options options) {
if (!segment.isEmpty()) {
elements.add(options.shouldDecodeAndParseSegments() ?
decodeAndParsePathSegment(segment) :
new DefaultPathSegment(segment, separatorElement));
DefaultPathSegment.from(segment, separatorElement));
}
if (end == -1) {
break;
Expand All @@ -136,13 +135,13 @@ private static PathSegment decodeAndParsePathSegment(String segment) {
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);
}
}

Expand Down Expand Up @@ -226,32 +225,44 @@ public String encodedSequence() {
}


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;

private final String valueToMatch;

private final MultiValueMap<String, String> parameters;

/**
* Factory for segments without decoding and parsing.
*/
static DefaultPathSegment from(String value, DefaultSeparator separator) {
String valueToMatch = value.contains(separator.encodedSequence()) ?
value.replaceAll(separator.encodedSequence(), separator.value()) : value;
return from(value, valueToMatch);
}

/**
* Constructor for decoded and parsed segments.
* Factory for decoded and parsed segments.
*/
DefaultPathSegment(String value, String valueToMatch, MultiValueMap<String, String> params) {
this.value = value;
this.valueToMatch = valueToMatch;
this.parameters = CollectionUtils.unmodifiableMultiValueMap(params);
static DefaultPathSegment from(String value, String valueToMatch) {
return new DefaultPathSegment(value, valueToMatch, EMPTY_PARAMS);
}

/**
* 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, 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()) ?
value.replaceAll(separator.encodedSequence(), separator.value()) : value;
this.parameters = EMPTY_PARAMS;
this.valueToMatch = valueToMatch;
this.parameters = params;
}


Expand Down
14 changes: 8 additions & 6 deletions spring-web/src/main/java/org/springframework/http/server/PathContainer.java
View file
@@ -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.
Expand Down Expand Up @@ -124,6 +124,7 @@ interface PathSegment extends Element {

/**
* Path parameters associated with this path segment.
* @return an unmodifiable map containing the parameters
*/
MultiValueMap<String, String> parameters();
}
Expand All @@ -136,15 +137,16 @@ interface PathSegment extends Element {
class Options {

/**
* Options for HTTP URL paths:
* <p>Separator '/' with URL decoding and parsing of path params.
* Options for HTTP URL paths.
* <p>Separator '/' with URL decoding and parsing of path parameters.
*/
public final static Options HTTP_PATH = Options.create('/', true);

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

Expand Down
53 changes: 42 additions & 11 deletions spring-web/src/test/java/org/springframework/http/server/DefaultPathContainerTests.java
View file
Expand Up @@ -20,11 +20,14 @@

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}.
Expand All @@ -37,42 +40,66 @@ 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("%20a%20", " a ", new LinkedMultiValueMap<>());
testPathSegment("%20%20", " ", emptyMap());
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");
params.add("year", "2012");
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);

Expand Down Expand Up @@ -111,10 +138,10 @@ void path() {
}

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);
}

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

@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)
Expand All @@ -147,4 +174,8 @@ void pathWithCustomSeparator() {
assertThat(decodedSegments).containsExactly("a", "b.b", "c");
}

private static LinkedMultiValueMap<String, String> emptyMap() {
return new LinkedMultiValueMap<>();
}

}

0 comments on commit 3676084

Please sign in to comment.