From e5a1ac02289e8fde8ddbd05bbb21ac448c661ae3 Mon Sep 17 00:00:00 2001 From: Jo-Philipp Wich Date: Thu, 26 Jul 2018 22:12:45 +0200 Subject: [PATCH] treewide: rework rollback/apply workflow Rework the apply confirmation mechanism to be session agnostic in order to circumvent cross domain restrictions which prevent the JS code from issuing apply confirm requests in some cases, e.g. when changing the LAN IP. Confirmation calls may now be done from unauthenticated pages, as long as a matching confirmation token is sent along with the request. The reasoning behind this is that there is little security impact in confirming pending apply sessions, especially since those sessions can only be initiated while being authenticated. After this change, LuCI will now launch a confirmation process on every rendered page when a rollback is pending. The confirmation will happen regardless of whether the user is logged in or not, or if the current page is a CBI form or static template. A confirmation request now also requires a random one-time token which is rendered along with the confirmation JavaScript code in order to succeed. This token is not meant to provide security but to ensure that the confirm was triggered from an interactive browser session and not some background HTTP requests that happened to end up in the admin ui. As a consequence, the different apply/confirm/rollback code paths in CBI maps and the UCI change/revert pages have been consolidated into one common implementation residing in the common global theme agnostic footer template. Signed-off-by: Jo-Philipp Wich --- modules/luci-base/luasrc/dispatcher.lua | 15 ++- modules/luci-base/luasrc/model/uci.lua | 92 ++++++++++++++----- .../luasrc/view/cbi/apply_widget.htm | 10 +- modules/luci-base/luasrc/view/cbi/map.htm | 15 --- modules/luci-base/luasrc/view/footer.htm | 25 ++++- .../luasrc/controller/admin/uci.lua | 26 ++++-- .../luasrc/view/admin_uci/changes.htm | 10 +- .../luasrc/view/admin_uci/revert.htm | 11 +-- 8 files changed, 129 insertions(+), 75 deletions(-) diff --git a/modules/luci-base/luasrc/dispatcher.lua b/modules/luci-base/luasrc/dispatcher.lua index 6d5a8f4d3d..6cf2712eb4 100644 --- a/modules/luci-base/luasrc/dispatcher.lua +++ b/modules/luci-base/luasrc/dispatcher.lua @@ -893,8 +893,6 @@ local function _cbi(self, ...) local pageaction = true local parsechain = { } - local is_rollback, time_remaining = uci:rollback_pending() - for i, res in ipairs(maps) do if res.apply_needed and res.parsechain then local c @@ -921,8 +919,6 @@ local function _cbi(self, ...) for i, res in ipairs(maps) do res:render({ firstmap = (i == 1), - applymap = applymap, - confirmmap = (is_rollback and time_remaining or nil), redirect = redirect, messages = messages, pageaction = pageaction, @@ -932,11 +928,12 @@ local function _cbi(self, ...) if not config.nofooter then tpl.render("cbi/footer", { - flow = config, - pageaction = pageaction, - redirect = redirect, - state = state, - autoapply = config.autoapply + flow = config, + pageaction = pageaction, + redirect = redirect, + state = state, + autoapply = config.autoapply, + trigger_apply = applymap }) end end diff --git a/modules/luci-base/luasrc/model/uci.lua b/modules/luci-base/luasrc/model/uci.lua index 92c0d8f699..b2c1e463bf 100644 --- a/modules/luci-base/luasrc/model/uci.lua +++ b/modules/luci-base/luasrc/model/uci.lua @@ -147,19 +147,31 @@ function apply(self, rollback) local _, err if rollback then + local sys = require "luci.sys" local conf = require "luci.config" - local timeout = tonumber(conf and conf.apply and conf.apply.rollback or "") or 0 + local timeout = tonumber(conf and conf.apply and conf.apply.rollback or 30) or 0 _, err = call("apply", { - timeout = (timeout > 30) and timeout or 30, + timeout = (timeout > 30) and timeout or 30, rollback = true }) if not err then + local now = os.time() + local token = sys.uniqueid(16) + util.ubus("session", "set", { - ubus_rpc_session = session_id, - values = { rollback = os.time() + timeout } + ubus_rpc_session = "00000000000000000000000000000000", + values = { + rollback = { + token = token, + session = session_id, + timeout = now + timeout + } + } }) + + return token end else _, err = call("changes", {}) @@ -184,40 +196,72 @@ function apply(self, rollback) return (err == nil), ERRSTR[err] end -function confirm(self) - local _, err = call("confirm", {}) - if not err then - util.ubus("session", "set", { - ubus_rpc_session = session_id, - values = { rollback = 0 } +function confirm(self, token) + local is_pending, time_remaining, rollback_sid, rollback_token = self:rollback_pending() + + if is_pending then + if token ~= rollback_token then + return false, "Permission denied" + end + + local _, err = util.ubus("uci", "confirm", { + ubus_rpc_session = rollback_sid }) + + if not err then + util.ubus("session", "set", { + ubus_rpc_session = "00000000000000000000000000000000", + values = { rollback = {} } + }) + end + + return (err == nil), ERRSTR[err] end - return (err == nil), ERRSTR[err] + + return false, "No data" end function rollback(self) - local _, err = call("rollback", {}) - if not err then - util.ubus("session", "set", { - ubus_rpc_session = session_id, - values = { rollback = 0 } + local is_pending, time_remaining, rollback_sid = self:rollback_pending() + + if is_pending then + local _, err = util.ubus("uci", "rollback", { + ubus_rpc_session = rollback_sid }) + + if not err then + util.ubus("session", "set", { + ubus_rpc_session = "00000000000000000000000000000000", + values = { rollback = {} } + }) + end + + return (err == nil), ERRSTR[err] end - return (err == nil), ERRSTR[err] + + return false, "No data" end function rollback_pending(self) - local deadline, err = util.ubus("session", "get", { - ubus_rpc_session = session_id, + local rv, err = util.ubus("session", "get", { + ubus_rpc_session = "00000000000000000000000000000000", keys = { "rollback" } }) - if type(deadline) == "table" and - type(deadline.values) == "table" and - type(deadline.values.rollback) == "number" and - deadline.values.rollback > os.time() + local now = os.time() + + if type(rv) == "table" and + type(rv.values) == "table" and + type(rv.values.rollback) == "table" and + type(rv.values.rollback.token) == "string" and + type(rv.values.rollback.session) == "string" and + type(rv.values.rollback.timeout) == "number" and + rv.values.rollback.timeout > now then - return true, deadline.values.rollback - os.time() + return true, + rv.values.rollback.timeout - now, + rv.values.rollback.session, + rv.values.rollback.token end return false, ERRSTR[err] diff --git a/modules/luci-base/luasrc/view/cbi/apply_widget.htm b/modules/luci-base/luasrc/view/cbi/apply_widget.htm index f76846ee87..4d7e9c56ea 100644 --- a/modules/luci-base/luasrc/view/cbi/apply_widget.htm +++ b/modules/luci-base/luasrc/view/cbi/apply_widget.htm @@ -1,4 +1,4 @@ -<% export("cbi_apply_widget", function(redirect_ok) -%> +<% export("cbi_apply_widget", function(redirect_ok, rollback_token) -%>