diff --git a/.github/workflows/brakeman-analysis.yml b/.github/workflows/brakeman-analysis.yml index fb4cc3f295..28aa3e5ef0 100644 --- a/.github/workflows/brakeman-analysis.yml +++ b/.github/workflows/brakeman-analysis.yml @@ -5,12 +5,12 @@ name: Brakeman Scan on: push: - branches: [main] + branches: + - 7.0-stable pull_request: # The branches below must be a subset of the branches above - branches: [main] - schedule: - - cron: "40 4 * * 2" + branches: + - 7.0-stable jobs: brakeman-scan: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f7a676c008..cf3af45c44 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,6 +1,10 @@ name: Test -on: [push, pull_request] +on: + push: + branches: + - 7.0-stable + pull_request: jobs: RSpec: diff --git a/app/controllers/alchemy/admin/base_controller.rb b/app/controllers/alchemy/admin/base_controller.rb index c4b6bab458..fce65a03d7 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" @@ -107,13 +128,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 0915802aa2..db588a2232 100644 --- a/app/controllers/alchemy/admin/pages_controller.rb +++ b/app/controllers/alchemy/admin/pages_controller.rb @@ -183,14 +183,15 @@ def unlock respond_to do |format| format.js format.html do - redirect_to( - params[:redirect_to].presence || admin_pages_path, - allow_other_host: true - ) + redirect_to(unlock_redirect_path, allow_other_host: true) end end end + def unlock_redirect_path + safe_redirect_path(fallback: admin_pages_path) + end + # Sets the page public and updates the published_at attribute that is used as cache_key # def publish 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 271b785e20..934776083c 100644 --- a/spec/controllers/alchemy/admin/base_controller_spec.rb +++ b/spec/controllers/alchemy/admin/base_controller_spec.rb @@ -118,4 +118,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 diff --git a/spec/requests/alchemy/admin/pages_controller_spec.rb b/spec/requests/alchemy/admin/pages_controller_spec.rb index cbf4b2f127..53da29e88b 100644 --- a/spec/requests/alchemy/admin/pages_controller_spec.rb +++ b/spec/requests/alchemy/admin/pages_controller_spec.rb @@ -683,10 +683,10 @@ module Alchemy end context "if passing :redirect_to through params" do - subject { post unlock_admin_page_path(page, redirect_to: "this/path") } + subject { post unlock_admin_page_path(page, redirect_to: "/admin/path") } it "should redirect to the given path" do - is_expected.to redirect_to("this/path") + is_expected.to redirect_to("/admin/path") end end end