Session and cookie middleware don't share respect options

[thumbnail]

Initially asked on slack

When combining middleware.session and middleware.cookie with custom options, the encoding/decoding doesn't work as expected. Either no cookies get encoded, or none get decoded. It seems to happen because the session-middleware manually invokes cookie-middleware functions instead of relying on the chain.

We need this in our app to add signatures to all cookies; without making it a concern of each handler.

Repro

(ns ring-repro.core
  (:require
   [ring.middleware.cookies :as middleware.cookies]
   [ring.middleware.session :as middleware.session]
   [ring.util.codec :as codec])
  (:import
   (java.util Base64)))

(defn handler [request]
  (println (:cookies request))
  {:status 200
   :headers {"Content-Type" "text/html"}
   :cookies {"test_cookie" {:value "my contents"}}
   :session {:user-id #uuid"16efc919-1661-4136-a797-8cada63b73d9"}
   :body "Hello World"})

(defn base64-encode [m]
  (let [[[k v]] (seq m)]
    (codec/form-encode {k (.encodeToString (Base64/getEncoder) (.getBytes v))})))

(defn base64-decode [v]
  (String. (.decode (Base64/getDecoder) (.getBytes (codec/form-decode-str v)))))

(def app
  (-> handler
      (middleware.cookies/wrap-cookies {:decoder base64-decode
                                        :encoder base64-encode})
      #_(middleware.session/wrap-session)))

(comment
 (use 'ring.adapter.jetty)

 (def server (atom nil))
 (swap! server (fn [x]
                 (some-> x (.stop))
                 (run-jetty app {:port  3000
                                 :join? false}))))

steps:

Sanity check; this should work as intended:

1. clear cookies
2. visit localhost:3000 twice, notice output: {test_cookie {:value my contents}}

(working as intended)

Adding session middleware

1. uncomment session middleware on L28, revisit localhost:3000
2. notice output: {test_cookie {:value bXkgY29udGVudHM=}, ring-session {:value bacded51-211e-4f3a-b755-ade76886d1a9}}
3. The session is present, but the cookie isn't decoded. Also the ring-session cookie isn't encoded.

Note that placing wrap-session before wrap-cookies will url-encode the test_cookie, and skip encoding (breaking on decoding, as cookies are plaintext instead of base64).

Workaround

If the cookie-middleware is removed from the session-middleware by removing these lines: https://github.com/ring-clojure/ring/blob/ac821a51d0d30f97f3471f026bb4898bebaaae94/ring-core/src/ring/middleware/session.clj#L75 https://github.com/ring-clojure/ring/blob/ac821a51d0d30f97f3471f026bb4898bebaaae94/ring-core/src/ring/middleware/session.clj#L40

the session-middleware and cookie-middleware play along nicely

[weavejester]

Thanks for raising this issue. My thought for resolving it would be to add an option :set-cookies? to the session middlewware that defaults to true, but can be set to false in order to disable this behaviour.

Of course, the ideal would be to not have the session and cookie middleware complected to begin with, but unfortunately we have to live with past mistakes to ensure backward compatibility. However, we may be able to solve this issue in Ring 2.0.

[thumbnail]

My thought for resolving it would be to add an option :set-cookies? to the session middleware that defaults to true, but can be set to false in order to disable this behaviour.

I'd be happy to contribute such an option if want! just LMK.

but unfortunately we have to live with past mistakes to ensure backward compatibility.

Yeah this was my concern too. Another possible solution would to create a new namespace. (lacinia-pedestal did a similar thing a while back).

However, we may be able to solve this issue in Ring 2.0.

In the issue about ring 2.0 you mention it should maintain 1.0 compatibility, wouldn't that be the same issue?

---

On a separate thought; Originally I wanted to encrypt the cookies (very similar to the session.cookie store). I ended up implementing a very similar solution as cookie encoder/decoder, but it would be nice to extract the encryption from ring.middleware.session.cookie into separate encoders/decoders instead. WDYT?

[weavejester]

I'd be happy to contribute such an option if want! just LMK.

Sure. PRs are welcome.

In the issue about ring 2.0 you mention it should maintain 1.0 compatibility, wouldn't that be the same issue?

For performance purposes, I was considering ring.middleware2.* namespaces that contain middleware that will only work with the updated request maps, and as the namespace differs, we can fix any legacy issues with these functions as well. The ring.middleware namespace is intended work with both request maps, and should be backward compatible.

On a separate thought; Originally I wanted to encrypt the cookies (very similar to the session.cookie store). I ended up implementing a very similar solution as cookie encoder/decoder, but it would be nice to extract the encryption from ring.middleware.session.cookie into separate encoders/decoders instead. WDYT?

My first thought is that it might be too niche a requirement to have as part of Ring core.

[thumbnail]

Sure. PRs are welcome.

Allright, I have a fix locally will create the PR when I have a test ready too!

My first thought is that it might be too niche a requirement to have as part of Ring core.

That seems fair 👍 . it's more about decomplecting the session.cookie store. Moving the encryption to cookie-encoders/decoder lowers the responsibility for the session-stores too. By splitting the two, ring allows consumers to use different session-stores (without duplicating the encryption logic), or change the encryption logic without duplicating the session store.

But I agree that it's a niche ;). I just noticed I needed to duplicate the encryption while building a database-backed session-store.

[weavejester]

Note: A fix for this issue should also resolve #392.