From 959cf61647b582b7153eaa7627ae439e2d06c7b5 Mon Sep 17 00:00:00 2001 From: Ondrej Kraus Date: Fri, 23 Nov 2018 18:34:37 +0100 Subject: [PATCH 1/2] Sanitize request fragment in ResourceUrlEncodingFilter Prior to this change, ResourceUrlEncodingFilter would try to resolve the resource path using request URL without removing fragment first, whereas only paths should be used. This commit synchronizes behavior of ResourceUrlEncodingFilter with behavior of ResourceUrlProvider. Issue: SPR-17535 --- .../resource/ResourceUrlEncodingFilter.java | 16 +++++++++--- .../ResourceUrlEncodingFilterTests.java | 26 +++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilter.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilter.java index e50a48e812..f36ac7b649 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilter.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilter.java @@ -115,7 +115,7 @@ public class ResourceUrlEncodingFilter extends GenericFilterBean { return null; } if (this.indexLookupPath != null && url.startsWith(this.prefixLookupPath)) { - int suffixIndex = getQueryParamsIndex(url); + int suffixIndex = getEndPathIndex(url); String suffix = url.substring(suffixIndex); String lookupPath = url.substring(this.indexLookupPath, suffixIndex); lookupPath = this.resourceUrlProvider.getForLookupPath(lookupPath); @@ -126,9 +126,17 @@ public class ResourceUrlEncodingFilter extends GenericFilterBean { return null; } - private int getQueryParamsIndex(String url) { - int index = url.indexOf('?'); - return (index > 0 ? index : url.length()); + private int getEndPathIndex(String lookupPath) { + int suffixIndex = lookupPath.length(); + int queryIndex = lookupPath.indexOf('?'); + if (queryIndex > 0) { + suffixIndex = queryIndex; + } + int hashIndex = lookupPath.indexOf('#'); + if (hashIndex > 0) { + suffixIndex = Math.min(suffixIndex, hashIndex); + } + return suffixIndex; } } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilterTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilterTests.java index b704445c42..c24b830bc3 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilterTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilterTests.java @@ -173,4 +173,30 @@ public class ResourceUrlEncodingFilterTests { }); } + @Test // SPR-17535 + public void encodeURLWitFragment() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/foo"); + request.setContextPath("/"); + MockHttpServletResponse response = new MockHttpServletResponse(); + + this.filter.doFilter(request, response, (req, res) -> { + req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); + String result = ((HttpServletResponse) res).encodeURL("/resources/bar.css#something"); + assertEquals("/resources/bar-11e16cf79faee7ac698c805cf28248d2.css#something", result); + }); + } + + @Test // SPR-13374 and SPR-17535 combined + public void encodeURLWitFragmentAndRequestParams() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/foo"); + request.setContextPath("/"); + MockHttpServletResponse response = new MockHttpServletResponse(); + + this.filter.doFilter(request, response, (req, res) -> { + req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); + String result = ((HttpServletResponse) res).encodeURL("/resources/bar.css?foo=bar&url=http://example.org#something"); + assertEquals("/resources/bar-11e16cf79faee7ac698c805cf28248d2.css?foo=bar&url=http://example.org#something", result); + }); + } + } From 3eee118b44afbd1c87e8b00fb52eb803df4dcabf Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Mon, 26 Nov 2018 16:00:47 -0500 Subject: [PATCH 2/2] Polish --- .../resource/ResourceUrlEncodingFilter.java | 17 ++-- .../ResourceUrlEncodingFilterTests.java | 94 ++++++------------- 2 files changed, 38 insertions(+), 73 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilter.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilter.java index f36ac7b649..ea9ebb1064 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilter.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilter.java @@ -126,17 +126,16 @@ public class ResourceUrlEncodingFilter extends GenericFilterBean { return null; } - private int getEndPathIndex(String lookupPath) { - int suffixIndex = lookupPath.length(); - int queryIndex = lookupPath.indexOf('?'); - if (queryIndex > 0) { - suffixIndex = queryIndex; + private int getEndPathIndex(String path) { + int end = path.indexOf('?'); + int fragmentIndex = path.indexOf('#'); + if (fragmentIndex != -1 && (end == -1 || fragmentIndex < end)) { + end = fragmentIndex; } - int hashIndex = lookupPath.indexOf('#'); - if (hashIndex > 0) { - suffixIndex = Math.min(suffixIndex, hashIndex); + if (end == -1) { + end = path.length(); } - return suffixIndex; + return end; } } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilterTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilterTests.java index c24b830bc3..2e4015cb3f 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilterTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlEncodingFilterTests.java @@ -15,9 +15,11 @@ */ package org.springframework.web.servlet.resource; +import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import javax.servlet.ServletException; import javax.servlet.http.HttpServletResponse; import org.junit.Before; @@ -68,27 +70,17 @@ public class ResourceUrlEncodingFilterTests { @Test public void encodeURL() throws Exception { - MockHttpServletRequest request = new MockHttpServletRequest("GET", "/"); - MockHttpServletResponse response = new MockHttpServletResponse(); - - this.filter.doFilter(request, response, (req, res) -> { - req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); - String result = ((HttpServletResponse) res).encodeURL("/resources/bar.css"); - assertEquals("/resources/bar-11e16cf79faee7ac698c805cf28248d2.css", result); - }); + testEncodeUrl(new MockHttpServletRequest("GET", "/"), + "/resources/bar.css", "/resources/bar-11e16cf79faee7ac698c805cf28248d2.css"); } @Test - public void encodeURLWithContext() throws Exception { + public void encodeUrlWithContext() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/context/foo"); request.setContextPath("/context"); - MockHttpServletResponse response = new MockHttpServletResponse(); - this.filter.doFilter(request, response, (req, res) -> { - req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); - String result = ((HttpServletResponse) res).encodeURL("/context/resources/bar.css"); - assertEquals("/context/resources/bar-11e16cf79faee7ac698c805cf28248d2.css", result); - }); + testEncodeUrl(request, "/context/resources/bar.css", + "/context/resources/bar-11e16cf79faee7ac698c805cf28248d2.css"); } @@ -96,9 +88,8 @@ public class ResourceUrlEncodingFilterTests { public void encodeUrlWithContextAndForwardedRequest() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/context/foo"); request.setContextPath("/context"); - MockHttpServletResponse response = new MockHttpServletResponse(); - this.filter.doFilter(request, response, (req, res) -> { + this.filter.doFilter(request, new MockHttpServletResponse(), (req, res) -> { req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); request.setRequestURI("/forwarded"); request.setContextPath("/"); @@ -111,52 +102,35 @@ public class ResourceUrlEncodingFilterTests { public void encodeContextPathUrlWithoutSuffix() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/context"); request.setContextPath("/context"); - MockHttpServletResponse response = new MockHttpServletResponse(); - this.filter.doFilter(request, response, (req, res) -> { - req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); - String result = ((HttpServletResponse) res).encodeURL("/context/resources/bar.css"); - assertEquals("/context/resources/bar-11e16cf79faee7ac698c805cf28248d2.css", result); - }); + testEncodeUrl(request, "/context/resources/bar.css", + "/context/resources/bar-11e16cf79faee7ac698c805cf28248d2.css"); } @Test public void encodeContextPathUrlWithSuffix() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/context/"); request.setContextPath("/context"); - MockHttpServletResponse response = new MockHttpServletResponse(); - this.filter.doFilter(request, response, (req, res) -> { - req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); - String result = ((HttpServletResponse) res).encodeURL("/context/resources/bar.css"); - assertEquals("/context/resources/bar-11e16cf79faee7ac698c805cf28248d2.css", result); - }); + testEncodeUrl(request, "/context/resources/bar.css", + "/context/resources/bar-11e16cf79faee7ac698c805cf28248d2.css"); } @Test // SPR-13018 - public void encodeEmptyURLWithContext() throws Exception { + public void encodeEmptyUrlWithContext() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/context/foo"); request.setContextPath("/context"); - MockHttpServletResponse response = new MockHttpServletResponse(); - this.filter.doFilter(request, response, (req, res) -> { - req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); - String result = ((HttpServletResponse) res).encodeURL("?foo=1"); - assertEquals("?foo=1", result); - }); + testEncodeUrl(request, "?foo=1", "?foo=1"); } @Test // SPR-13374 - public void encodeURLWithRequestParams() throws Exception { + public void encodeUrlWithRequestParams() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/foo"); request.setContextPath("/"); - MockHttpServletResponse response = new MockHttpServletResponse(); - this.filter.doFilter(request, response, (req, res) -> { - req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); - String result = ((HttpServletResponse) res).encodeURL("/resources/bar.css?foo=bar&url=http://example.org"); - assertEquals("/resources/bar-11e16cf79faee7ac698c805cf28248d2.css?foo=bar&url=http://example.org", result); - }); + testEncodeUrl(request, "/resources/bar.css?foo=bar&url=http://example.org", + "/resources/bar-11e16cf79faee7ac698c805cf28248d2.css?foo=bar&url=http://example.org"); } @Test // SPR-13847 @@ -164,38 +138,30 @@ public class ResourceUrlEncodingFilterTests { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/context-path/index"); request.setContextPath("/context-path"); request.setServletPath(""); - MockHttpServletResponse response = new MockHttpServletResponse(); - this.filter.doFilter(request, response, (req, res) -> { - req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); - String result = ((HttpServletResponse) res).encodeURL("index?key=value"); - assertEquals("index?key=value", result); - }); + testEncodeUrl(request, "index?key=value", "index?key=value"); } @Test // SPR-17535 - public void encodeURLWitFragment() throws Exception { + public void encodeUrlWithFragment() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/foo"); request.setContextPath("/"); - MockHttpServletResponse response = new MockHttpServletResponse(); - this.filter.doFilter(request, response, (req, res) -> { - req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); - String result = ((HttpServletResponse) res).encodeURL("/resources/bar.css#something"); - assertEquals("/resources/bar-11e16cf79faee7ac698c805cf28248d2.css#something", result); - }); + testEncodeUrl(request, "/resources/bar.css#something", + "/resources/bar-11e16cf79faee7ac698c805cf28248d2.css#something"); + + testEncodeUrl(request, + "/resources/bar.css?foo=bar&url=http://example.org#something", + "/resources/bar-11e16cf79faee7ac698c805cf28248d2.css?foo=bar&url=http://example.org#something"); } - @Test // SPR-13374 and SPR-17535 combined - public void encodeURLWitFragmentAndRequestParams() throws Exception { - MockHttpServletRequest request = new MockHttpServletRequest("GET", "/foo"); - request.setContextPath("/"); - MockHttpServletResponse response = new MockHttpServletResponse(); + private void testEncodeUrl(MockHttpServletRequest request, String url, String expected) + throws ServletException, IOException { - this.filter.doFilter(request, response, (req, res) -> { + this.filter.doFilter(request, new MockHttpServletResponse(), (req, res) -> { req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); - String result = ((HttpServletResponse) res).encodeURL("/resources/bar.css?foo=bar&url=http://example.org#something"); - assertEquals("/resources/bar-11e16cf79faee7ac698c805cf28248d2.css?foo=bar&url=http://example.org#something", result); + String result = ((HttpServletResponse) res).encodeURL(url); + assertEquals(expected, result); }); }