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

fix: CSP Security hole in tryConvertExpr (new Function(...)) #139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

setvik
Copy link

@setvik setvik commented Oct 5, 2023

Addresses the CSP security hole identified in the issue linked below and allow ClayGL to be used on sites with strict Content Security Policies:

Please make claygl CSP compatible when "script-src unsafe-eval" is used.

Copy link

@Jas2042 Jas2042 left a comment

Choose a reason for hiding this comment

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

Great suff....
My main concern is:

  • Performance: Did you benchmark against old unsafe code?
  • Large extra dependencies

Really need feedback from devs / owners on how important these are to evaluate the acceptability of the solutions

catch (e) {
throw new Error('Invalid expression.');
}

Copy link

Choose a reason for hiding this comment

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

return func; ?

};

// Try run t
try {
Copy link

Choose a reason for hiding this comment

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

Remove debug code


const func = function(width, height, dpr) {
const context = { width, height, dpr };
const mexp = new Mexp()
Copy link

Choose a reason for hiding this comment

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

If the expressions are as simple as the bug implies then rather than ast and all the overhead of generating it perhaps simple str substitution and split could do the job.
This would remove the need for esprima and the astToMathExpression() function

This could be better but something like:

expression = expression.replaceAll('width", width).replaceAll('height", height).replaceAll('dpr", dpr).replaceAll("[", "").replaceAll("]", "");

expressions = expression.split(",")

if (expressions.length === 1) {
    return mexp.eval(expressions[0]);
}

return [ mexp.eval(expressions[0]), mexp.eval(expressions[1]) ];

Probably needs some exception handling as well


const func = function(width, height, dpr) {
const context = { width, height, dpr };
const mexp = new Mexp()
Copy link

Choose a reason for hiding this comment

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

Can the const mexp value move out of the func scope so in instantiated for each call?

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 this pull request may close these issues.

2 participants