Ember.js: WARNING: Binding style attributes may introduce cross-site scripting vulnerabilities

[MarkMurphy]

Ember version 1.11

This warning does not go away after binding a style attribute with a safe string as indicated here: http://emberjs.com/deprecations/v1.x/#toc_warning-when-binding-style-attributes

[rwjblue]

Likely a duplicate of https://github.com/emberjs/ember.js/issues/10783 and https://github.com/emberjs/ember.js/issues/10794.

It does indeed appear to work properly here: http://emberjs.jsbin.com/hemaga/1/edit?html,js,output

Can you please tweak/update that demo to show the bug?

[MarkMurphy]

@rwjblue

This reproduces the bug: http://emberjs.jsbin.com/lenabuvuki/2

[MarkMurphy]

@rwjblue Might be a duplicate of #10794 but why is that closed? Having to unwrap quotes from style bindings is not what I would consider an ideal solution. I don't want my models to be responsible for adding the style attribute to the value I want applied. For one, it's not very elegant in terms of separating presentational from functional and secondly it's sort of an anti-pattern in terms of keeping code following the D.R.Y. paradigm. For example, if I wanted to use the attribute "color" for several different inline style properties, I would have to write a method for each unique combination of properties that I want to apply as opposed to just doing something like this:

<div style="background-color: {{color}}; font-color: {{color}}; border-color: {{color}}"></div>

[mlenderink]

@rwjblue @MarkMurphy I've got the same problem. A helper function creates a random number which is used to randomly determine the top of an element.

<div style="top: {{unbound random-number 'rem' from='-3.5' to='-3.0'}}"></div>

[anlumo]

