Prevent duplicate session cookies if possible [IMMUTANT-272]

If ring and the servlet container are using the same session cookie
name, we can detect that and remove ring's version of the cookie from
the response.

docs/src/org/web.org

@@ -251,8 +251,9 @@
   session cookie (=JSESSIONID= by default) is itself managed at the
   servlet level. Any options other than =:store= passed to
   =ring.middleware.session/wrap-session= (=:cookie-attrs=,
-  =:cookie-name=, or =:root=) will therefore be ignored. To set the
-  cookie name or attributes, see [[#web-session-options][session options]].
+  =:cookie-name=, or =:root=) won't affect the actual cookie used to
+  store the session id client-side. To set the cookie name or
+  attributes, see [[#web-session-options][session options]].
 
 ** Setting session timeout and cookie attributes
    :PROPERTIES:
@@ -266,9 +267,9 @@
 
    #+begin_src clojure
      (ns my.ns
-       (:require [immutant.web.session :as session]))
+       (:require [immutant.web.session :as immutant-session]))
 
-     (session/set-session-timeout! 1440) ;; 1 day
+     (immutant-session/set-session-timeout! 1440) ;; 1 day
    #+end_src
 
    You can also override the attributes used for the session cookie
@@ -290,19 +291,67 @@
 
    #+begin_src clojure
      (ns my.ns
-       (:require [immutant.web.session :as session]))
+       (:require [immutant.web.session :as immutant-session]))
 
-     (session/set-session-cookie-attributes! :cookie-name "my-session")
+     (immutant-session/set-session-cookie-attributes!
+      :cookie-name "my-session")
 
-     (session/set-session-cookie-attributes!
+     (immutant-session/set-session-cookie-attributes!
       :http-only true
       :max-age   86400)
+
    #+end_src
 
    Changes made by either of these functions apply to all of the web
    endpoints deployed within application, since they all share the
    same session.
 
+** Duplicate session cookies
+
+   Since sessions using the [[./apidoc/immutant.web.session.html#var-servlet-store][servlet-store]] are managed at the container
+   level, the cookie used to convey the session id to the client is
+   managed outside of Ring. However, Ring is unaware of this
+   management, and will attempt to send its own cookie (named
+   "ring-session" by default). This can cause to cookies with the same
+   value (the session id) but different names ("JSESSIONID" and
+   "ring-session") to be sent to the client. This situation is
+   harmless, other than the extra few bytes needed for each request.
+
+   You can prevent this by ensuring Ring's =:cookie-name= is the same
+   name used by the session container. Immutant will detect this case,
+   and prevent the cookie duplication. 
+
+   There are three options for achieving this cookie name parity:
+
+   #+begin_src clojure
+     (ns my.ns
+       (:require [ring.middleware.session :as ring-session]
+                 [immutant.web :as web]
+                 [immutant.web.session :as immutant-session]))
+
+     ;; option 1: pass the default "JSESSIONID" name to Ring's wrap-session
+     (web/start
+      (ring-session/wrap-session
+       #'my-handler
+       {:store       (immutant-session/servlet-store)
+        :cookie-name "JSESSIONID"}))
+
+     ;; option 2: set the container's cookie name to Ring's default of "ring-session"
+     (immutant-session/set-session-cookie-attributes!
+      :cookie-name "ring-session")
+
+     ;; option 3: use a non-default cookie name
+     (web/start
+      (ring-session/wrap-session
+       #'my-handler
+       {:store       (immutant-session/servlet-store)
+        :cookie-name "session-schmession"}))
+
+     (immutant-session/set-session-cookie-attributes!
+      :cookie-name "session-schmession")
+   #+end_src
+
+
 * Locating dirs within the application root
 
   When a web server is embedded within an application, it's fine to

integration-tests/apps/ring/sessions/src/sessions/core.clj

@@ -35,12 +35,14 @@
                       (apply hash-map (clojure.string/split sess-data #":"))
                       {})))))
 
-(defn init-immutant-session []
-  (web/start "/immutant"
-             (ring-session/wrap-session
-              handler
-              {:store (immutant-session/servlet-store)})))
-
+(defn init-immutant-session [path & [cookie-name]]
+  (let [opts {:store (immutant-session/servlet-store)}]
+    (web/start path
+               (ring-session/wrap-session
+                handler
+                (if cookie-name
+                  (assoc opts :cookie-name cookie-name)
+                  opts)))))
 
 (defn init-ring-session [store]
   (web/start "/ring"
@@ -58,11 +60,14 @@
               clear-handler
               {:store store})))
 
