From 3d62e5dd981f5857e6b6020d117c77d93b660890 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Fri, 5 Aug 2016 12:01:16 -0400 Subject: [PATCH] SECURITY: XSS issue on Admin users list --- .../admin/templates/users-list-show.hbs | 6 ++-- .../discourse/controllers/login.js.es6 | 7 +++-- .../templates/modal/not-activated.hbs | 2 +- app/services/user_activator.rb | 2 +- spec/services/user_activator_spec.rb | 10 +++++++ .../acceptance/admin-users-list-test.js.es6 | 11 +++++++ .../acceptance/sign-in-test.js.es6 | 30 +++++++++++++++++-- .../helpers/create-pretender.js.es6 | 17 +++++++++++ 8 files changed, 75 insertions(+), 10 deletions(-) create mode 100644 test/javascripts/acceptance/admin-users-list-test.js.es6 diff --git a/app/assets/javascripts/admin/templates/users-list-show.hbs b/app/assets/javascripts/admin/templates/users-list-show.hbs index aa478dcd6..879161e3d 100644 --- a/app/assets/javascripts/admin/templates/users-list-show.hbs +++ b/app/assets/javascripts/admin/templates/users-list-show.hbs @@ -21,7 +21,7 @@ {{#conditional-loading-spinner condition=refreshing}} {{#if model}} - +
{{#if showApproval}} @@ -42,7 +42,7 @@ {{#each model as |user|}} - + {{#if showApproval}} - + diff --git a/app/assets/javascripts/discourse/controllers/login.js.es6 b/app/assets/javascripts/discourse/controllers/login.js.es6 index 4303de475..4abefa97b 100644 --- a/app/assets/javascripts/discourse/controllers/login.js.es6 +++ b/app/assets/javascripts/discourse/controllers/login.js.es6 @@ -3,6 +3,7 @@ import ModalFunctionality from 'discourse/mixins/modal-functionality'; import showModal from 'discourse/lib/show-modal'; import { setting } from 'discourse/lib/computed'; import { findAll } from 'discourse/models/login-method'; +import { escape } from 'pretty-text/sanitizer'; // This is happening outside of the app via popup const AuthErrors = @@ -63,11 +64,11 @@ export default Ember.Controller.extend(ModalFunctionality, { // Successful login if (result.error) { self.set('loggingIn', false); - if( result.reason === 'not_activated' ) { + if (result.reason === 'not_activated') { self.send('showNotActivated', { username: self.get('loginName'), - sentTo: result.sent_to_email, - currentEmail: result.current_email + sentTo: escape(result.sent_to_email), + currentEmail: escape(result.current_email) }); } else if (result.reason === 'suspended' ) { self.send("closeModal"); diff --git a/app/assets/javascripts/discourse/templates/modal/not-activated.hbs b/app/assets/javascripts/discourse/templates/modal/not-activated.hbs index f8df2639e..9d6620cbc 100644 --- a/app/assets/javascripts/discourse/templates/modal/not-activated.hbs +++ b/app/assets/javascripts/discourse/templates/modal/not-activated.hbs @@ -3,7 +3,7 @@ {{{i18n 'login.sent_activation_email_again' currentEmail=currentEmail}}} {{else}} {{{i18n 'login.not_activated' sentTo=sentTo}}} - {{i18n 'login.resend_activation_email'}} + {{i18n 'login.resend_activation_email'}} {{/if}}
diff --git a/app/services/user_activator.rb b/app/services/user_activator.rb index c2d5c9dfe..6b151874c 100644 --- a/app/services/user_activator.rb +++ b/app/services/user_activator.rb @@ -51,7 +51,7 @@ class EmailActivator < UserActivator user_id: user.id, 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 diff --git a/spec/services/user_activator_spec.rb b/spec/services/user_activator_spec.rb index 43609ee47..3a7974983 100644 --- a/spec/services/user_activator_spec.rb +++ b/spec/services/user_activator_spec.rb @@ -27,5 +27,15 @@ describe UserActivator do user.reload expect(user.email_tokens.last.created_at).to be_within_one_second_of(Time.zone.now) end + + it "escapes the email in the message" do + user = Fabricate(:user, email: 'eviltrout@example.com') + activator = EmailActivator.new(user, nil, nil, nil) + msg = activator.activate + + expect(msg).to match(/eviltrout@example.com/) + expect(msg).not_to match(//) + end + end end diff --git a/test/javascripts/acceptance/admin-users-list-test.js.es6 b/test/javascripts/acceptance/admin-users-list-test.js.es6 new file mode 100644 index 000000000..a391ff9b7 --- /dev/null +++ b/test/javascripts/acceptance/admin-users-list-test.js.es6 @@ -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'); + }); +}); diff --git a/test/javascripts/acceptance/sign-in-test.js.es6 b/test/javascripts/acceptance/sign-in-test.js.es6 index 32ee35b03..d8efb4be1 100644 --- a/test/javascripts/acceptance/sign-in-test.js.es6 +++ b/test/javascripts/acceptance/sign-in-test.js.es6 @@ -13,7 +13,7 @@ test("sign in", () => { fillIn('#login-account-password', 'incorrect'); click('.modal-footer .btn-primary'); 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"); }); @@ -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(), 'eviltrout@example.com'); + ok(!exists('.modal-body small'), 'it escapes the email address'); + }); + + click('.modal-body .resend-link'); + andThen(() => { + equal(find('.modal-body b').text(), 'current@example.com'); + ok(!exists('.modal-body small'), 'it escapes the email address'); + }); + + + }); +}); + + test("create account", () => { visit("/"); click("header .sign-up-button"); @@ -55,5 +82,4 @@ test("create account", () => { andThen(() => { ok(exists('.modal-footer .btn-primary:disabled'), "create account is disabled"); }); - }); diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index 85d7c9371..fc2c537c1 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/test/javascripts/helpers/create-pretender.js.es6 @@ -149,9 +149,19 @@ export default function() { if (data.password === 'correct') { return response({username: 'eviltrout'}); } + + if (data.password === 'not-activated') { + return response({ error: "not active", + reason: "not_activated", + sent_to_email: 'eviltrout@example.com', + current_email: 'current@example.com' }); + } + return response(400, {error: 'invalid login'}); }); + this.post('/users/action/send_activation_email', success); + this.get('/users/hp.json', function() { return response({"value":"32faff1b1ef1ac3","challenge":"61a3de0ccf086fb9604b76e884d75801"}); }); @@ -242,6 +252,13 @@ export default function() { const siteText = {id: 'site.test', value: 'Test McTest'}; const overridden = {id: 'site.overridden', value: 'Overridden', overridden: true }; + + this.get('/admin/users/list/active.json', () => { + return response(200, [ + {id: 1, username: 'eviltrout', email: 'eviltrout@example.com'} + ]); + }); + this.get('/admin/customize/site_texts', request => { if (request.queryParams.overridden) {
{{input type="checkbox" checked=selectAll}}
{{#if user.can_approve}} @@ -52,7 +52,7 @@ {{/if}} {{avatar user imageSize="small"}} {{#link-to 'adminUser' user}}{{unbound user.username}}{{/link-to}}{{{unbound user.email}}} {{{unbound user.last_emailed_age}}} {{{unbound user.last_seen_age}}} {{{unbound user.topics_entered}}}