Skip to content

Commit 0e28bee

Browse files
rstoyanchevrstoyanchev
committed
Clean duplicate separators in resource URLs
Most Servlet containers do this anyway, but not all, and not consistently for forward and backslashes. Issue: SPR-16616
1 parent fdd756c commit 0e28bee

File tree

4 files changed

+231
-51
lines changed

4 files changed

+231
-51
lines changed

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

Lines changed: 48 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,15 @@ protected Mono<Resource> getResource(ServerWebExchange exchange) {
328328
if (path.contains("%")) {
329329
try {
330330
// Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars
331-
if (isInvalidPath(URLDecoder.decode(path, "UTF-8"))) {
331+
String decodedPath = URLDecoder.decode(path, "UTF-8");
332+
if (isInvalidPath(decodedPath)) {
333+
if (logger.isTraceEnabled()) {
334+
logger.trace("Ignoring invalid resource path with escape sequences [" + path + "].");
335+
}
336+
return Mono.empty();
337+
}
338+
decodedPath = processPath(decodedPath);
339+
if (isInvalidPath(decodedPath)) {
332340
if (logger.isTraceEnabled()) {
333341
logger.trace("Ignoring invalid resource path with escape sequences [" + path + "].");
334342
}
@@ -352,12 +360,47 @@ protected Mono<Resource> getResource(ServerWebExchange exchange) {
352360
}
353361

354362
/**
355-
* Process the given resource path to be used.
356-
* <p>The default implementation replaces any combination of leading '/' and
357-
* control characters (00-1F and 7F) with a single "/" or "". For example
358-
* {@code " // /// //// foo/bar"} becomes {@code "/foo/bar"}.
363+
* Process the given resource path.
364+
* <p>The default implementation replaces:
365+
* <ul>
366+
* <li>Backslash with forward slash.
367+
* <li>Duplicate occurrences of slash with a single slash.
368+
* <li>Any combination of leading slash and control characters (00-1F and 7F)
369+
* with a single "/" or "". For example {@code " / // foo/bar"}
370+
* becomes {@code "/foo/bar"}.
371+
* </ul>
372+
* @since 3.2.12
359373
*/
360374
protected String processPath(String path) {
375+
path = StringUtils.replace(path, "\\", "/");
376+
path = cleanDuplicateSlashes(path);
377+
return cleanLeadingSlash(path);
378+
}
379+
380+
private String cleanDuplicateSlashes(String path) {
381+
StringBuilder sb = null;
382+
char prev = 0;
383+
for (int i = 0; i < path.length(); i++) {
384+
char curr = path.charAt(i);
385+
try {
386+
if ((curr == '/') && (prev == '/')) {
387+
if (sb == null) {
388+
sb = new StringBuilder(path.substring(0, i));
389+
}
390+
continue;
391+
}
392+
if (sb != null) {
393+
sb.append(path.charAt(i));
394+
}
395+
}
396+
finally {
397+
prev = curr;
398+
}
399+
}
400+
return sb != null ? sb.toString() : path;
401+
}
402+
403+
private String cleanLeadingSlash(String path) {
361404
boolean slash = false;
362405
for (int i = 0; i < path.length(); i++) {
363406
if (path.charAt(i) == '/') {

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

Lines changed: 66 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -54,13 +54,10 @@
5454
import org.springframework.web.server.ResponseStatusException;
5555
import org.springframework.web.server.ServerWebExchange;
5656

57-
import static org.hamcrest.Matchers.instanceOf;
58-
import static org.junit.Assert.assertEquals;
59-
import static org.junit.Assert.assertNull;
60-
import static org.junit.Assert.assertSame;
61-
import static org.junit.Assert.assertThat;
62-
import static org.junit.Assert.assertTrue;
63-
import static org.junit.Assert.fail;
57+
import static org.hamcrest.Matchers.*;
58+
import static org.junit.Assert.*;
59+
import static org.mockito.ArgumentMatchers.any;
60+
import static org.mockito.Mockito.*;
6461

6562
/**
6663
* Unit tests for {@link ResourceWebHandler}.
@@ -240,39 +237,85 @@ public void getMediaTypeWithFavorPathExtensionOff() throws Exception {
240237
}
241238

242239
@Test
243-
public void invalidPath() throws Exception {
240+
public void testInvalidPath() throws Exception {
241+
242+
// Use mock ResourceResolver: i.e. we're only testing upfront validations...
243+
244+
Resource resource = mock(Resource.class);
245+
when(resource.getFilename()).thenThrow(new AssertionError("Resource should not be resolved"));
246+
when(resource.getInputStream()).thenThrow(new AssertionError("Resource should not be resolved"));
247+
ResourceResolver resolver = mock(ResourceResolver.class);
248+
when(resolver.resolveResource(any(), any(), any(), any())).thenReturn(Mono.just(resource));
249+
250+
ResourceWebHandler handler = new ResourceWebHandler();
251+
handler.setLocations(Collections.singletonList(new ClassPathResource("test/", getClass())));
252+
handler.setResourceResolvers(Collections.singletonList(resolver));
253+
handler.afterPropertiesSet();
254+
255+
testInvalidPath("../testsecret/secret.txt", handler);
256+
testInvalidPath("test/../../testsecret/secret.txt", handler);
257+
testInvalidPath(":/../../testsecret/secret.txt", handler);
258+
259+
Resource location = new UrlResource(getClass().getResource("./test/"));
260+
this.handler.setLocations(Collections.singletonList(location));
261+
Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt"));
262+
String secretPath = secretResource.getURL().getPath();
263+
264+
testInvalidPath("file:" + secretPath, handler);
265+
testInvalidPath("/file:" + secretPath, handler);
266+
testInvalidPath("url:" + secretPath, handler);
267+
testInvalidPath("/url:" + secretPath, handler);
268+
testInvalidPath("/../.." + secretPath, handler);
269+
testInvalidPath("/%2E%2E/testsecret/secret.txt", handler);
270+
testInvalidPath("/%2E%2E/testsecret/secret.txt", handler);
271+
testInvalidPath("%2F%2F%2E%2E%2F%2F%2E%2E" + secretPath, handler);
272+
}
273+
274+
private void testInvalidPath(String requestPath, ResourceWebHandler handler) {
275+
ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get(""));
276+
setPathWithinHandlerMapping(exchange, requestPath);
277+
StepVerifier.create(handler.handle(exchange))
278+
.expectErrorSatisfies(err -> {
279+
assertThat(err, instanceOf(ResponseStatusException.class));
280+
assertEquals(HttpStatus.NOT_FOUND, ((ResponseStatusException) err).getStatus());
281+
}).verify(TIMEOUT);
282+
}
283+
284+
@Test
285+
public void testResolvePathWithTraversal() throws Exception {
244286
for (HttpMethod method : HttpMethod.values()) {
245-
testInvalidPath(method);
287+
testResolvePathWithTraversal(method);
246288
}
247289
}
248290

249-
private void testInvalidPath(HttpMethod httpMethod) throws Exception {
291+
private void testResolvePathWithTraversal(HttpMethod httpMethod) throws Exception {
250292
Resource location = new ClassPathResource("test/", getClass());
251293
this.handler.setLocations(Collections.singletonList(location));
252294

253-
testInvalidPath(httpMethod, "../testsecret/secret.txt", location);
254-
testInvalidPath(httpMethod, "test/../../testsecret/secret.txt", location);
255-
testInvalidPath(httpMethod, ":/../../testsecret/secret.txt", location);
295+
testResolvePathWithTraversal(httpMethod, "../testsecret/secret.txt", location);
296+
testResolvePathWithTraversal(httpMethod, "test/../../testsecret/secret.txt", location);
297+
testResolvePathWithTraversal(httpMethod, ":/../../testsecret/secret.txt", location);
256298

257299
location = new UrlResource(getClass().getResource("./test/"));
258300
this.handler.setLocations(Collections.singletonList(location));
259301
Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt"));
260302
String secretPath = secretResource.getURL().getPath();
261303

262-
testInvalidPath(httpMethod, "file:" + secretPath, location);
263-
testInvalidPath(httpMethod, "/file:" + secretPath, location);
264-
testInvalidPath(httpMethod, "url:" + secretPath, location);
265-
testInvalidPath(httpMethod, "/url:" + secretPath, location);
266-
testInvalidPath(httpMethod, "////../.." + secretPath, location);
267-
testInvalidPath(httpMethod, "/%2E%2E/testsecret/secret.txt", location);
268-
testInvalidPath(httpMethod, "url:" + secretPath, location);
304+
testResolvePathWithTraversal(httpMethod, "file:" + secretPath, location);
305+
testResolvePathWithTraversal(httpMethod, "/file:" + secretPath, location);
306+
testResolvePathWithTraversal(httpMethod, "url:" + secretPath, location);
307+
testResolvePathWithTraversal(httpMethod, "/url:" + secretPath, location);
308+
testResolvePathWithTraversal(httpMethod, "////../.." + secretPath, location);
309+
testResolvePathWithTraversal(httpMethod, "/%2E%2E/testsecret/secret.txt", location);
310+
testResolvePathWithTraversal(httpMethod, "%2F%2F%2E%2E%2F%2Ftestsecret/secret.txt", location);
311+
testResolvePathWithTraversal(httpMethod, "url:" + secretPath, location);
269312

270313
// The following tests fail with a MalformedURLException on Windows
271-
// testInvalidPath(location, "/" + secretPath);
272-
// testInvalidPath(location, "/ " + secretPath);
314+
// testResolvePathWithTraversal(location, "/" + secretPath);
315+
// testResolvePathWithTraversal(location, "/ " + secretPath);
273316
}
274317

275-
private void testInvalidPath(HttpMethod httpMethod, String requestPath, Resource location) throws Exception {
318+
private void testResolvePathWithTraversal(HttpMethod httpMethod, String requestPath, Resource location) throws Exception {
276319
ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.method(httpMethod, ""));
277320
setPathWithinHandlerMapping(exchange, requestPath);
278321
StepVerifier.create(this.handler.handle(exchange))

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

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -520,7 +520,15 @@ protected Resource getResource(HttpServletRequest request) throws IOException {
520520
if (path.contains("%")) {
521521
try {
522522
// Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars
523-
if (isInvalidPath(URLDecoder.decode(path, "UTF-8"))) {
523+
String decodedPath = URLDecoder.decode(path, "UTF-8");
524+
if (isInvalidPath(decodedPath)) {
525+
if (logger.isTraceEnabled()) {
526+
logger.trace("Ignoring invalid resource path with escape sequences [" + path + "].");
527+
}
528+
return null;
529+
}
530+
decodedPath = processPath(decodedPath);
531+
if (isInvalidPath(decodedPath)) {
524532
if (logger.isTraceEnabled()) {
525533
logger.trace("Ignoring invalid resource path with escape sequences [" + path + "].");
526534
}
@@ -543,13 +551,47 @@ protected Resource getResource(HttpServletRequest request) throws IOException {
543551
}
544552

545553
/**
546-
* Process the given resource path to be used.
547-
* <p>The default implementation replaces any combination of leading '/' and
548-
* control characters (00-1F and 7F) with a single "/" or "". For example
549-
* {@code " // /// //// foo/bar"} becomes {@code "/foo/bar"}.
554+
* Process the given resource path.
555+
* <p>The default implementation replaces:
556+
* <ul>
557+
* <li>Backslash with forward slash.
558+
* <li>Duplicate occurrences of slash with a single slash.
559+
* <li>Any combination of leading slash and control characters (00-1F and 7F)
560+
* with a single "/" or "". For example {@code " / // foo/bar"}
561+
* becomes {@code "/foo/bar"}.
562+
* </ul>
550563
* @since 3.2.12
551564
*/
552565
protected String processPath(String path) {
566+
path = StringUtils.replace(path, "\\", "/");
567+
path = cleanDuplicateSlashes(path);
568+
return cleanLeadingSlash(path);
569+
}
570+
571+
private String cleanDuplicateSlashes(String path) {
572+
StringBuilder sb = null;
573+
char prev = 0;
574+
for (int i = 0; i < path.length(); i++) {
575+
char curr = path.charAt(i);
576+
try {
577+
if ((curr == '/') && (prev == '/')) {
578+
if (sb == null) {
579+
sb = new StringBuilder(path.substring(0, i));
580+
}
581+
continue;
582+
}
583+
if (sb != null) {
584+
sb.append(path.charAt(i));
585+
}
586+
}
587+
finally {
588+
prev = curr;
589+
}
590+
}
591+
return sb != null ? sb.toString() : path;
592+
}
593+
594+
private String cleanLeadingSlash(String path) {
553595
boolean slash = false;
554596
for (int i = 0; i < path.length(); i++) {
555597
if (path.charAt(i) == '/') {

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

Lines changed: 70 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.springframework.web.servlet.HandlerMapping;
4444

4545
import static org.junit.Assert.*;
46+
import static org.mockito.Mockito.*;
4647

4748
/**
4849
* Unit tests for {@link ResourceHttpRequestHandler}.
@@ -303,41 +304,84 @@ public String getVirtualServerName() {
303304
}
304305

305306
@Test
306-
public void invalidPath() throws Exception {
307+
public void testInvalidPath() throws Exception {
308+
309+
// Use mock ResourceResolver: i.e. we're only testing upfront validations...
310+
311+
Resource resource = mock(Resource.class);
312+
when(resource.getFilename()).thenThrow(new AssertionError("Resource should not be resolved"));
313+
when(resource.getInputStream()).thenThrow(new AssertionError("Resource should not be resolved"));
314+
ResourceResolver resolver = mock(ResourceResolver.class);
315+
when(resolver.resolveResource(any(), any(), any(), any())).thenReturn(resource);
316+
317+
ResourceHttpRequestHandler handler = new ResourceHttpRequestHandler();
318+
handler.setLocations(Collections.singletonList(new ClassPathResource("test/", getClass())));
319+
handler.setResourceResolvers(Collections.singletonList(resolver));
320+
handler.setServletContext(new TestServletContext());
321+
handler.afterPropertiesSet();
322+
323+
testInvalidPath("../testsecret/secret.txt", handler);
324+
testInvalidPath("test/../../testsecret/secret.txt", handler);
325+
testInvalidPath(":/../../testsecret/secret.txt", handler);
326+
327+
Resource location = new UrlResource(getClass().getResource("./test/"));
328+
this.handler.setLocations(Collections.singletonList(location));
329+
Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt"));
330+
String secretPath = secretResource.getURL().getPath();
331+
332+
testInvalidPath("file:" + secretPath, handler);
333+
testInvalidPath("/file:" + secretPath, handler);
334+
testInvalidPath("url:" + secretPath, handler);
335+
testInvalidPath("/url:" + secretPath, handler);
336+
testInvalidPath("/../.." + secretPath, handler);
337+
testInvalidPath("/%2E%2E/testsecret/secret.txt", handler);
338+
testInvalidPath("/%2E%2E/testsecret/secret.txt", handler);
339+
testInvalidPath("%2F%2F%2E%2E%2F%2F%2E%2E" + secretPath, handler);
340+
}
341+
342+
private void testInvalidPath(String requestPath, ResourceHttpRequestHandler handler) throws Exception {
343+
this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, requestPath);
344+
this.response = new MockHttpServletResponse();
345+
handler.handleRequest(this.request, this.response);
346+
assertEquals(HttpStatus.NOT_FOUND.value(), this.response.getStatus());
347+
}
348+
349+
@Test
350+
public void resolvePathWithTraversal() throws Exception {
307351
for (HttpMethod method : HttpMethod.values()) {
308352
this.request = new MockHttpServletRequest("GET", "");
309353
this.response = new MockHttpServletResponse();
310-
testInvalidPath(method);
354+
testResolvePathWithTraversal(method);
311355
}
312356
}
313357

314-
private void testInvalidPath(HttpMethod httpMethod) throws Exception {
358+
private void testResolvePathWithTraversal(HttpMethod httpMethod) throws Exception {
315359
this.request.setMethod(httpMethod.name());
316360

317361
Resource location = new ClassPathResource("test/", getClass());
318362
this.handler.setLocations(Collections.singletonList(location));
319363

320-
testInvalidPath(location, "../testsecret/secret.txt");
321-
testInvalidPath(location, "test/../../testsecret/secret.txt");
322-
testInvalidPath(location, ":/../../testsecret/secret.txt");
364+
testResolvePathWithTraversal(location, "../testsecret/secret.txt");
365+
testResolvePathWithTraversal(location, "test/../../testsecret/secret.txt");
366+
testResolvePathWithTraversal(location, ":/../../testsecret/secret.txt");
323367

324368
location = new UrlResource(getClass().getResource("./test/"));
325369
this.handler.setLocations(Collections.singletonList(location));
326370
Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt"));
327371
String secretPath = secretResource.getURL().getPath();
328372

329-
testInvalidPath(location, "file:" + secretPath);
330-
testInvalidPath(location, "/file:" + secretPath);
331-
testInvalidPath(location, "url:" + secretPath);
332-
testInvalidPath(location, "/url:" + secretPath);
333-
testInvalidPath(location, "/" + secretPath);
334-
testInvalidPath(location, "////../.." + secretPath);
335-
testInvalidPath(location, "/%2E%2E/testsecret/secret.txt");
336-
testInvalidPath(location, "/ " + secretPath);
337-
testInvalidPath(location, "url:" + secretPath);
373+
testResolvePathWithTraversal(location, "file:" + secretPath);
374+
testResolvePathWithTraversal(location, "/file:" + secretPath);
375+
testResolvePathWithTraversal(location, "url:" + secretPath);
376+
testResolvePathWithTraversal(location, "/url:" + secretPath);
377+
testResolvePathWithTraversal(location, "/" + secretPath);
378+
testResolvePathWithTraversal(location, "////../.." + secretPath);
379+
testResolvePathWithTraversal(location, "/%2E%2E/testsecret/secret.txt");
380+
testResolvePathWithTraversal(location, "%2F%2F%2E%2E%2F%2Ftestsecret/secret.txt");
381+
testResolvePathWithTraversal(location, "/ " + secretPath);
338382
}
339383

340-
private void testInvalidPath(Resource location, String requestPath) throws Exception {
384+
private void testResolvePathWithTraversal(Resource location, String requestPath) throws Exception {
341385
this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, requestPath);
342386
this.response = new MockHttpServletResponse();
343387
this.handler.handleRequest(this.request, this.response);
@@ -356,7 +400,8 @@ public void ignoreInvalidEscapeSequence() throws Exception {
356400
}
357401

358402
@Test
359-
public void processPath() throws Exception {
403+
public void processPath() {
404+
// Unchanged
360405
assertSame("/foo/bar", this.handler.processPath("/foo/bar"));
361406
assertSame("foo/bar", this.handler.processPath("foo/bar"));
362407

@@ -382,10 +427,17 @@ public void processPath() throws Exception {
382427
assertEquals("/", this.handler.processPath("/"));
383428
assertEquals("/", this.handler.processPath("///"));
384429
assertEquals("/", this.handler.processPath("/ / / "));
430+
assertEquals("/", this.handler.processPath("\\/ \\/ \\/ "));
431+
432+
// duplicate slash or backslash
433+
assertEquals("/foo/ /bar/baz/", this.handler.processPath("//foo/ /bar//baz//"));
434+
assertEquals("/foo/ /bar/baz/", this.handler.processPath("\\\\foo\\ \\bar\\\\baz\\\\"));
435+
assertEquals("foo/bar", this.handler.processPath("foo\\\\/\\////bar"));
436+
385437
}
386438

387439
@Test
388-
public void initAllowedLocations() throws Exception {
440+
public void initAllowedLocations() {
389441
PathResourceResolver resolver = (PathResourceResolver) this.handler.getResourceResolvers().get(0);
390442
Resource[] locations = resolver.getAllowedLocations();
391443

0 commit comments

Comments
 (0)