I just worked on this, and I was able to quench a few of those warnings by doing <div style={{style}}> instead of <div style="{{style}}">. Apparently Ember doesn't detect htmlSafe strings when they're inside quotes of an attribute (<div style="margin: {{margin}}px"> didn't work either).

[MarkMurphy]

@anlumo we know but that's not what we want. See my previous comment.

[jesenko]

I have same use case as @MarkMurphy. If concatenation of styles in templates will not be supported in the upcoming versions of Ember, I will probably just move that to computed property and then use it as style={{style}}.

Maybe someone from core team (@rwjblue?) can shed some light on whether style concatenation in templates will be supported?

I think that

<div style="background-color: {{color}}; font-color: {{color}}; border-color: {{color}}"></div>

should report warnings, while escaped version

<div style="background-color: {{{color}}}; font-color: {{{color}}}; border-color: {{{color}}}"></div>

should not. concat helper would probably have to track whether all concatenated strings are escaped and then mark it as safe.

[mixonic]

we should be respecting the safe strings, absolutely.

Maybe someone from core team (@rwjblue?) can shed some light on whether style concatenation in templates will be supported?

Of course. It should work with safe strings today, it is a bug that it does not. Hence this issue.

In the longer term we would also like to support safe {{ usage, but it requires a bunch of research into the security implications.

[jesenko]

@mixonic Thanks for clarification, I was not sure on the status of this one as two related issues were already closed...

[mixonic]

@jesenko if you can do any research to clarify the relationship between the tickets it would definitely be useful. I'm not familiar with the other tickets, but updating the repro JSBin to canary still causes the issue and that is good enough.

[MarkMurphy]

@mixonic @rwjblue Probably a good time to update the label on this one.

[MarkMurphy]

@rwjblue in #10794 you mentioned:

Using quoted value results in a pass through concat helper method, which makes the final value not a SafeString.

I believe that happens here: https://github.com/emberjs/ember.js/blob/680f997ed0958c420abdcd0b1673111aee26afe7/packages/ember-metal/lib/streams/utils.js#L158-L195

If that is in fact the case, do you know what needs to happen to fix this? Maybe I can help, just need direction.

[MarkMurphy]

ok so I did some digging and now understand what the problem is.

The issue is in fact to do with the concat helper found here.

I added the following code to the concat method just before return array.join(separator);:

var map = array.map(function (element) {
  return '{ value: "' + element + '", safe: ' + (element instanceof Ember.Handlebars.SafeString) + ' }';
}).join(", ");

function isSafeString(element) {
  return (element instanceof Ember.Handlebars.SafeString);
}

console.log('some safe:', array.some(isSafeString), ', concat values: [' + map + ']');
console.log('all safe:', array.every(isSafeString), ', concat values: [' + map + ']');

With the following template and model:

<span class="project-color" style="background-color: {{project.safeColor}};"></span>

`import DS from 'ember-data'`

Project = DS.Model.extend({
  # Attributes
  color: DS.attr('string')
  safeColor: (function(){
    return new Ember.Handlebars.SafeString(this.get('color'))
  }).property('color')
})

# Fixture Data
Project.reopenClass({
  FIXTURES: [
    {
      id: 1,
      color: '#2196f3'
    },
    {
      id: 2,
      color: '#864ca3'
    }
  ]
})

`export default Project`

I get the following web console output:

some safe: true , concat values: [{ value: "background-color: ", safe: false }, { value: "#864ca3", safe: true }, { value: ";", safe: false }]
all safe: false , concat values: [{ value: "background-color: ", safe: false }, { value: "#864ca3", safe: true }, { value: ";", safe: false }]

So I guess we can't just make concat return a new SafeString because how do we know background-color: and ; should be safe?

UPDATE

concat would need to know that background-color: and ; were not variables and can be assumed safe (please correct me if I'm wrong is assuming that).

So what happens before concat is called? It seems like whatever parses the DOM element's attributes and then calls concat is what we need to look at.

[MarkMurphy]

@rwjblue what about having the ember-template-compiler make attribute strings html safe? in other words why not add a buildSafeString to https://github.com/tildeio/htmlbars/blob/master/packages/htmlbars-syntax/lib/builders.js

This is what ember-template-compiler is doing now:

function prepareConcatPart(node) {
      switch (node.type) {
        case 'TextNode': return builders.string(node.chars);
        case 'MustacheStatement': return unwrapMustache(node);
        default:
          throw new Error("Unsupported node in quoted attribute value: " + node.type);
      }
    }

I'm saying:

function prepareConcatPart(node) {
      switch (node.type) {
        case 'TextNode': return builders.safeString(node.chars);
        case 'MustacheStatement': return unwrapMustache(node);
        default:
          throw new Error("Unsupported node in quoted attribute value: " + node.type);
      }
    }

Notice the builder.string became builders.safeString

[GendelfLugansk]

Hello. I have the same warning, but I didn't bind style attribute.
My template:

<form {{action 'register' on='submit'}}>
  <div class="row">
    <div class="col l3 hide-on-med-and-down">&nbsp;</div>
    <div class="col s12 m12 l6">
      Here is my registration form
      <div class="row">
        <div class="col s12">{{#if loading}}{{md-loader}}{{/if}}</div>
      </div>
    </div>
  </div>
</form>

Controller:

export default Ember.Controller.extend({
  loading: false,
  actions: {
    register: function () {
      this.set('loading', true); //this line produces warning
      Ember.$.post(url, data).then(function () {
        //doing something and redirect
      }, (response) => { /*do smth about errors*/ }).always(() => { this.set('loading', false); });
    }
  }
});

When I set loading to true in my controller, I get warning "WARNING: Binding style attributes may introduce cross-site scripting vulnerabilities"

[AnvarPK]

i have a same warning , but i solved it with model, its working fine without warnings

model

import DS from 'ember-data';

export default DS.Model.extend({
proColor: DS.attr(),
bgColor: Ember.computed(function () {
var color = this.get('proColor');
return new Ember.Handlebars.SafeString("background-color: " + color);
}).property('proColor'),

});

template

< div style={{model.bgColor}} >

[sheriffderek]

For anyone with running across this warning: here's an example of how to deal with it. The documentation alludes to some escapeCSS() method, but I think that's just pseudo code - because I can't find that anywhere. http://emberjs.com/deprecations/v1.x/#toc_binding-style-attributes

'image' model

...
export default PostModel.extend({
  better_featured_image: DS.attr(),
  safeSrc: Ember.computed('better_featured_image', function() {
    return new Ember.String.htmlSafe( "background-image: url(" + this.get('better_featured_image').source_url + ")" );
  })
});

'element-slider' component template

{{#each images as |image|}} {{!-- model fed into component in route --}}
  <li class='image-w slide' style={{image.safeSrc}}>
    {{!-- ... --}}
  </li>
{{/each}}

[localpcguy]

Noticed today that while there is a warning for binding to a style attribute, there is no warning when adding dynamic bound content in a style block (i.e. between style tags). Seems like the same types of vulnerabilities could be generated in both places.

<style> 
  .classname { 
    width: {{boundProp}}px; 
  }
  {{boundStyleString}} 
</style>

[allthesignals]

The anchor for this warning seems broken: https://emberjs.com/deprecations/v1.x/#toc_binding-style-attributes - can't find it on the page

[pixelhandler]

@AnvarPK @GendelfLugansk @MarkMurphy @acorncom @allthesignals @anlumo @jesenko @localpcguy @mixonic @mlenderink @rwjblue @sheriffderek is this still an issue, perhaps we should close or create a new reproduction of this, what do you think?

[webark]

at the very least the anchor for the warning should be fixed. It would be nice if the issue was fixed, but it's just a warning, so it can be ignored.

[sheriffderek]

@AnvarPK @GendelfLugansk @MarkMurphy @acorncom @allthesignals @anlumo @jesenko @localpcguy @mixonic @mlenderink @rwjblue @sheriffderek is this still an issue, perhaps we should close or create a new reproduction of this, what do you think?

I haven't had any issues _with this_ in a long time / use htmlSafe + don't use quotes / - but my fear was that other people would have a hard time tracking it down. I did. (actually - _I still do_ - since trying to find links to reference here wasn't easy)

If there was even one sentence on this page: https://guides.emberjs.com/release/templates/binding-element-attributes/ - that had (if not a full example) even a sentence with a link to info Ember.String.htmlSafe https://www.emberjs.com/api/ember/2.15/functions/Ember.String/htmlSafe (preferably a 3.4 / the most up to date mention of it - couldn't find one) - then we could cut down on the stack overflow questions regarding this.

This bit is nice - https://guides.emberjs.com/release/templates/writing-helpers / It's helper specific - However - It shows the latest import style. : )

_We_ can ignore it - but for the person trying out Ember / this could be a frustration - and those add up quickly. _If_ there is a warning, it should lead you to documentation on how to address the thing it's warning you about.

[pixelhandler]

@emberjs/learning-team-managers perhps worth documenting ? do we have a section on security?

[locks]

The link is now working properly!