From 3288eb392f7545687329b8cba18888425a938715 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 26 May 2015 15:25:05 +0200 Subject: [PATCH] 468313 - PushCacheFilter wrongly associates primary resources to themselves. Fixed by not associating secondary resources that are the same as the primary resource. --- .../http2/client/PushCacheFilterTest.java | 130 ++++++++++++++++-- .../jetty/servlets/PushCacheFilter.java | 43 +++--- 2 files changed, 141 insertions(+), 32 deletions(-) diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/PushCacheFilterTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/PushCacheFilterTest.java index c157d06ca03..861fdd18d16 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/PushCacheFilterTest.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/PushCacheFilterTest.java @@ -23,6 +23,7 @@ import java.nio.charset.StandardCharsets; import java.util.EnumSet; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; + import javax.servlet.DispatcherType; import javax.servlet.ServletException; import javax.servlet.ServletOutputStream; @@ -32,6 +33,7 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http2.ErrorCode; import org.eclipse.jetty.http2.api.Session; @@ -82,7 +84,7 @@ public class PushCacheFilterTest extends AbstractTest HttpFields primaryFields = new HttpFields(); MetaData.Request primaryRequest = newRequest("GET", primaryResource, primaryFields); final CountDownLatch warmupLatch = new CountDownLatch(1); - session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -94,7 +96,7 @@ public class PushCacheFilterTest extends AbstractTest HttpFields secondaryFields = new HttpFields(); secondaryFields.put(HttpHeader.REFERER, primaryURI); MetaData.Request secondaryRequest = newRequest("GET", secondaryResource, secondaryFields); - session.newStream(new HeadersFrame(0, secondaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, secondaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -111,7 +113,7 @@ public class PushCacheFilterTest extends AbstractTest primaryRequest = newRequest("GET", primaryResource, primaryFields); final CountDownLatch primaryResponseLatch = new CountDownLatch(1); final CountDownLatch pushLatch = new CountDownLatch(1); - session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public Stream.Listener onPush(Stream stream, PushPromiseFrame frame) @@ -167,7 +169,7 @@ public class PushCacheFilterTest extends AbstractTest HttpFields primaryFields = new HttpFields(); MetaData.Request primaryRequest = newRequest("GET", primaryResource, primaryFields); final CountDownLatch warmupLatch = new CountDownLatch(1); - session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -179,7 +181,7 @@ public class PushCacheFilterTest extends AbstractTest HttpFields secondaryFields = new HttpFields(); secondaryFields.put(HttpHeader.REFERER, primaryURI); MetaData.Request secondaryRequest = newRequest("GET", secondaryResource, secondaryFields); - session.newStream(new HeadersFrame(0, secondaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, secondaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -196,7 +198,7 @@ public class PushCacheFilterTest extends AbstractTest primaryRequest = newRequest("GET", primaryResource, primaryFields); final CountDownLatch primaryResponseLatch = new CountDownLatch(1); final CountDownLatch pushLatch = new CountDownLatch(1); - session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public Stream.Listener onPush(Stream stream, PushPromiseFrame frame) @@ -231,7 +233,7 @@ public class PushCacheFilterTest extends AbstractTest secondaryFields.put(HttpHeader.REFERER, primaryURI); MetaData.Request secondaryRequest = newRequest("GET", secondaryResource, secondaryFields); final CountDownLatch secondaryResponseLatch = new CountDownLatch(1); - session.newStream(new HeadersFrame(0, secondaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, secondaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -267,7 +269,7 @@ public class PushCacheFilterTest extends AbstractTest HttpFields primaryFields = new HttpFields(); MetaData.Request primaryRequest = newRequest("GET", primaryResource, primaryFields); final CountDownLatch warmupLatch = new CountDownLatch(1); - session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public void onHeaders(Stream stream, HeadersFrame frame) @@ -278,7 +280,7 @@ public class PushCacheFilterTest extends AbstractTest HttpFields secondaryFields = new HttpFields(); secondaryFields.put(HttpHeader.REFERER, primaryURI); MetaData.Request secondaryRequest = newRequest("GET", secondaryResource, secondaryFields); - session.newStream(new HeadersFrame(0, secondaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, secondaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -297,7 +299,7 @@ public class PushCacheFilterTest extends AbstractTest primaryRequest = newRequest("GET", primaryResource, primaryFields); final CountDownLatch primaryResponseLatch = new CountDownLatch(1); final CountDownLatch pushLatch = new CountDownLatch(1); - session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public void onHeaders(Stream stream, HeadersFrame frame) @@ -354,7 +356,7 @@ public class PushCacheFilterTest extends AbstractTest HttpFields primaryFields = new HttpFields(); MetaData.Request primaryRequest = newRequest("GET", primaryResource, primaryFields); final CountDownLatch warmupLatch = new CountDownLatch(1); - session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -367,7 +369,7 @@ public class PushCacheFilterTest extends AbstractTest HttpFields secondaryFields = new HttpFields(); secondaryFields.put(HttpHeader.REFERER, primaryURI); MetaData.Request secondaryRequest = newRequest("GET", secondaryResource, secondaryFields); - session.newStream(new HeadersFrame(0, secondaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, secondaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -379,7 +381,7 @@ public class PushCacheFilterTest extends AbstractTest HttpFields tertiaryFields = new HttpFields(); tertiaryFields.put(HttpHeader.REFERER, secondaryURI); MetaData.Request tertiaryRequest = newRequest("GET", tertiaryResource, tertiaryFields); - session.newStream(new HeadersFrame(0, tertiaryRequest, null, true), new Promise.Adapter(), new Adapter() + session.newStream(new HeadersFrame(0, tertiaryRequest, null, true), new Promise.Adapter<>(), new Adapter() { @Override public void onData(Stream stream, DataFrame frame, Callback callback) @@ -402,9 +404,8 @@ public class PushCacheFilterTest extends AbstractTest primaryRequest = newRequest("GET", primaryResource, primaryFields); final CountDownLatch primaryResponseLatch = new CountDownLatch(1); final CountDownLatch pushLatch = new CountDownLatch(2); - session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter(), new Stream.Listener.Adapter() + session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() { - @Override public void onData(Stream stream, DataFrame frame, Callback callback) { @@ -436,4 +437,103 @@ public class PushCacheFilterTest extends AbstractTest Assert.assertTrue(pushLatch.await(5, TimeUnit.SECONDS)); Assert.assertTrue(primaryResponseLatch.await(5, TimeUnit.SECONDS)); } + + @Test + public void testSelfPush() throws Exception + { + // The test case is that of a login page, for example. + // When the user sends the credentials to the login page, + // the login may fail and redirect to the same login page, + // perhaps with different query parameters. + // In this case a request for the login page will push + // the login page itself, which will generate the pushed + // request for the login page, which will push the login + // page itself, etc. which is not the desired behavior. + + final String primaryResource = "/login.html"; + start(new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + { + ServletOutputStream output = response.getOutputStream(); + String credentials = request.getParameter("credentials"); + if (credentials == null) + { + output.print("LOGIN"); + } + else if ("secret".equals(credentials)) + { + output.print("OK"); + } + else + { + response.setStatus(HttpStatus.TEMPORARY_REDIRECT_307); + response.setHeader(HttpHeader.LOCATION.asString(), primaryResource); + } + } + }); + final String primaryURI = "http://localhost:" + connector.getLocalPort() + servletPath + primaryResource; + + final Session session = newClient(new Session.Listener.Adapter()); + + // Login with the wrong credentials, causing a redirect to self. + HttpFields primaryFields = new HttpFields(); + MetaData.Request primaryRequest = newRequest("GET", primaryResource + "?credentials=wrong", primaryFields); + final CountDownLatch warmupLatch = new CountDownLatch(1); + session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onHeaders(Stream stream, HeadersFrame frame) + { + if (frame.isEndStream()) + { + MetaData.Response response = (MetaData.Response)frame.getMetaData(); + if (response.getStatus() == HttpStatus.TEMPORARY_REDIRECT_307) + { + // Follow the redirect. + String location = response.getFields().get(HttpHeader.LOCATION); + HttpFields redirectFields = new HttpFields(); + redirectFields.put(HttpHeader.REFERER, primaryURI); + MetaData.Request redirectRequest = newRequest("GET", location, redirectFields); + session.newStream(new HeadersFrame(0, redirectRequest, null, true), new Promise.Adapter<>(), new Adapter() + { + @Override + public void onData(Stream stream, DataFrame frame, Callback callback) + { + if (frame.isEndStream()) + warmupLatch.countDown(); + } + }); + } + } + } + }); + Assert.assertTrue(warmupLatch.await(5, TimeUnit.SECONDS)); + + Thread.sleep(1000); + + // Login with the right credentials, there must be no push. + primaryRequest = newRequest("GET", primaryResource + "?credentials=secret", primaryFields); + final CountDownLatch primaryResponseLatch = new CountDownLatch(1); + final CountDownLatch pushLatch = new CountDownLatch(1); + session.newStream(new HeadersFrame(0, primaryRequest, null, true), new Promise.Adapter<>(), new Stream.Listener.Adapter() + { + @Override + public void onData(Stream stream, DataFrame frame, Callback callback) + { + if (frame.isEndStream()) + primaryResponseLatch.countDown(); + } + + @Override + public Stream.Listener onPush(Stream stream, PushPromiseFrame frame) + { + pushLatch.countDown(); + return null; + } + }); + Assert.assertFalse(pushLatch.await(1, TimeUnit.SECONDS)); + Assert.assertTrue(primaryResponseLatch.await(5, TimeUnit.SECONDS)); + } } diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/PushCacheFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/PushCacheFilter.java index 0237c601fdb..2565a0276ee 100644 --- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/PushCacheFilter.java +++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/PushCacheFilter.java @@ -30,6 +30,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; + import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; @@ -167,38 +168,46 @@ public class PushCacheFilter implements Filter if (referrerPath.startsWith(request.getContextPath())) { String referrerPathNoContext = referrerPath.substring(request.getContextPath().length()); - PrimaryResource primaryResource = _cache.get(referrerPathNoContext); - if (primaryResource != null) + if (!referrerPathNoContext.equals(path)) { - long primaryTimestamp = primaryResource._timestamp.get(); - if (primaryTimestamp != 0) + PrimaryResource primaryResource = _cache.get(referrerPathNoContext); + if (primaryResource != null) { - RequestDispatcher dispatcher = request.getServletContext().getRequestDispatcher(path); - if (now - primaryTimestamp < TimeUnit.MILLISECONDS.toNanos(_associatePeriod)) + long primaryTimestamp = primaryResource._timestamp.get(); + if (primaryTimestamp != 0) { - ConcurrentMap associated = primaryResource._associated; - // Not strictly concurrent-safe, just best effort to limit associations. - if (associated.size() <= _maxAssociations) + RequestDispatcher dispatcher = request.getServletContext().getRequestDispatcher(path); + if (now - primaryTimestamp < TimeUnit.MILLISECONDS.toNanos(_associatePeriod)) { - if (associated.putIfAbsent(path, dispatcher) == null) + ConcurrentMap associated = primaryResource._associated; + // Not strictly concurrent-safe, just best effort to limit associations. + if (associated.size() <= _maxAssociations) + { + if (associated.putIfAbsent(path, dispatcher) == null) + { + if (LOG.isDebugEnabled()) + LOG.debug("Associated {} to {}", path, referrerPathNoContext); + } + } + else { if (LOG.isDebugEnabled()) - LOG.debug("Associated {} to {}", path, referrerPathNoContext); + LOG.debug("Not associated {} to {}, exceeded max associations of {}", path, referrerPathNoContext, _maxAssociations); } } else { if (LOG.isDebugEnabled()) - LOG.debug("Not associated {} to {}, exceeded max associations of {}", path, referrerPathNoContext, _maxAssociations); + LOG.debug("Not associated {} to {}, outside associate period of {}ms", path, referrerPathNoContext, _associatePeriod); } } - else - { - if (LOG.isDebugEnabled()) - LOG.debug("Not associated {} to {}, outside associate period of {}ms", path, referrerPathNoContext, _associatePeriod); - } } } + else + { + if (LOG.isDebugEnabled()) + LOG.debug("Not associated {} to {}, referring to self", path, referrerPathNoContext); + } } } }