Skip to content

Commit 7a5844f

Browse files
bclozelbclozel
committed
Reject unsafe resource handling locations
As of gh-36692, Spring logs a WARN message when an unsafe resource handling location is configured. This change now rejects entirely such setups by failing before the application starts up. Closes gh-36695
1 parent 0d14aa5 commit 7a5844f

12 files changed

Lines changed: 264 additions & 45 deletions

File tree

‎spring-webflux/src/main/java/org/springframework/web/reactive/resource/ResourceHandlerUtils.java‎

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import java.io.IOException;
2020
import java.net.URLDecoder;
2121
import java.nio.charset.StandardCharsets;
22-
import java.util.Locale;
2322

2423
import org.apache.commons.logging.Log;
2524
import org.apache.commons.logging.LogFactory;
@@ -69,17 +68,15 @@ else if (location instanceof FileSystemResource fileSystemResource) {
6968
}
7069
else if (location instanceof ClassPathResource classPathResource) {
7170
path = classPathResource.getPath();
72-
if (path.isEmpty() || "/".equals(path)) {
73-
logger.warn("Resource location '" + location + "' is considered unsafe " +
74-
"and should not be used as it provides access to the entire classpath.");
75-
}
71+
Assert.isTrue(!path.isEmpty() && !"/".equals(path),
72+
() -> "Resource location '" + location + "' is considered unsafe " +
73+
"and cannot be used as it provides access to the entire classpath.");
7674
}
7775
else if (location instanceof ContextResource contextResource) {
7876
path = contextResource.getPathWithinContext();
79-
if ("/".equals(path)) {
80-
logger.warn("Resource location '" + location + "' is considered unsafe " +
81-
"and should not be used as it provides access to the root servlet context.");
82-
}
77+
Assert.isTrue(!"/".equals(path),
78+
() -> "Resource location '" + location + "' is considered unsafe " +
79+
"and cannot be used as it provides access to the root servlet context.");
8380
}
8481
else if (location instanceof UrlResource) {
8582
path = location.getURL().toExternalForm();
@@ -176,7 +173,6 @@ public static boolean shouldIgnoreInputPath(String path) {
176173
/**
177174
* Checks for invalid resource input paths rejecting the following:
178175
* <ul>
179-
* <li>Paths that contain "WEB-INF" or "META-INF"
180176
* <li>Paths that contain "../" after a call to
181177
* {@link StringUtils#cleanPath}.
182178
* <li>Paths that represent a {@link ResourceUtils#isUrl
@@ -189,14 +185,6 @@ public static boolean shouldIgnoreInputPath(String path) {
189185
* @return {@code true} if the path is invalid, {@code false} otherwise
190186
*/
191187
public static boolean isInvalidPath(String path) {
192-
String pathLowerCase = path.toLowerCase(Locale.ROOT);
193-
if (pathLowerCase.contains("web-inf") || pathLowerCase.contains("meta-inf")) {
194-
if (logger.isWarnEnabled()) {
195-
logger.warn(LogFormatUtils.formatValue(
196-
"Path with \"WEB-INF\" or \"META-INF\": [" + path + "]", -1, true));
197-
}
198-
return true;
199-
}
200188
if (path.contains(":/")) {
201189
String relativePath = (path.charAt(0) == '/' ? path.substring(1) : path);
202190
if (ResourceUtils.isUrl(relativePath) || relativePath.startsWith("url:")) {

‎spring-webflux/src/test/java/org/springframework/web/reactive/resource/ResourceHandlerUtilsTests.java‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
/*
2+
* Copyright 2002-present the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.web.reactive.resource;
18+
19+
import java.io.File;
20+
import java.io.IOException;
21+
import java.io.InputStream;
22+
import java.net.URI;
23+
import java.net.URL;
24+
25+
import org.junit.jupiter.api.Test;
26+
27+
import org.springframework.core.io.ClassPathResource;
28+
import org.springframework.core.io.ContextResource;
29+
import org.springframework.core.io.FileSystemResource;
30+
import org.springframework.core.io.PathResource;
31+
import org.springframework.core.io.UrlResource;
32+
33+
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
34+
import static org.assertj.core.api.Assertions.assertThatNoException;
35+
36+
/**
37+
* Tests for {@link ResourceHandlerUtils}.
38+
*/
39+
class ResourceHandlerUtilsTests {
40+
41+
@Test
42+
@SuppressWarnings("removal")
43+
void assertResourceLocation() throws Exception {
44+
assertThatNoException().isThrownBy(() ->
45+
ResourceHandlerUtils.assertResourceLocation(new ClassPathResource("test/")));
46+
47+
assertThatNoException().isThrownBy(() ->
48+
ResourceHandlerUtils.assertResourceLocation(new FileSystemResource("test/")));
49+
50+
assertThatNoException().isThrownBy(() ->
51+
ResourceHandlerUtils.assertResourceLocation(new PathResource("test/")));
52+
53+
assertThatNoException().isThrownBy(() ->
54+
ResourceHandlerUtils.assertResourceLocation(new UrlResource("file:/test/")));
55+
56+
assertThatNoException().isThrownBy(() ->
57+
ResourceHandlerUtils.assertResourceLocation(new TestContextResource("/test/")));
58+
}
59+
60+
@Test
61+
void assertResourceLocationShouldRejectNull() {
62+
assertThatIllegalArgumentException().isThrownBy(() ->
63+
ResourceHandlerUtils.assertResourceLocation(null))
64+
.withMessage("Resource location must not be null");
65+
}
66+
67+
@Test
68+
void assertResourceLocationShouldRejectNotEndWithSlash() {
69+
assertThatIllegalArgumentException().isThrownBy(() ->
70+
ResourceHandlerUtils.assertResourceLocation(new ClassPathResource("test")))
71+
.withMessageContaining("Resource location does not end with slash");
72+
}
73+
74+
@Test
75+
void assertResourceLocationShouldRejectUnsafeClassPathResource() {
76+
assertThatIllegalArgumentException().isThrownBy(() ->
77+
ResourceHandlerUtils.assertResourceLocation(new ClassPathResource("")))
78+
.withMessageContaining("is considered unsafe");
79+
80+
assertThatIllegalArgumentException().isThrownBy(() ->
81+
ResourceHandlerUtils.assertResourceLocation(new ClassPathResource("/")))
82+
.withMessageContaining("is considered unsafe");
83+
}
84+
85+
@Test
86+
void assertResourceLocationShouldRejectUnsafeContextResource() {
87+
assertThatIllegalArgumentException().isThrownBy(() ->
88+
ResourceHandlerUtils.assertResourceLocation(new TestContextResource("/")))
89+
.withMessageContaining("is considered unsafe");
90+
}
91+
92+
private static class TestContextResource implements ContextResource {
93+
94+
private final String path;
95+
96+
TestContextResource(String path) {
97+
this.path = path;
98+
}
99+
100+
@Override
101+
public String getPathWithinContext() {
102+
return this.path;
103+
}
104+
105+
@Override
106+
public boolean exists() {
107+
return false;
108+
}
109+
110+
@Override
111+
public URL getURL() throws IOException {
112+
throw new UnsupportedOperationException();
113+
}
114+
115+
@Override
116+
public URI getURI() throws IOException {
117+
throw new UnsupportedOperationException();
118+
}
119+
120+
@Override
121+
public File getFile() throws IOException {
122+
throw new UnsupportedOperationException();
123+
}
124+
125+
@Override
126+
public long contentLength() throws IOException {
127+
return 0;
128+
}
129+
130+
@Override
131+
public long lastModified() throws IOException {
132+
return 0;
133+
}
134+
135+
@Override
136+
public org.springframework.core.io.Resource createRelative(String relativePath) throws IOException {
137+
throw new UnsupportedOperationException();
138+
}
139+
140+
@Override
141+
public String getFilename() {
142+
return null;
143+
}
144+
145+
@Override
146+
public String getDescription() {
147+
return "TestContextResource";
148+
}
149+
150+
@Override
151+
public InputStream getInputStream() throws IOException {
152+
throw new UnsupportedOperationException();
153+
}
154+
}
155+
156+
}

‎spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHandlerUtils.java‎

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import java.io.IOException;
2020
import java.net.URLDecoder;
2121
import java.nio.charset.StandardCharsets;
22-
import java.util.Locale;
2322

2423
import org.apache.commons.logging.Log;
2524
import org.apache.commons.logging.LogFactory;
@@ -70,17 +69,15 @@ else if (location instanceof FileSystemResource fileSystemResource) {
7069
}
7170
else if (location instanceof ClassPathResource classPathResource) {
7271
path = classPathResource.getPath();
73-
if (path.isEmpty() || "/".equals(path)) {
74-
logger.warn("Resource location '" + location + "' is considered unsafe " +
75-
"and should not be used as it provides access to the entire classpath.");
76-
}
72+
Assert.isTrue(!path.isEmpty() && !"/".equals(path),
73+
() -> "Resource location '" + location + "' is considered unsafe " +
74+
"and cannot be used as it provides access to the entire classpath.");
7775
}
7876
else if (location instanceof ContextResource contextResource) {
7977
path = contextResource.getPathWithinContext();
80-
if ("/".equals(path)) {
81-
logger.warn("Resource location '" + location + "' is considered unsafe " +
82-
"and should not be used as it provides access to the root servlet context.");
83-
}
78+
Assert.isTrue(!"/".equals(path),
79+
() -> "Resource location '" + location + "' is considered unsafe " +
80+
"and cannot be used as it provides access to the root servlet context.");
8481
}
8582
else if (location instanceof UrlResource) {
8683
path = location.getURL().toExternalForm();
@@ -177,7 +174,6 @@ public static boolean shouldIgnoreInputPath(String path) {
177174
/**
178175
* Checks for invalid resource input paths rejecting the following:
179176
* <ul>
180-
* <li>Paths that contain "WEB-INF" or "META-INF"
181177
* <li>Paths that contain "../" after a call to
182178
* {@link org.springframework.util.StringUtils#cleanPath}.
183179
* <li>Paths that represent a {@link org.springframework.util.ResourceUtils#isUrl
@@ -190,14 +186,6 @@ public static boolean shouldIgnoreInputPath(String path) {
190186
* @return {@code true} if the path is invalid, {@code false} otherwise
191187
*/
192188
public static boolean isInvalidPath(String path) {
193-
String pathLowerCase = path.toLowerCase(Locale.ROOT);
194-
if (pathLowerCase.contains("web-inf") || pathLowerCase.contains("meta-inf")) {
195-
if (logger.isWarnEnabled()) {
196-
logger.warn(LogFormatUtils.formatValue(
197-
"Path with \"WEB-INF\" or \"META-INF\": [" + path + "]", -1, true));
198-
}
199-
return true;
200-
}
201189
if (path.contains(":/")) {
202190
String relativePath = (path.charAt(0) == '/' ? path.substring(1) : path);
203191
if (ResourceUtils.isUrl(relativePath) || relativePath.startsWith("url:")) {

‎spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/DelegatingWebMvcConfigurationTests.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ public void addViewControllers(ViewControllerRegistry registry) {
239239
}
240240
@Override
241241
public void addResourceHandlers(ResourceHandlerRegistry registry) {
242-
registry.addResourceHandler("/resources/**").addResourceLocations("/");
242+
registry.addResourceHandler("/resources/**").addResourceLocations("/static");
243243
}
244244
};
245245

@@ -304,7 +304,7 @@ public void addViewControllers(ViewControllerRegistry registry) {
304304
}
305305
@Override
306306
public void addResourceHandlers(ResourceHandlerRegistry registry) {
307-
registry.addResourceHandler("/resources/**").addResourceLocations("/");
307+
registry.addResourceHandler("/resources/**").addResourceLocations("/static");
308308
}
309309
};
310310

‎spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHandlerUtilsTests.java‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
/*
2+
* Copyright 2002-present the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.web.servlet.resource;
18+
19+
import org.junit.jupiter.api.Test;
20+
21+
import org.springframework.core.io.ClassPathResource;
22+
import org.springframework.core.io.FileSystemResource;
23+
import org.springframework.core.io.PathResource;
24+
import org.springframework.core.io.UrlResource;
25+
import org.springframework.web.context.support.ServletContextResource;
26+
import org.springframework.web.testfixture.servlet.MockServletContext;
27+
28+
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
29+
import static org.assertj.core.api.Assertions.assertThatNoException;
30+
31+
/**
32+
* Tests for {@link ResourceHandlerUtils}.
33+
*/
34+
class ResourceHandlerUtilsTests {
35+
36+
@Test
37+
@SuppressWarnings("removal")
38+
void assertResourceLocation() throws Exception {
39+
assertThatNoException().isThrownBy(() ->
40+
ResourceHandlerUtils.assertResourceLocation(new ClassPathResource("test/")));
41+
42+
assertThatNoException().isThrownBy(() ->
43+
ResourceHandlerUtils.assertResourceLocation(new FileSystemResource("test/")));
44+
45+
assertThatNoException().isThrownBy(() ->
46+
ResourceHandlerUtils.assertResourceLocation(new PathResource("test/")));
47+
48+
assertThatNoException().isThrownBy(() ->
49+
ResourceHandlerUtils.assertResourceLocation(new UrlResource("file:/test/")));
50+
51+
assertThatNoException().isThrownBy(() ->
52+
ResourceHandlerUtils.assertResourceLocation(new ServletContextResource(new MockServletContext(), "/test/")));
53+
}
54+
55+
@Test
56+
void assertResourceLocationShouldRejectNull() {
57+
assertThatIllegalArgumentException().isThrownBy(() ->
58+
ResourceHandlerUtils.assertResourceLocation(null))
59+
.withMessage("Resource location must not be null");
60+
}
61+
62+
@Test
63+
void assertResourceLocationShouldRejectNotEndWithSlash() {
64+
assertThatIllegalArgumentException().isThrownBy(() ->
65+
ResourceHandlerUtils.assertResourceLocation(new ClassPathResource("test")))
66+
.withMessageContaining("Resource location does not end with slash");
67+
}
68+
69+
@Test
70+
void assertResourceLocationShouldRejectUnsafeClassPathResource() {
71+
assertThatIllegalArgumentException().isThrownBy(() ->
72+
ResourceHandlerUtils.assertResourceLocation(new ClassPathResource("")))
73+
.withMessageContaining("is considered unsafe");
74+
75+
assertThatIllegalArgumentException().isThrownBy(() ->
76+
ResourceHandlerUtils.assertResourceLocation(new ClassPathResource("/")))
77+
.withMessageContaining("is considered unsafe");
78+
}
79+
80+
@Test
81+
void assertResourceLocationShouldRejectUnsafeContextResource() {
82+
assertThatIllegalArgumentException().isThrownBy(() ->
83+
ResourceHandlerUtils.assertResourceLocation(new ServletContextResource(new MockServletContext(), "/")))
84+
.withMessageContaining("is considered unsafe");
85+
}
86+
87+
}

‎spring-webmvc/src/test/resources/org/springframework/web/servlet/config/mvc-config-custom-pattern-parser.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,6 @@
1212
</mvc:annotation-driven>
1313

1414
<mvc:view-controller path="/foo"/>
15-
<mvc:resources mapping="/resources/**" location="/, classpath:/META-INF/"/>
15+
<mvc:resources mapping="/resources/**" location="/static, classpath:/META-INF/"/>
1616

1717
</beans>

‎spring-webmvc/src/test/resources/org/springframework/web/servlet/config/mvc-config-deprecated-path-matcher.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,6 @@
1212
</mvc:annotation-driven>
1313

1414
<mvc:view-controller path="/foo"/>
15-
<mvc:resources mapping="/resources/**" location="/, classpath:/META-INF/"/>
15+
<mvc:resources mapping="/resources/**" location="/static, classpath:/META-INF/"/>
1616

1717
</beans>

‎spring-webmvc/src/test/resources/org/springframework/web/servlet/config/mvc-config-path-matching-mappings.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
xsi:schemaLocation="http://www.springframework.org/schema/beans https://www.springframework.org/schema/beans/spring-beans.xsd
66
http://www.springframework.org/schema/mvc https://www.springframework.org/schema/mvc/spring-mvc.xsd">
77

8-
<mvc:resources mapping="/resources/**" location="/, classpath:/META-INF/" />
8+
<mvc:resources mapping="/resources/**" location="/static, classpath:/META-INF/" />
99

1010
<mvc:annotation-driven>
1111
<mvc:path-matching

0 commit comments

Comments
 (0)