fix(client): All template writes are by default HTML escaped. #4296

shane-tomlinson · 2016-10-18T16:24:39Z

A new Mustache helper function unsafeTranslate, has been added
for unsafe writes, to be used when inserting HTML.

All interpolation variables passed to unsafeTranslate must be
prefixed with escaped or else an exception is thrown. This is
to remind the developer to escape the variable in the most
appropriate way.

The t helper function now HTML escapes by default.

Rename some function/variable names for consistency:

displaySuccessUnsafe => unsafeDisplaySuccess
displayErrorUnsafe => unsafeDisplayError
successUnsafe => unsafeSuccess

shane-tomlinson · 2016-10-18T16:26:59Z

app/scripts/views/base.js

+     */
+    unsafeTranslate (text, context = this.getContext()) {
+      if (Strings.hasUnsafeVariables(text)) {
+        throw new Error(`Unsafe string to translate: ${text}`);


Instead of a hard exception, maybe just log these for a train to see if we missed any?

shane-tomlinson · 2016-10-19T11:05:57Z

app/scripts/lib/strings.js

+   * @returns {Boolean} true if string has HTML, false otw.
+   */
+  function hasHTML(string) {
+    if (/&\D+;/i.test(string)) {


Extract these regexp"s into constants

shane-tomlinson · 2016-10-19T11:07:49Z

app/scripts/views/base.js

+     */
+    unsafeTranslate (text, context = this.getContext()) {
+      if (Strings.hasUnsafeVariables(text)) {
+        const err = AuthErrors.toError('UNSAFE_VARIABLE_NAME');


Maybe rename the error to UNSAFE_INTERPOLATION_VARIABLE_NAME

shane-tomlinson · 2016-10-19T11:08:33Z

app/scripts/views/mixins/open-webmail-mixin.js

@@ -75,7 +75,7 @@ define(function (require, exports, module) {

      if (email && isOpenWebmailButtonVisible) {
        _.extend(context, {
-          unsafeWebmailLink: this.getWebmailLink(email),
+          escapedWebmailLink: this.getWebmailLink(email),


I don"t see where this is escaped!

shane-tomlinson · 2016-10-19T11:09:08Z

app/scripts/views/mixins/password-prompt-mixin.js

@@ -19,8 +19,7 @@ define(function (require, exports, module) {
  const TOOLTIP_MESSAGES = {
    FOCUS_PROMPT_MESSAGE: t('8 characters minimum, but longer if you plan to sync passwords.'),
    INITIAL_PROMPT_MESSAGE: t('A strong, unique password will keep your Firefox data safe from intruders.'),
-    WARNING_PROMPT_MESSAGE: t('This is a common password; please consider another one.'),
-    WARNING_PROMPT_MESSAGE_WITH_LINK: t('This is a common password; please consider another one.')


the _WITH_LINK variant didn"t have a link, so I consolidated the two messages.

shane-tomlinson · 2016-10-19T11:11:47Z

app/tests/spec/views/base.js

@@ -551,6 +597,14 @@ define(function (require, exports, module) {
      });
    });

+    describe('unsafeDisplaySuccess', () => {
+      it('allows HTML in messages', () => {
+        const expected = 'a message<div>with html</div>';


use HTML_MESSAGE instead.

shane-tomlinson · 2016-10-19T11:12:24Z

app/tests/spec/views/base.js

+          view.unsafeTranslate(HTML_MESSAGE), UNESCAPED_HTML_TRANSLATION);
+        assert.isFalse(view.logError.called);
+
+        view.unsafeTranslate('%(unsafeVariableName)s');


Extract this string to a constant.

shane-tomlinson · 2016-10-19T15:02:36Z

@mozilla/fxa-devs - who wants it?

seanmonstar

I absolutely love the explicit naming: unsafeTranslate and escapedFoo. This PR just gives me a warm fuzzy feeling inside. Thank you!

seanmonstar · 2016-10-19T18:12:34Z

app/scripts/views/base.js

+      if (Strings.hasHTML(text)) {
+        const err = AuthErrors.toError('HTML_WILL_BE_ESCAPED');
+        err.string = text;
+        this.logError(err);


In production does this error get sent to us, like sentry or something?

Yup, exactly, the string itself will be part of the message displayed in Sentry.

seanmonstar · 2016-10-19T18:20:43Z

app/scripts/lib/strings.js

-  var t = function (msg) {
-    return msg;
-  };
+  const HTML_CHAR_CODE_REGEXP = /&\D+;/i;


This won"t catch when the escape code is numeric. > and ? are equivalent, this regex won"t catch the latter.

seanmonstar · 2016-10-19T18:24:20Z

app/scripts/lib/strings.js

+   * @returns {Boolean} true if has unsafe variables, false otw.
+   */
+  function hasUnsafeVariables(string) {
+    if (UNNAMED_VARIABLE_REGEXP.test(string)) {


This could use || instead:

return UNNAMED_VARIABLE_REGEXP.test(string) || UNSAFE_VARIABLE_REGEXP.test(string);

If you feel the ifs read more easily, ignore me.

seanmonstar · 2016-10-19T18:27:49Z

app/scripts/lib/strings.js

+  const NAMED_VARIABLE_REGEXP = /\%\(([a-zA-Z]+)\)s/g;
+  const UNNAMED_VARIABLE_REGEXP = /%s/g;
+  // safe variables have an `escaped` prefix.
+  const UNSAFE_VARIABLE_REGEXP = /\%\(((?!escaped)[a-zA-Z]+)\)s/g;


We"ve been moving away from a postfix-Hungarian notation for RegExps in the auth server. I don"t know if that"s desirable here as well.

shane-tomlinson · 2016-10-21T13:08:04Z

@seanmonstar - updated!

seanmonstar

All tidy! Looks good from here, except that Travis went red. It might be because of the DNS outage, so I just gave it a kick. Merge when travis is green!

A new Mustache helper function `unsafeTranslate`, has been added for unsafe writes, to be used when inserting HTML. All interpolation variables passed to `unsafeTranslate` must be prefixed with `escaped` or else an exception is thrown. This is to remind the developer to escape the variable in the most appropriate way. The `t` helper function now HTML escapes by default. Rename some function/variable names for consistency: * `displaySuccessUnsafe` => `unsafeDisplaySuccess` * `displayErrorUnsafe` => `unsafeDisplayError` * `successUnsafe` => `unsafeSuccess`

rfk · 2016-11-08T05:09:39Z

@shane-tomlinson does this fix the issue reported by cure53?

shane-tomlinson · 2016-11-08T12:07:05Z

@rfk - yes

shane-tomlinson added WIP waffle:active labels Oct 18, 2016

shane-tomlinson commented Oct 18, 2016

View reviewed changes

shane-tomlinson self-assigned this Oct 18, 2016

shane-tomlinson force-pushed the shane-tomlinson/unsafe-html-in-templates branch 3 times, most recently from be82ecd to 9a6ad53 Compare October 19, 2016 10:58

shane-tomlinson commented Oct 19, 2016

View reviewed changes

shane-tomlinson force-pushed the shane-tomlinson/unsafe-html-in-templates branch 2 times, most recently from 3a94829 to 3e085e5 Compare October 19, 2016 11:53

shane-tomlinson removed the WIP label Oct 19, 2016

shane-tomlinson changed the title ~~WIP: fix(client): All template writes are by default HTML escaped.~~ fix(client): All template writes are by default HTML escaped. Oct 19, 2016

shane-tomlinson added waffle:review and removed waffle:active labels Oct 19, 2016

shane-tomlinson removed their assignment Oct 19, 2016

seanmonstar reviewed Oct 19, 2016

View reviewed changes

shane-tomlinson force-pushed the shane-tomlinson/unsafe-html-in-templates branch from 3e085e5 to 8aaa194 Compare October 21, 2016 08:59

shane-tomlinson force-pushed the shane-tomlinson/unsafe-html-in-templates branch from 8aaa194 to 47b1317 Compare October 21, 2016 13:11

shane-tomlinson assigned seanmonstar Oct 21, 2016

seanmonstar approved these changes Oct 21, 2016

View reviewed changes

shane-tomlinson force-pushed the shane-tomlinson/unsafe-html-in-templates branch from 47b1317 to fd6a8d0 Compare October 24, 2016 11:46

shane-tomlinson force-pushed the shane-tomlinson/unsafe-html-in-templates branch from fd6a8d0 to 06d994f Compare October 25, 2016 12:00

shane-tomlinson merged commit 4329101 into master Oct 25, 2016

shane-tomlinson deleted the shane-tomlinson/unsafe-html-in-templates branch October 25, 2016 14:57

shane-tomlinson removed the waffle:review label Oct 25, 2016

shane-tomlinson mentioned this pull request Oct 26, 2016

"open gmail" link on confirm_reset_password opens /confirm_reset_password #4327

Closed

rfk added this to the FxA-0: quality milestone Nov 8, 2016

shane-tomlinson mentioned this pull request Nov 18, 2016

Unescaped HTML entity on French login page #4427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(client): All template writes are by default HTML escaped. #4296

fix(client): All template writes are by default HTML escaped. #4296

shane-tomlinson commented Oct 18, 2016

shane-tomlinson Oct 18, 2016

shane-tomlinson Oct 19, 2016

shane-tomlinson Oct 19, 2016

shane-tomlinson Oct 19, 2016

shane-tomlinson Oct 19, 2016

shane-tomlinson Oct 19, 2016

shane-tomlinson Oct 19, 2016

shane-tomlinson commented Oct 19, 2016

seanmonstar left a comment

seanmonstar Oct 19, 2016

shane-tomlinson Oct 21, 2016

seanmonstar Oct 19, 2016

seanmonstar Oct 19, 2016 •

edited

Loading

seanmonstar Oct 19, 2016

shane-tomlinson commented Oct 21, 2016

seanmonstar left a comment

rfk commented Nov 8, 2016

shane-tomlinson commented Nov 8, 2016 •

edited

Loading

fix(client): All template writes are by default HTML escaped. #4296

fix(client): All template writes are by default HTML escaped. #4296

Conversation

shane-tomlinson commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shane-tomlinson commented Oct 19, 2016

seanmonstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmonstar Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shane-tomlinson commented Oct 21, 2016

seanmonstar left a comment

Choose a reason for hiding this comment

rfk commented Nov 8, 2016

shane-tomlinson commented Nov 8, 2016 • edited Loading

seanmonstar Oct 19, 2016 •

edited

Loading

shane-tomlinson commented Nov 8, 2016 •

edited

Loading