From cb44f2746e8854fac384af96a53ef1155d9cd05e Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Wed, 16 Nov 2016 10:26:09 +0100 Subject: [PATCH] Prevent resource transformation of gzipped CSS files When resolved through the `GzipResourceResolver`, CSS files can be resolved as their pre-gzipped variant, if a ".gz" file is present in the configured resource locations. Such resources are gzipped and thus should not be transformed by `CssLinkResourceTransformer`s, since rewriting those would need to uncompress/transform/recompress. This would lead to poorer performances than resolving plain resources and delegating compression to the container. This commit checks for `GzippedResource` instances in `CssLinkResourceTransformer` and avoids processing them. Issue: SPR-14773 --- .../resource/CssLinkResourceTransformer.java | 3 +- .../resource/GzipResourceResolver.java | 2 +- .../CssLinkResourceTransformerTests.java | 69 ++++++++++++++----- .../resource/CssLinkResourceTransformer.java | 3 +- .../resource/GzipResourceResolver.java | 2 +- .../CssLinkResourceTransformerTests.java | 28 +++++++- 6 files changed, 82 insertions(+), 25 deletions(-) diff --git a/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/CssLinkResourceTransformer.java b/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/CssLinkResourceTransformer.java index bcae71eb43..287f57cc2c 100644 --- a/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/CssLinkResourceTransformer.java +++ b/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/CssLinkResourceTransformer.java @@ -73,7 +73,8 @@ public class CssLinkResourceTransformer extends ResourceTransformerSupport { return transformerChain.transform(exchange, resource) .then(newResource -> { String filename = newResource.getFilename(); - if (!"css".equals(StringUtils.getFilenameExtension(filename))) { + if (!"css".equals(StringUtils.getFilenameExtension(filename)) || + resource instanceof GzipResourceResolver.GzippedResource) { return Mono.just(newResource); } diff --git a/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/GzipResourceResolver.java b/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/GzipResourceResolver.java index 9bfe310504..1e46e8fa23 100644 --- a/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/GzipResourceResolver.java +++ b/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/GzipResourceResolver.java @@ -76,7 +76,7 @@ public class GzipResourceResolver extends AbstractResourceResolver { } - private static final class GzippedResource extends AbstractResource implements HttpResource { + static final class GzippedResource extends AbstractResource implements HttpResource { private final Resource original; diff --git a/spring-web-reactive/src/test/java/org/springframework/web/reactive/resource/CssLinkResourceTransformerTests.java b/spring-web-reactive/src/test/java/org/springframework/web/reactive/resource/CssLinkResourceTransformerTests.java index faeacb2ab9..49bbc52917 100644 --- a/spring-web-reactive/src/test/java/org/springframework/web/reactive/resource/CssLinkResourceTransformerTests.java +++ b/spring-web-reactive/src/test/java/org/springframework/web/reactive/resource/CssLinkResourceTransformerTests.java @@ -16,6 +16,11 @@ package org.springframework.web.reactive.resource; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -23,6 +28,7 @@ import java.util.List; import org.junit.Before; import org.junit.Test; import org.mockito.Mockito; +import reactor.test.StepVerifier; import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.Resource; @@ -80,9 +86,6 @@ public class CssLinkResourceTransformerTests { public void transform() throws Exception { initExchange(HttpMethod.GET, "/static/main.css"); Resource css = new ClassPathResource("test/main.css", getClass()); - TransformedResource actual = - (TransformedResource) this.transformerChain.transform(this.exchange, css) - .blockMillis(5000); String expected = "\n" + "@import url(\"/static/bar-11e16cf79faee7ac698c805cf28248d2.css\");\n" + @@ -92,17 +95,24 @@ public class CssLinkResourceTransformerTests { "@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"); - assertEquals(expected, result); + StepVerifier.create(this.transformerChain.transform(this.exchange, css).cast(TransformedResource.class)) + .consumeNextWith(resource -> { + String result = new String(resource.getByteArray(), StandardCharsets.UTF_8); + result = StringUtils.deleteAny(result, "\r"); + assertEquals(expected, result); + }) + .expectComplete().verify(); } @Test public void transformNoLinks() throws Exception { initExchange(HttpMethod.GET, "/static/foo.css"); Resource expected = new ClassPathResource("test/foo.css", getClass()); - Resource actual = this.transformerChain.transform(this.exchange, expected).blockMillis(5000); - assertSame(expected, actual); + StepVerifier.create(this.transformerChain.transform(this.exchange, expected)) + .consumeNextWith(resource -> { + assertSame(expected, resource); + }) + .expectComplete().verify(); } @Test @@ -113,15 +123,15 @@ public class CssLinkResourceTransformerTests { Collections.singletonList(new CssLinkResourceTransformer())); Resource externalCss = new ClassPathResource("test/external.css", getClass()); - Resource resource = transformerChain.transform(this.exchange, externalCss).blockMillis(5000); - TransformedResource transformedResource = (TransformedResource) resource; - - String expected = "@import url(\"http://example.org/fonts/css\");\n" + - "body { background: url(\"file:///home/spring/image.png\") }\n" + - "figure { background: url(\"//example.org/style.css\")}"; - String result = new String(transformedResource.getByteArray(), "UTF-8"); - result = StringUtils.deleteAny(result, "\r"); - assertEquals(expected, result); + StepVerifier.create(transformerChain.transform(this.exchange, externalCss).cast(TransformedResource.class)) + .consumeNextWith(resource -> { + String expected = "@import url(\"http://example.org/fonts/css\");\n" + + "body { background: url(\"file:///home/spring/image.png\") }\n" + + "figure { background: url(\"//example.org/style.css\")}"; + String result = new String(resource.getByteArray(), StandardCharsets.UTF_8); + result = StringUtils.deleteAny(result, "\r"); + assertEquals(expected, result); + }).expectComplete().verify(); Mockito.verify(resolverChain, Mockito.never()) .resolveUrlPath("http://example.org/fonts/css", Collections.singletonList(externalCss)); @@ -135,8 +145,29 @@ public class CssLinkResourceTransformerTests { public void transformWithNonCssResource() throws Exception { initExchange(HttpMethod.GET, "/static/images/image.png"); Resource expected = new ClassPathResource("test/images/image.png", getClass()); - Resource actual = this.transformerChain.transform(this.exchange, expected).blockMillis(5000); - assertSame(expected, actual); + StepVerifier.create(this.transformerChain.transform(this.exchange, expected)) + .expectNext(expected) + .expectComplete().verify(); + } + + @Test + public void transformWithGzippedResource() throws Exception { + initExchange(HttpMethod.GET, "/static/main.css"); + Resource original = new ClassPathResource("test/main.css", getClass()); + createTempCopy("main.css", "main.css.gz"); + GzipResourceResolver.GzippedResource expected = new GzipResourceResolver.GzippedResource(original); + StepVerifier.create(this.transformerChain.transform(this.exchange, expected)) + .expectNext(expected) + .expectComplete().verify(); + } + + private void createTempCopy(String filePath, String copyFilePath) throws IOException { + Resource location = new ClassPathResource("test/", CssLinkResourceTransformerTests.class); + Path original = Paths.get(location.getFile().getAbsolutePath(), filePath); + Path copy = Paths.get(location.getFile().getAbsolutePath(), copyFilePath); + Files.deleteIfExists(copy); + Files.copy(original, copy); + copy.toFile().deleteOnExit(); } private void initExchange(HttpMethod method, String url) { 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 f20f4b5072..adadd3c2e6 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 @@ -70,7 +70,8 @@ public class CssLinkResourceTransformer extends ResourceTransformerSupport { resource = transformerChain.transform(request, resource); String filename = resource.getFilename(); - if (!"css".equals(StringUtils.getFilenameExtension(filename))) { + if (!"css".equals(StringUtils.getFilenameExtension(filename)) || + resource instanceof GzipResourceResolver.GzippedResource) { return resource; } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/GzipResourceResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/GzipResourceResolver.java index 91f06eccaf..7c30083db6 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/GzipResourceResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/GzipResourceResolver.java @@ -78,7 +78,7 @@ public class GzipResourceResolver extends AbstractResourceResolver { } - private static final class GzippedResource extends AbstractResource implements HttpResource { + static final class GzippedResource extends AbstractResource implements HttpResource { private final Resource original; 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 e83c25266d..172b1d62c0 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 @@ -16,6 +16,11 @@ package org.springframework.web.servlet.resource; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -87,7 +92,7 @@ public class CssLinkResourceTransformerTests { "@import '/static/foo-e36d2e05253c6c7085a91522ce43a0b4.css';\n\n" + "body { background: url(\"/static/images/image-f448cd1d5dba82b774f3202c878230b3.png\") }\n"; - String result = new String(actual.getByteArray(), "UTF-8"); + String result = new String(actual.getByteArray(), StandardCharsets.UTF_8); result = StringUtils.deleteAny(result, "\r"); assertEquals(expected, result); } @@ -114,7 +119,7 @@ public class CssLinkResourceTransformerTests { String expected = "@import url(\"http://example.org/fonts/css\");\n" + "body { background: url(\"file:///home/spring/image.png\") }\n" + "figure { background: url(\"//example.org/style.css\")}"; - String result = new String(transformedResource.getByteArray(), "UTF-8"); + String result = new String(transformedResource.getByteArray(), StandardCharsets.UTF_8); result = StringUtils.deleteAny(result, "\r"); assertEquals(expected, result); @@ -134,4 +139,23 @@ public class CssLinkResourceTransformerTests { assertSame(expected, actual); } + @Test + public void transformWithGzippedResource() throws Exception { + this.request = new MockHttpServletRequest("GET", "/static/main.css"); + Resource original = new ClassPathResource("test/main.css", getClass()); + createTempCopy("main.css", "main.css.gz"); + GzipResourceResolver.GzippedResource expected = new GzipResourceResolver.GzippedResource(original); + Resource actual = this.transformerChain.transform(this.request, expected); + assertSame(expected, actual); + } + + private void createTempCopy(String filePath, String copyFilePath) throws IOException { + Resource location = new ClassPathResource("test/", CssLinkResourceTransformerTests.class); + Path original = Paths.get(location.getFile().getAbsolutePath(), filePath); + Path copy = Paths.get(location.getFile().getAbsolutePath(), copyFilePath); + Files.deleteIfExists(copy); + Files.copy(original, copy); + copy.toFile().deleteOnExit(); + } + }