Merge pull request #3939 from jsoref/logout-kill-sessions

[JENKINS-25046] Drop cookie sessions when logging out

core/src/main/java/hudson/security/SecurityRealm.java

@@ -68,6 +68,7 @@ import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.util.List;
 import java.util.Map;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
@@ -290,15 +291,50 @@ public abstract class SecurityRealm extends AbstractDescribableImpl<SecurityReal
         Authentication auth = SecurityContextHolder.getContext().getAuthentication();
         SecurityContextHolder.clearContext();
 
-        // reset remember-me cookie
-        Cookie cookie = new Cookie(ACEGI_SECURITY_HASHED_REMEMBER_ME_COOKIE_KEY,"");
+        String contextPath = req.getContextPath().length() > 0 ? req.getContextPath() : "/";
+        resetRememberMeCookie(req, rsp, contextPath);
+        clearStaleSessionCookies(req, rsp, contextPath);
+
+        rsp.sendRedirect2(getPostLogOutUrl(req,auth));
+    }
+
+    private void resetRememberMeCookie(StaplerRequest req, StaplerResponse rsp, String contextPath) {
+        Cookie cookie = new Cookie(ACEGI_SECURITY_HASHED_REMEMBER_ME_COOKIE_KEY, "");
         cookie.setMaxAge(0);
         cookie.setSecure(req.isSecure());
         cookie.setHttpOnly(true);
-        cookie.setPath(req.getContextPath().length()>0 ? req.getContextPath() : "/");
+        cookie.setPath(contextPath);
         rsp.addCookie(cookie);
+    }
 
-        rsp.sendRedirect2(getPostLogOutUrl(req,auth));
+    private void clearStaleSessionCookies(StaplerRequest req, StaplerResponse rsp, String contextPath) {
+        /* While "executableWar.jetty.sessionIdCookieName" and
+         * "executableWar.jetty.disableCustomSessionIdCookieName"
+         * <https://github.com/jenkinsci/extras-executable-war/blob/6558df699d1366b18d045d2ffda3e970df377873/src/main/java/Main.java#L79-L97>
+         * can influence the current running behavior of the generated session cookie, we aren't interested
+         * in either of them at all.
+         *
+         * What matters to us are any stale cookies.
+         * Those cookies would have been created by this jenkins in a different incarnation, when it
+         * could, perhaps, have had different configuration flags, including for those configurables.
+         *
+         * Thus, we unconditionally zap all JSESSIONID. cookies.
+         * a new cookie will be generated by sendRedirect2(...)
+         *
+         * We don't care about JSESSIONID cookies outside our path because it's the browser's
+         * responsibility not to send them to us in the first place.
+         */
+        final String cookieName = "JSESSIONID.";
+        for (Cookie cookie : req.getCookies()) {
+            if (cookie.getName().startsWith(cookieName)) {
+                LOGGER.log(Level.FINE, "Removing cookie {0} during logout", cookie.getName());
+                // one reason users log out is to clear their session(s)
+                // so tell the browser to drop all old sessions
+                cookie.setMaxAge(0);
+                cookie.setValue("");
+                rsp.addCookie(cookie);
+            }
+        }
     }
 
     /**

test/src/test/java/hudson/security/SecurityRealmTest.java

@@ -23,10 +23,11 @@
  */
 package hudson.security;
 
+import com.gargoylesoftware.htmlunit.CookieManager;
 import com.gargoylesoftware.htmlunit.WebResponse;
-import com.gargoylesoftware.htmlunit.util.NameValuePair;
+import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import com.gargoylesoftware.htmlunit.util.Cookie;
 import hudson.security.captcha.CaptchaSupport;
-import org.hamcrest.CoreMatchers;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
@@ -34,9 +35,11 @@ import org.jvnet.hudson.test.JenkinsRule;
 
 import java.io.IOException;
 import java.io.OutputStream;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+
+import java.util.Calendar;
+import java.util.Collections;
+import java.util.Date;
+import java.util.Random;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertEquals;
@@ -79,4 +82,61 @@ public class SecurityRealmTest {
         public void generateImage(String id, OutputStream ios) throws IOException {
         }
     }
+
+    static void addSessionCookie(CookieManager manager, String domain, String path, Date date) {
+        manager.addCookie(new Cookie(domain, "JSESSIONID."+Integer.toHexString(new Random().nextInt()),
+                "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx",
+                path,
+                date,
+                false));
+    }
+
+    @Test
+    public void many_sessions_logout() throws Exception {
+        final String WILL_NOT_BE_SENT = "/will-not-be-sent";
+        final String LOCALHOST = "localhost";
+        final String JSESSIONID = "JSESSIONID";
+
+        JenkinsRule.WebClient wc = j.createWebClient();
+        CookieManager manager = wc.getCookieManager();
+        manager.setCookiesEnabled(true);
+        wc.goTo("login");
+
+        Calendar calendar = Calendar.getInstance();
+        calendar.add(Calendar.DAY_OF_YEAR, 1);
+        Date tomorrow = calendar.getTime();
+        Collections.nCopies(8, 1)
+                .stream()
+                .forEach(i -> addSessionCookie(manager, LOCALHOST, "/jenkins", tomorrow));
+        addSessionCookie(manager, LOCALHOST, WILL_NOT_BE_SENT, tomorrow);
+
+        HtmlPage page = wc.goTo("logout");
+
+        int unexpectedSessionCookies = 2;
+
+        StringBuilder builder = new StringBuilder();
+        builder.append("Session cookies: ");
+
+        for (Cookie cookie : manager.getCookies()) {
+            if (cookie.getName().startsWith(JSESSIONID)) {
+                String path = cookie.getPath();
+
+                builder.append(cookie.getName());
+                if (path != null)
+                    builder.append("; Path=").append(path);
+                builder.append("\n");
+
+                if (WILL_NOT_BE_SENT.equals(path)) {
+                    // Because it wasn't sent and thus wasn't deleted.
+                    --unexpectedSessionCookies;
+                } else if (JSESSIONID.equals(cookie.getName())) {
+                    // Because this test harness isn't winstone and the cleaning
+                    // code is only responsible for deleting "JSESSIONID." cookies.
+                    --unexpectedSessionCookies;
+                }
+            }
+        }
+        System.err.println(builder.toString());
+        assertThat(unexpectedSessionCookies, is(0));
+    }
 }