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:28:43Z

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:43:42Z

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:43:42Z

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:43:42Z

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:43:42Z

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:43:42Z

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:43:42Z

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:29:30Z

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:29:30Z

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:29:30Z

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:29:30Z

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 reviewed Oct 18, 2016

View 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 Oct 18, 2016

shane-tomlinson reviewed Oct 19, 2016

View changes

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

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 changes

shane-tomlinson force-pushed the shane-tomlinson/unsafe-html-in-templates branch from 3e085e5 to 8aaa194 Oct 21, 2016

shane-tomlinson force-pushed the shane-tomlinson/unsafe-html-in-templates branch from 8aaa194 to 47b1317 Oct 21, 2016

shane-tomlinson assigned seanmonstar Oct 21, 2016

seanmonstar approved these changes Oct 21, 2016

View changes

shane-tomlinson force-pushed the shane-tomlinson/unsafe-html-in-templates branch from 47b1317 to fd6a8d0 Oct 24, 2016

shane-tomlinson force-pushed the shane-tomlinson/unsafe-html-in-templates branch from fd6a8d0 to 06d994f Oct 25, 2016

shane-tomlinson deleted the shane-tomlinson/unsafe-html-in-templates branch Oct 25, 2016

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

mozilla / fxa-content-server Archived

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shane-tomlinson commented Oct 19, 2016

seanmonstar left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shane-tomlinson commented Oct 21, 2016

seanmonstar left a comment

rfk commented Nov 8, 2016

shane-tomlinson commented Nov 8, 2016 •

edited

mozilla / fxa-content-server Archived

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

This comment has been minimized.

shane-tomlinson Oct 18, 2016 Author Member

This comment has been minimized.

shane-tomlinson Oct 19, 2016 Author Member

This comment has been minimized.

shane-tomlinson Oct 19, 2016 Author Member

This comment has been minimized.

shane-tomlinson Oct 19, 2016 Author Member

This comment has been minimized.

shane-tomlinson Oct 19, 2016 Author Member

This comment has been minimized.

shane-tomlinson Oct 19, 2016 Author Member

This comment has been minimized.

shane-tomlinson Oct 19, 2016 Author Member

shane-tomlinson commented Oct 19, 2016

seanmonstar left a comment

This comment has been minimized.

seanmonstar Oct 19, 2016 Member

This comment has been minimized.

shane-tomlinson Oct 21, 2016 Author Member

This comment has been minimized.

seanmonstar Oct 19, 2016 Member

This comment has been minimized.

seanmonstar Oct 19, 2016 • edited Member

This comment has been minimized.

seanmonstar Oct 19, 2016 Member

shane-tomlinson commented Oct 21, 2016

seanmonstar left a comment

rfk commented Nov 8, 2016

shane-tomlinson commented Nov 8, 2016 • edited

shane-tomlinson Oct 18, 2016
Author Member

shane-tomlinson Oct 19, 2016
Author Member

shane-tomlinson Oct 19, 2016
Author Member

shane-tomlinson Oct 19, 2016
Author Member

shane-tomlinson Oct 19, 2016
Author Member

shane-tomlinson Oct 19, 2016
Author Member

shane-tomlinson Oct 19, 2016
Author Member

seanmonstar Oct 19, 2016
Member

shane-tomlinson Oct 21, 2016
Author Member

seanmonstar Oct 19, 2016
Member

seanmonstar Oct 19, 2016 •

edited

Member

seanmonstar Oct 19, 2016
Member

shane-tomlinson commented Nov 8, 2016 •

edited