reagent.cookies/set! {:secure? true :same-site :none} not behaving correctly?
KaliszAd opened this issue · 21 comments
I try to set SameSite=None; Secure cookies but it doesn't seem to work. It seems like the max-age has no effect either. Previously, I at least had Secure cookies, when the SameSite option wasn't present.
...
[org.clojure/clojurescript "1.10.866"]
[reagent "0.10.0"]
[reagent-utils "0.3.4"]
[thheller/shadow-cljs "2.14.5"]
...
The behaviour is as desired besides the missing SameSite option with just these dependency changes:
[org.clojure/clojurescript "1.10.742"]
[reagent "0.10.0"]
[reagent-utils "0.3.3"]
[thheller/shadow-cljs "2.11.26"] (which brings with it ClojureScript 1.10.773 and that is the actually running version)
This might be caused by API changes in Google closure library that's bundled with different versions of cljs. ClojureScript 1.10.866 just updated to a newer version of Google closure compiler. If you'd have time to take a look at what the correct API to use is, I can help with a PR and pushing out a new version.
I thought this was handled in Support new GCL and same-site attribute #18 12 days ago :-) that was touching exactly this behaviour it seems. @axrs would be probably better equipped to look into this?
The problem I have is, the Expiry time of the cookies in the problematic setting is set to session instead of a date way in the future. This would indicate, reagent-utils or GCL sees max-age of -1 (which is the default, when nothing is passed). Also the Secure option is missing and there is no SameSite attribute. So basically, as if no options were passed at all.
Ah yeah, sounds like there's a bit more work needed there, @axrs any chance you'd be able to take a look?
I'll jump in and have a look.
I've had a look and run the test using:
ClojureScript 1.10.866
GCL v20210505
(which are the 2 versions specified in shadow-cljs 2.11.26)
The following generates the cookie:
(cookies/set! k "multi-opt-cookie" {:same-site :none
:max-age 60
:secure? true})multi-opt-cookie=\"multi-opt-cookie\";expires=Mon, 21 Jun 2021 05:46:54 GMT;secure;samesite=none
Which has valid secure, samesite and expires attributes.
Please note (from https://shadow-cljs.github.io/docs/UsersGuide.html#Leiningen)
It is very important that shadow-cljs is used with proper matching org.clojure/clojurescript and closure-compiler versions. You can check via lein deps :tree and the required versions are listed on clojars (on the right side).
[org.clojure/clojurescript "1.10.742"] is not the correct version for [thheller/shadow-cljs "2.11.26"].
Running ClojureScript 1.10.742 (which uses an older version of GCL than what Shadow-CLJS uses)
(cookies/set! k "multi-opt-cookie" {:same-site :none
:max-age 60
:secure? true})Correctly generates (without the same site)
multi-opt-cookie=\"multi-opt-cookie\";expires=Mon, 21 Jun 2021 06:01:44 GMT;secure
@KaliszAd When you're testing the cookie, you're using a HTTPS connection right?
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite#fixing_common_warnings
@axrs Yes, everything is HTTPS. Development, testing and production.
This is "the old" behaviour I see:
This is the incorrect behaviour I see.
We wrap the (cookies/set! ...) for use with re-frame. Here it is slightly redacted at :domain. This enables us to work with events like this:
(-> effects
...
(cookies/add-cookie #:cookie{:key :feedback-button-hidden
:value true
:secure true
;:same-site :none
:max-age 2147483647})
...)(defn add-cookie
"Adds a new cookie described by a map. The supported keys are:
:cookie/key - name of cookie.
:cookie/value - value of cookie.
:cookie/secure - true if should be secure.
:cookie/max-age - the count in seconds.
:cookie/same-site - same-site attribute of either :strict, :lax, or :none (default)."
[effects m]
(update-in effects [:cookies :cookies/update-keys] vectors/conj-vd m))
(defn remove-cookie [effects k]
(update-in effects [:cookies :cookies/remove-keys] vectors/conj-vd k))
(rf/reg-fx
:cookies
(fn [{:cookies/keys [update-keys remove-keys]}]
(doseq [k (vectors/wrap-vd remove-keys)]
(cookies/remove! k "/" (get-domain)))
(doseq [{:cookie/keys [key value secure same-site max-age]} (vectors/wrap-vd update-keys)]
(cookies/set! key value (maps/prune-nils {:secure? secure
:same-site same-site
:max-age max-age
:path "/"
:domain "example.com"})))))I don't think, this is where the problem lies. In the REPL, this is what I get with shadow-cljs 2.11.26, which I don't think cannot include a GCL from May, when it was released in March.:
*clojurescript-version*
=> "1.10.773"
@axrs I seem to get it working with the new ClojureScript and shadow-cljs... but only in dev. If I use shadow-cljs watch client it seems to work with proper Expiration set to max-age, Secure as true and SameSite=None correctly as expected. If I use shadow-cljs release client and reload everything just using this build, I get the bad behaviour - e.g. Expiration set to session, Secure set to false and SameSite=None in Firefox, in Chrome SameSite is missing altogether. Investigating further.
@axrs I noticed the behaviour on our staging server, where a clean build happens on a fresh VM each time (GitHub Actions). I looked at the dependency graph, only ClojureScript 1.10.866 is present. Though I see:
[com.google.javascript/closure-compiler-unshaded "v20210505"]
...
[org.clojure/google-closure-library-third-party "0.0-20201211-3e6c510d"]
[org.clojure/google-closure-library "0.0-20201211-3e6c510d"]
as a dependency to shadow-cljs 2.14.5. Perhaps this makes problems?
They do cause an issue. All three libraries:
- com.google.javascript/closure-compiler-unshaded
- org.clojure/google-closure-library-third-party
- org.clojure/google-closure-library
Need to match the shadow-cljs defined versions. As mentioned in https://shadow-cljs.github.io/docs/UsersGuide.html#Leiningen
What versions of three libraries are being included? You can use https://shadow-cljs.github.io/docs/UsersGuide.html#build-report to see what version is actually being bundled/used.
@axrs really thankful for your inputs! You are completely right: org.clojure/google-closure-library @ mvn: 0.0-20201211-3e6c510d
Just trying the leiningen thing now.
We use the following. Defining shadow-cljs first gives it's dependencies preference. Excluding the other 3 from clojurescript ensures they pickup the version from shadow.
:dependencies [[thheller/shadow-cljs "2.14.5"]
[org.clojure/clojurescript "1.10.866" :exclusions [com.google.javascript/closure-compiler-unshaded org.clojure/google-closure-library org.clojure/google-closure-library-third-party]]
...
@axrs I cannot get it to work. Either the app doesn't load at all (with some failures in the console tab of the browser, like undefined something in shared.js) or the cookies don't work as described.
Are the dependencies complete? Have you tested the cookies as described with these dependencies? Because shadow-cljs seems to bring with it an older GCL from 2020, just the compiler seems to be from this year.
@axrs @theller writes in the Clojurians shadow-cljs Slack channel:
I'd probably just use the GCL code directly without the reagent-utils
looks like something in the GCL code changed. it is now a separate namespace apparently
https://github.com/google/closure-library/blob/master/closure/goog/net/cookies_deprecated.js
cookies_deprecated.js
goog.net.cookiesis what reagent-utils uses but it is nowgoog.net.CookiesI guess?
but as far as I know SameSite must be set by the server?
it really doesn't matter if you set it on the client? or does it? not sure (edited)
https://github.com/google/closure-library/blob/master/closure/goog/net/cookies.js#L167-L229 I'd just port this to clojure and be done with it
goog.net.Cookies.prototype.set = function(name, value, options) {
'use strict';
/** @type {number|undefined} */
let maxAge;
/** @type {string|undefined} */
...
Perhaps the problems stems from the fact, that in development there is the full GCL and therefore the goog.net.Cookies dependency is present and the release build removes it because it thinks it isn't used? This is quite the assumption...
Ok, I have rewritten basically the whole namespace more to my liking and to not depend on Google Closure Library in any way. The code is smaller (< 100 lines), has better error checks and most importantly fixes the behaviour for me. I might push it somewhere if there is any interest.
So you can't just set max-age. You need to atleast also specify secure? for your cookie to be set. Is this intended? If so I can maybe suggest a doc change.
Yes, specifying same-site really only makes sense, if you also specify secure? to be true.
I'm running into the same issue, even with the latest version of everything.
I'm using shadow-cljs, and the issue disappears when I use {:optimizations :simple}, it occurs only with {:optimizations :advanced}. This makes it difficult to debug, because it only occurs with minified code... I'm thinking it may be caused by dead-code elimination removing something incorrectly?
Interestingly, I still run into the issue even when not using reagent-utils, but using goog.net.cookies directly. So it doesn't seem to be caused by this library, but rather something in the combo shadow-cljs/GCL.
@KaliszAd you mention re-implementing without Closure, I'd be interested in your implementation :)
I'm running into the same issue, even with the latest version of everything.
I'm using shadow-cljs, and the issue disappears when I use
{:optimizations :simple}, it occurs only with{:optimizations :advanced}. This makes it difficult to debug, because it only occurs with minified code... I'm thinking it may be caused by dead-code elimination removing something incorrectly?Interestingly, I still run into the issue even when not using reagent-utils, but using
goog.net.cookiesdirectly. So it doesn't seem to be caused by this library, but rather something in the combo shadow-cljs/GCL.@KaliszAd you mention re-implementing without Closure, I'd be interested in your implementation :)
Suit yourself, consider this to be under the MIT or EPL (same as Clojure) licence. It is not complete-complete, you need some utility functions that you should be able to throw out for your needs or reimplement quickly. This works for us since this summer or so in production. We haven't observed any issues.
,,,
(defn key->string
"Valid key is a keyword or a string that doesn't contain any '=', ';', or other more subtle forms of white space
(even special codes). If the input is a keyword, it is converted into a string and the leading colon is removed.
This is more strict than IETF RFC2109 and RFC2965, it is also more strict than goog.net.Cookies.prototype.isValidName."
[cookie-key]
(let [cookie-key' (when (keyword? cookie-key) (str-util/keyword->str cookie-key))]
(when (and (string? cookie-key')
(re-matches #"[^;=\s]*" cookie-key')
(re-matches str-util/undesirable-whitespace-checker cookie-key')) ; Removes pretty much all thinkable whitespace
cookie-key')))
(defn value->string
"Valid value doesn't contain any ';' or line break or other more subtle forms of white space (even special codes).
This is more strict than IETF RFC2109 and RFC2965, it is also more strict than
goog.net.Cookies.prototype.isValidName and the Google Closure Library mentions possible truncation on line breaks
and parsing failures on semicolons."
[value-as-string]
(when (and (or (string? value-as-string))
(re-matches #"[^;\r\n]*" value-as-string)
(re-matches str-util/undesirable-whitespace-checker value-as-string)) ; Removes pretty much all thinkable whitespace
value-as-string))
(defn get-cookies
"Retrieves cookies and parses them into a map of keywords to EDN expressions.
Gracefully handles e.g. invalid EDN in as the value or a title with disallowed characters. In such a case
at least the rest of the cookies will be parsed."
[]
(when-let [cookie-pairs (not-empty (map #(str/split % "=") (str/split (.-cookie js/document) "; ")))]
(->> cookie-pairs
(map (fn [[k v]] [(keyword k) (try (reader/read-string v)
(catch js/Error e
(js/console.log "Failure parsing cookie value:" (.-message e)) nil))]))
maps/prune-nils ; removes key-value pair where the value is nil
(into {}) not-empty)))
(defn get-cookie-by-name
"Gets just a single cookie, accepts a string or a keyword."
[cookie-key]
(get (get-cookies) cookie-key))
(defn set-cookie!
"Sets a single cookie by providing a cookie name as keyword or string, an arbitrary EDN value
(which will be stored as a serialized string) and a map of attributes.
Following optional parameters may be passed in as a map:
:cookie/expires - Optional date e.g. 2030-09-19T17:15:13 or 1999-09-19 when the cookie should expire.
:cookie/max-age - Optional time difference to current time in seconds. Session cookie if neither expires nor
max-age is provided. If max-age is e.g. -1 the cookie will be removed,
if max-age is :cookie/max then 2147483647 (about 68 years) is used.
:cookie/path - path of the cookie, defaults to the full request path.
:cookie/domain - domain of the cookie, when null the browser will use the full request host name.
:cookie/secure - boolean specifying whether the cookie should only be sent over a secure channel. (default true)
:cookie/same-site - A keyword of either :cookie/strict, :cookie/lax, or :cookie/none (default).
Consult https://developer.mozilla.org/en-US/docs/Web/API/Document/cookie for the specification of the JavaScript API."
[cookie-key cookie-value {:cookie/keys [expires max-age path domain same-site secure]
:or {same-site :cookie/none
secure true}}]
(let [checked-key (key->string cookie-key)
checked-value-as-edn (value->string (pr-str cookie-value))
options (maps/prune-nils {:expires (when expires
(str (date/parse expires)))
:max-age (if (= max-age :cookie/max) 2147483647 max-age)
:path path
:domain domain
:sameSite same-site
:secure (boolean secure)})
cookie-setup-string (str checked-key "=" checked-value-as-edn "; "
(->> options
(map (fn [[k v]] (str (str-util/keyword->str k) ; stringifys a keyword (removes the leading colon also)
(when (not (true? v))
(str "=" (if (keyword? v) (name v) v))))))
(str/join "; ")))]
(if (and (< (count cookie-setup-string) 4000)
checked-key
checked-value-as-edn)
(set! (.-cookie js/document) cookie-setup-string)
(js/console.log "Cookie setup failed, the cookie exceeded 4000 B or the cookie-key or -value are invalid."))))
(defn remove-cookie!
"Removes a cookie by setting negative max-age, optionally for a specific path and/or domain"
([cookie-key]
(set-cookie! cookie-key "" #:cookie{:max-age -1}))
([cookie-key path domain]
(set-cookie! cookie-key "" #:cookie{:max-age -1
:path path
:domain domain})))