From 9f218c9924b883207a3463a29314c92032cf06df Mon Sep 17 00:00:00 2001
From: Matt Jankowski <matt@jankowski.online>
Date: Thu, 19 Oct 2023 11:25:54 -0400
Subject: [PATCH] Refactor appeal partial to avoid brakeman XSS warning
 (#25880)

---
 app/helpers/admin/disputes_helper.rb          | 19 +++++++++++
 .../admin/disputes/appeals/_appeal.html.haml  |  2 +-
 config/brakeman.ignore                        | 33 -------------------
 .../admin/disputes/appeals_controller_spec.rb |  8 +++--
 spec/helpers/admin/disputes_helper_spec.rb    | 21 ++++++++++++
 5 files changed, 47 insertions(+), 36 deletions(-)
 create mode 100644 app/helpers/admin/disputes_helper.rb
 create mode 100644 spec/helpers/admin/disputes_helper_spec.rb

diff --git a/app/helpers/admin/disputes_helper.rb b/app/helpers/admin/disputes_helper.rb
new file mode 100644
index 0000000000..366a470ed2
--- /dev/null
+++ b/app/helpers/admin/disputes_helper.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module Admin
+  module DisputesHelper
+    def strike_action_label(appeal)
+      t(key_for_action(appeal),
+        scope: 'admin.strikes.actions',
+        name: content_tag(:span, appeal.strike.account.username, class: 'username'),
+        target: content_tag(:span, appeal.account.username, class: 'target'))
+        .html_safe
+    end
+
+    private
+
+    def key_for_action(appeal)
+      AccountWarning.actions.slice(appeal.strike.action).keys.first
+    end
+  end
+end
diff --git a/app/views/admin/disputes/appeals/_appeal.html.haml b/app/views/admin/disputes/appeals/_appeal.html.haml
index 3f6efb856e..d5611211ed 100644
--- a/app/views/admin/disputes/appeals/_appeal.html.haml
+++ b/app/views/admin/disputes/appeals/_appeal.html.haml
@@ -4,7 +4,7 @@
       = image_tag appeal.account.avatar.url(:original), alt: '', width: 40, height: 40, class: 'avatar'
     .log-entry__content
       .log-entry__title
-        = t(appeal.strike.action, scope: 'admin.strikes.actions', name: content_tag(:span, appeal.strike.account.username, class: 'username'), target: content_tag(:span, appeal.account.username, class: 'target')).html_safe
+        = strike_action_label(appeal)
       .log-entry__timestamp
         %time.formatted{ datetime: appeal.strike.created_at.iso8601 }
           = l(appeal.strike.created_at)
diff --git a/config/brakeman.ignore b/config/brakeman.ignore
index 9f85ccb6a4..d5c0b94436 100644
--- a/config/brakeman.ignore
+++ b/config/brakeman.ignore
@@ -1,38 +1,5 @@
 {
   "ignored_warnings": [
-    {
-      "warning_type": "Cross-Site Scripting",
-      "warning_code": 2,
-      "fingerprint": "71cf98c8235b5cfa9946b5db8fdc1a2f3a862566abb34e4542be6f3acae78233",
-      "check_name": "CrossSiteScripting",
-      "message": "Unescaped model attribute",
-      "file": "app/views/admin/disputes/appeals/_appeal.html.haml",
-      "line": 7,
-      "link": "https://brakemanscanner.org/docs/warning_types/cross_site_scripting",
-      "code": "t((Unresolved Model).new.strike.action, :scope => \"admin.strikes.actions\", :name => content_tag(:span, (Unresolved Model).new.strike.account.username, :class => \"username\"), :target => content_tag(:span, (Unresolved Model).new.account.username, :class => \"target\"))",
-      "render_path": [
-        {
-          "type": "template",
-          "name": "admin/disputes/appeals/index",
-          "line": 20,
-          "file": "app/views/admin/disputes/appeals/index.html.haml",
-          "rendered": {
-            "name": "admin/disputes/appeals/_appeal",
-            "file": "app/views/admin/disputes/appeals/_appeal.html.haml"
-          }
-        }
-      ],
-      "location": {
-        "type": "template",
-        "template": "admin/disputes/appeals/_appeal"
-      },
-      "user_input": "(Unresolved Model).new.strike",
-      "confidence": "Weak",
-      "cwe_id": [
-        79
-      ],
-      "note": ""
-    },
     {
       "warning_type": "Cross-Site Scripting",
       "warning_code": 4,
diff --git a/spec/controllers/admin/disputes/appeals_controller_spec.rb b/spec/controllers/admin/disputes/appeals_controller_spec.rb
index 4afe074921..3f4175a281 100644
--- a/spec/controllers/admin/disputes/appeals_controller_spec.rb
+++ b/spec/controllers/admin/disputes/appeals_controller_spec.rb
@@ -18,10 +18,14 @@ RSpec.describe Admin::Disputes::AppealsController do
   describe 'GET #index' do
     let(:current_user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) }
 
-    it 'lists appeals' do
+    before { appeal }
+
+    it 'returns a page that lists details of appeals' do
       get :index
 
-      expect(response).to have_http_status(200)
+      expect(response).to have_http_status(:success)
+      expect(response.body).to include("<span class=\"username\">#{strike.account.username}</span>")
+      expect(response.body).to include("<span class=\"target\">#{appeal.account.username}</span>")
     end
   end
 
diff --git a/spec/helpers/admin/disputes_helper_spec.rb b/spec/helpers/admin/disputes_helper_spec.rb
new file mode 100644
index 0000000000..5f9a85df86
--- /dev/null
+++ b/spec/helpers/admin/disputes_helper_spec.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Admin::DisputesHelper do
+  describe 'strike_action_label' do
+    it 'returns html describing the appeal' do
+      adam = Account.new(username: 'Adam')
+      becky = Account.new(username: 'Becky')
+      strike = AccountWarning.new(account: adam, action: :suspend)
+      appeal = Appeal.new(strike: strike, account: becky)
+
+      expected = <<~OUTPUT.strip
+        <span class="username">Adam</span> suspended <span class="target">Becky</span>'s account
+      OUTPUT
+      result = helper.strike_action_label(appeal)
+
+      expect(result).to eq(expected)
+    end
+  end
+end