Merge pull request #2661 from tvdeyen/fix-brakeman-offense

Fix brakeman offense
AlchemyCMS · Dec 28, 2023 · 91ba7bc · 91ba7bc
2 parents f7fa610 + d471682
commit 91ba7bc
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 43 deletions.
diff --git a/.github/workflows/brakeman-analysis.yml b/.github/workflows/brakeman-analysis.yml
@@ -25,11 +25,11 @@ jobs:
       - name: Set up Ruby
         uses: ruby/setup-ruby@v1
         with:
-          ruby-version: "3.0"
+          ruby-version: "3.2"
 
       - name: Setup Brakeman
         env:
-          BRAKEMAN_VERSION: "5.4" # SARIF support is provided in Brakeman version 4.10+
+          BRAKEMAN_VERSION: "6.1" # SARIF support is provided in Brakeman version 4.10+
         run: |
           gem install brakeman --version $BRAKEMAN_VERSION
 

diff --git a/app/controllers/alchemy/admin/pages_controller.rb b/app/controllers/alchemy/admin/pages_controller.rb
@@ -183,14 +183,19 @@ 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
+        if params[:redirect_to].to_s.match?(/\A\/admin\/(layout_)?pages/)
+          params[:redirect_to]
+        else
+          admin_pages_path
+        end
+      end
+
       # Sets the page public and updates the published_at attribute that is used as cache_key
       #
       def publish

diff --git a/config/brakeman.ignore b/config/brakeman.ignore
@@ -46,40 +46,6 @@
       ],
       "note": "Because we actually can't know all attributes each inheriting controller supports, we permit all resource model params. It is adviced that all inheriting controllers implement this method and provide its own set of permitted attributes. As this all happens inside the password protected /admin namespace this can be considered a false positive."
     },
-    {
-      "warning_type": "Dynamic Render Path",
-      "warning_code": 15,
-      "fingerprint": "384ec61125c6390d59fb7ebcf52792ba284bfd463d70d4ef552ab6c328e776f6",
-      "check_name": "Render",
-      "message": "Render path contains parameter value",
-      "file": "app/views/alchemy/admin/elements/fold.js.erb",
-      "line": 11,
-      "link": "https://brakemanscanner.org/docs/warning_types/dynamic_render_path/",
-      "code": "render(action => Alchemy::ElementEditor.new(Element.find(params[:id])), {})",
-      "render_path": [
-        {
-          "type": "controller",
-          "class": "Alchemy::Admin::ElementsController",
-          "method": "fold",
-          "line": 98,
-          "file": "app/controllers/alchemy/admin/elements_controller.rb",
-          "rendered": {
-            "name": "alchemy/admin/elements/fold",
-            "file": "app/views/alchemy/admin/elements/fold.js.erb"
-          }
-        }
-      ],
-      "location": {
-        "type": "template",
-        "template": "alchemy/admin/elements/fold"
-      },
-      "user_input": "params[:id]",
-      "confidence": "Weak",
-      "cwe_id": [
-        22
-      ],
-      "note": ""
-    },
     {
       "warning_type": "Cross-Site Scripting",
       "warning_code": 4,

diff --git a/spec/requests/alchemy/admin/pages_controller_spec.rb b/spec/requests/alchemy/admin/pages_controller_spec.rb
@@ -683,10 +683,28 @@ module Alchemy
           end
 
           context "if passing :redirect_to through params" do
-            subject { post unlock_admin_page_path(page, redirect_to: "this/path") }
+            context "that is admin layout pages path" do
+              subject { post unlock_admin_page_path(page, redirect_to: "/admin/layout_pages") }
 
-            it "should redirect to the given path" do
-              is_expected.to redirect_to("this/path")
+              it "should redirect to the given path" do
+                is_expected.to redirect_to("/admin/layout_pages")
+              end
+            end
+
+            context "that is admin pages path" do
+              subject { post unlock_admin_page_path(page, redirect_to: "/admin/pages") }
+
+              it "should redirect to the given path" do
+                is_expected.to redirect_to("/admin/pages")
+              end
+            end
+
+            context "that is another path" do
+              subject { post unlock_admin_page_path(page, redirect_to: "/this/path") }
+
+              it "should redirect to admin_pages_path" do
+                is_expected.to redirect_to(admin_pages_path)
+              end
             end
           end
         end