-(defn init-immutant-clearer []
-  (web/start "/clear"
-             (ring-session/wrap-session
-              clear-handler
-              {:store (immutant-session/servlet-store)})))
+(defn init-immutant-clearer [path & [cookie-name]]
+  (let [opts  {:store (immutant-session/servlet-store)}]
+    (web/start path
+               (ring-session/wrap-session
+                clear-handler
+                (if cookie-name
+                  (assoc opts :cookie-name cookie-name)
+                  opts)))))
 
 (defn init-session-attrs []
   (web/start "/session-attrs"
@@ -73,6 +78,8 @@
 (defn init-all []
   (let [ring-mem-store (ring.middleware.session.memory/memory-store)]
     (init-ring-session ring-mem-store)
-    (init-immutant-session)
+    (init-immutant-session "/immutant")
+    (init-immutant-session "/immutant-jsessionid" "JSESSIONID")
     (init-ring-clearer ring-mem-store)
-    (init-immutant-clearer)))
+    (init-immutant-clearer "/clear")
+    (init-immutant-clearer "/clear-jsessionid" "JSESSIONID")))

integration-tests/src/test/clojure/immutant/integs/web/sessions.clj

@@ -59,6 +59,15 @@
    (get-with-cookies "immutant" "ham=biscuit")
    (is (= #{"JSESSIONID" "a-cookie" "ring-session"} (set (keys @cookies)))))
 
+(deftest session-clearing-for-immutant-sessions-using-jsessionid
+  (is (= {"ham" "biscuit"} (get-with-cookies "immutant-jsessionid" "ham=biscuit")))
+  (is (not (seq (get-with-cookies "clear-jsessionid"))))
+  (is (not (seq (get-with-cookies "immutant-jsessionid")))))
+
+(deftest immutant-session-with-jssessionid-name-should-not-have-a-ring-session-cookie
+   (get-with-cookies "immutant-jsessionid" "ham=biscuit")
+   (is (= #{"JSESSIONID" "a-cookie"} (set (keys @cookies)))))
+
 (deftest basic-ring-session-test
   (are [expected query-string] (= expected (get-with-cookies "ring" query-string))
        {"ham" "biscuit"}                    "ham=biscuit"

modules/web/src/main/clojure/immutant/web/servlet.clj

@@ -17,6 +17,7 @@
 
 (ns ^{:no-doc true} immutant.web.servlet
   (:use [immutant.web.internal :only [current-servlet-request]]
+        [immutant.web.session.internal :only [servlet-session-wrapper]]
         [immutant.util :only [with-tccl]])
   (:require [ring.util.servlet :as servlet])
   (:import javax.servlet.http.HttpServletRequest))
@@ -39,7 +40,7 @@
       (with-tccl
         (.setCharacterEncoding response "UTF-8")
         (if-let [response-map (binding [current-servlet-request request]
-                                (handler
+                                ((servlet-session-wrapper handler)
                                  (assoc (servlet/build-request-map request)
                                    :context (context request)
                                    :path-info (path-info request))))]
@@ -62,4 +63,4 @@
     (.getServletInfo servlet)))
 
 (defn proxy-servlet [servlet]
-  (ServletProxy. servlet))
+  (ServletProxy. servlet))

modules/web/src/main/clojure/immutant/web/session.clj

@@ -70,18 +70,24 @@
   (when-let [ctx (registry/get "web-context")]
     (.setSessionTimeout ctx minutes)))
 
+(defn session-timeout
+  "Returns the current session timeout in minutes."
+  []
+  (when-let [ctx (registry/get "web-context")]
+    (.getSessionTimeout ctx)))
+
 (defn set-session-cookie-attributes!
   "Set session cookie attributes. Accepts the following kwargs, many
    of which are analagous to the ring session cookie attributes
    [default]:
 
    :cookie-name  The name of the cookie [JSESSIONID]
    :domain       The domain name where the cookie is valid [nil]
-   :path         The path where the cookie is valid [the context path]
    :http-only    Should the cookie be used only for http? [false]
-   :secure       Should the cookie be used only for secure connections? [false]
    :max-age      The amount of time the cookie should be retained by the
                  client, in seconds [-1, meaning 'never expire']
+   :path         The path where the cookie is valid [the context path]
+   :secure       Should the cookie be used only for secure connections? [false]
 
    This function can be called multiple times, and will only alter the
    attributes passed to it.
@@ -96,10 +102,25 @@
                         (f cookie (attrs key))))]
       (set-param :cookie-name #(.setName %1 %2))
       (set-param :domain      #(.setDomain %1 %2))
-      (set-param :path        #(.setPath %1 %2))
       (set-param :http-only   #(.setHttpOnly %1 (boolean %2)))
-      (set-param :secure      #(.setSecure %1 (boolean %2)))
-      (set-param :max-age     #(.setMaxAge %1 %2)))))
+      (set-param :max-age     #(.setMaxAge %1 %2))
+      (set-param :path        #(.setPath %1 %2))
+      (set-param :secure      #(.setSecure %1 (boolean %2))))))
+
+;; TODO: this gets called for each request, so should be type hinted
+;; doing so may require bringing in a jbossweb jar as a dependency to
+;; the public web artifact. See IMMUTANT-271
+(defn session-cookie-attributes
+  "Returns a map of the current session cookie attributes."
+  []
+  (when-let [ctx (registry/get "web-context")]
+    (let [cookie (.getSessionCookie ctx)]
+      {:cookie-name (.getName cookie)
+       :domain      (.getDomain cookie)
+       :http-only   (.isHttpOnly cookie)
+       :max-age     (.getMaxAge cookie)
+       :path        (.getPath cookie)
+       :secure      (.isSecure cookie)})))
 
 
 

modules/web/src/main/clojure/immutant/web/session/internal.clj

@@ -0,0 +1,49 @@
+;; Copyright 2008-2013 Red Hat, Inc, and individual contributors.
+;; 
+;; This is free software; you can redistribute it and/or modify it
+;; under the terms of the GNU Lesser General Public License as
+;; published by the Free Software Foundation; either version 2.1 of
+;; the License, or (at your option) any later version.
+;; 
+;; This software is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+;; Lesser General Public License for more details.
+;; 
+;; You should have received a copy of the GNU Lesser General Public
+;; License along with this software; if not, write to the Free
+;; Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+;; 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+
+(ns ^:no-doc immutant.web.session.internal
+  (:use [immutant.web.internal :only [current-servlet-request]])
+  (:require [immutant.util :as util]
+            [immutant.web.session :as session]))
+
+(def ^:private cookie-encoder
+  (util/try-resolve-any
+   'ring.util.codec/form-encode  ;; ring >= 1.1.0
+   'ring.util.codec/url-encode))
+
+(defn ^:private cookie-matches-servlet-session-cookie?
+  [^javax.servlet.http.HttpSession session ^String cookie]
+  (.startsWith cookie
+               (str (:cookie-name (session/session-cookie-attributes))
+                    \=
+                    (cookie-encoder (.getId session)))))
+
+(defn ^:internal servlet-session-wrapper
+  "Middleware that attempts to prevent duplicate cookies when the
+  servlet session is being used. If the servlet session is active and
+  the response includes a cookie with the same name and id, it is
+  stripped."
+  [handler]
+  (fn [request]
+    (let [response (handler request)
+          session (.getSession current-servlet-request false)]
+      (if (and session (session/using-servlet-session? session))
+        (update-in response [:headers "Set-Cookie"]
+                   #(remove
+                     (partial cookie-matches-servlet-session-cookie? session)
+                     %))
+        response))))

modules/web/src/test/clojure/immutant/web/session/test/internal.clj

@@ -0,0 +1,46 @@
+;; Copyright 2008-2012 Red Hat, Inc, and individual contributors.
+;;
+;; This is free software; you can redistribute it and/or modify it
+;; under the terms of the GNU Lesser General Public License as
+;; published by the Free Software Foundation; either version 2.1 of
+;; the License, or (at your option) any later version.
+;;
+;; This software is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+;; Lesser General Public License for more details.
+;;
+;; You should have received a copy of the GNU Lesser General Public
+;; License along with this software; if not, write to the Free
+;; Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+;; 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+
+(ns immutant.web.session.test.internal
+  (:use immutant.web.session.internal
+        clojure.test)
+  (:require [immutant.web.test.session-utils :as util]
+            [ring.middleware.cookies :as cookies]))
+
+(defn create-response [cookies _]
+  {:headers {"Set-Cookie" cookies}})
+
+(defn mock-session-cookie-attributes []
+  {:cookie-name "the-session"})
+
+(deftest handler-should-strip-cookie-with-servlet-session-name
+  (with-redefs [immutant.web.session/session-cookie-attributes mock-session-cookie-attributes]
+    (are [session-id cookies expected]
+         (= expected
+            (util/session-fixture
+             session-id
+             #(get-in
+               ((servlet-session-wrapper (partial create-response
+                                                  (#'cookies/write-cookies cookies)))
+                nil)
+               [:headers "Set-Cookie"])))
+
+         "session-id" {:the-session "session-id"}                 []
+         "session-id" {:the-session "session-id", :ham "biscuit"} ["ham=biscuit"]
+         "session-id" {:my-session "session-id"}                  ["my-session=session-id"]
+         "session-id" {:the-session "foo"}                        ["the-session=foo"]
+         "has space"  {:the-session "has space"}                  [])))

modules/web/src/test/clojure/immutant/web/test/session_utils.clj

@@ -19,20 +19,21 @@
   (:require [immutant.web.internal :as webint]))
 
 (defn create-mock-session [session-id]
-  (let [store (java.util.Hashtable.)]
-    (proxy [javax.servlet.http.HttpSession]
-        []
-      (getAttribute [key]
-        (.get store key))
-      (removeAttribute [key]
-        (.remove store key))
-      (setAttribute [key value]
-        (.put store key value))
-      (invalidate []
-        (.clear store)
-        nil)
-      (getId []
-        session-id))))
+  (doto (let [store (java.util.Hashtable.)]
+          (proxy [javax.servlet.http.HttpSession]
+              []
+            (getAttribute [key]
+              (.get store key))
+            (removeAttribute [key]
+              (.remove store key))
+            (setAttribute [key value]
+              (.put store key value))
+            (invalidate []
+              (.clear store)
+              nil)
+            (getId []
+              session-id)))
+    (.setAttribute @#'immutant.web.session/session-key true)))
 
 (def ^{:dynamic true} mock-session nil)