CVE-2021-25988: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: In “ifme”, versions 1.0.0 to v7.31.4 are vulnerable against stored XSS vulnerability (notifications section) which can be directly triggered by sending an ally request to the admin.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 5.4
Published: 2021-12-29T09:15Z
Repository: https://github.com/ifmeorg/ifme
Language: Ruby
Files changed: app/controllers/notifications_controller.rb, app/helpers/notifications_helper.rb, spec/factories/notification.rb, spec/helpers/notifications_helper_spec.rb, spec/models/notification_spec.rb, spec/requests/notifications_spec.rb
Commit date: 2021-11-18 23:01:15 -0500
Commit message: [#2052] Fix stored XSS in Notifications

Diff: diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb
index d5b9b2c6..33e82827 100644
--- a/app/controllers/notifications_controller.rb
+++ b/app/controllers/notifications_controller.rb
@@ -44,10 +44,11 @@ class NotificationsController < ApplicationController
   end
 
   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)
     when /new_ally_request/ then new_ally_request_link(uniqueid, data)
     when /group/ then group_link(uniqueid, data)
diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb
index 32925b4c..927ff0aa 100644
--- a/app/helpers/notifications_helper.rb
+++ b/app/helpers/notifications_helper.rb
@@ -4,48 +4,48 @@ module NotificationsHelper
   def comment_link(uniqueid, data)
     comment = comment_for_type(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]
     )
     notification_link(uniqueid, comment[:path], notification)
   end
 
   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
   end
 
   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]}"
     notification_link(uniqueid, link, notification)
   end
 
   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]
     )
     link = specific_meeting_link(data[:type], data[:typeid], data[:group_id])
     notification_link(uniqueid, link, notification)
@@ -95,6 +95,10 @@ module NotificationsHelper
   end
 
   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
diff --git a/spec/factories/notification.rb b/spec/factories/notification.rb
index 3ef5a090..da5a841b 100644
--- a/spec/factories/notification.rb
+++ b/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
diff --git a/spec/helpers/notifications_helper_spec.rb b/spec/helpers/notifications_helper_spec.rb
index 5a647678..ca0fea7d 100644
--- a/spec/helpers/notifications_helper_spec.rb
+++ b/spec/helpers/notifications_helper_spec.rb
@@ -2,11 +2,11 @@
 
 describe NotificationsHelper do
   let(:uniqueid) { 'uniqueid' }
 
   describe '#comment_link' do
-    it 'returns correct link' do
+    it 'returns the correct link' do
       data = {
         cutoff: false,
         user: 'Julia Nguyen',
         comment: 'Hello',
         typename: 'typename',
@@ -14,31 +14,64 @@ describe NotificationsHelper do
         typeid: 1,
         commentable_id: 1
       }
       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
     let(:data) do
       {
@@ -46,36 +79,61 @@ describe NotificationsHelper do
         user: 'Julia Nguyen',
         group: 'Group name',
         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
 
   describe '#meeting_link' do
     let(:data) do
@@ -86,34 +144,61 @@ describe NotificationsHelper do
         typename: 'Meeting name',
         group_id: 1,
         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
diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb
index 5c23111c..94cb5a57 100644
--- a/spec/models/notification_spec.rb
+++ b/spec/models/notification_spec.rb
@@ -11,10 +11,12 @@
 #  created_at :datetime         not null
 #  updated_at :datetime         not null
 #
 
 describe Notification do
+  let(:user) { create(:user1) }
+
   context 'with relations' do
     it { is_expected.to belong_to :user }
   end
 
   context 'with validations' do
@@ -22,11 +24,11 @@ describe Notification do
     it { is_expected.to validate_presence_of :uniqueid }
     it { is_expected.to validate_presence_of :data }
   end
 
   it 'is valid with valid attributes' do
-    notification = build(:notification)
+    notification = build(:notification, user: user)
 
     expect(notification).to be_valid
   end
 
   it 'is invalid without a user_id' do
diff --git a/spec/requests/notifications_spec.rb b/spec/requests/notifications_spec.rb
index b3d01344..6ba752ba 100644
--- a/spec/requests/notifications_spec.rb
+++ b/spec/requests/notifications_spec.rb
@@ -193,20 +193,32 @@ describe 'Notification', type: :request do
   end
 
   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
         sign_in user
 

Vulnerable code:       name: data[:user],
      name: data[:user]
    link_html = "<a href=\"#{link}\">#{data[:user]}</a>"
      name: data[:user],
      name: data[:user],
data = {
  cutoff: false,
  user: 'Julia Nguyen',
  comment: 'Hello',
  typename: 'typename',
  type: 'type_comment_moment',
  typeid: 1,
  commentable_id: 1
}
    user_id { 1 }

    trait :with_user do
      association :user, factory: :user
    end
    it 'returns correct link' do
    it 'returns correct link' do
    it 'returns correct link' do
      it 'returns correct link' do
      it 'returns correct link' do
      it 'returns correct link' do
      it 'returns correct link' do
      it 'returns correct link' do
      it 'returns correct link' do
      it 'returns correct link' do
      it 'returns correct link' do
    notification = build(:notification)
    let!(:other_user_notification) { create(:notification, user: other_user) }

      let!(:notification) { create(:notification, user: user) }
      let!(:notification_two) { create(:notification, user: user) }
        '<a id="MyString" href="/moments/1">Julia Nguyen commented "Hello" on typename</a>'
        { fetch_notifications: [notification_link, notification_link] }.to_json
Fixed code:     data[:email] = User.find(data[:user_id]).email # In case data[:name] is invalid
      name: name_or_email(data),
      name: name_or_email(data)
    link_html = "<a href=\"#{link}\">#{name_or_email(data)}</a>"
      name: name_or_email(data),
      name: name_or_email(data),

  def name_or_email(data)
    sanitize(data[:user]).presence || data[:email]
  end
    data = {
      cutoff: false,
      user: 'Almond Butters',
      comment: 'Hello',
      typename: 'typename',
      type: 'type_comment_moment',
      typeid: 1,
      commentable_id: 1,
    }
    it 'returns the correct link' do

    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
    it 'returns the correct link' do

    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
    it 'returns the correct link' do

    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
    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
      it 'returns the correct link' do

      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
      it 'returns the correct link' do

      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
      it 'returns the correct link' do

      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
      it 'returns the correct link' do

      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
    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
      it 'returns the correct link' do

      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
      it 'returns the correct link' do

      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
      it 'returns the correct link' do

      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
      it 'returns the correct link' do

      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
  let(:user) { create(:user1) }

    notification = build(: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
        )
      }
        '<a id="MyString" href="/moments/1">Almond Butters commented "Hello" on typename</a>'
        { fetch_notifications: [notification_link] }.to_json