#Safety of Roll.safeEval
gaunt pewter
Not sure if this really makes it safe, there might be more escape hatches. But it's better than the current state, from my understanding.
Safety of Roll.safeEval
Also, is there a reason (other than the use of with for this piece of code), that foundry is not using strict mode in general? That seems like a... risky thing to me...
mossy pelican
It's still possible to escape such a sandbox:
Roll.safeEval('(() => {ceil.constructor("console.log(\\"foo\\")")(); return 0;})()');
You could maybe use a Proxy to make a more bulletproof sandbox, though I'm not sure if it's possible to make a truly bulletproof one. What would be the motivation though? Protecting users from pasting strange formulae into AE fields?
Also, is there a reason (other than the use of with for this piece of code), that foundry is not using strict mode in general? That seems like a... risky thing to me...
What do you mean by 'using strict mode in general'? Do you have an example of a place it's not being used where it should be?
gaunt pewter
You could maybe use a `Proxy` to make a more bulletproof sandbox, though I'm not...
Yes, that’s the motivation. Because right now, this effectively allows users to perform arbitrary code execution in all other connected users‘ clients. Not an issue if you trust your players, but not everybody does (fully), and it’s just a bit dangerous
It’s already using a Proxy (Roll.MATH_PROXY). I don’t think the snippet you wrote would work for that reason, but I’ll try it myself
gaunt pewter
> Also, is there a reason (other than the use of with for this piece of code), t...
Not a specific location, but just in general. I was under the impression that "use strict"; is kinda the default in this day and age, but it’s not used by foundry at all, from what I understand.
mossy pelican
Well ES6 modules and class bodies are all strict mode anyway, which is 99% of foundry's code
mossy pelican
A script which contains mostly classes, yes
gaunt pewter
Well, I won’t argue about it, I was just surprised by it.
mossy pelican
mossy pelican
I think it's not possible to fully sandbox eval. Since we have a bespoke parser with Roll though, we could maybe use that
gaunt pewter
yeah, I guess I'll need to either do that, or do some manual parsing... :/
mossy pelican
new Roll('1 + 1').evaluate({async: false}).total will handle a fair bit
The main issue is that async: false is going away
gaunt pewter
it even works when there is nothing in the context at all, since you can do
Roll.safeEval('(() => {(function foo() {}).constructor("console.log(\\"foo\\")")(); return 0;})()');
yeah, I'll have to look into that... Probably not going to actually instantiate a roll, since there can be structure in my values that wouldn't be accepted by the parsing logic. But I can take inspiration from it...
gaunt pewter
But is there any sense in having safeEval at all, then?
rigid zealot
Unless .roll() can remain synchronous, safeEval will remain necessary.
It's used by Roll.replaceFormulaData() -> Roll.safeEval() combo in places where async code is impossible.
gaunt pewter
I think I now have a proper solution for this. It might be a bit overkill, but I'm pretty happy with it. The idea is pretty simple: I wrote a relatively simple lexer for a relevant subset of JavaScript. Then I define a set of safe allowed identifiers (which for example does not include constructor, etc.). And before actually evaluating an expression, I run the input through the lexer, and check if there are any identifier tokens that would not be allowed. And only if that is not the case, is the expression actually evaluated.
This still gives a lot of flexibility, since you can easily evaluate any expression that just uses the allowed identifiers (which basically is just the properties of Math, true, false, null, and undefined, and a few safe operators), that conforms to the subset of JS that is supported (which exlcudes arrow functions, for example).
but it should be safe
rigid zealot
I think I now have a proper solution for this. It might be a bit overkill, but I...
Why not just copy _evaluateSync?
I might have to do that myself.
Edit: Nope, it relies on async:false, too.
Edit2: Needs more overrides, one for RollTerm, too, which unfortunately requires monkeypatch.