From 679b661e199502fc47f47917dd9a54aea98529b2 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 7 Oct 2016 22:25:56 +0200 Subject: [PATCH] Resolve absolute resource links in ResourceTransformers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, `ResourceTransformer` implementations would resolve internal links to other resources: both relative and absolute request paths. For relative request paths, those transformers would call `ResourceTransformerSupport.resolveUrlPath` with the resource path, as provided in the original file. This can cause problems when a `CachingResourceResolver` is configured in the resolver chain, because this resolver is caching resources, deriving the cache key from the given resource path — this can cause collisions for cases like this: resources/ |--foo/ | |--foo.css (imports style.css) | |--style.css |--bar/ | |--bar.css (imports style.css) | |--style.css The first "style.css" resolved resource is then cached and will be given to any request asking for "style.css". To avoid those issues, this commit improves the `ResourceTransformer` implementations to calculate the absolute request path before asking the chain to resolve the resource URL, thus avoiding duplications. The resource chain will be then asked to resolve "/foo/style/css" or "/bar/style.css". Issue: SPR-14597 --- .../resource/AppCacheManifestTransformer.java | 2 +- .../resource/CssLinkResourceTransformer.java | 2 +- .../resource/ResourceTransformerSupport.java | 15 ++++++ .../AppCacheManifestTransformerTests.java | 54 +++++++++++-------- .../CssLinkResourceTransformerTests.java | 40 +++++++++----- 5 files changed, 76 insertions(+), 37 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/AppCacheManifestTransformer.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/AppCacheManifestTransformer.java index daee253997f..6b26266542d 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/AppCacheManifestTransformer.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/AppCacheManifestTransformer.java @@ -146,7 +146,7 @@ public class AppCacheManifestTransformer extends ResourceTransformerSupport { Resource appCacheResource = transformerChain.getResolverChain() .resolveResource(null, info.getLine(), Collections.singletonList(resource)); - String path = resolveUrlPath(info.getLine(), request, resource, transformerChain); + String path = resolveUrlPath(toAbsolutePath(info.getLine(), request), request, resource, transformerChain); if (logger.isTraceEnabled()) { logger.trace("Link modified: " + path + " (original: " + info.getLine() + ")"); } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/CssLinkResourceTransformer.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/CssLinkResourceTransformer.java index 9c002430edf..f20f4b50722 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/CssLinkResourceTransformer.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/CssLinkResourceTransformer.java @@ -102,7 +102,7 @@ public class CssLinkResourceTransformer extends ResourceTransformerSupport { String link = content.substring(linkSegment.getStart(), linkSegment.getEnd()); String newLink = null; if (!hasScheme(link)) { - newLink = resolveUrlPath(link, request, resource, transformerChain); + newLink = resolveUrlPath(toAbsolutePath(link, request), request, resource, transformerChain); } if (logger.isTraceEnabled()) { if (newLink != null && !link.equals(newLink)) { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceTransformerSupport.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceTransformerSupport.java index c3f0b5e2d6f..ebf5254a7e0 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceTransformerSupport.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceTransformerSupport.java @@ -20,6 +20,7 @@ import java.util.Collections; import javax.servlet.http.HttpServletRequest; import org.springframework.core.io.Resource; +import org.springframework.util.StringUtils; /** * A base class for a {@code ResourceTransformer} with an optional helper method @@ -86,6 +87,20 @@ public abstract class ResourceTransformerSupport implements ResourceTransformer } } + /** + * Transform the given relative request path to an absolute path, + * taking the path of the given request as a point of reference. + * The resulting path is also cleaned from sequences like "path/..". + * @param path the relative path to transform + * @param request the referer request + * @return the absolute request path for the given resource path + */ + protected String toAbsolutePath(String path, HttpServletRequest request) { + String requestPath = this.getResourceUrlProvider().getUrlPathHelper().getRequestUri(request); + String absolutePath = StringUtils.applyRelativePath(requestPath, path); + return StringUtils.cleanPath(absolutePath); + } + private ResourceUrlProvider findResourceUrlProvider(HttpServletRequest request) { if (this.resourceUrlProvider != null) { return this.resourceUrlProvider; diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/AppCacheManifestTransformerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/AppCacheManifestTransformerTests.java index 59f0a72ae8e..8bcf940f872 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/AppCacheManifestTransformerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/AppCacheManifestTransformerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -28,14 +28,14 @@ import org.junit.Test; import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.Resource; +import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.util.FileCopyUtils; import static org.junit.Assert.*; import static org.mockito.BDDMockito.*; /** - * Unit tests for - * {@link AppCacheManifestTransformer}. + * Unit tests for {@link AppCacheManifestTransformer}. * * @author Brian Clozel */ @@ -49,13 +49,34 @@ public class AppCacheManifestTransformerTests { @Before public void setup() { + ClassPathResource allowedLocation = new ClassPathResource("test/", getClass()); + ResourceHttpRequestHandler resourceHandler = new ResourceHttpRequestHandler(); + ResourceUrlProvider resourceUrlProvider = new ResourceUrlProvider(); + resourceUrlProvider.setHandlerMap(Collections.singletonMap("/static/**", resourceHandler)); + + VersionResourceResolver versionResolver = new VersionResourceResolver(); + versionResolver.setStrategyMap(Collections.singletonMap("/**", new ContentVersionStrategy())); + PathResourceResolver pathResolver = new PathResourceResolver(); + pathResolver.setAllowedLocations(allowedLocation); + List resolvers = Arrays.asList(versionResolver, pathResolver); + ResourceResolverChain resolverChain = new DefaultResourceResolverChain(resolvers); + + CssLinkResourceTransformer cssLinkResourceTransformer = new CssLinkResourceTransformer(); + cssLinkResourceTransformer.setResourceUrlProvider(resourceUrlProvider); + List transformers = Arrays.asList(cssLinkResourceTransformer); + this.chain = new DefaultResourceTransformerChain(resolverChain, transformers); this.transformer = new AppCacheManifestTransformer(); - this.chain = mock(ResourceTransformerChain.class); - this.request = mock(HttpServletRequest.class); + this.transformer.setResourceUrlProvider(resourceUrlProvider); + + resourceHandler.setResourceResolvers(resolvers); + resourceHandler.setResourceTransformers(transformers); + resourceHandler.setLocations(Collections.singletonList(allowedLocation)); } @Test public void noTransformIfExtensionNoMatch() throws Exception { + this.chain = mock(ResourceTransformerChain.class); + this.request = mock(HttpServletRequest.class); Resource resource = mock(Resource.class); given(resource.getFilename()).willReturn("foobar.file"); given(this.chain.transform(this.request, resource)).willReturn(resource); @@ -66,6 +87,8 @@ public class AppCacheManifestTransformerTests { @Test public void syntaxErrorInManifest() throws Exception { + this.chain = mock(ResourceTransformerChain.class); + this.request = mock(HttpServletRequest.class); Resource resource = new ClassPathResource("test/error.appcache", getClass()); given(this.chain.transform(this.request, resource)).willReturn(resource); @@ -75,31 +98,18 @@ public class AppCacheManifestTransformerTests { @Test public void transformManifest() throws Exception { - - VersionResourceResolver versionResolver = new VersionResourceResolver(); - versionResolver.setStrategyMap(Collections.singletonMap("/**", new ContentVersionStrategy())); - - PathResourceResolver pathResolver = new PathResourceResolver(); - pathResolver.setAllowedLocations(new ClassPathResource("test/", getClass())); - - List resolvers = Arrays.asList(versionResolver, pathResolver); - ResourceResolverChain resolverChain = new DefaultResourceResolverChain(resolvers); - - List transformers = new ArrayList<>(); - transformers.add(new CssLinkResourceTransformer()); - this.chain = new DefaultResourceTransformerChain(resolverChain, transformers); - + this.request = new MockHttpServletRequest("GET", "/static/test.appcache"); Resource resource = new ClassPathResource("test/test.appcache", getClass()); Resource result = this.transformer.transform(this.request, resource, this.chain); byte[] bytes = FileCopyUtils.copyToByteArray(result.getInputStream()); String content = new String(bytes, "UTF-8"); assertThat("should rewrite resource links", content, - Matchers.containsString("foo-e36d2e05253c6c7085a91522ce43a0b4.css")); + Matchers.containsString("/static/foo-e36d2e05253c6c7085a91522ce43a0b4.css")); assertThat("should rewrite resource links", content, - Matchers.containsString("bar-11e16cf79faee7ac698c805cf28248d2.css")); + Matchers.containsString("/static/bar-11e16cf79faee7ac698c805cf28248d2.css")); assertThat("should rewrite resource links", content, - Matchers.containsString("js/bar-bd508c62235b832d960298ca6c0b7645.js")); + Matchers.containsString("/static/js/bar-bd508c62235b832d960298ca6c0b7645.js")); assertThat("should not rewrite external resources", content, Matchers.containsString("//example.org/style.css")); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/CssLinkResourceTransformerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/CssLinkResourceTransformerTests.java index 05703e0804c..e83c25266dc 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/CssLinkResourceTransformerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/CssLinkResourceTransformerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -36,6 +36,7 @@ import static org.junit.Assert.*; * {@link org.springframework.web.servlet.resource.CssLinkResourceTransformer}. * * @author Rossen Stoyanchev + * @author Brian Clozel * @since 4.1 */ public class CssLinkResourceTransformerTests { @@ -47,34 +48,44 @@ public class CssLinkResourceTransformerTests { @Before public void setUp() { + ClassPathResource allowedLocation = new ClassPathResource("test/", getClass()); + ResourceHttpRequestHandler resourceHandler = new ResourceHttpRequestHandler(); + VersionResourceResolver versionResolver = new VersionResourceResolver(); versionResolver.setStrategyMap(Collections.singletonMap("/**", new ContentVersionStrategy())); - PathResourceResolver pathResolver = new PathResourceResolver(); - pathResolver.setAllowedLocations(new ClassPathResource("test/", getClass())); - + pathResolver.setAllowedLocations(allowedLocation); List resolvers = Arrays.asList(versionResolver, pathResolver); - List transformers = Arrays.asList(new CssLinkResourceTransformer()); + + ResourceUrlProvider resourceUrlProvider = new ResourceUrlProvider(); + resourceUrlProvider.setHandlerMap(Collections.singletonMap("/static/**", resourceHandler)); + + CssLinkResourceTransformer cssLinkResourceTransformer = new CssLinkResourceTransformer(); + cssLinkResourceTransformer.setResourceUrlProvider(resourceUrlProvider); + List transformers = Arrays.asList(cssLinkResourceTransformer); + + resourceHandler.setResourceResolvers(resolvers); + resourceHandler.setResourceTransformers(transformers); + resourceHandler.setLocations(Collections.singletonList(allowedLocation)); ResourceResolverChain resolverChain = new DefaultResourceResolverChain(resolvers); this.transformerChain = new DefaultResourceTransformerChain(resolverChain, transformers); - - this.request = new MockHttpServletRequest(); } @Test public void transform() throws Exception { + this.request = new MockHttpServletRequest("GET", "/static/main.css"); Resource css = new ClassPathResource("test/main.css", getClass()); TransformedResource actual = (TransformedResource) this.transformerChain.transform(this.request, css); String expected = "\n" + - "@import url(\"bar-11e16cf79faee7ac698c805cf28248d2.css\");\n" + - "@import url('bar-11e16cf79faee7ac698c805cf28248d2.css');\n" + - "@import url(bar-11e16cf79faee7ac698c805cf28248d2.css);\n\n" + - "@import \"foo-e36d2e05253c6c7085a91522ce43a0b4.css\";\n" + - "@import 'foo-e36d2e05253c6c7085a91522ce43a0b4.css';\n\n" + - "body { background: url(\"images/image-f448cd1d5dba82b774f3202c878230b3.png\") }\n"; + "@import url(\"/static/bar-11e16cf79faee7ac698c805cf28248d2.css\");\n" + + "@import url('/static/bar-11e16cf79faee7ac698c805cf28248d2.css');\n" + + "@import url(/static/bar-11e16cf79faee7ac698c805cf28248d2.css);\n\n" + + "@import \"/static/foo-e36d2e05253c6c7085a91522ce43a0b4.css\";\n" + + "@import '/static/foo-e36d2e05253c6c7085a91522ce43a0b4.css';\n\n" + + "body { background: url(\"/static/images/image-f448cd1d5dba82b774f3202c878230b3.png\") }\n"; String result = new String(actual.getByteArray(), "UTF-8"); result = StringUtils.deleteAny(result, "\r"); @@ -83,6 +94,7 @@ public class CssLinkResourceTransformerTests { @Test public void transformNoLinks() throws Exception { + this.request = new MockHttpServletRequest("GET", "/static/foo.css"); Resource expected = new ClassPathResource("test/foo.css", getClass()); Resource actual = this.transformerChain.transform(this.request, expected); assertSame(expected, actual); @@ -90,6 +102,7 @@ public class CssLinkResourceTransformerTests { @Test public void transformExtLinksNotAllowed() throws Exception { + this.request = new MockHttpServletRequest("GET", "/static/external.css"); ResourceResolverChain resolverChain = Mockito.mock(DefaultResourceResolverChain.class); ResourceTransformerChain transformerChain = new DefaultResourceTransformerChain(resolverChain, Arrays.asList(new CssLinkResourceTransformer())); @@ -115,6 +128,7 @@ public class CssLinkResourceTransformerTests { @Test public void transformWithNonCssResource() throws Exception { + this.request = new MockHttpServletRequest("GET", "/static/images/image.png"); Resource expected = new ClassPathResource("test/images/image.png", getClass()); Resource actual = this.transformerChain.transform(this.request, expected); assertSame(expected, actual);