diff --git a/app/controllers/alchemy/admin/base_controller.rb b/app/controllers/alchemy/admin/base_controller.rb index e36f133e67..7bb14dde84 100644 --- a/app/controllers/alchemy/admin/base_controller.rb +++ b/app/controllers/alchemy/admin/base_controller.rb @@ -31,6 +31,27 @@ def leave private + def safe_redirect_path(path = params[:redirect_to], fallback: admin_path) + if is_safe_redirect_path?(path) + path + elsif is_safe_redirect_path?(fallback) + fallback + else + admin_path + end + end + + def is_safe_redirect_path?(path) + mount_path = alchemy.root_path + path.to_s.match? %r{^#{mount_path}admin/} + end + + def relative_referer_path(referer = request.referer) + return unless referer + + URI(referer).path + end + # Disable layout rendering for xhr requests. def set_layout request.xhr? ? false : "alchemy/admin" @@ -106,13 +127,16 @@ def render_errors_or_redirect(object, redirect_url, flash_notice) # Does redirects for html and js requests # + # Makes sure that the redirect path is safe. + # def do_redirect_to(url_or_path) + redirect_path = safe_redirect_path(url_or_path) respond_to do |format| format.js { - @redirect_url = url_or_path + @redirect_url = redirect_path render :redirect } - format.html { redirect_to url_or_path } + format.html { redirect_to redirect_path } end end diff --git a/app/controllers/alchemy/admin/languages_controller.rb b/app/controllers/alchemy/admin/languages_controller.rb index 835b074ed4..5587ba7224 100644 --- a/app/controllers/alchemy/admin/languages_controller.rb +++ b/app/controllers/alchemy/admin/languages_controller.rb @@ -40,7 +40,7 @@ def destroy def switch @language = set_alchemy_language(params[:language_id]) session[:alchemy_language_id] = @language.id - do_redirect_to request.referer || alchemy.admin_dashboard_path + do_redirect_to relative_referer_path || alchemy.admin_dashboard_path end private diff --git a/app/controllers/alchemy/admin/pages_controller.rb b/app/controllers/alchemy/admin/pages_controller.rb index 4501715696..db588a2232 100644 --- a/app/controllers/alchemy/admin/pages_controller.rb +++ b/app/controllers/alchemy/admin/pages_controller.rb @@ -189,11 +189,7 @@ def unlock end def unlock_redirect_path - if params[:redirect_to].to_s.match?(/\A\/admin\/(layout_)?pages/) - params[:redirect_to] - else - admin_pages_path - end + safe_redirect_path(fallback: admin_pages_path) end # Sets the page public and updates the published_at attribute that is used as cache_key diff --git a/app/controllers/alchemy/admin/resources_controller.rb b/app/controllers/alchemy/admin/resources_controller.rb index 430fb21c6c..82c17d6c43 100644 --- a/app/controllers/alchemy/admin/resources_controller.rb +++ b/app/controllers/alchemy/admin/resources_controller.rb @@ -78,7 +78,7 @@ def destroy flash[:error] = resource_instance_variable.errors.full_messages.join(", ") end flash_notice_for_resource_action - do_redirect_to resource_url_proxy.url_for(search_filter_params.merge(action: "index")) + do_redirect_to resource_url_proxy.url_for(search_filter_params.merge(action: "index", only_path: true)) end def resource_handler diff --git a/spec/controllers/alchemy/admin/base_controller_spec.rb b/spec/controllers/alchemy/admin/base_controller_spec.rb index 0cc4e51de9..ae0938168e 100644 --- a/spec/controllers/alchemy/admin/base_controller_spec.rb +++ b/spec/controllers/alchemy/admin/base_controller_spec.rb @@ -211,4 +211,132 @@ def index end end end + + describe "#safe_redirect_path" do + subject { controller.send(:safe_redirect_path) } + + context "when params[:redirect_to] is present" do + before do + allow(controller).to receive(:params) { {redirect_to: redirect_url} } + end + + context "and it is not an external URL" do + let(:redirect_url) { "/admin/pages" } + + it "redirects to given path" do + is_expected.to eq("/admin/pages") + end + end + + context "and it is an external URL" do + let(:redirect_url) { "https://evil.com" } + + context "and no fallback is given" do + it "redirects to default fallback path" do + is_expected.to eq("/admin") + end + end + + context "and a fallback is given" do + subject { controller.send(:safe_redirect_path, fallback: "/admin/pages") } + + context "which is a safe path" do + it "redirects to given fallback path" do + is_expected.to eq("/admin/pages") + end + end + + context "which is not a safe path" do + subject { controller.send(:safe_redirect_path, fallback: "evil.com") } + + it "redirects to default fallback path" do + is_expected.to eq("/admin") + end + end + end + end + end + + context "when params[:redirect_to] is not present" do + context "and another path is given" do + subject { controller.send(:safe_redirect_path, redirect_path) } + + context "which is a safe path" do + let(:redirect_path) { "/admin/pages" } + + it "redirects to given path" do + is_expected.to eq("/admin/pages") + end + end + + context "which is not a safe path" do + let(:redirect_path) { "evil.com" } + + it "redirects to default fallback path" do + is_expected.to eq("/admin") + end + end + end + + context "and no fallback is given" do + it "redirects to default fallback path" do + is_expected.to eq("/admin") + end + end + + context "and a fallback is given" do + subject { controller.send(:safe_redirect_path, fallback: "/admin/pages") } + + context "which is a safe path" do + it "redirects to given fallback path" do + is_expected.to eq("/admin/pages") + end + end + + context "which is not a safe path" do + subject { controller.send(:safe_redirect_path, fallback: "evil.com") } + + it "redirects to default fallback path" do + is_expected.to eq("/admin") + end + end + end + end + end + + describe "#is_safe_redirect_path?" do + subject { controller.send(:is_safe_redirect_path?, path) } + + context "path is not an external URL" do + let(:path) { "/admin/pages" } + + it { is_expected.to be(true) } + end + + context "path is an external URL" do + let(:path) { "https://evil.com" } + + it { is_expected.to be(false) } + end + + context "alchemy is mounted under a path" do + before do + allow(controller).to receive(:alchemy) do + double(root_path: "/cms/") + end + end + + context "path is not an external URL" do + let(:path) { "/cms/admin/pages" } + + it { is_expected.to be(true) } + end + + context "path is an external URL" do + let(:path) { "https://evil.com" } + + it { is_expected.to be(false) } + end + end + end end