search

cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.08k stars 399 forks source link

don't crash js on failed injection #1556

Closed firstdorsal closed 3 years ago

firstdorsal commented 3 years ago

currently jss is crashing in an react material-ui csp env this fixes the crash

Corresponding Issue(s):

What Would You Like to Add/Fix?

Todo

Expectations on Changes

Changelog

kof commented 3 years ago

Thanks, can you add a test?

firstdorsal commented 3 years ago

hi, thank you for your quick response. i would love to but have no idea how. i'm just using your project as a dependency for react material-ui and dont even know what it is actually about xD currently your script is creating a js object access error that breaks further js/react execution on trying to inject css in a site with a strict content security policy. this breaking error is fixed by adding the null propagation operator.

kof commented 3 years ago

Can you reproduce ig on codesandbox?

On Sun, Sep 19, 2021, 15:26 Paul Hennig @.***> wrote:

hi, thank you for your quick response. i would love to but have no idea how. i'm just using your project as a dependency for react material-ui and dont even know what it is actually about xD currently your script is creating a js object access error that breaks further js/react execution on trying to inject css in a site with a strict content security policy. this breaking error is fixed by adding the null propagation operator.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/cssinjs/jss/pull/1556#issuecomment-922473669, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAM4WGATPPOYBOHXL5P5PTUCXQHHANCNFSM5EJGLD4Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

firstdorsal commented 3 years ago

nope way to complicated to do it in sandbox.

i just tried to create a docker image for you but it is still not easy to recreate because of the moving parts.

this is the exact error that pops up: image

kof commented 3 years ago

First step towards this getting fixed is to reproduce it in a clean setup, without business logic. Without a test we can not merge this since we can't guarantee its not going to break some other way unless we have a use case encoded into a test.

kof commented 3 years ago

afaik cssRules can be only null if the sheet was not attached to the dom, which needs to be fixed in a different place, this fix only works around the actual problem and might actually lead to some other problem in the same case, so unless we have a clear test, I will have to close this PR