Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make multiple slash sanitization optional in web util #34242

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ public UriComponentsBuilder encode(Charset charset) {
* @return the URI components
*/
public UriComponents build() {
return build(false);
return build(false, true);
}

/**
Expand All @@ -297,19 +297,39 @@ public UriComponents build() {
* characters that should have been encoded.
*/
public UriComponents build(boolean encoded) {
return buildInternal(encoded ? EncodingHint.FULLY_ENCODED :
(this.encodeTemplate ? EncodingHint.ENCODE_TEMPLATE : EncodingHint.NONE));
return build(encoded, true);
}

/**
* Variant of {@link #build()} to create a {@link UriComponents} instance
* when components are already fully encoded. In addition, this method allows
* to control whether the double slashes in the path should be replaced with
* a single slash.
* @param encoded whether the components in this builder are already encoded
* @param sanitizePath whether the double slashes should be replaced with single slash
* @return the URI components
* @throws IllegalArgumentException if any of the components contain illegal
* characters that should have been encoded.
*/
public UriComponents build(boolean encoded, boolean sanitizePath) {
EncodingHint hint = encoded ? EncodingHint.FULLY_ENCODED :
(this.encodeTemplate ? EncodingHint.ENCODE_TEMPLATE : EncodingHint.NONE);
return buildInternal(hint, sanitizePath);
}

private UriComponents buildInternal(EncodingHint hint) {
return buildInternal(hint, true);
}

private UriComponents buildInternal(EncodingHint hint, boolean sanitizePath) {
UriComponents result;
if (this.ssp != null) {
result = new OpaqueUriComponents(this.scheme, this.ssp, this.fragment);
}
else {
MultiValueMap<String, String> queryParams = new LinkedMultiValueMap<>(this.queryParams);
HierarchicalUriComponents uric = new HierarchicalUriComponents(this.scheme, this.fragment,
this.userInfo, this.host, this.port, this.pathBuilder.build(), queryParams,
this.userInfo, this.host, this.port, this.pathBuilder.build(sanitizePath), queryParams,
hint == EncodingHint.FULLY_ENCODED);
result = (hint == EncodingHint.ENCODE_TEMPLATE ? uric.encodeTemplate(this.charset) : uric);
}
Expand Down Expand Up @@ -771,7 +791,7 @@ public enum ParserType {

private interface PathComponentBuilder {

@Nullable PathComponent build();
@Nullable PathComponent build(boolean sanitizePath);

PathComponentBuilder cloneBuilder();
}
Expand Down Expand Up @@ -823,11 +843,11 @@ public void addPath(String path) {
}

@Override
public PathComponent build() {
public PathComponent build(boolean sanitizePath) {
int size = this.builders.size();
List<PathComponent> components = new ArrayList<>(size);
for (PathComponentBuilder componentBuilder : this.builders) {
PathComponent pathComponent = componentBuilder.build();
PathComponent pathComponent = componentBuilder.build(sanitizePath);
if (pathComponent != null) {
components.add(pathComponent);
}
Expand Down Expand Up @@ -861,12 +881,12 @@ public void append(String path) {
}

@Override
public @Nullable PathComponent build() {
public @Nullable PathComponent build(boolean sanitizePath) {
if (this.path.isEmpty()) {
return null;
}
String sanitized = getSanitizedPath(this.path);
return new HierarchicalUriComponents.FullPathComponent(sanitized);
String path = sanitizePath ? getSanitizedPath(this.path) : this.path.toString();
return new HierarchicalUriComponents.FullPathComponent(path);
}

private static String getSanitizedPath(final StringBuilder path) {
Expand Down Expand Up @@ -911,7 +931,7 @@ public void append(String... pathSegments) {
}

@Override
public @Nullable PathComponent build() {
public @Nullable PathComponent build(boolean sanitizePath) {
Copy link
Author

@vishnugt vishnugt Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we don't use the sanitizePath flag, but I couldn't come up with any other approach that requires minimal changes while addressing the sanitizePath flag, especially considering that two of the three implementations make use of it. If you have a better solution, I’d be happy to implement it.

return (this.pathSegments.isEmpty() ? null :
new HierarchicalUriComponents.PathSegmentComponent(this.pathSegments));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ public class UrlPathHelper {

private boolean removeSemicolonContent = true;

private boolean sanitizePath = true;

private String defaultEncoding = WebUtils.DEFAULT_CHARACTER_ENCODING;

private boolean readOnly = false;
Expand Down Expand Up @@ -145,6 +147,22 @@ public boolean shouldRemoveSemicolonContent() {
return this.removeSemicolonContent;
}

/**
* Set if double slashes to be replaced by single slash in the request URI.
* <p>Default is "true".
*/
public void setSanitizePath(boolean sanitizePath) {
checkReadOnly();
this.sanitizePath = sanitizePath;
}

/**
* Whether configured to replace double slashes with single slash from the request URI.
*/
public boolean shouldSanitizePath() {
return this.sanitizePath;
}

/**
* Set the default character encoding to use for URL decoding.
* Default is ISO-8859-1, according to the Servlet spec.
Expand Down Expand Up @@ -392,12 +410,16 @@ else if (index1 == requestUri.length()) {
}

/**
* Sanitize the given path. Uses the following rules:
* Sanitize the given path if {code shouldSanitizePath()} is true.
* Uses the following rules:
* <ul>
* <li>replace all "//" by "/"</li>
* </ul>
*/
private static String getSanitizedPath(final String path) {
private String getSanitizedPath(final String path) {
Copy link
Author

@vishnugt vishnugt Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a private static method that had 2 usages in the class, made it non-static to use the flag.
I can abstract the actual removing slash logic into a static method and have a non-static wrapper if you think that makes sense.

if (!shouldSanitizePath()) {
return path;
}
int start = path.indexOf("//");
if (start == -1) {
return path;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -849,11 +849,23 @@ void toUriStringWithCurlyBraces(ParserType parserType) {
}

@Test // gh-26012
void verifyDoubleSlashReplacedWithSingleOne() {
void verifyDoubleSlashReplacedWithSingleDefault() {
String path = UriComponentsBuilder.fromPath("/home/").path("/path").build().getPath();
assertThat(path).isEqualTo("/home/path");
}

@Test // gh-34076
void verifyDoubleSlashNotReplacedAsSingleSlash() {
String path = UriComponentsBuilder.fromPath("/home/").path("/path").build(true, false).getPath();
assertThat(path).isEqualTo("/home//path");
}

@Test // gh-34076
void verifyDoubleSlashReplacedAsSingleSlashWithConfig() {
String path = UriComponentsBuilder.fromPath("/home/").path("/path").build(true, true).getPath();
assertThat(path).isEqualTo("/home/path");
}

@ParameterizedTest
@EnumSource
void validPort(ParserType parserType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,20 @@ void getRequestUri() {
assertThat(helper.getRequestUri(request)).isEqualTo("/home/path");
}

@Test // gh-34076
void getRequestUriWithSanitizingDisabled() {
helper.setSanitizePath(false);
request.setRequestURI("/home/" + "/path");
assertThat(helper.getRequestUri(request)).isEqualTo("/home//path");
}

@Test // gh-34076
void getRequestUriWithSanitizingEnabled() {
helper.setSanitizePath(true);
request.setRequestURI("/home/" + "/path");
assertThat(helper.getRequestUri(request)).isEqualTo("/home/path");
}

@Test
void getRequestRemoveSemicolonContent() {
helper.setRemoveSemicolonContent(true);
Expand Down
Loading