Skip to content

Commit b9ebdaa

Browse files
rstoyanchevrstoyanchev
committed
Backport clean duplicate separators in resource URLs
Issue: SPR-16616
1 parent 4187e04 commit b9ebdaa

File tree

3 files changed

+168
-66
lines changed

3 files changed

+168
-66
lines changed

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

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2017 the original author or authors.
2+
* Copyright 2002-2018 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.
@@ -245,21 +245,7 @@ else if (resource instanceof ServletContextResource) {
245245
return true;
246246
}
247247
locationPath = (locationPath.endsWith("/") || locationPath.isEmpty() ? locationPath : locationPath + "/");
248-
if (!resourcePath.startsWith(locationPath)) {
249-
return false;
250-
}
251-
252-
if (resourcePath.contains("%")) {
253-
// Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars...
254-
if (URLDecoder.decode(resourcePath, "UTF-8").contains("../")) {
255-
if (logger.isTraceEnabled()) {
256-
logger.trace("Resolved resource path contains \"../\" after decoding: " + resourcePath);
257-
}
258-
return false;
259-
}
260-
}
261-
262-
return true;
248+
return (resourcePath.startsWith(locationPath) && !isInvalidEncodedPath(resourcePath));
263249
}
264250

265251
private String encodeIfNecessary(String path, HttpServletRequest request, Resource location) {
@@ -291,8 +277,30 @@ private String encodeIfNecessary(String path, HttpServletRequest request, Resour
291277
}
292278

293279
private boolean shouldEncodeRelativePath(Resource location) {
294-
return location instanceof UrlResource &&
295-
this.urlPathHelper != null && this.urlPathHelper.isUrlDecode();
280+
return (location instanceof UrlResource && this.urlPathHelper != null && this.urlPathHelper.isUrlDecode());
281+
}
282+
283+
private boolean isInvalidEncodedPath(String resourcePath) {
284+
if (resourcePath.contains("%")) {
285+
// Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars...
286+
try {
287+
String decodedPath = URLDecoder.decode(resourcePath, "UTF-8");
288+
int separatorIndex = decodedPath.indexOf("..") + 2;
289+
if (separatorIndex > 1 && separatorIndex < decodedPath.length()) {
290+
char separator = decodedPath.charAt(separatorIndex);
291+
if (separator == '/' || separator == '\\') {
292+
if (logger.isTraceEnabled()) {
293+
logger.trace("Resolved resource path contains \"../\" after decoding: " + resourcePath);
294+
}
295+
}
296+
return true;
297+
}
298+
}
299+
catch (UnsupportedEncodingException ex) {
300+
// Should never happen...
301+
}
302+
}
303+
return false;
296304
}
297305

298306
}

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

Lines changed: 81 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2017 the original author or authors.
2+
* Copyright 2002-2018 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.
@@ -17,6 +17,7 @@
1717
package org.springframework.web.servlet.resource;
1818

