Issue #3421 Duplicate session set-cookie (#3426)

Added Response.replaceCookieuse replaceCookie in sessions
unit tests

Signed-off-by: Greg Wilkins <gregw@webtide.com>

jetty-security/src/main/java/org/eclipse/jetty/security/authentication/LoginAuthenticator.java

@@ -111,7 +111,7 @@ public abstract class LoginAuthenticator implements Authenticator
                         s.renewId(request);
                         s.setAttribute(Session.SESSION_CREATED_SECURE, Boolean.TRUE);
                         if (s.isIdChanged() && (response instanceof Response))
-                            ((Response)response).addCookie(s.getSessionHandler().getSessionCookie(s, request.getContextPath(), request.isSecure()));
+                            ((Response)response).replaceCookie(s.getSessionHandler().getSessionCookie(s, request.getContextPath(), request.isSecure()));
                         if (LOG.isDebugEnabled())
                             LOG.debug("renew {}->{}", oldId, s.getId());
                     }

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java

@@ -1527,7 +1527,7 @@ public class Request implements HttpServletRequest
             if (getRemoteUser() != null)
                 s.setAttribute(Session.SESSION_CREATED_SECURE, Boolean.TRUE);
             if (s.isIdChanged() && _sessionHandler.isUsingCookies())
-                _channel.getResponse().addCookie(_sessionHandler.getSessionCookie(s, getContextPath(), isSecure()));
+                _channel.getResponse().replaceCookie(_sessionHandler.getSessionCookie(s, getContextPath(), isSecure()));
         }
 
         return session.getId();
@@ -1570,7 +1570,7 @@ public class Request implements HttpServletRequest
         _session = _sessionHandler.newHttpSession(this);
         HttpCookie cookie = _sessionHandler.getSessionCookie(_session,getContextPath(),isSecure());
         if (cookie != null)
-            _channel.getResponse().addCookie(cookie);
+            _channel.getResponse().replaceCookie(cookie);
 
         return _session;
     }

jetty-server/src/main/java/org/eclipse/jetty/server/Response.java

@@ -24,7 +24,9 @@ import java.nio.channels.IllegalSelectorException;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumSet;
+import java.util.Iterator;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.Locale;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Supplier;
@@ -194,6 +196,94 @@ public class Response implements HttpServletResponse
                 cookie.isHttpOnly());
     }
 
+    /**
+     * Replace (or add) a cookie.
+     * Using name, path and domain, look for a matching set-cookie header and replace it.
+     * @param cookie The cookie to add/replace
+     */
+    public void replaceCookie(HttpCookie cookie)
+    {
+        for (ListIterator<HttpField> i = _fields.listIterator(); i.hasNext();)
+        {
+            HttpField field = i.next();
+
+            if (field.getHeader() == HttpHeader.SET_COOKIE)
+            {
+                String old_set_cookie = field.getValue();
+                String name = cookie.getName();
+                if (!old_set_cookie.startsWith(name) || old_set_cookie.length()<= name.length() || old_set_cookie.charAt(name.length())!='=')
+                    continue;
+
+                String domain = cookie.getDomain();
+                if (domain!=null)
+                {
+                    if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance()==CookieCompliance.RFC2965)
+                    {
+                        StringBuilder buf = new StringBuilder();
+                        buf.append(";Domain=");
+                        quoteOnlyOrAppend(buf,domain,isQuoteNeededForCookie(domain));
+                        domain = buf.toString();
+                    }
+                    else
+                    {
+                        domain = ";Domain="+domain;
+                    }
+                    if (!old_set_cookie.contains(domain))
+                        continue;
+                }
+                else if (old_set_cookie.contains(";Domain="))
+                    continue;
+
+                String path = cookie.getPath();
+                if (path!=null)
+                {
+                    if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance()==CookieCompliance.RFC2965)
+                    {
+                        StringBuilder buf = new StringBuilder();
+                        buf.append(";Path=");
+                        quoteOnlyOrAppend(buf,path,isQuoteNeededForCookie(path));
+                        path = buf.toString();
+                    }
+                    else
+                    {
+                        path = ";Path="+path;
+                    }
+                    if (!old_set_cookie.contains(path))
+                        continue;
+                }
+                else if (old_set_cookie.contains(";Path="))
+                    continue;
+
+                if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance() == CookieCompliance.RFC2965)
+                    i.set(new HttpField(HttpHeader.CONTENT_ENCODING.SET_COOKIE, newRFC2965SetCookie(
+                            cookie.getName(),
+                            cookie.getValue(),
+                            cookie.getDomain(),
+                            cookie.getPath(),
+                            cookie.getMaxAge(),
+                            cookie.getComment(),
+                            cookie.isSecure(),
+                            cookie.isHttpOnly(),
+                            cookie.getVersion())
+                    ));
+                else
+                    i.set(new HttpField(HttpHeader.CONTENT_ENCODING.SET_COOKIE, newRFC6265SetCookie(
+                            cookie.getName(),
+                            cookie.getValue(),
+                            cookie.getDomain(),
+                            cookie.getPath(),
+                            cookie.getMaxAge(),
+                            cookie.isSecure(),
+                            cookie.isHttpOnly()
+                    )));
+                return;
+            }
+        }
+
+        // Not replaced, so add normally
+        addCookie(cookie);
+    }
+
     @Override
     public void addCookie(Cookie cookie)
     {
@@ -257,6 +347,18 @@ public class Response implements HttpServletResponse
             final long maxAge,
             final boolean isSecure,
             final boolean isHttpOnly)
