[7.0-stable] Use safe redirect paths in admin redirects

.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:

.github/workflows/test.yml

@@ -1,6 +1,10 @@
 name: Test
 
-on: [push, pull_request]
+on:
+  push:
+    branches:
+      - 7.0-stable
+  pull_request:
 
 jobs:
   RSpec:

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
 

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

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

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

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

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