SECURITY: XSS issue on Admin users list

This commit is contained in:
Robin Ward 2016-08-05 12:01:16 -04:00
parent 429f27ec96
commit 3d62e5dd98
8 changed files with 75 additions and 10 deletions

View file

@ -21,7 +21,7 @@
{{#conditional-loading-spinner condition=refreshing}} {{#conditional-loading-spinner condition=refreshing}}
{{#if model}} {{#if model}}
<table class='table'> <table class='table users-list'>
<tr> <tr>
{{#if showApproval}} {{#if showApproval}}
<th>{{input type="checkbox" checked=selectAll}}</th> <th>{{input type="checkbox" checked=selectAll}}</th>
@ -42,7 +42,7 @@
</tr> </tr>
{{#each model as |user|}} {{#each model as |user|}}
<tr class="{{user.selected}} {{unless user.active 'not-activated'}}"> <tr class="user {{user.selected}} {{unless user.active 'not-activated'}}">
{{#if showApproval}} {{#if showApproval}}
<td> <td>
{{#if user.can_approve}} {{#if user.can_approve}}
@ -52,7 +52,7 @@
{{/if}} {{/if}}
<td><a href="{{unbound user.path}}" data-user-card="{{unbound user.username}}">{{avatar user imageSize="small"}}</a></td> <td><a href="{{unbound user.path}}" data-user-card="{{unbound user.username}}">{{avatar user imageSize="small"}}</a></td>
<td>{{#link-to 'adminUser' user}}{{unbound user.username}}{{/link-to}}</td> <td>{{#link-to 'adminUser' user}}{{unbound user.username}}{{/link-to}}</td>
<td>{{{unbound user.email}}}</td> <td class='email'>{{unbound user.email}}</td>
<td>{{{unbound user.last_emailed_age}}}</td> <td>{{{unbound user.last_emailed_age}}}</td>
<td>{{{unbound user.last_seen_age}}}</td> <td>{{{unbound user.last_seen_age}}}</td>
<td>{{{unbound user.topics_entered}}}</td> <td>{{{unbound user.topics_entered}}}</td>

View file

@ -3,6 +3,7 @@ import ModalFunctionality from 'discourse/mixins/modal-functionality';
import showModal from 'discourse/lib/show-modal'; import showModal from 'discourse/lib/show-modal';
import { setting } from 'discourse/lib/computed'; import { setting } from 'discourse/lib/computed';
import { findAll } from 'discourse/models/login-method'; import { findAll } from 'discourse/models/login-method';
import { escape } from 'pretty-text/sanitizer';
// This is happening outside of the app via popup // This is happening outside of the app via popup
const AuthErrors = const AuthErrors =
@ -63,11 +64,11 @@ export default Ember.Controller.extend(ModalFunctionality, {
// Successful login // Successful login
if (result.error) { if (result.error) {
self.set('loggingIn', false); self.set('loggingIn', false);
if( result.reason === 'not_activated' ) { if (result.reason === 'not_activated') {
self.send('showNotActivated', { self.send('showNotActivated', {
username: self.get('loginName'), username: self.get('loginName'),
sentTo: result.sent_to_email, sentTo: escape(result.sent_to_email),
currentEmail: result.current_email currentEmail: escape(result.current_email)
}); });
} else if (result.reason === 'suspended' ) { } else if (result.reason === 'suspended' ) {
self.send("closeModal"); self.send("closeModal");

View file

@ -3,7 +3,7 @@
{{{i18n 'login.sent_activation_email_again' currentEmail=currentEmail}}} {{{i18n 'login.sent_activation_email_again' currentEmail=currentEmail}}}
{{else}} {{else}}
{{{i18n 'login.not_activated' sentTo=sentTo}}} {{{i18n 'login.not_activated' sentTo=sentTo}}}
<a href {{action "sendActivationEmail"}}>{{i18n 'login.resend_activation_email'}}</a> <a href {{action "sendActivationEmail"}} class="resend-link">{{i18n 'login.resend_activation_email'}}</a>
{{/if}} {{/if}}
</div> </div>
<div class="modal-footer"> <div class="modal-footer">

View file

@ -51,7 +51,7 @@ class EmailActivator < UserActivator
user_id: user.id, user_id: user.id,
email_token: email_token.token email_token: email_token.token
) )
I18n.t("login.activate_email", email: user.email) I18n.t("login.activate_email", email: Rack::Utils.escape_html(user.email))
end end
end end

View file

@ -27,5 +27,15 @@ describe UserActivator do
user.reload user.reload
expect(user.email_tokens.last.created_at).to be_within_one_second_of(Time.zone.now) expect(user.email_tokens.last.created_at).to be_within_one_second_of(Time.zone.now)
end end
it "escapes the email in the message" do
user = Fabricate(:user, email: '<strike>eviltrout@example.com</strike>')
activator = EmailActivator.new(user, nil, nil, nil)
msg = activator.activate
expect(msg).to match(/eviltrout@example.com/)
expect(msg).not_to match(/<strike>/)
end
end end
end end

View file

@ -0,0 +1,11 @@
import { acceptance } from "helpers/qunit-helpers";
acceptance("Admin - Users List", { loggedIn: true });
test("lists users", () => {
visit("/admin/users/list/active");
andThen(() => {
ok(exists('.users-list .user'));
ok(!exists('.user:eq(0) .email small'), 'escapes email');
});
});

View file

@ -13,7 +13,7 @@ test("sign in", () => {
fillIn('#login-account-password', 'incorrect'); fillIn('#login-account-password', 'incorrect');
click('.modal-footer .btn-primary'); click('.modal-footer .btn-primary');
andThen(() => { andThen(() => {
ok(exists('#modal-alert:visible', 'it displays the login error')); ok(exists('#modal-alert:visible'), 'it displays the login error');
not(exists('.modal-footer .btn-primary:disabled'), "enables the login button"); not(exists('.modal-footer .btn-primary:disabled'), "enables the login button");
}); });
@ -25,6 +25,33 @@ test("sign in", () => {
}); });
}); });
test("sign in - not activated", () => {
visit("/");
andThen(() => {
click("header .login-button");
andThen(() => {
ok(exists('.login-modal'), "it shows the login modal");
});
fillIn('#login-account-name', 'eviltrout');
fillIn('#login-account-password', 'not-activated');
click('.modal-footer .btn-primary');
andThen(() => {
equal(find('.modal-body b').text(), '<small>eviltrout@example.com</small>');
ok(!exists('.modal-body small'), 'it escapes the email address');
});
click('.modal-body .resend-link');
andThen(() => {
equal(find('.modal-body b').text(), '<small>current@example.com</small>');
ok(!exists('.modal-body small'), 'it escapes the email address');
});
});
});
test("create account", () => { test("create account", () => {
visit("/"); visit("/");
click("header .sign-up-button"); click("header .sign-up-button");
@ -55,5 +82,4 @@ test("create account", () => {
andThen(() => { andThen(() => {
ok(exists('.modal-footer .btn-primary:disabled'), "create account is disabled"); ok(exists('.modal-footer .btn-primary:disabled'), "create account is disabled");
}); });
}); });

View file

@ -149,9 +149,19 @@ export default function() {
if (data.password === 'correct') { if (data.password === 'correct') {
return response({username: 'eviltrout'}); return response({username: 'eviltrout'});
} }
if (data.password === 'not-activated') {
return response({ error: "not active",
reason: "not_activated",
sent_to_email: '<small>eviltrout@example.com</small>',
current_email: '<small>current@example.com</small>' });
}
return response(400, {error: 'invalid login'}); return response(400, {error: 'invalid login'});
}); });
this.post('/users/action/send_activation_email', success);
this.get('/users/hp.json', function() { this.get('/users/hp.json', function() {
return response({"value":"32faff1b1ef1ac3","challenge":"61a3de0ccf086fb9604b76e884d75801"}); return response({"value":"32faff1b1ef1ac3","challenge":"61a3de0ccf086fb9604b76e884d75801"});
}); });
@ -242,6 +252,13 @@ export default function() {
const siteText = {id: 'site.test', value: 'Test McTest'}; const siteText = {id: 'site.test', value: 'Test McTest'};
const overridden = {id: 'site.overridden', value: 'Overridden', overridden: true }; const overridden = {id: 'site.overridden', value: 'Overridden', overridden: true };
this.get('/admin/users/list/active.json', () => {
return response(200, [
{id: 1, username: 'eviltrout', email: '<small>eviltrout@example.com</small>'}
]);
});
this.get('/admin/customize/site_texts', request => { this.get('/admin/customize/site_texts', request => {
if (request.queryParams.overridden) { if (request.queryParams.overridden) {