+    {
+        String set_cookie = newRFC6265SetCookie(name, value, domain, path, maxAge, isSecure, isHttpOnly);
+
+        // add the set cookie
+        _fields.add(HttpHeader.SET_COOKIE, set_cookie);
+
+        // Expire responses with set-cookie headers so they do not get cached.
+        _fields.put(__EXPIRES_01JAN1970);
+        
+    }
+
+    private String newRFC6265SetCookie(String name, String value, String domain, String path, long maxAge, boolean isSecure, boolean isHttpOnly)
     {
         // Check arguments
         if (name == null || name.length() == 0)
@@ -301,13 +403,7 @@ public class Response implements HttpServletResponse
             buf.append(";Secure");
         if (isHttpOnly)
             buf.append(";HttpOnly");
-        
+        return buf.toString();
-        // add the set cookie
-        _fields.add(HttpHeader.SET_COOKIE, buf.toString());
-
-        // Expire responses with set-cookie headers so they do not get cached.
-        _fields.put(__EXPIRES_01JAN1970);
-        
     }
 
     /**
@@ -333,6 +429,17 @@ public class Response implements HttpServletResponse
             final boolean isSecure,
             final boolean isHttpOnly,
             int version)
+    {
+        String set_cookie = newRFC2965SetCookie(name, value, domain, path, maxAge, comment, isSecure, isHttpOnly, version);
+
+        // add the set cookie
+        _fields.add(HttpHeader.SET_COOKIE, set_cookie);
+
+        // Expire responses with set-cookie headers so they do not get cached.
+        _fields.put(__EXPIRES_01JAN1970);
+    }
+
+    private String newRFC2965SetCookie(String name, String value, String domain, String path, long maxAge, String comment, boolean isSecure, boolean isHttpOnly, int version)
     {
         // Check arguments
         if (name == null || name.length() == 0)
@@ -413,12 +520,7 @@ public class Response implements HttpServletResponse
             buf.append(";Comment=");
             quoteOnlyOrAppend(buf,comment,isQuoteNeededForCookie(comment));
         }
-        
+        return buf.toString();
-        // add the set cookie
-        _fields.add(HttpHeader.SET_COOKIE, buf.toString());
-
-        // Expire responses with set-cookie headers so they do not get cached.
-        _fields.put(__EXPIRES_01JAN1970);
     }
 
 

jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java

@@ -1658,7 +1658,7 @@ public class SessionHandler extends ScopedHandler
                 HttpCookie cookie = access(existingSession,request.isSecure());
                 // Handle changed ID or max-age refresh, but only if this is not a redispatched request
                 if ((cookie != null) && (request.getDispatcherType() == DispatcherType.ASYNC || request.getDispatcherType() == DispatcherType.REQUEST))
-                    baseRequest.getResponse().addCookie(cookie);
+                    baseRequest.getResponse().replaceCookie(cookie);
             }
 
             if (LOG.isDebugEnabled())

jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java

@@ -21,6 +21,7 @@ package org.eclipse.jetty.server;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.hamcrest.CoreMatchers.allOf;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
@@ -37,7 +38,6 @@ import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.LineNumberReader;
 import java.io.PrintWriter;
-import java.net.HttpCookie;
 import java.net.Inet4Address;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
@@ -59,6 +59,7 @@ import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
 
 import org.eclipse.jetty.http.CookieCompliance;
+import org.eclipse.jetty.http.HttpCookie;
 import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpHeader;
@@ -1019,7 +1020,7 @@ public class ResponseTest
     @Test
     public void testAddCookie_JavaNet() throws Exception
     {
-        HttpCookie cookie = new HttpCookie("foo", URLEncoder.encode("bar;baz", UTF_8.toString()));
+        java.net.HttpCookie cookie = new java.net.HttpCookie("foo", URLEncoder.encode("bar;baz", UTF_8.toString()));
         cookie.setPath("/secure");
         
         assertEquals("foo=\"bar%3Bbaz\";$Path=\"/secure\"", cookie.toString());
@@ -1061,6 +1062,38 @@ public class ResponseTest
         assertFalse(set.hasMoreElements());
     }
 
+    @Test
+    public void testReplaceHttpCookie()
+    {
+        Response response = getResponse();
+
+        response.replaceCookie(new HttpCookie("Foo","123456"));
+        response.replaceCookie(new HttpCookie("Foo","123456", "A", "/path"));
+        response.replaceCookie(new HttpCookie("Foo","123456", "B", "/path"));
+
+        response.replaceCookie(new HttpCookie("Bar","123456"));
+        response.replaceCookie(new HttpCookie("Bar","123456",null, "/left"));
+        response.replaceCookie(new HttpCookie("Bar","123456", null, "/right"));
+
+        response.replaceCookie(new HttpCookie("Bar","value", null, "/right"));
+        response.replaceCookie(new HttpCookie("Bar","value",null, "/left"));
+        response.replaceCookie(new HttpCookie("Bar","value"));
+
+        response.replaceCookie(new HttpCookie("Foo","value", "B", "/path"));
+        response.replaceCookie(new HttpCookie("Foo","value", "A", "/path"));
+        response.replaceCookie(new HttpCookie("Foo","value"));
+
+        assertThat(Collections.list(response.getHttpFields().getValues("Set-Cookie")),
+                contains(
+                        "Foo=value",
+                        "Foo=value;Path=/path;Domain=A",
+                        "Foo=value;Path=/path;Domain=B",
+                        "Bar=value",
+                        "Bar=value;Path=/left",
+                        "Bar=value;Path=/right"
+                ));
+    }
+
     @Test
     public void testFlushAfterFullContent() throws Exception
     {

tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/CreationTest.java

@@ -19,6 +19,7 @@
 
 package org.eclipse.jetty.server.session;
 
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -40,11 +41,13 @@ import javax.servlet.http.HttpSession;
 import org.eclipse.jetty.client.HttpClient;
 import org.eclipse.jetty.client.api.ContentResponse;
 import org.eclipse.jetty.client.api.Request;
+import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.eclipse.jetty.servlet.ServletHolder;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.StacklessLogging;
+import org.hamcrest.Matchers;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
@@ -241,8 +244,56 @@ public class CreationTest
 
 
 
+    /**
+     * Create and then invalidate and then create a session in the same request
+     * @throws Exception
+     */
+    @Test
+    public void testSessionCreateInvalidateCreate() throws Exception
+    {
+        String contextPath = "";
+        String servletMapping = "/server";
+        int inactivePeriod = 20;
+        int scavengePeriod = 3;
+        DefaultSessionCacheFactory cacheFactory = new DefaultSessionCacheFactory();
+        cacheFactory.setEvictionPolicy(SessionCache.NEVER_EVICT);
+        SessionDataStoreFactory storeFactory = new TestSessionDataStoreFactory();
+        TestServer server1 = new TestServer(0, inactivePeriod, scavengePeriod, cacheFactory, storeFactory);
+        TestServlet servlet = new TestServlet();
+        ServletHolder holder = new ServletHolder(servlet);
+        ServletContextHandler contextHandler = server1.addContext(contextPath);
+        TestContextScopeListener scopeListener = new TestContextScopeListener();
+        contextHandler.addEventListener(scopeListener);
+        contextHandler.addServlet(holder, servletMapping);
+        servlet.setStore(contextHandler.getSessionHandler().getSessionCache().getSessionDataStore());
+        server1.start();
+        int port1 = server1.getPort();
 
+        try (StacklessLogging stackless = new StacklessLogging(Log.getLogger("org.eclipse.jetty.server.session")))
+        {
+            HttpClient client = new HttpClient();
+            client.start();
+            String url = "http://localhost:" + port1 + contextPath + servletMapping+"?action=createinvcreate&check=false";
 
+            CountDownLatch synchronizer = new CountDownLatch(1);
+            scopeListener.setExitSynchronizer(synchronizer);
+
+            //make a request to set up a session on the server
+            ContentResponse response = client.GET(url);
+            assertEquals(HttpServletResponse.SC_OK,response.getStatus());
+
+            //ensure request has finished being handled
+            synchronizer.await(5, TimeUnit.SECONDS);
+
+            //check that the session does not exist
+            assertTrue(contextHandler.getSessionHandler().getSessionCache().getSessionDataStore().exists(servlet._id));
+            assertThat(response.getHeaders().getValuesList(HttpHeader.SET_COOKIE).size(), Matchers.is(1));
+        }
+        finally
+        {
+            server1.stop();
+        }
+    }
 
     /**
      * Create a session in a context, forward to another context and create a 
@@ -437,6 +488,14 @@ public class CreationTest
                     assertNull(request.getSession(false));
                     assertNotNull(session);
                 }
+                else if ("createinvcreate".equals(action))
+                {
+                    session.invalidate();
+                    assertNull(request.getSession(false));
+                    assertNotNull(session);
+                    session = request.getSession(true);
+                    _id = session.getId();
+                }
             }
         }
     }

tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/SessionRenewTest.java

@@ -51,9 +51,6 @@ public class SessionRenewTest
 {
     protected TestServer _server;
     
- 
-    
-    
     /**
      * Tests renewing a session id when sessions are not being cached.
      * @throws Exception
@@ -236,9 +233,7 @@ public class SessionRenewTest
                 assertNull(session);
 
                 if (((Session)afterSession).isIdChanged())
-                {
+                    ((org.eclipse.jetty.server.Response)response).replaceCookie(sessionManager.getSessionCookie(afterSession, request.getContextPath(), request.isSecure()));
-                    ((org.eclipse.jetty.server.Response)response).addCookie(sessionManager.getSessionCookie(afterSession, request.getContextPath(), request.isSecure()));
-                }
             }
         }
     }