HTML-safe aware String Escaping

[sandstrom]

Background

Ember used to have a function escapeExpression (still in the code-base, but not public) living under String.escapeExpression.

During the move to module imports it wasn't ported (deliberately). However, htmlSafe and isHtmlSafe where ported.

Problem

If anyone wants to implement escapeExpression themselves, now that it isn't publicly available, they'll have to rely on private APIs of SafeString#toHTML (unless I've missed something obvious).

An implementation would look something like this:

import { isHTMLSafe } from '@ember/template';

const ESCAPE = {
  '&': '&',
  '<': '&lt;',
  '>': '&gt;',
  '"': '&quot;',
  "'": '&#x27;',
  '`': '&#x60;',
  '=': '&#x3D;',
};

const BAD_CHARS = /[&<>"'`=]/g;
const POSSIBLE = /[&<>"'`=]/;

let escapeChar = function(chr) {
  return ESCAPE[chr];
};

// borrowed from Handlebars.utils.escapeexpression
// https://github.com/emberjs/ember.js/pull/18791
let escapeExpression = function(string) {
  if (typeof string !== 'string') {
    // don't escape SafeStrings, since they're already safe
    if (string && isHTMLSafe(string)) {
      return string.toHTML(); // private API, since SafeString isn't public
    } else if (string === null) {
      return '';
    } else if (!string) {
      return String(string);
    }

    // Force a string conversion as this will be done by the append regardless and
    // the regex test will do this transparently behind the scenes, causing issues if
    // an object's to string has escaped characters in it.
    string = String(string);
  }

  if (!POSSIBLE.test(string)) {
    return string;
  }

  return string.replace(BAD_CHARS, escapeChar);
};

Questions

1. Have I missed something, or is this only possibly relying on the toHTML on SafeString, which is private?

2. Is this by design or by accident?

Possible solutions

• Make SafeString#toHTML public
• Make escapeExpression public.
• Let it slide, this probably isn't major issue for most people and can be handled in 'app land'.

---

I've also opened a PR that was closed. This is fine. But I'm opening this RFC issue since I'm not sure if it was closed knowing that private API must be relied on.

emberjs/ember.js#18791

[locks]

What are you trying to accomplish that you need escapeExpression?

[sandstrom]

@locks We use it in a bunch of helpers, before marking code as safe.

This isn't the best example, but just to give an idea:

import Ember from 'ember';

// Emit the value or a fallback value

const DEFAULT_FALLBACK = '--';
const LOW_EMPHASIS_CLASS = 'low-emphasis';

let escape = Ember.Handlebars.Utils.escapeExpression;

export default Ember.Helper.helper(function(args, options) {
  let value     = args[0];
  let fallback  = args[1] || DEFAULT_FALLBACK;

  if (Ember.isNone(value)) {
    if (options.deEmphasis) {
      return Ember.String.htmlSafe(`<span class='${LOW_EMPHASIS_CLASS}'>${escape(fallback)}</span>`);
    } else {
      return fallback;
    }
  } else {
    return value;
  }
});

[locks]

Why do you not continue to use Handlebars.Utils.escapeExpression?

[sandstrom]

@locks I'm fine doing that, this isn't a big issue for us, we only use this in a handful of places.

But since SafeString is an Ember-thing we'd rely on Handlebars.Utils.escapeExpression to track the API of SafeString and its toHTML method.

They are currently in sync, so it's fine, but it's kind of weird that a dependency need to "know about" the API of one of its consumers (the implementation of SafeString lives in Ember).

[cincodenada]

I would like to add my use case here: a very common helper is nl2br, converting newlines to line breaks so they are rendered. A naive implementation (which I am currently fixing in my app) looks something like this:

import { helper } from '@ember/component/helper'
import { htmlSafe } from '@ember/template'

export function nl2br (params) {
  const [text] = params
  return htmlSafe(text.replace(/(\r\n|\n|\r)/gm, '<br>'))
}

export default helper(nl2br)

Which allows me to use {{text_with_newlines | nl2br}}

However, because it requires marking the whole string as htmlSafe, this introduces a vulnerability - if the text with newlines is user-created, they can now inject whatever they want. So this is what I'm doing now:

import { helper } from '@ember/component/helper'
import { htmlSafe } from '@ember/template'
import Ember from 'ember'
const htmlEscape = Ember.Utils.Handlebars.escapeExpression

export function nl2br (params) {
  const [text] = params
  return htmlSafe(htmlEscape(text).replace(/(\r\n|\n|\r)/gm, '<br>'))
}

export default helper(nl2br)

Which closes the vulnerability.

We are on Ember 3.8, but looking to upgrade in the near future. It seems like this might be an upgrade concern, according to discussion in ember.js/#16817 - I have gathered from that and other threads that the "better" way to do this is importing all of Handlebars to get to the escapeExpression utility, or using some other escaping library. My intention was to use the same escaping that Ember does internally, but I'm confused by some discussion in that issue.

This post stated that Ember doesn't use escapeExpression internally any more, and this post expands on that to say Ember doesn't do any escaping internally any more.

What does this mean? There has to be some escaping going on somewhere, right? Regardless of whether Ember uses it directly or not, what is used to accomplish escaping/template rendering? Is it still Handlebars, or does Glimmer re-implement a handlebars-like syntax, or something else entirely?

[enspandi]

@cincodenada Maybe you want to have a look at dompurify; We're using it successfully in production https://github.com/cure53/DOMPurify/

[cincodenada]

@cincodenada Maybe you want to have a look at dompurify; We're using it successfully in production https://github.com/cure53/DOMPurify/

Thanks, for the suggestion! I'm aware of dompurify (and use it elsewhere in our app in fact), but here I specifically do not want to do sanitization. In general I discourage santization when escaping is feasible, because escaping will always be safer than sanitization, as it removes any possibility of HTML being rendered at all.

For most cases where you just want to protect yourself from XSS attacks being embedded in text, escaping is simpler and safer. This is a common confusion! For more information on the differences, there are some good posts that go into more detail: Injection Prevention: Sanitizing vs Escaping and Don't Sanitize, Do Escape. If you do need to render some HTML in Javascript I agree dompurify is your best bet, but in my opinion recommending sanitization when escaping fits the requirements makes the web a little bit less secure.

As to the original issue: if I did want an external library I can use Handlebars, or a standalone escaping package like stringify-entities, and that may be what I end up doing. My main goal here was to just use the same escaping code that Ember is, for consistency, and understand what Ember is using.

Also, I wanted to explain why having escaping available via Ember directly is useful, but I understand that if escaping is being delegated entirely to a sub-dependency, it makes sense to for end-users or Ember add-ons to just use that dependency or a similar one when they need to do manual escaping.

[wagenet]

I'm closing this due to inactivity. This doesn't mean that the idea presented here is invalid, but that, unfortunately, nobody has taken the effort to spearhead it and bring it to completion. Please feel free to advocate for it if you believe that this is still worth pursuing. Thanks!

[sandstrom]

In case someone finds this, I still think my arguments in the top comment makes sense.

However, we've gradually stopped using Ember.String.htmlSafe in our code. For helpers, we no longer have them return any HTML. Because of that, the nuances that I wanted to correct with this issue, are no longer a problem for us.

Since time is a scarce resource, I won't prioritize bringing this to completion.