1919
import java.io.IOException;
20+
import java.io.UnsupportedEncodingException;
2021
import java.net.URLDecoder;
2122
import java.nio.charset.Charset;
2223
import java.util.ArrayList;
@@ -507,46 +508,75 @@ protected Resource getResource(HttpServletRequest request) throws IOException {
507508
throw new IllegalStateException("Required request attribute '" +
508509
HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE + "' is not set");
509510
}
511+
510512
path = processPath(path);
511513
if (!StringUtils.hasText(path) || isInvalidPath(path)) {
512514
if (logger.isTraceEnabled()) {
513515
logger.trace("Ignoring invalid resource path [" + path + "]");
514516
}
515517
return null;
516518
}
517-
if (path.contains("%")) {
518-
try {
519-
// Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars
520-
if (isInvalidPath(URLDecoder.decode(path, "UTF-8"))) {
521-
if (logger.isTraceEnabled()) {
522-
logger.trace("Ignoring invalid resource path with escape sequences [" + path + "].");
523-
}
524-
return null;
525-
}
526-
}
527-
catch (IllegalArgumentException ex) {
528-
// ignore
519+
if (isInvalidEncodedPath(path)) {
520+
if (logger.isTraceEnabled()) {
521+
logger.trace("Ignoring invalid resource path with escape sequences [" + path + "]");
529522
}
523+
return null;
530524
}
525+
531526
ResourceResolverChain resolveChain = new DefaultResourceResolverChain(getResourceResolvers());
532527
Resource resource = resolveChain.resolveResource(request, path, getLocations());
533528
if (resource == null || getResourceTransformers().isEmpty()) {
534529
return resource;
535530
}
531+
536532
ResourceTransformerChain transformChain =
537533
new DefaultResourceTransformerChain(resolveChain, getResourceTransformers());
538534
resource = transformChain.transform(request, resource);
539535
return resource;
540536
}
541537

542538
/**
543-
* Process the given resource path to be used.
544-
* <p>The default implementation replaces any combination of leading '/' and
545-
* control characters (00-1F and 7F) with a single "/" or "". For example
546-
* {@code " // /// //// foo/bar"} becomes {@code "/foo/bar"}.
539+
* Process the given resource path.
540+
* <p>The default implementation replaces:
541+
* <ul>
542+
* <li>Backslash with forward slash.
543+
* <li>Duplicate occurrences of slash with a single slash.
544+
* <li>Any combination of leading slash and control characters (00-1F and 7F)
545+
* with a single "/" or "". For example {@code " / // foo/bar"}
546+
* becomes {@code "/foo/bar"}.
547+
* </ul>
547548
* @since 3.2.12
548549
*/
549550
protected String processPath(String path) {
551+
path = StringUtils.replace(path, "\\", "/");
552+
path = cleanDuplicateSlashes(path);
553+
return cleanLeadingSlash(path);
554+
}
555+
556+
private String cleanDuplicateSlashes(String path) {
557+
StringBuilder sb = null;
558+
char prev = 0;
559+
for (int i = 0; i < path.length(); i++) {
560+
char curr = path.charAt(i);
561+
try {
562+
if ((curr == '/') && (prev == '/')) {
563+
if (sb == null) {
564+
sb = new StringBuilder(path.substring(0, i));
565+
}
566+
continue;
567+
}
568+
if (sb != null) {
569+
sb.append(path.charAt(i));
570+
}
571+
}
572+
finally {
573+
prev = curr;
574+
}
575+
}
576+
return sb != null ? sb.toString() : path;
577+
}
578+
579+
private String cleanLeadingSlash(String path) {
550580
boolean slash = false;
551581
for (int i = 0; i < path.length(); i++) {
552582
if (path.charAt(i) == '/') {
@@ -556,16 +586,44 @@ else if (path.charAt(i) > ' ' && path.charAt(i) != 127) {
556586
if (i == 0 || (i == 1 && slash)) {
557587
return path;
558588
}
559-
path = slash ? "/" + path.substring(i) : path.substring(i);
589+
path = (slash ? "/" + path.substring(i) : path.substring(i));
560590
if (logger.isTraceEnabled()) {
561-
logger.trace("Path after trimming leading '/' and control characters: " + path);
591+
logger.trace("Path after trimming leading '/' and control characters: [" + path + "]");
562592
}
563593
return path;
564594
}
565595
}
566596
return (slash ? "/" : "");
567597
}
568598

599+
/**
600+
* Check whether the given path contains invalid escape sequences.
601+
* @param path the path to validate
602+
* @return {@code true} if the path is invalid, {@code false} otherwise
603+
*/
604+
private boolean isInvalidEncodedPath(String path) {
605+
if (path.contains("%")) {
606+
try {
607+
// Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars
608+
String decodedPath = URLDecoder.decode(path, "UTF-8");
609+
if (isInvalidPath(decodedPath)) {
610+
return true;
611+
}
612+
decodedPath = processPath(decodedPath);
613+
if (isInvalidPath(decodedPath)) {
614+
return true;
615+
}
616+
}
617+
catch (IllegalArgumentException ex) {
618+
// Should never happen...
619+
}
620+
catch (UnsupportedEncodingException ex) {
621+
// Should never happen...
622+
}
623+
}
624+
return false;
625+
}
626+
569627
/**
570628
* Identifies invalid resource paths. By default rejects:
571629
* <ul>
@@ -580,32 +638,24 @@ else if (path.charAt(i) > ' ' && path.charAt(i) != 127) {
580638
* path starts predictably with a single '/' or does not have one.
581639
* @param path the path to validate
582640
* @return {@code true} if the path is invalid, {@code false} otherwise
641+
* @since 3.0.6
583642
*/
584643
protected boolean isInvalidPath(String path) {
585-
if (logger.isTraceEnabled()) {
586-
logger.trace("Applying \"invalid path\" checks to path: " + path);
587-
}
588644
if (path.contains("WEB-INF") || path.contains("META-INF")) {
589-
if (logger.isTraceEnabled()) {
590-
logger.trace("Path contains \"WEB-INF\" or \"META-INF\".");
591-
}
645+
logger.trace("Path contains \"WEB-INF\" or \"META-INF\".");
592646
return true;
593647
}
594648
if (path.contains(":/")) {
595649
String relativePath = (path.charAt(0) == '/' ? path.substring(1) : path);
596650
if (ResourceUtils.isUrl(relativePath) || relativePath.startsWith("url:")) {
597-
if (logger.isTraceEnabled()) {
598-
logger.trace("Path represents URL or has \"url:\" prefix.");
599-
}
651+
logger.trace("Path represents URL or has \"url:\" prefix.");
600652
return true;
601653
}
602654
}
603655
if (path.contains("..")) {
604656
path = StringUtils.cleanPath(path);
605657
if (path.contains("../")) {
606-
if (logger.isTraceEnabled()) {
607-
logger.trace("Path contains \"../\" after call to StringUtils#cleanPath.");
608-
}
658+
logger.trace("Path contains \"../\" after call to StringUtils#cleanPath.");
609659
return true;
610660
}
611661
}

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

Lines changed: 61 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2017 the original author or authors.
2+
* Copyright 2002-2018 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.
@@ -46,6 +46,7 @@
4646
import org.springframework.web.servlet.HandlerMapping;
4747

4848
import static org.junit.Assert.*;
49+
import static org.mockito.Mockito.*;
4950

5051
/**
5152
* Unit tests for {@link ResourceHttpRequestHandler}.
@@ -310,41 +311,84 @@ public String getVirtualServerName() {
310311
}
311312

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

321-
private void testInvalidPath(HttpMethod httpMethod) throws Exception {
365+
private void testResolvePathWithTraversal(HttpMethod httpMethod) throws Exception {
322366
this.request.setMethod(httpMethod.name());
323367

324368
Resource location = new ClassPathResource("test/", getClass());
325369
this.handler.setLocations(Collections.singletonList(location));
326370

327-
testInvalidPath(location, "../testsecret/secret.txt");
328-
testInvalidPath(location, "test/../../testsecret/secret.txt");
329-
testInvalidPath(location, ":/../../testsecret/secret.txt");
371+
testResolvePathWithTraversal(location, "../testsecret/secret.txt");
372+
testResolvePathWithTraversal(location, "test/../../testsecret/secret.txt");
373+
testResolvePathWithTraversal(location, ":/../../testsecret/secret.txt");
330374

331375
location = new UrlResource(getClass().getResource("./test/"));
332376
this.handler.setLocations(Collections.singletonList(location));
333377
Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt"));
334378
String secretPath = secretResource.getURL().getPath();
335379

336-
testInvalidPath(location, "file:" + secretPath);
337-
testInvalidPath(location, "/file:" + secretPath);
338-
testInvalidPath(location, "url:" + secretPath);
339-
testInvalidPath(location, "/url:" + secretPath);
340-
testInvalidPath(location, "/" + secretPath);
341-
testInvalidPath(location, "////../.." + secretPath);
342-
testInvalidPath(location, "/%2E%2E/testsecret/secret.txt");
343-
testInvalidPath(location, "/ " + secretPath);
344-
testInvalidPath(location, "url:" + secretPath);
380+
testResolvePathWithTraversal(location, "file:" + secretPath);
381+
testResolvePathWithTraversal(location, "/file:" + secretPath);
382+
testResolvePathWithTraversal(location, "url:" + secretPath);
383+
testResolvePathWithTraversal(location, "/url:" + secretPath);
384+
testResolvePathWithTraversal(location, "/" + secretPath);
385+
testResolvePathWithTraversal(location, "////../.." + secretPath);
386+
testResolvePathWithTraversal(location, "/%2E%2E/testsecret/secret.txt");
387+
testResolvePathWithTraversal(location, "%2F%2F%2E%2E%2F%2Ftestsecret/secret.txt");
388+
testResolvePathWithTraversal(location, "/ " + secretPath);
345389
}
346390

347-
private void testInvalidPath(Resource location, String requestPath) throws Exception {
391+
private void testResolvePathWithTraversal(Resource location, String requestPath) throws Exception {
348392
this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, requestPath);
349393
this.response = new MockHttpServletResponse();
350394
this.handler.handleRequest(this.request, this.response);

0 commit comments

Comments
 (0)