Fomantic-ui: [Content Security Policy] Make the framework CSP-compliant

[lubber-de]

Feature Request

Description

Basically it seems, to make the framework CSP compliant, all the places using inline styles by $().attr('style','something') should be replaced by $().css()

Original SUI Issue

https://github.com/Semantic-Org/Semantic-UI/issues/5630

Original SUI PR

https://github.com/Semantic-Org/Semantic-UI/pull/5660

The original PR only covered transition, but .attr('style') is also used in accordion, shape, dropdown, modal

[dylmye]

I can pick this up! Is there anything else required beyond this .css change? Is there a CLA I need to sign? Thanks

[hammy2899]

@dylmye As far as I am aware the CSS change is the only thing @lubber-de should be able to give more input on this because he has been doing a lot of security changes recently.

As for contributing all you need to do is read the following CONTRIBUTION guide and our CODE OF CONDUCT, no CLA 😎

[dylmye]

@hammy2899 brilliant! I've joined the Discord now so I can keep this more on topic. I'll see what I can do when I have time. 🎉

[dylmye]

Hello! Sorry it took so long to get on this, I've been super busy.

It appears that the only remaining places that we're using the $().attr('style', [...]) syntax is to unset the attribute before calling $().removeAttr('style') or to check that the attribute is set, eg https://github.com/fomantic/Fomantic-UI/blob/master/dist/components/accordion.js#L353-L377

Is there any reason why we still need to unset the attribute before calling removeAttr, was this an ie8 fix?

Thanks

---

edit 1: with the exception of one line in transition where we're using overrideStyle - I'll make sure to replace this, but this is blocked by the fact that .css ignores !important

[lubber-de]

@dylmye Deeply test the transition module, it temporary relies on inline block display statements using "!important" to make sure it gets overridden. It also tries to determine if there might be inline styles and works with them if given. Also to make sure to correctly reset the content after the transition.

Regarding the .attr('style','')...don't really know why it was implemented (in 2014, the commit log does not reveal any explaining informations unfortunately). We have to test if simply removing it still works in current browsers. shape, accordion, transition and modal are affected

[birthdaycorp]

I love the work you all have done on this! But I unfortunately can't use it as you don't allow theming. It's unfortunate that the Semantic UI crowd still hasn't accepted the PR for CSP compliance. 😓The transition.js included in that PR remains unchanged two years later.😒

If changing the attr() to css() is all it takes to update the framework for CSP compliance, exactly which folder do I make the changes in? dist or src? I'm running Semantic UI from the assets folder in my Rails app, and neither seems to have had an impact on the modal.js use of inline styling.

[lubber-de]

exactly which folder do I make the changes in? dist or src?

If you want to contribute to Fomantic and provide a PR, pleasse always use the src folder. The dist folder is generated on build autmatically

[lubber-de]

🤔 Well, to be honest: After looking at the original SUI issue again, i really don't understand why the change from attr('style','...') to .css({}) makes any difference in CSP. Could somebody explain? I really doubt the original PR really makes that code csp compliant.

Both variants result in the exact same node change, both add the inline style attribute.
See https://jsfiddle.net/lubber/v3ey2w9x/3/

Both methods will add style='border:1px solid black' to the nodes

For my (now) understanding to be CSP compliant, absolutely no inline styles should be used, which will be difficult, especially for the transition module, which uses that technology to dynamically override the display-style (which is not necessarily block all the time

[birthdaycorp]

Yeah that was my thought exactly. I thought I was crazy! .css() adds inline styling too.

[lubber-de]

But I unfortunately can't use it as you don't allow theming.

Please explain: You can theme fomantic via LESS just like you can with SUI...What am i missing that makes you think we dont allow theming?

[birthdaycorp]

But I unfortunately can't use it as you don't allow theming.

Please explain: You can theme fomantic via LESS just like you can with SUI...What am i missing that makes you think we dont allow theming?

I was mistaken! I was looking at an issue in the Fomantic-UI-SASS gem repo. _That_ doesn't allow theming. This does!

[lubber-de]

Yes, the SASS repo is precompiled for Ruby and not maintained by the core team of fomantic. We only offered to host that repo under the fomantic organization.
Please create an issue at https://github.com/fomantic/Fomantic-UI-SASS to convince the developer to convert the LESS of fomantic into equivalent SASS to make that repo name more "worth" 😉

[fernandops26]

Hi guys, it's been a long time since I left the PR for review, however my approach does not aim to prevent the style from being added in a linear way, my PR focuses on avoiding using the .attr() function because it allows adding any attribute that is NOT IT'S STYLES, that's why CSP is complaining.

[lubber-de]

@fernandops26 Thank you for the clarification 👍 , i really wasn't aware of this.
Here is a codepen (jsfiddle does not allow meta tags) to reproduce the issue

https://codepen.io/lubber/pen/Yzqxxmp

Setting the styles via .attr() (first line) violates the CSP rule, while .css() (second line) still works.

However, as already mentioned in the issue description: .attr() is used in many more components, not just the transition module. So a proper fix should cover all modules.

In case codenpen does not work, here's a short source-snippet to reproduce

<html>
<head>
  <meta http-equiv="Content-Security-Policy" content="style-src 'self'">
</head>
<body>
<div id="test1">
  I am a test 1
</div>
<div id="test2">
  I am a test
</div>
</body>
<script src="assets/library/jquery341.js"></script>
<script>
//fails
  $('#test1').attr('style','border:1px solid black;');
// still works
  $('#test2').css({border:'1px solid black'});
</script>

</html>