Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FIX: Mark invites flash messages as HTML safe. (#15539) 

* FIX: Mark invites flash messages as HTML safe.
This change should be safe as all user inputs included in the errors are sanitized before sending it back to the client.

Context: https://meta.discourse.org/t/html-tags-are-explicit-after-latest-update/214220

* If somebody adds a new error message that includes user input and doesn't sanitize it, using html-safe suddenly becomes unsafe again. As an extra layer of protection, we make the client sanitize the error message received from the backend.

* Escape user input instead of sanitizing
This commit is contained in:
Roman Rizzi 2022-01-18 09:38:31 -03:00 committed by GitHub
parent 7329b766cb
commit 5ee31cbf7d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 44 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
app/assets/javascripts/discourse/app/controllers/create-invite.js
View File

@ -11,6 +11,7 @@ import Group from "discourse/models/group";
import Invite from "discourse/models/invite";
import I18n from "I18n";
import { FORMAT } from "select-kit/components/future-date-input-selector";
import { sanitize } from "discourse/lib/text";
export default Controller.extend(
ModalFunctionality,
@ -130,7 +131,7 @@ export default Controller.extend(
if (result.warnings) {
this.setProperties({
flashText: result.warnings.join(","),
flashText: sanitize(result.warnings.join(",")),
flashClass: "warning",
flashLink: !this.editing,
});
@ -139,7 +140,7 @@ export default Controller.extend(
this.send("closeModal");
} else {
this.setProperties({
flashText: I18n.t("user.invited.invite.invite_saved"),
flashText: sanitize(I18n.t("user.invited.invite.invite_saved")),
flashClass: "success",
flashLink: !this.editing,
});
@ -148,7 +149,7 @@ export default Controller.extend(
})
.catch((e) =>
this.setProperties({
flashText: extractError(e),
flashText: sanitize(extractError(e)),
flashClass: "error",
flashLink: false,
})

4
app/assets/javascripts/discourse/app/templates/modal/create-invite.hbs
View File

@ -2,7 +2,7 @@
<div id="modal-alert" role="alert" class="alert alert-{{flashClass}}">
{{#if flashLink}}
<div class="input-group invite-link">
<label for="invite-link">{{flashText}} {{i18n "user.invited.invite.instructions"}}</label>
<label for="invite-link">{{html-safe flashText}} {{i18n "user.invited.invite.instructions"}}</label>
<div class="link-share-container">
{{input
name="invite-link"
@ -14,7 +14,7 @@
</div>
</div>
{{else}}
{{flashText}}
{{html-safe flashText}}
{{/if}}
</div>
{{/if}}

5
app/controllers/invites_controller.rb
View File

@ -200,7 +200,10 @@ class InvitesController < ApplicationController
if new_email
if Invite.where.not(id: invite.id).find_by(email: new_email.downcase, invited_by_id: current_user.id)&.redeemable?
return render_json_error(I18n.t("invite.invite_exists", email: new_email), status: 409)
return render_json_error(
I18n.t("invite.invite_exists", email: CGI.escapeHTML(new_email)),
status: 409
)
end
end

26
app/models/invite.rb
View File

@ -62,12 +62,7 @@ class Invite < ActiveRecord::Base
if user && user.id != self.invited_users&.first&.user_id
@email_already_exists = true
errors.add(:base, I18n.t(
"invite.user_exists",
email: email,
username: user.username,
base_path: Discourse.base_path
))
errors.add(:base, user_exists_error_msg(email, user.username))
end
end
@ -106,12 +101,7 @@ class Invite < ActiveRecord::Base
email = Email.downcase(opts[:email]) if opts[:email].present?
if user = find_user_by_email(email)
raise UserExists.new(I18n.t(
"invite.user_exists",
email: email,
username: user.username,
base_path: Discourse.base_path
))
raise UserExists.new(new.user_exists_error_msg(email, user.username))
end
if email.present?
@ -293,9 +283,19 @@ class Invite < ActiveRecord::Base
self.domain.downcase!
if self.domain !~ Invite::DOMAIN_REGEX
self.errors.add(:base, I18n.t('invite.domain_not_allowed', domain: self.domain))
self.errors.add(:base, I18n.t('invite.domain_not_allowed'))
end
end
def user_exists_error_msg(email, username)
sanitized_email = CGI.escapeHTML(email)
sanitized_username = CGI.escapeHTML(username)
I18n.t(
"invite.user_exists",
email: sanitized_email, username: sanitized_username, base_path: Discourse.base_path
)
end
end
# == Schema Information

2
lib/validators/email_validator.rb
View File

@ -5,7 +5,7 @@ class EmailValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value)
unless value =~ EmailValidator.email_regex
if Invite === record && attribute == :email
record.errors.add(:base, I18n.t(:'invite.invalid_email', email: value))
record.errors.add(:base, I18n.t(:'invite.invalid_email', email: CGI.escapeHTML(value)))
else
record.errors.add(attribute, I18n.t(:'user.email.invalid'))
end

23
spec/models/invite_spec.rb
View File

@ -4,6 +4,8 @@ require 'rails_helper'
describe Invite do
fab!(:user) { Fabricate(:user) }
let(:xss_email) { "<b onmouseover=alert('wufff!')>email</b><script>alert('test');</script>@test.com" }
let(:escaped_email) { "&lt;b onmouseover=alert(&#39;wufff!&#39;)&gt;email&lt;/b&gt;&lt;script&gt;alert(&#39;test&#39;);&lt;/script&gt;@test.com" }
context 'validators' do
it { is_expected.to validate_presence_of :invited_by_id }
@ -14,10 +16,11 @@ describe Invite do
expect(invite).to be_valid
end
it 'does not allow invites with invalid emails' do
invite = Fabricate.build(:invite, email: 'John Doe <john.doe@example.com>')
it 'escapes the invalid email before attaching the error message' do
invite = Fabricate.build(:invite, email: xss_email)
expect(invite.valid?).to eq(false)
expect(invite.errors.full_messages).to include(I18n.t('invite.invalid_email', email: invite.email))
expect(invite.errors.full_messages).to include(I18n.t('invite.invalid_email', email: escaped_email))
end
it 'does not allow an invite with the same email as an existing user' do
@ -82,6 +85,20 @@ describe Invite do
.to raise_error(Invite::UserExists)
end
it 'escapes the email_address when raising an existing user error' do
user.email = xss_email
user.save(validate: false)
expect { Invite.generate(user, email: user.email) }
.to raise_error(
Invite::UserExists,
I18n.t(
'invite.user_exists',
email: escaped_email, username: user.username, base_path: Discourse.base_path
)
)
end
context 'via email' do
it 'can be created and a job is enqueued to email the invite' do
invite = Invite.generate(user, email: 'test@example.com')