Skip to content

Commit

Permalink
Refactor appeal partial to avoid brakeman XSS warning (mastodon#25880)
Browse files Browse the repository at this point in the history
  • Loading branch information
mjankowski authored Oct 19, 2023
1 parent bcd0171 commit 9f218c9
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 36 deletions.
19 changes: 19 additions & 0 deletions app/helpers/admin/disputes_helper.rb
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion app/views/admin/disputes/appeals/_appeal.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
33 changes: 0 additions & 33 deletions config/brakeman.ignore
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
8 changes: 6 additions & 2 deletions spec/controllers/admin/disputes/appeals_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,14 @@
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

Expand Down
21 changes: 21 additions & 0 deletions spec/helpers/admin/disputes_helper_spec.rb
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 9f218c9

Please sign in to comment.