Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binding style attributes warning #11395

Closed
zyllorion opened this issue Jun 10, 2015 · 9 comments
Closed

Binding style attributes warning #11395

zyllorion opened this issue Jun 10, 2015 · 9 comments
Labels

Comments

@zyllorion
Copy link

WARNING: Binding style attributes may introduce cross-site scripting vulnerabilities; please ensure that values being bound are properly escaped. For more information, including how to disable this warning, see http://emberjs.com/deprecations/v1.x/#toc_binding-style-attributes.

Unfortunately there doesn't seem to be a way to disable the warning on the link given.

Code for escapeCSS is not given so I cannot use this and should not need to as it is just a calculated RGB value.

I do return a ().htmlSafe()

Thanks.

@rwjblue
Copy link
Member

rwjblue commented Jun 10, 2015

Can you please provide a demo JSBin so we can help you a bit better here?

@bantic
Copy link
Member

bantic commented Jun 16, 2015

Here's a demo jsbin showing this behavior. Using a computed property inside a style string generates this warning even if the value of the CP is a safe string.

Ember.set('color', Ember.String.htmlSafe('blue');
<div style="color: {{color}};"></div> // generates console warning

Ember.set('myStyle', Ember.String.htmlSafe("color: blue;"));
<div style={{myStyle}}></div> // no console warning
<div style="{{myStyle}}"></div> // does show console warning

@rwjblue
Copy link
Member

rwjblue commented Jun 16, 2015

Yes, @bantic is correct. You can return a SafeString from a CP and use that unquoted in the template to avoid a warning, but if you quote the property it will trigger this warning. We would like to avoid this warning when concatenating all safe strings, but that will have to occur in a future version....

@mixonic
Copy link
Member

mixonic commented Mar 27, 2016

Specifically, we would accept a patch that causes <div style="{{mySafeStyle}}"></div> to be treated as "safe". However <div style="color: {{mySafeColor}};"></div> should remain unsafe. It can probably get a better error message though.

@luxzeitlos
Copy link
Contributor

@mixonic why would <div style="color: {{mySafeColor}}; width:{{mySafeWidth}};"></div> be unsafe? Seems like an unnecessary limitation to me.

@mixonic
Copy link
Member

mixonic commented Sep 14, 2016

@luxferresum to know that mySafeColor is a color and that color is escaped/validated a certain way means we must parse CSS. Just like to know <a href={{foo}}> doesn't contain an XSS attack Ember parses HTML according to the spec and ensures the contents of foo are escaped properly. After parsing we need analysis for what properties (like background-url) have vulnerabilities and what they are so we can provide test cases.

It is just a big problem, and needs a champion to own it and execute.

@luxzeitlos
Copy link
Contributor

@mixonic Well that would be awesome, but its only true if mySafeColor is not a safe string.
I think if {{mySafeColor}} is the result of Ember.String.htmlSafe() there should be no warning at all. Same with {{{mySafeColor}}}.

@Serabe
Copy link
Member

Serabe commented Sep 14, 2016

Just think that, in general, the concatenation of two safe strings does not need to be a safe string. This currently happens in code too:

2016-09-14 at 21 27

If you were to move that to a property, you would need to mark the final string as safe, even if you don't think it is needed.

I think Ember should fall on the safe side and the only case where a safe string can be guaranteed to still be a safe string is in the <div style="{{foo}}"/> case.

@luxzeitlos
Copy link
Contributor

It's an unnecessary warning, it works, and people like me will just go with the warning if there is no easy way around. Building the entire style property in JS is not really a fancy solution.

Also isn't that what @rwjblue said?

We would like to avoid this warning when concatenating all safe strings, but that will have to occur in a future version....

That two safe strings are a normal string is normal and a JS limitation. There is no way to overload +. A safe string is always safe. By creating a safe string you basically say "I know this is safe, believe me" to your framework. If you can't say this to your framework its highly annoying.

Serabe added a commit to Serabe/ember.js that referenced this issue Sep 14, 2016
Given the following template:

```hbs
<div style="{{myStyle}}"></div>
```

It behaves exactly like:

```hbs
<div style={{myStyle}}></div>
```

Fix emberjs#11395
@Serabe Serabe added Has PR and removed Help Wanted labels Sep 14, 2016
Serabe added a commit to Serabe/ember.js that referenced this issue Sep 15, 2016
Given the following template:

```hbs
<div style="{{myStyle}}"></div>
```

It behaves exactly like:

```hbs
<div style={{myStyle}}></div>
```

Fix emberjs#11395
Serabe added a commit to Serabe/ember.js that referenced this issue Oct 10, 2016
Given the following template:

```hbs
<div style="{{myStyle}}"></div>
```

It behaves exactly like:

```hbs
<div style={{myStyle}}></div>
```

Fix emberjs#11395
stefanpenner pushed a commit to chadhietala/ember.js that referenced this issue Nov 4, 2016
Given the following template:

```hbs
<div style="{{myStyle}}"></div>
```

It behaves exactly like:

```hbs
<div style={{myStyle}}></div>
```

Fix emberjs#11395
rwjblue pushed a commit that referenced this issue Nov 7, 2016
Given the following template:

```hbs
<div style="{{myStyle}}"></div>
```

It behaves exactly like:

```hbs
<div style={{myStyle}}></div>
```

Fix #11395

(cherry picked from commit fd20634)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants