From a72f8088972c7154e3400e59449de5bd2c5023e5 Mon Sep 17 00:00:00 2001 From: "Chinmay D. Pai" Date: Thu, 18 Jun 2020 18:09:27 +0530 Subject: [PATCH 001/140] 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 002/140] 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'); From aae9133594a915b19506144749674b567730b398 Mon Sep 17 00:00:00 2001 From: "Chinmay D. Pai" Date: Wed, 24 Jun 2020 11:15:24 +0530 Subject: [PATCH 003/140] fix: disallow access to signup page when disabled signup page is still accessible through the URL even when the button is not shown. this disables access to the page when signups are disabled. Signed-off-by: Chinmay D. Pai --- frappe/www/login.html | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/frappe/www/login.html b/frappe/www/login.html index ebbff748ec..f1131db8cf 100644 --- a/frappe/www/login.html +++ b/frappe/www/login.html @@ -70,20 +70,29 @@
From 5b828adab7d56d09d1745ff74c2870223b78d7a0 Mon Sep 17 00:00:00 2001 From: Chinmay Pai Date: Wed, 24 Jun 2020 11:31:10 +0530 Subject: [PATCH 004/140] chore: use default alignment for elements Co-authored-by: Suraj Shetty <13928957+surajshetty3416@users.noreply.github.com> --- frappe/www/login.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/www/login.html b/frappe/www/login.html index f1131db8cf..d57f126022 100644 --- a/frappe/www/login.html +++ b/frappe/www/login.html @@ -85,8 +85,8 @@
{{_("Signup Disabled")}}
-

{{_("Signups have been disabled for this website.")}}

-
{{ _("Home") }}
+

{{_("Signups have been disabled for this website.")}}

+ {%- endif -%}
From c53c0c43f4080620f69ffd7efe8809988002dd05 Mon Sep 17 00:00:00 2001 From: "Chinmay D. Pai" Date: Wed, 24 Jun 2020 19:45:53 +0530 Subject: [PATCH 005/140] fix: allow darkgrey indicator for disabled signup Signed-off-by: Chinmay D. Pai --- frappe/templates/includes/login/login.js | 2 +- frappe/www/login.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/templates/includes/login/login.js b/frappe/templates/includes/login/login.js index 37b8acb328..9fa22468d3 100644 --- a/frappe/templates/includes/login/login.js +++ b/frappe/templates/includes/login/login.js @@ -97,7 +97,7 @@ login.reset_sections = function(hide) { $("section.for-forgot").toggle(false); $("section.for-signup").toggle(false); } - $('section .indicator').each(function() { + $('section:not(.signup-disabled) .indicator').each(function() { $(this).removeClass().addClass('indicator').addClass('blue') .text($(this).attr('data-text')); }); diff --git a/frappe/www/login.html b/frappe/www/login.html index d57f126022..9d00892b18 100644 --- a/frappe/www/login.html +++ b/frappe/www/login.html @@ -68,7 +68,7 @@ {{ _("Forgot Password?") }}

-