From a72f8088972c7154e3400e59449de5bd2c5023e5 Mon Sep 17 00:00:00 2001 From: "Chinmay D. Pai" Date: Thu, 18 Jun 2020 18:09:27 +0530 Subject: [PATCH 1/2] fix: make cookies more secure * add HTTPOnly to sid, so sid will no longer be accessible through javascript. * mark all cookies Secure by default over HTTPS. * set SameSite to Strict for all cookies by default, preventing cross-origin cookie access. * remove redundant publish_realtime for setting csrf_token on the client-side. Signed-off-by: Chinmay D. Pai --- frappe/auth.py | 21 ++++++++++++++++----- frappe/public/js/frappe/desk.js | 9 --------- frappe/public/js/frappe/request.js | 4 ++-- frappe/sessions.py | 7 ------- frappe/website/js/website.js | 9 ++------- 5 files changed, 20 insertions(+), 30 deletions(-) diff --git a/frappe/auth.py b/frappe/auth.py index 1353acf10f..ab3624bee8 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -333,12 +333,20 @@ class CookieManager: # sid expires in 3 days expires = datetime.datetime.now() + datetime.timedelta(days=3) if frappe.session.sid: - self.cookies["sid"] = {"value": frappe.session.sid, "expires": expires} + self.set_cookie("sid", frappe.session.sid, expires=expires, httponly=True) if frappe.session.session_country: - self.cookies["country"] = {"value": frappe.session.get("session_country")} + self.set_cookie("country", frappe.session.session_country) - def set_cookie(self, key, value, expires=None): - self.cookies[key] = {"value": value, "expires": expires} + def set_cookie(self, key, value, expires=None, secure=False, httponly=False, samesite="Strict"): + if not secure: + secure = frappe.local.request.scheme == "https" + self.cookies[key] = { + "value": value, + "expires": expires, + "secure": secure, + "httponly": httponly, + "samesite": samesite + } def delete_cookie(self, to_delete): if not isinstance(to_delete, (list, tuple)): @@ -349,7 +357,10 @@ class CookieManager: def flush_cookies(self, response): for key, opts in self.cookies.items(): response.set_cookie(key, quote((opts.get("value") or "").encode('utf-8')), - expires=opts.get("expires")) + expires=opts.get("expires"), + secure=opts.get("secure"), + httponly=opts.get("httponly"), + samesite=opts.get("samesite")) # expires yesterday! expires = datetime.datetime.now() + datetime.timedelta(days=-1) diff --git a/frappe/public/js/frappe/desk.js b/frappe/public/js/frappe/desk.js index 79a78717cb..2e80dbfd85 100644 --- a/frappe/public/js/frappe/desk.js +++ b/frappe/public/js/frappe/desk.js @@ -101,15 +101,6 @@ frappe.Application = Class.extend({ frappe.ui.startup_setup_dialog.show(); } - // listen to csrf_update - frappe.realtime.on("csrf_generated", function(data) { - // handles the case when a user logs in again from another tab - // and it leads to invalid request in the current tab - if (data.csrf_token && data.sid===frappe.get_cookie("sid")) { - frappe.csrf_token = data.csrf_token; - } - }); - frappe.realtime.on("version-update", function() { var dialog = frappe.msgprint({ message:__("The application has been updated to a new version, please refresh this page"), diff --git a/frappe/public/js/frappe/request.js b/frappe/public/js/frappe/request.js index ea4de99249..e378499f35 100644 --- a/frappe/public/js/frappe/request.js +++ b/frappe/public/js/frappe/request.js @@ -125,7 +125,7 @@ frappe.request.call = function(opts) { message: __('The resource you are looking for is not available')}); }, 403: function(xhr) { - if (frappe.get_cookie('sid')==='Guest') { + if (frappe.session.user === 'Guest') { // session expired frappe.app.handle_session_expired(); } @@ -320,7 +320,7 @@ frappe.request.cleanup = function(opts, r) { if(r) { // session expired? - Guest has no business here! - if(r.session_expired || frappe.get_cookie("sid")==="Guest") { + if (r.session_expired || frappe.session.user === "Guest") { frappe.app.handle_session_expired(); return; } diff --git a/frappe/sessions.py b/frappe/sessions.py index d317d6caf3..7a018bb0aa 100644 --- a/frappe/sessions.py +++ b/frappe/sessions.py @@ -172,13 +172,6 @@ def generate_csrf_token(): frappe.local.session.data.csrf_token = frappe.generate_hash() frappe.local.session_obj.update(force=True) - # send sid and csrf token to the user - # handles the case when a user logs in again from another tab - # and it leads to invalid request in the current tab - frappe.publish_realtime(event="csrf_generated", - message={"sid": frappe.local.session.sid, "csrf_token": frappe.local.session.data.csrf_token}, - user=frappe.session.user, after_commit=True) - class Session: def __init__(self, user, resume=False, full_name=None, user_type=None): self.sid = cstr(frappe.form_dict.get('sid') or diff --git a/frappe/website/js/website.js b/frappe/website/js/website.js index b52a3fef63..f2c142fe4c 100644 --- a/frappe/website/js/website.js +++ b/frappe/website/js/website.js @@ -183,10 +183,6 @@ $.extend(frappe, { .html('

' +text+'
').appendTo(document.body); }, - get_sid: function() { - var sid = frappe.get_cookie("sid"); - return sid && sid !== "Guest"; - }, send_message: function(opts, btn) { return frappe.call({ type: "POST", @@ -212,8 +208,7 @@ $.extend(frappe, { }); }, render_user: function() { - var sid = frappe.get_cookie("sid"); - if(sid && sid!=="Guest") { + if (frappe.is_user_logged_in()) { $(".btn-login-area").toggle(false); $(".logged-in").toggle(true); $(".full-name").html(frappe.get_cookie("full_name")); @@ -323,7 +318,7 @@ $.extend(frappe, { return $(".navbar .search, .sidebar .search"); }, is_user_logged_in: function() { - return frappe.get_cookie("sid") && frappe.get_cookie("sid") !== "Guest"; + return frappe.session.user !== "Guest"; }, add_switch_to_desk: function() { $('.switch-to-desk').removeClass('hidden'); From a29fab70ac5653c81bda5d076a06db83789e47d0 Mon Sep 17 00:00:00 2001 From: "Chinmay D. Pai" Date: Thu, 18 Jun 2020 23:14:19 +0530 Subject: [PATCH 2/2] chore: improve logic for checking logged in user frappe.session.user sometimes gets set after the function runs, so current implementation would return undefined !== "Guest" as True, causing Guest user to be set as a logged-in user. Signed-off-by: Chinmay D. Pai --- frappe/website/js/website.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/website/js/website.js b/frappe/website/js/website.js index f2c142fe4c..cd9609cfd3 100644 --- a/frappe/website/js/website.js +++ b/frappe/website/js/website.js @@ -318,7 +318,7 @@ $.extend(frappe, { return $(".navbar .search, .sidebar .search"); }, is_user_logged_in: function() { - return frappe.session.user !== "Guest"; + return frappe.get_cookie("user_id") !== "Guest" && frappe.session.user !== "Guest"; }, add_switch_to_desk: function() { $('.switch-to-desk').removeClass('hidden');