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

Issues with Content Security Policy #166

Closed
kailashrdave opened this issue Nov 13, 2019 · 13 comments · Fixed by #169
Closed

Issues with Content Security Policy #166

kailashrdave opened this issue Nov 13, 2019 · 13 comments · Fixed by #169

Comments

@kailashrdave
Copy link

We are using CASL for our project to check permissions.
recently we encountered that there are some issues with CSP in our application.
Going further, we found that it is because of sift.js which has something like following in javascsript...

return"string"==typeof e?new Function("obj","return "+e)

CASL uses "sift" for Mongo DB like filters in javascript.
I tried to go through CASL documentation, but found nothing related to CSP or Security.
Has anybody got this issue before? Is it possible to fix this without adding "unsafe-eval"

following is the code snippet from stif.js which causes problem

stiff-js-error

@stalniy
Copy link
Contributor

stalniy commented Nov 18, 2019

@crcn Any thoughts?

@kailashrdave
Copy link
Author

@crcn , @stalniy , could you please help to fix this issue.

@stalniy
Copy link
Contributor

stalniy commented Dec 5, 2019

@crcn the simplest way to support this from your side is to provide CSP compliant version as a separate bundle e.g. sift.csp.min.js

Update: This version of bundle won't support new Function in $where operator

@stalniy
Copy link
Contributor

stalniy commented Dec 5, 2019

I can send a PR for this, otherwise I would be forced to publish a forked version...

@crcn
Copy link
Owner

crcn commented Dec 5, 2019

the simplest way to support this from your side is to provide CSP compliant version as a separate bundle e.g. sift.csp.min.js

great idea 👍. If you want to submit a PR I'd be happy to merge it in.

@crcn
Copy link
Owner

crcn commented Dec 5, 2019

However, what are your thoughts around removing the eval code? That would obviously require a major version bump.

@stalniy
Copy link
Contributor

stalniy commented Dec 5, 2019

You don’t need a major version bump. You will have just an additional file. So eventually there will be 2 files: sift.js and sift.csp.js

People will be able to use whatever file fits their needs.

The plan is to add if statement with process.env.CSP_ENABLED. Using webpack or rollup (roll up is better to library development, less overhead in the bundle) I can inject this property and create 2 different files:

  • one the same as now (sift.js) CSP_ENABLED = false
  • and csp compliant version CSP_ENABLED = true

Uglifier can optimize this if (i.e. remove it)

@stalniy
Copy link
Contributor

stalniy commented Dec 6, 2019

Are you ok to replace webpack with rollup? because I see that you started to integrate it :)

@stalniy
Copy link
Contributor

stalniy commented Dec 6, 2019

No worries I will use webpack :)

@stalniy
Copy link
Contributor

stalniy commented Dec 6, 2019

Done, now there is a new sift.csp.min.js file, which people can require using:

  • require('sift/sift.csp.min')
  • import sift from 'sift/sift.csp.min'

in order to use CSP compliant version, for the rest of the world everything remains the same, so this is a minor version update

@crcn crcn closed this as completed in #169 Dec 6, 2019
@crcn
Copy link
Owner

crcn commented Dec 6, 2019

Thanks @stalniy! Published to NPM as 9.0.1. Lmk if your issue is fixed @kailashrdave.

@stalniy
Copy link
Contributor

stalniy commented Dec 9, 2019

I missed one important thing... Webpack uses window as default global variable and for proper UMD build it should be this

@stalniy
Copy link
Contributor

stalniy commented Dec 9, 2019

please check #170

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants