Skip to content
Permalink
Browse files Browse the repository at this point in the history
[#2052] Fix stored XSS in Notifications
  • Loading branch information
julianguyen committed Nov 19, 2021
1 parent cabb944 commit 720a470
Show file tree
Hide file tree
Showing 6 changed files with 136 additions and 37 deletions.
1 change: 1 addition & 0 deletions app/controllers/notifications_controller.rb
Expand Up @@ -46,6 +46,7 @@ def convert_to_hash(string_obj)
def render_notification(notification)
uniqueid = notification[:uniqueid]
data = convert_to_hash(notification[:data])
data[:email] = User.find(data[:user_id]).email # In case data[:name] is invalid
case data[:type]
when /comment/ then comment_link(uniqueid, data)
when /accepted_ally_request/ then accepted_ally_link(uniqueid, data)
Expand Down
14 changes: 9 additions & 5 deletions app/helpers/notifications_helper.rb
Expand Up @@ -6,7 +6,7 @@ def comment_link(uniqueid, data)
i18n_key = data[:cutoff] ? 'truncated' : 'full'
notification = t(
"notifications.comment.#{i18n_key}",
name: data[:user],
name: name_or_email(data),
comment: strip_tags(data[:comment]),
typename: data[:typename]
)
Expand All @@ -16,15 +16,15 @@ def comment_link(uniqueid, data)
def accepted_ally_link(uniqueid, data)
notification = t(
'notifications.ally.accepted',
name: data[:user]
name: name_or_email(data)
)
link = "/profile?uid=#{data[:uid]}"
notification_link(uniqueid, link, notification)
end

def new_ally_request_link(uniqueid, data)
link = "/profile?uid=#{data[:uid]}"
link_html = "<a href=\"#{link}\">#{data[:user]}</a>"
link_html = "<a href=\"#{link}\">#{name_or_email(data)}</a>"
# rubocop:disable Layout/LineLength
"<div id=\"#{uniqueid}\"><div>#{t('notifications.ally.sent_html', link_to_user: link_html)}</div>#{request_actions(data[:user_id])}</div>"
# rubocop:enable Layout/LineLength
Expand All @@ -33,7 +33,7 @@ def new_ally_request_link(uniqueid, data)
def group_link(uniqueid, data)
notification = t(
"notifications.group.#{data[:type]}",
name: data[:user],
name: name_or_email(data),
group_name: data[:group]
)
link = "/groups/#{data[:group_id]}"
Expand All @@ -43,7 +43,7 @@ def group_link(uniqueid, data)
def meeting_link(uniqueid, data)
notification = t(
"notifications.meeting.#{data[:type]}",
name: data[:user],
name: name_or_email(data),
group_name: data[:group],
meeting_name: data[:typename]
)
Expand Down Expand Up @@ -97,4 +97,8 @@ def comment_for_type(data)
def notification_link(uniqueid, link, notification)
"<a id=\"#{uniqueid}\" href=\"#{link}\">#{notification}</a>"
end

def name_or_email(data)
sanitize(data[:user]).presence || data[:email]
end
end
23 changes: 9 additions & 14 deletions spec/factories/notification.rb
@@ -1,22 +1,17 @@
# frozen_string_literal: true
data = {
cutoff: false,
user: 'Julia Nguyen',
comment: 'Hello',
typename: 'typename',
type: 'type_comment_moment',
typeid: 1,
commentable_id: 1
}

FactoryBot.define do
factory :notification do
data = {
cutoff: false,
user: 'Almond Butters',
comment: 'Hello',
typename: 'typename',
type: 'type_comment_moment',
typeid: 1,
commentable_id: 1,
}
uniqueid { 'MyString' }
data { data.to_json }
user_id { 1 }

trait :with_user do
association :user, factory: :user
end
end
end
107 changes: 96 additions & 11 deletions spec/helpers/notifications_helper_spec.rb
Expand Up @@ -4,7 +4,7 @@
let(:uniqueid) { 'uniqueid' }

describe '#comment_link' do
it 'returns correct link' do
it 'returns the correct link' do
data = {
cutoff: false,
user: 'Julia Nguyen',
Expand All @@ -16,27 +16,60 @@
}
expect(comment_link(uniqueid, data)).to eq('<a id="uniqueid" href="/moments/1">Julia Nguyen commented "Hello" on typename</a>')
end

it 'sanitizes and returns the correct link' do
data = {
cutoff: false,
user: '<IFRAME SRC="javascript:alert(document.domain);"></IFRAME>',
email: 'julia@example.com',
comment: 'Hello',
typename: 'typename',
type: 'type_comment_moment',
typeid: 1,
commentable_id: 1
}
expect(comment_link(uniqueid, data)).to eq('<a id="uniqueid" href="/moments/1">julia@example.com commented "Hello" on typename</a>')
end
end

describe '#accepted_ally_link' do
it 'returns correct link' do
it 'returns the correct link' do
data = {
user: 'Julia Nguyen',
uid: 'uid'
}
expect(accepted_ally_link(uniqueid, data)).to eq('<a id="uniqueid" href="/profile?uid=uid">Julia Nguyen accepted your ally request!</a>')
end

it 'sanitizes and returns the correct link' do
data = {
user: '<IFRAME SRC="javascript:alert(document.domain);"></IFRAME>',
email: 'julia@example.com',
uid: 'uid'
}
expect(accepted_ally_link(uniqueid, data)).to eq('<a id="uniqueid" href="/profile?uid=uid">julia@example.com accepted your ally request!</a>')
end
end

describe '#new_ally_request_link' do
it 'returns correct link' do
it 'returns the correct link' do
data = {
user: 'Julia Nguyen',
uid: 'uid',
user_id: 1
}
expect(new_ally_request_link(uniqueid, data)).to eq('<div id="uniqueid"><div>&lt;a href=&quot;/profile?uid=uid&quot;&gt;Julia Nguyen&lt;/a&gt; sent an ally request!</div><div><a rel="nofollow" data-method="post" href="/allies/add?ally_id=1">Accept</a> | <a data-confirm="Are you sure?" rel="nofollow" data-method="post" href="/allies/remove?ally_id=1">Reject</a></div></div>')
end

it 'sanitizes and returns the correct link' do
data = {
user: '<IFRAME SRC="javascript:alert(document.domain);"></IFRAME>',
email: 'julia@example.com',
uid: 'uid',
user_id: 1
}
expect(new_ally_request_link(uniqueid, data)).to eq('<div id="uniqueid"><div>&lt;a href=&quot;/profile?uid=uid&quot;&gt;julia@example.com&lt;/a&gt; sent an ally request!</div><div><a rel="nofollow" data-method="post" href="/allies/add?ally_id=1">Accept</a> | <a data-confirm="Are you sure?" rel="nofollow" data-method="post" href="/allies/remove?ally_id=1">Reject</a></div></div>')
end
end

describe '#group_link' do
Expand All @@ -48,32 +81,57 @@
group_id: 1
}
end
let(:dirty_data) do
{
type: type,
user: '<IFRAME SRC="javascript:alert(document.domain);"></IFRAME>',
email: 'julia@example.com',
group: 'Group name',
group_id: 1
}
end
context 'type is new_group' do
let(:type) { 'new_group' }
it 'returns correct link' do
it 'returns the correct link' do
expect(group_link(uniqueid, data)).to eq('<a id="uniqueid" href="/groups/1">Julia Nguyen created a group "Group name"</a>')
end

it 'sanitizes and returns the correct link' do
expect(group_link(uniqueid, dirty_data)).to eq('<a id="uniqueid" href="/groups/1">julia@example.com created a group "Group name"</a>')
end
end

context 'type is new_group_member' do
let(:type) { 'new_group_member' }
it 'returns correct link' do
it 'returns the correct link' do
expect(group_link(uniqueid, data)).to eq('<a id="uniqueid" href="/groups/1">Julia Nguyen joined your group "Group name"</a>')
end

it 'sanitizes and returns the correct link' do
expect(group_link(uniqueid, dirty_data)).to eq('<a id="uniqueid" href="/groups/1">julia@example.com joined your group "Group name"</a>')
end
end

context 'type is add_group_leader' do
let(:type) { 'add_group_leader' }
it 'returns correct link' do
it 'returns the correct link' do
expect(group_link(uniqueid, data)).to eq('<a id="uniqueid" href="/groups/1">Julia Nguyen became a leader of "Group name"</a>')
end

it 'sanitizes and returns the correct link' do
expect(group_link(uniqueid, dirty_data)).to eq('<a id="uniqueid" href="/groups/1">julia@example.com became a leader of "Group name"</a>')
end
end

context 'type is remove_group_leader' do
let(:type) { 'remove_group_leader' }
it 'returns correct link' do
it 'returns the correct link' do
expect(group_link(uniqueid, data)).to eq('<a id="uniqueid" href="/groups/1">Julia Nguyen is no longer a leader of "Group name"</a>')
end

it 'sanitizes and returns the correct link' do
expect(group_link(uniqueid, dirty_data)).to eq('<a id="uniqueid" href="/groups/1">julia@example.com is no longer a leader of "Group name"</a>')
end
end
end

Expand All @@ -88,32 +146,59 @@
typeid: 1
}
end
let(:dirty_data) do
{
type: type,
user: '<IFRAME SRC="javascript:alert(document.domain);"></IFRAME>',
email: 'julia@example.com',
group: 'Group name',
typename: 'Meeting name',
group_id: 1,
typeid: 1
}
end
context 'type is new_meeting' do
let(:type) { 'new_meeting' }
it 'returns correct link' do
it 'returns the correct link' do
expect(meeting_link(uniqueid, data)).to eq('<a id="uniqueid" href="/meetings/1">Julia Nguyen created a new meeting "Meeting name" for "Group name"</a>')
end

it 'sanitizes and returns the correct link' do
expect(meeting_link(uniqueid, dirty_data)).to eq('<a id="uniqueid" href="/meetings/1">julia@example.com created a new meeting "Meeting name" for "Group name"</a>')
end
end

context 'type is remove_meeting' do
let(:type) { 'remove_meeting' }
it 'returns correct link' do
it 'returns the correct link' do
expect(meeting_link(uniqueid, data)).to eq('<a id="uniqueid" href="/groups/1">Julia Nguyen has cancelled "Meeting name" for "Group name"</a>')
end

it 'sanitizes and returns the correct link' do
expect(meeting_link(uniqueid, dirty_data)).to eq('<a id="uniqueid" href="/groups/1">julia@example.com has cancelled "Meeting name" for "Group name"</a>')
end
end

context 'type is update_meeting' do
let(:type) { 'update_meeting' }
it 'returns correct link' do
it 'returns the correct link' do
expect(meeting_link(uniqueid, data)).to eq('<a id="uniqueid" href="/meetings/1">Julia Nguyen has updated "Meeting name" for "Group name"</a>')
end

it 'sanitizes and returns the correct link' do
expect(meeting_link(uniqueid, dirty_data)).to eq('<a id="uniqueid" href="/meetings/1">julia@example.com has updated "Meeting name" for "Group name"</a>')
end
end

context 'type is join_meeting' do
let(:type) { 'join_meeting' }
it 'returns correct link' do
it 'returns the correct link' do
expect(meeting_link(uniqueid, data)).to eq('<a id="uniqueid" href="/meetings/1">Julia Nguyen has joined "Meeting name" for "Group name"</a>')
end

it 'sanitizes and returns the correct link' do
expect(meeting_link(uniqueid, dirty_data)).to eq('<a id="uniqueid" href="/meetings/1">julia@example.com has joined "Meeting name" for "Group name"</a>')
end
end
end
end
4 changes: 3 additions & 1 deletion spec/models/notification_spec.rb
Expand Up @@ -13,6 +13,8 @@
#

describe Notification do
let(:user) { create(:user1) }

context 'with relations' do
it { is_expected.to belong_to :user }
end
Expand All @@ -24,7 +26,7 @@
end

it 'is valid with valid attributes' do
notification = build(:notification)
notification = build(:notification, user: user)

expect(notification).to be_valid
end
Expand Down
24 changes: 18 additions & 6 deletions spec/requests/notifications_spec.rb
Expand Up @@ -195,16 +195,28 @@
describe '#fetch_notifications' do
let(:user) { create(:user1) }
let(:other_user) { create(:user2) }
let!(:other_user_notification) { create(:notification, user: other_user) }

context 'when the user is signed in' do
let!(:notification) { create(:notification, user: user) }
let!(:notification_two) { create(:notification, user: user) }
let!(:notification) {
create(
:notification,
user: user,
data: {
cutoff: false,
user: 'Almond Butters',
comment: 'Hello',
typename: 'typename',
type: 'type_comment_moment',
typeid: 1,
commentable_id: 1,
user_id: other_user.id
}.to_json
)
}
let(:notification_link) do
'<a id="MyString" href="/moments/1">Julia Nguyen commented "Hello" on typename</a>'
'<a id="MyString" href="/moments/1">Almond Butters commented "Hello" on typename</a>'
end
let(:expected_result) do
{ fetch_notifications: [notification_link, notification_link] }.to_json
{ fetch_notifications: [notification_link] }.to_json
end

before do
Expand Down

0 comments on commit 720a470

Please sign in to comment.