cjss-group / CJSS

A CSS based web framework
https://cjss.js.org/
MIT License
670 stars 20 forks source link

eval is evil :( #7

Closed KargJonas closed 5 years ago

scottkellum commented 5 years ago

Is there any other option to evaluate strings as JS from CSS?

xlf1024 commented 5 years ago

Probably e.g. using the function constructor. However, afaik, eval is considered evil because it executes strings as js, thus creating an additional way for malicious code to enter your site, so any other way of doing the same would't be better, just less readable. However, for --data one could probably switch to JSON.parse, which does what you promise for --data without enabling code injection.

KargJonas commented 5 years ago

Well if you don't plan on using computed properties, I'd use JSON.parse for the data. JSON.parse gives you nice errors if you forget to close a string or stuff like that.

c-harding commented 5 years ago

Using the function constructor also allows you to specify this, rather than needing to perform a regex substitution.

c-harding commented 5 years ago

I've mostly fixed this in KargJonas:#1, but I left --data as a globally evaluated JavaScript string.

How much of a breaking change would it be to downgrade data to just a plain old JSON object, given that the documentation does specify that it should be formatted as JSON? It's worth noting that the example is invalid JSON, as it uses single quotes.

c-harding commented 5 years ago

Not sure what the best way of ordering the pull requests is. My pull request to fix this (KargJonas:#1) builds upon #17, and also includes #18 (my fix for #15). If I create a pull request for my branch xsanda:remove-eval here, merging it will also indirectly merge #17 and #18.

KargJonas commented 5 years ago

Honestly I've lost track of whats going on at this point.. I'll trust you to do the right thing. :sweat_smile:

KargJonas commented 5 years ago

How much of a breaking change would it be to downgrade data to just a plain old JSON object, given that the documentation does specify that it should be formatted as JSON?

That's the right thing to do. We should implement --computed for stuff that has to be evaluated..

scottkellum commented 5 years ago

How much of a breaking change would it be to downgrade data to just a plain old JSON object

Y’all are being too serious here. This project should be treated like alpha software at the moment. Yes, it’s published, but with a warning that people shouldn’t use it. It’s also a very new technique and my JS skills are not particularly refined so this project requires plenty of polish.

c-harding commented 5 years ago

Ok. I wonder if we should change data to use native stored data for future use too?

KargJonas commented 5 years ago

So i was able to replace the "eval" for --js. #21

scottkellum commented 5 years ago

Closed with #21 . Thank you @KargJonas 🙏