From 88e9dcef0cfef6a53708efb061083119ee3f21ec Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 29 Aug 2019 09:20:50 +0300 Subject: [PATCH 1/4] Consistently apply onCompletion/onError handling Follow-up change in addition to dd22b8fd. See gh-23096 --- .../AbstractListenerReadPublisher.java | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerReadPublisher.java b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerReadPublisher.java index 401e6c867a..b28b6e47a0 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerReadPublisher.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerReadPublisher.java @@ -224,6 +224,23 @@ public abstract class AbstractListenerReadPublisher implements Publisher { } } + private void handleCompletionOrErrorBeforeDemand() { + State state = this.state.get(); + if (!state.equals(State.UNSUBSCRIBED) && !state.equals(State.SUBSCRIBING)) { + if (this.completionBeforeDemand) { + rsReadLogger.trace(getLogPrefix() + "Completed before demand"); + this.state.get().onAllDataRead(this); + } + Throwable ex = this.errorBeforeDemand; + if (ex != null) { + if (rsReadLogger.isTraceEnabled()) { + rsReadLogger.trace(getLogPrefix() + "Completed with error before demand: " + ex); + } + this.state.get().onError(this, ex); + } + } + } + private Subscription createSubscription() { return new ReadSubscription(); } @@ -283,7 +300,7 @@ public abstract class AbstractListenerReadPublisher implements Publisher { publisher.subscriber = subscriber; subscriber.onSubscribe(subscription); publisher.changeState(SUBSCRIBING, NO_DEMAND); - handleCompletionOrErrorBeforeDemand(publisher); + publisher.handleCompletionOrErrorBeforeDemand(); } else { throw new IllegalStateException("Failed to transition to SUBSCRIBING, " + @@ -294,30 +311,13 @@ public abstract class AbstractListenerReadPublisher implements Publisher { @Override void onAllDataRead(AbstractListenerReadPublisher publisher) { publisher.completionBeforeDemand = true; - handleCompletionOrErrorBeforeDemand(publisher); + publisher.handleCompletionOrErrorBeforeDemand(); } @Override void onError(AbstractListenerReadPublisher publisher, Throwable ex) { publisher.errorBeforeDemand = ex; - handleCompletionOrErrorBeforeDemand(publisher); - } - - private void handleCompletionOrErrorBeforeDemand(AbstractListenerReadPublisher publisher) { - if (publisher.state.get().equals(NO_DEMAND)) { - if (publisher.completionBeforeDemand) { - rsReadLogger.trace(publisher.getLogPrefix() + "Completed before demand"); - publisher.state.get().onAllDataRead(publisher); - } - Throwable ex = publisher.errorBeforeDemand; - if (ex != null) { - if (rsReadLogger.isTraceEnabled()) { - String prefix = publisher.getLogPrefix(); - rsReadLogger.trace(prefix + "Completed with error before demand: " + ex); - } - publisher.state.get().onError(publisher, ex); - } - } + publisher.handleCompletionOrErrorBeforeDemand(); } }, @@ -337,11 +337,13 @@ public abstract class AbstractListenerReadPublisher implements Publisher { @Override void onAllDataRead(AbstractListenerReadPublisher publisher) { publisher.completionBeforeDemand = true; + publisher.handleCompletionOrErrorBeforeDemand(); } @Override void onError(AbstractListenerReadPublisher publisher, Throwable ex) { publisher.errorBeforeDemand = ex; + publisher.handleCompletionOrErrorBeforeDemand(); } }, From c2d71922d7a1ee1cfaf805bf4f51a8a6893ab050 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 29 Aug 2019 10:52:16 +0300 Subject: [PATCH 2/4] Fix for change in Jetty 9.4.20.v20190813 Closes gh-23500 --- .../jetty/JettyRequestUpgradeStrategy.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/spring-websocket/src/main/java/org/springframework/web/socket/server/jetty/JettyRequestUpgradeStrategy.java b/spring-websocket/src/main/java/org/springframework/web/socket/server/jetty/JettyRequestUpgradeStrategy.java index 0f89b01e0a..904b477aec 100644 --- a/spring-websocket/src/main/java/org/springframework/web/socket/server/jetty/JettyRequestUpgradeStrategy.java +++ b/spring-websocket/src/main/java/org/springframework/web/socket/server/jetty/JettyRequestUpgradeStrategy.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 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. @@ -17,6 +17,7 @@ package org.springframework.web.socket.server.jetty; import java.io.IOException; +import java.lang.reflect.Method; import java.security.Principal; import java.util.ArrayList; import java.util.List; @@ -28,6 +29,7 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.websocket.api.WebSocketPolicy; import org.eclipse.jetty.websocket.api.extensions.ExtensionConfig; +import org.eclipse.jetty.websocket.api.extensions.ExtensionFactory; import org.eclipse.jetty.websocket.server.HandshakeRFC6455; import org.eclipse.jetty.websocket.server.WebSocketServerFactory; @@ -38,7 +40,9 @@ import org.springframework.http.server.ServerHttpResponse; import org.springframework.http.server.ServletServerHttpRequest; import org.springframework.http.server.ServletServerHttpResponse; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.CollectionUtils; +import org.springframework.util.ReflectionUtils; import org.springframework.web.context.ServletContextAware; import org.springframework.web.socket.WebSocketExtension; import org.springframework.web.socket.WebSocketHandler; @@ -167,7 +171,7 @@ public class JettyRequestUpgradeStrategy implements RequestUpgradeStrategy, Serv } private List buildWebSocketExtensions() { - Set names = this.factory.getExtensionFactory().getExtensionNames(); + Set names = getExtensionNames(); List result = new ArrayList<>(names.size()); for (String name : names) { result.add(new WebSocketExtension(name)); @@ -175,6 +179,18 @@ public class JettyRequestUpgradeStrategy implements RequestUpgradeStrategy, Serv return result; } + @SuppressWarnings({"unchecked"}) + private Set getExtensionNames() { + try { + return this.factory.getExtensionFactory().getExtensionNames(); + } + catch (IncompatibleClassChangeError ex) { + // 9.4.20.v20190813: ExtensionFactory (abstract class -> interface) + Method method = ClassUtils.getMethod(ExtensionFactory.class, "getExtensionNames"); + return (Set) ReflectionUtils.invokeMethod(method, this.factory.getExtensionFactory()); + } + } + @Override public void upgrade(ServerHttpRequest request, ServerHttpResponse response, String selectedProtocol, List selectedExtensions, Principal user, From bd8f94ad7b5253ad5a08126dec51d00f22eb0175 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 29 Aug 2019 11:11:17 +0300 Subject: [PATCH 3/4] Upgrade to Jetty 9.4.20.v20190813 --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 5e9a997798..e2c916b40f 100644 --- a/build.gradle +++ b/build.gradle @@ -34,7 +34,7 @@ ext { groovyVersion = "2.5.7" hsqldbVersion = "2.4.1" jackson2Version = "2.9.9" - jettyVersion = "9.4.19.v20190610" + jettyVersion = "9.4.20.v20190813" junit5Version = "5.3.2" kotlinVersion = "1.2.71" log4jVersion = "2.11.2" From 4e4ec266b2899422b4afe8290a1afd3732d2cad1 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 29 Aug 2019 14:58:03 +0300 Subject: [PATCH 4/4] Adjust error response in ResourceUrlEncodingFilter Failure to find the lookup path now results in 400 instead of 500 reflecting the presence of some issue with the input path. Closes gh-23508 --- .../resource/ResourceUrlEncodingFilter.java | 15 ++++++--- ...esourceUrlProviderExposingInterceptor.java | 10 ++++-- .../ResourceUrlEncodingFilterTests.java | 31 +++++++++++++++++-- 3 files changed, 47 insertions(+), 9 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 77b71215b4..c2a4486a3f 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 @@ -96,10 +96,7 @@ public class ResourceUrlEncodingFilter extends GenericFilterBean { String lookupPath = pathHelper.getLookupPathForRequest(this); this.indexLookupPath = requestUri.lastIndexOf(lookupPath); if (this.indexLookupPath == -1) { - throw new IllegalStateException( - "Failed to find lookupPath '" + lookupPath + "' within requestUri '" + requestUri + "'. " + - "Does the path have invalid encoded characters for characterEncoding '" + - getRequest().getCharacterEncoding() + "'?"); + throw new LookupPathIndexException(lookupPath, requestUri); } this.prefixLookupPath = requestUri.substring(0, this.indexLookupPath); if ("/".equals(lookupPath) && !"/".equals(requestUri)) { @@ -164,4 +161,14 @@ public class ResourceUrlEncodingFilter extends GenericFilterBean { } } + + @SuppressWarnings("serial") + static class LookupPathIndexException extends IllegalArgumentException { + + LookupPathIndexException(String lookupPath, String requestUri) { + super("Failed to find lookupPath '" + lookupPath + "' within requestUri '" + requestUri + "'. " + + "This could be because the path has invalid encoded characters or isn't normalized."); + } + } + } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlProviderExposingInterceptor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlProviderExposingInterceptor.java index 9572843c43..49ec8e25d8 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlProviderExposingInterceptor.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceUrlProviderExposingInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2019 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. @@ -20,6 +20,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.springframework.util.Assert; +import org.springframework.web.bind.ServletRequestBindingException; import org.springframework.web.servlet.handler.HandlerInterceptorAdapter; /** @@ -48,7 +49,12 @@ public class ResourceUrlProviderExposingInterceptor extends HandlerInterceptorAd public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception { - request.setAttribute(RESOURCE_URL_PROVIDER_ATTR, this.resourceUrlProvider); + try { + request.setAttribute(RESOURCE_URL_PROVIDER_ATTR, this.resourceUrlProvider); + } + catch (ResourceUrlEncodingFilter.LookupPathIndexException ex) { + throw new ServletRequestBindingException(ex.getMessage(), ex); + } return true; } 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 6fa6f7294a..0bc6fcd92f 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 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. @@ -19,7 +19,9 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import javax.servlet.FilterChain; import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.junit.Before; @@ -28,6 +30,7 @@ import org.junit.Test; import org.springframework.core.io.ClassPathResource; import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.mock.web.test.MockHttpServletResponse; +import org.springframework.web.bind.ServletRequestBindingException; import static org.junit.Assert.*; @@ -155,14 +158,36 @@ public class ResourceUrlEncodingFilterTests { "/resources/bar-11e16cf79faee7ac698c805cf28248d2.css?foo=bar&url=https://example.org#something"); } + @Test // gh-23508 + public void invalidLookupPath() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setRequestURI("/a/b/../logo.png"); + request.setServletPath("/a/logo.png"); + + this.filter.doFilter(request, new MockHttpServletResponse(), (req, res) -> { + try { + ResourceUrlProviderExposingInterceptor interceptor = + new ResourceUrlProviderExposingInterceptor(this.urlProvider); + + interceptor.preHandle((HttpServletRequest) req, (HttpServletResponse) res, new Object()); + fail(); + } + catch (Exception ex) { + assertEquals(ServletRequestBindingException.class, ex.getClass()); + } + }); + } + private void testEncodeUrl(MockHttpServletRequest request, String url, String expected) throws ServletException, IOException { - this.filter.doFilter(request, new MockHttpServletResponse(), (req, res) -> { + FilterChain chain = (req, res) -> { req.setAttribute(ResourceUrlProviderExposingInterceptor.RESOURCE_URL_PROVIDER_ATTR, this.urlProvider); String result = ((HttpServletResponse) res).encodeURL(url); assertEquals(expected, result); - }); + }; + + this.filter.doFilter(request, new MockHttpServletResponse(), chain); } }