From a6c848cc9f002c9cbce83ba9a16da42f76405cd0 Mon Sep 17 00:00:00 2001 From: Shane Tomlinson Date: Mon, 25 Jul 2016 19:30:29 +0100 Subject: [PATCH] fix(client): Hide all visible passwords on form submit. (#3969) * fix(password): save passwords as passwords, not text Fixes #3799 * fix(client): Hide all visible passwords on form submit. This builds on @TDA's work. Before the form is submit, convert all visible passwords from [type=text] to [type=password] so that they are not saved as form data. In the process, I noticed two problems that are fixed: * The cursor was not being correctly replaced to its original position once a password field was converted. * If `setPasswordVisibility` was called programatically to toggle the state of the password field, the corresponding "Show" checkbox was not kept in sync. fixes #3799 * chore(client): `wekbit` => `webkit` r=@vladikoff --- app/scripts/views/base.js | 31 ++-- app/scripts/views/form.js | 2 + app/scripts/views/mixins/password-mixin.js | 84 ++++++++--- app/tests/lib/helpers.js | 33 +++-- app/tests/spec/views/base.js | 48 ++++++- app/tests/spec/views/form.js | 6 + app/tests/spec/views/mixins/password-mixin.js | 135 +++++++++++++----- 7 files changed, 255 insertions(+), 84 deletions(-) diff --git a/app/scripts/views/base.js b/app/scripts/views/base.js index b23c35403..f0e55e96e 100644 --- a/app/scripts/views/base.js +++ b/app/scripts/views/base.js @@ -670,18 +670,14 @@ define(function (require, exports, module) { /** * Safely focus an element */ - focus: function (which) { + focus (which) { try { - var focusEl = this.$(which); + const focusEl = this.$(which); // place the cursor at the end of the input when the // element is focused. + var self = this; focusEl.one('focus', function () { - try { - this.selectionStart = this.selectionEnd = this.value.length; - } catch (e) { - // This can blow up on password fields in Chrome. Drop the error on - // the ground, for whatever reason, it still behaves as we expect. - } + self.placeCursorAt(this, this.value.length); }); focusEl.get(0).focus(); } catch (e) { @@ -689,6 +685,25 @@ define(function (require, exports, module) { } }, + /** + * Place the cursor at the given position within the input element + * + * @param {selector | element} which + * @param {number} selectionStart - defaults to 0. + * @param {number} selectionEnd - defaults to selectionStart. + */ + placeCursorAt (which, selectionStart = 0, selectionEnd = selectionStart) { + const el = $(which).get(0); + + try { + el.selectionStart = selectionStart; + el.selectionEnd = selectionEnd; + } catch (e) { + // This can blow up on password fields in Chrome. Drop the error on + // the ground, for whatever reason, it still behaves as we expect. + } + }, + /** * Invoke the specified handler with the given event. Handler * can either be a function or a string. If a string, looks for diff --git a/app/scripts/views/form.js b/app/scripts/views/form.js index d8d71f286..a6727a920 100644 --- a/app/scripts/views/form.js +++ b/app/scripts/views/form.js @@ -172,6 +172,8 @@ define(function (require, exports, module) { event.stopImmediatePropagation(); } + this.trigger('submitStart'); + return p() .then(function () { if (self.isHalted()) { diff --git a/app/scripts/views/mixins/password-mixin.js b/app/scripts/views/mixins/password-mixin.js index f55d69ac0..811931d8d 100644 --- a/app/scripts/views/mixins/password-mixin.js +++ b/app/scripts/views/mixins/password-mixin.js @@ -7,7 +7,8 @@ define(function (require, exports, module) { 'use strict'; - var Constants = require('lib/constants'); + const $ = require('jquery'); + const Constants = require('lib/constants'); module.exports = { events: { @@ -15,7 +16,15 @@ define(function (require, exports, module) { 'keyup input.password': 'onPasswordKeyUp' }, - afterVisible: function () { + initialize () { + // An internal submitStart event is listened for instead of + // the `submit` DOM event because form.js already binds a submit + // event. Because of the way Cocktail wraps colliding functions, + // the form is always submit if a second event handler is added. + this.on('submitStart', () => this.hideVisiblePasswords()); + }, + + afterVisible () { if (this.isInExperiment && this.isInExperiment('showPassword')) { this.notifier.trigger('showPassword.triggered'); @@ -25,7 +34,7 @@ define(function (require, exports, module) { } }, - onPasswordVisibilityChange: function (event) { + onPasswordVisibilityChange (event) { var target = this.$(event.target); this.setPasswordVisibilityFromButton(target); @@ -39,39 +48,80 @@ define(function (require, exports, module) { this.focus(controlsSelector); }, - setPasswordVisibilityFromButton: function (button) { + setPasswordVisibilityFromButton (button) { var isVisible = this.$(button).is(':checked'); var targets = this.getAffectedPasswordInputs(button); - this.setPasswordVisibility(isVisible, targets); + this.setPasswordVisibility(targets, isVisible); }, - getAffectedPasswordInputs: function (button) { - var passwordField = this.$(button).siblings('.password'); + getAffectedPasswordInputs (button) { + var $passwordEl = this.$(button).siblings('.password'); if (this.$(button).data('synchronizeShow')) { - passwordField = this.$('.password'); + $passwordEl = this.$('.password'); } - return passwordField; + return $passwordEl; }, - setPasswordVisibility: function (isVisible, passwordField) { + /** + * Set the password visibility for an element. Ensure the "show" button's + * state is synchronized. + * + * @param {selector | element} which + * @param {boolean} isVisible + */ + setPasswordVisibility (which, isVisible) { + const $passwordEl = $(which); + const $showPasswordEl = $passwordEl.siblings('.show-password'); + + // Store the cursor position before updating the element type + // or else the cursor is set to before the first character. + + // `which` can match more than one element. If this is the case + // find the element that is focused. + let $focusedEl = $passwordEl.filter(':focus'); + let selectionStart; + let selectionEnd; + + if ($focusedEl.length) { + const focusedEl = $focusedEl.get(0); + selectionStart = focusedEl.selectionStart; + selectionEnd = focusedEl.selectionEnd; + } + try { if (isVisible) { - passwordField.attr('type', 'text').attr('autocomplete', 'off') + $passwordEl.attr('type', 'text').attr('autocomplete', 'off') .attr('autocorrect', 'off').attr('autocapitalize', 'off'); + $showPasswordEl.attr('checked', true); this.logViewEvent('password.visible'); } else { - passwordField.attr('type', 'password'); - passwordField.removeAttr('autocomplete') - .removeAttr('autocorrect').removeAttr('autocapitalize'); + $passwordEl.attr('type', 'password').removeAttr('autocomplete') + .removeAttr('autocorrect').removeAttr('autocapitalize'); + $showPasswordEl.removeAttr('checked'); this.logViewEvent('password.hidden'); } } catch (e) { // IE8 blows up when changing the type of the input field. Other // browsers might too. Ignore the error. } + + // Restore the cursor position if the element is in focus. + if ($focusedEl.length) { + this.placeCursorAt($focusedEl, selectionStart, selectionEnd); + } }, - onPasswordKeyUp: function () { + /** + * Hide all visible passwords to prevent passwords from being saved + * by the browser as text form data. + */ + hideVisiblePasswords () { + this.$el.find('.password[type=text]').each((index, el) => { + this.setPasswordVisibility(el, false); + }); + }, + + onPasswordKeyUp () { var values = []; // Values contains all password classes length @@ -88,11 +138,11 @@ define(function (require, exports, module) { } }, - showPasswordHelper: function () { + showPasswordHelper () { this.$('.input-help').css('opacity', '1'); }, - hidePasswordHelper: function () { + hidePasswordHelper () { // Hide all input-help classes except input-help-forgot-pw this.$('.input-help:not(.input-help-forgot-pw)').css('opacity', '0'); } diff --git a/app/tests/lib/helpers.js b/app/tests/lib/helpers.js index df751ad28..8a9eedfff 100644 --- a/app/tests/lib/helpers.js +++ b/app/tests/lib/helpers.js @@ -11,6 +11,23 @@ define(function (require, exports, module) { var ProfileMock = require('../mocks/profile.js'); var sinon = require('sinon'); + function noOp () {} + + function ifDocumentFocused(callback, done = noOp) { + if (document.hasFocus && document.hasFocus()) { + callback(); + } else { + const message = + 'Cannot check for focus - document does not have focus.\n' + + 'If this is in Travis-CI, Sauce Labs, or Opera, this is expected.\n' + + 'Otherwise, try focusing the test document instead of \n' + + 'another window or dev tools.'; + + console.warn(message); + done(); + } + } + function requiresFocus(callback, done) { // Give the document focus window.focus(); @@ -20,20 +37,7 @@ define(function (require, exports, module) { document.activeElement.blur(); } - if (document.hasFocus && document.hasFocus()) { - callback(); - } else { - var message = - 'Cannot check for focus - document does not have focus.\n' + - 'If this is in Travis-CI, Sauce Labs, or Opera, this is expected.\n' + - 'Otherwise, try focusing the test document instead of \n' + - 'another window or dev tools.'; - - console.warn(message); - if (done) { - done(); - } - } + ifDocumentFocused(callback, done); } function addFxaClientSpy(fxaClient) { @@ -164,6 +168,7 @@ define(function (require, exports, module) { createUid: createUid, emailToUser: emailToUser, getValueLabel: getValueLabel, + ifDocumentFocused: ifDocumentFocused, indexOfEvent: indexOfEvent, isErrorLogged: isErrorLogged, isEventLogged: isEventLogged, diff --git a/app/tests/spec/views/base.js b/app/tests/spec/views/base.js index 6d9972a40..aa16be644 100644 --- a/app/tests/spec/views/base.js +++ b/app/tests/spec/views/base.js @@ -361,7 +361,7 @@ define(function (require, exports, module) { it('focuses descendent element containing `autofocus` if html has `no-touch` class', function (done) { requiresFocus(function () { $('html').addClass('no-touch'); - // wekbit fails unless focusing another element first. + // webkit fails unless focusing another element first. $('#otherElement').focus(); var handlerCalled = false; @@ -589,20 +589,54 @@ define(function (require, exports, module) { }); }); - describe('focus', function () { - it('focuses an element', function (done) { - requiresFocus(function () { - // wekbit fails unless focusing another element first. + describe('focus', () => { + it('focuses an element, sets the cursor position', (done) => { + requiresFocus(() => { + // webkit fails unless focusing another element first. $('#otherElement').focus(); - view.$('#focusMe').one('focus', function () { + const elText = 'some text'; + const $focusEl = view.$('#focusMe').val(elText); + + // Use setTimeout because the selection is set in a `focus` handler + // and this `focus` handler is run first. + view.$('#focusMe').one('focus', () => setTimeout(() => { + const focusEl = $focusEl.get(0); + assert.equal(focusEl.selectionStart, elText.length); + assert.equal(focusEl.selectionEnd, elText.length); done(); - }); + }, 0)); + view.focus('#focusMe'); }, done); }); }); + describe('placeCursorAt', () => { + const elText = 'some text'; + let $focusEl; + let focusEl; + + beforeEach(() => { + $focusEl = view.$('#focusMe').val(elText); + focusEl = $focusEl.get(0); + }); + + it('places the cursor at the specified position if only start given', () => { + view.placeCursorAt('#focusMe', elText.length); + + assert.equal(focusEl.selectionStart, elText.length); + assert.equal(focusEl.selectionEnd, elText.length); + }); + + it('places the cursor at the specified selection if both given', () => { + view.placeCursorAt('#focusMe', elText.length - 2, elText.length); + + assert.equal(focusEl.selectionStart, elText.length - 2); + assert.equal(focusEl.selectionEnd, elText.length); + }); + }); + describe('trackChildView/untrackChildView', function () { it('trackChildView tracks a childView, untrackChildView untracks the childView', function () { var childView = new BaseView(); diff --git a/app/tests/spec/views/form.js b/app/tests/spec/views/form.js index 9e6c11de8..bea7ca862 100644 --- a/app/tests/spec/views/form.js +++ b/app/tests/spec/views/form.js @@ -164,6 +164,12 @@ define(function (require, exports, module) { }); describe('validateAndSubmit', function () { + it('triggers a `submitStart` event', (done) => { + view.on('submitStart', () => done()); + + view.validateAndSubmit(); + }); + it('submits form if isValid returns true', function () { view.formIsValid = true; view.enableSubmitIfValid(); diff --git a/app/tests/spec/views/mixins/password-mixin.js b/app/tests/spec/views/mixins/password-mixin.js index 59ef24681..a89c661af 100644 --- a/app/tests/spec/views/mixins/password-mixin.js +++ b/app/tests/spec/views/mixins/password-mixin.js @@ -5,24 +5,23 @@ define(function (require, exports, module) { 'use strict'; - var $ = require('jquery'); - var BaseView = require('views/base'); - var chai = require('chai'); - var Cocktail = require('cocktail'); - var ExperimentMixin = require('views/mixins/experiment-mixin'); - var Metrics = require('lib/metrics'); - var Notifier = require('lib/channels/notifier'); - var PasswordMixin = require('views/mixins/password-mixin'); - var Relier = require('models/reliers/relier'); - var sinon = require('sinon'); - var TestHelpers = require('../../../lib/helpers'); - var TestTemplate = require('stache!templates/test_template'); + const $ = require('jquery'); + const assert = require('chai').assert; + const BaseView = require('views/base'); + const Cocktail = require('cocktail'); + const ExperimentMixin = require('views/mixins/experiment-mixin'); + const Metrics = require('lib/metrics'); + const Notifier = require('lib/channels/notifier'); + const PasswordMixin = require('views/mixins/password-mixin'); + const Relier = require('models/reliers/relier'); + const sinon = require('sinon'); + const TestHelpers = require('../../../lib/helpers'); + const TestTemplate = require('stache!templates/test_template'); - var assert = chai.assert; - - var PasswordView = BaseView.extend({ + const PasswordView = BaseView.extend({ template: TestTemplate }); + Cocktail.mixin( PasswordView, PasswordMixin, @@ -30,9 +29,9 @@ define(function (require, exports, module) { ); describe('views/mixins/password-mixin', function () { - var view; - var relier; - var metrics; + let metrics; + let relier; + let view; beforeEach(function () { relier = new Relier(); @@ -94,7 +93,6 @@ define(function (require, exports, module) { view.afterVisible(); assert.isFalse(view.$('.show-password-label').is(':hidden')); }); - }); describe('onPasswordVisibilityChange', function () { @@ -114,27 +112,69 @@ define(function (require, exports, module) { }); }); + describe('setPasswordVisibility', () => { + it('to visible', () => { + const $passwordEl = view.$('#password'); + $passwordEl.val('password').focus(); + + const selectionStart = 1; + const selectionEnd = 2; + + const passwordEl = $passwordEl.get(0); + passwordEl.selectionStart = selectionStart; + passwordEl.selectionEnd = selectionEnd; + + view.setPasswordVisibility('#password', true); + + assert.equal($passwordEl.attr('type'), 'text'); + assert.equal($passwordEl.attr('autocapitalize'), 'off'); + assert.equal($passwordEl.attr('autocorrect'), 'off'); + + // Ensure the cursor is replaced to its original position. + // Only make the check if the document has focus, otherwise + // the test fails. Document may not have focus if the dev + // clicks out of the test window or when run in PhantomJS. + TestHelpers.ifDocumentFocused(() => { + assert.equal(passwordEl.selectionStart, selectionStart); + assert.equal(passwordEl.selectionEnd, selectionEnd); + }); + + // Ensure the show password state stays in sync + const $showPasswordEl = $passwordEl.siblings('.show-password'); + assert.isTrue($showPasswordEl.is(':checked')); + }); + + it('to hidden', () => { + view.setPasswordVisibility('#password', false); + + const $passwordEl = view.$('#password'); + assert.equal($passwordEl.attr('autocomplete'), null); + assert.equal($passwordEl.attr('autocapitalize'), null); + assert.equal($passwordEl.attr('autocorrect'), null); + + // Ensure the show password state stays in sync + const $showPasswordEl = $passwordEl.siblings('.show-password'); + assert.isFalse($showPasswordEl.is(':checked')); + }); + }); + describe('setPasswordVisibilityFromButton', function () { it('sets the password field to type=text if button is checked', function () { view.$('#show-password').attr('checked', 'checked'); view.setPasswordVisibilityFromButton('#show-password'); assert.equal(view.$('#password').attr('type'), 'text'); - assert.equal(view.$('#password').attr('autocapitalize'), 'off'); - assert.equal(view.$('#password').attr('autocorrect'), 'off'); }); it('sets the password field to type=password if button is unchecked', function () { view.$('#show-password').removeAttr('checked'); view.setPasswordVisibilityFromButton('#show-password'); assert.equal(view.$('#password').attr('type'), 'password'); - assert.isUndefined(view.$('#password').attr('autocapitalize')); - assert.isUndefined(view.$('#password').attr('autocorrect')); }); }); describe('clicking on unsynched/synched show buttons', function () { - it('gets pwd inputs to be shown', function () { - var targets = view.getAffectedPasswordInputs('#show-password'); + it('gets password inputs to be shown', function () { + let targets = view.getAffectedPasswordInputs('#show-password'); assert.equal(targets.length, 1); view.$('#show-password').data('synchronize-show', 'true'); @@ -147,26 +187,14 @@ define(function (require, exports, module) { it('pw field set to text when clicked', function () { view.$('.show-password').click(); assert.equal(view.$('#password').attr('type'), 'text'); - assert.equal(view.$('#password').attr('autocomplete'), 'off'); - assert.equal(view.$('#password').attr('autocapitalize'), 'off'); - assert.equal(view.$('#password').attr('autocorrect'), 'off'); assert.equal(view.$('#vpassword').attr('type'), 'text'); - assert.equal(view.$('#vpassword').attr('autocomplete'), 'off'); - assert.equal(view.$('#vpassword').attr('autocomplete'), 'off'); - assert.equal(view.$('#vpassword').attr('autocapitalize'), 'off'); }); it('pw field set to password when clicked again', function () { view.$('.show-password').click(); view.$('.show-password').click(); assert.equal(view.$('#password').attr('type'), 'password'); - assert.equal(view.$('#password').attr('autocomplete'), null); - assert.equal(view.$('#password').attr('autocapitalize'), null); - assert.equal(view.$('#password').attr('autocorrect'), null); - assert.equal($('#vpassword').attr('type'), 'password'); - assert.equal($('#vpassword').attr('autocomplete'), null); - assert.equal($('#vpassword').attr('autocapitalize'), null); - assert.equal($('#vpassword').attr('autocorrect'), null); + assert.equal(view.$('#vpassword').attr('type'), 'password'); }); it('logs whether the password is shown or hidden', function () { @@ -206,5 +234,36 @@ define(function (require, exports, module) { assert.equal(view.$('.input-help-forgot-pw').css('opacity'), '1'); }); }); + + describe('hideVisiblePasswords', () => { + it('sets all password fields to type `password`', () => { + const $passwordEls = view.$('.password'); + + assert.equal($passwordEls.length, 2); + + $passwordEls.each((index, el) => { + view.setPasswordVisibility(el, true); + assert.equal(el.type, 'text'); + }); + + view.hideVisiblePasswords(); + + $passwordEls.each((i, el) => { + assert.equal(el.type, 'password'); + }); + }); + }); + + describe('submitStart event', () => { + beforeEach(() => { + sinon.spy(view, 'hideVisiblePasswords'); + }); + + it('hides all visible passwords', () => { + assert.equal(view.hideVisiblePasswords.callCount, 0); + view.trigger('submitStart'); + assert.equal(view.hideVisiblePasswords.callCount, 1); + }); + }); }); });