Skip to content

Conversation

@stephanwlee
Copy link
Contributor

Setting the style manually generally is not recommended as it can
contain JavaScript invoking CSS.

Setting the style manually generally is not recommended as it can
contain JavaScript invoking CSS.
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this attack vector relevant in modern browsers? I looked into this
briefly a bit ago and the closest I could find was that if you can
insert arbitrary CSS then you can cause arbitrary GET requests via CSS
imports, but I didn’t see any way to actually invoke JS.

(Approval in any case, as this is nicer anyway.)

@stephanwlee
Copy link
Contributor Author

Is this attack vector relevant in modern browsers?

Not that I know of but polymer-resin (https://github.com/Polymer/polymer-resin) hates it. As usual, I won't say I understand why/how it decides to hate it but I was able to witness the hatred thru an experiment. It reads something like

Failed to sanitize attribute of <vz-line-chart2>: <vz-line-chart2 style="opacity: 0.3;">

@wchargin
Copy link
Contributor

Cool (I inferred as much from the branch name :-) ). If you do find a
proof-of-concept snippet for this vector, I’d love to see it.

@psybuzz
Copy link
Contributor

psybuzz commented Sep 18, 2019

Nice! I have a style question: do we prefer "data-" attributes instead of classnames?

@stephanwlee
Copy link
Contributor Author

do we prefer "data-" attributes instead of classnames

There is no policy but reflection to attribute is too darn easy for boolean ones. It is a lot more cumbersome to do something like https://www.npmjs.com/package/classnames. Also, not sure if this is relevant, but especially because styles are scoped per shadow DOM, such attributes do not tend to bite you in bad ways.

@stephanwlee stephanwlee merged commit ef51bc4 into tensorflow:master Sep 18, 2019
@stephanwlee stephanwlee deleted the resin branch September 18, 2019 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants