Fabric.js: Fabric js is not csp unsafe-eval compliant

[Bnaya]

Due the use in the new Function notation, when preventing eval via csp header (unsafe-eval),
the library breaks.
https://developer.mozilla.org/en-US/docs/Web/Security/CSP/Using_Content_Security_Policy
The use in csp become more and more common.

I believe it will be possible to implement it using regular js without new Function,
is there a reason to keep it that way?
https://github.com/kangax/fabric.js/blob/master/src/util/misc.js#L429-L434

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[kangax]

I believe I did it this way for performance. So that function is compiled as, say, this.set('originX', value) rather than this.set(property, value) where property (="originX") is saved in a closure and then needs to be resolved at a runtime. Identifier resolution takes time and closures take memory. I wanted getters/setters to be as fast and efficient as possible so that there'd be no reason to use plain property access.

However... I'd love to see the numbers on how much exactly we're saving here.

@jdalton could you shed some light on how this kind of pattern affects performance these days (and if it really matters)?

[Bnaya]

Imo its a micro optimization, and will be matters if you expects tens of thousands of calls if at all.
But it will be very interesting to see a real test case. (micro benchmarks might not give real picture)

[jdalton]

I'd say @Bnaya is right it looks like a micro-opt, I know some have used this technique to improve the performance of v8 in some perf sensitive situations but try it in a real world scenario & see if it hurts.

[Bnaya]

@kangax do you know about someone that using fabric as/part of game engine?
is game engines such opt can matters if the function is part of repetitive loops

https://github.com/Bnaya/fabric.js/commit/08de3c34632bfc4c1d67531d587886a75dad3346

[kangax]

I've heard about few cases of Fabric being used for games. But any frequent
action — usually animation or dragging — that uses getters/setters will
suffer (we just need to find out to which degree).

Oh, also rendering lots of objects — particles, etc.

On Tue, Aug 26, 2014 at 8:47 PM, Bnaya Peretz [email protected]
wrote:

@kangax https://github.com/kangax do you know about someone that using
fabric as/part of game engine?
is game engines such opt can matters if the function is part of repetitive
loops

—
Reply to this email directly or view it on GitHub
https://github.com/kangax/fabric.js/issues/1621#issuecomment-53469986.

[Bnaya]

Hmm i might have another idea:
A poll of all the possible accsesors and assign then where needed using the
loop.
On Aug 27, 2014 8:05 PM, "Juriy Zaytsev" [email protected] wrote:

I've heard about few cases of Fabric being used for games. But any
frequent
action — usually animation or dragging — that uses getters/setters will
suffer (we just need to find out to which degree).

Oh, also rendering lots of objects — particles, etc.

On Tue, Aug 26, 2014 at 8:47 PM, Bnaya Peretz [email protected]
wrote:

@kangax https://github.com/kangax do you know about someone that
using
fabric as/part of game engine?
is game engines such opt can matters if the function is part of
repetitive
loops

—
Reply to this email directly or view it on GitHub
https://github.com/kangax/fabric.js/issues/1621#issuecomment-53469986.

—
Reply to this email directly or view it on GitHub
https://github.com/kangax/fabric.js/issues/1621#issuecomment-53605934.

[kangax]

You mean to make new Function -based accessors optional? For user to
chose to "turn on"?

On Wed, Aug 27, 2014 at 7:15 PM, Bnaya Peretz [email protected]
wrote:

Hmm i might have another idea:
A poll of all the possible accsesors and assign then where needed using
the
loop.
On Aug 27, 2014 8:05 PM, "Juriy Zaytsev" [email protected]
wrote:

I've heard about few cases of Fabric being used for games. But any
frequent
action — usually animation or dragging — that uses getters/setters will
suffer (we just need to find out to which degree).

Oh, also rendering lots of objects — particles, etc.

On Tue, Aug 26, 2014 at 8:47 PM, Bnaya Peretz [email protected]

wrote:

@kangax https://github.com/kangax do you know about someone that
using
fabric as/part of game engine?
is game engines such opt can matters if the function is part of
repetitive
loops

—
Reply to this email directly or view it on GitHub
https://github.com/kangax/fabric.js/issues/1621#issuecomment-53469986.

—
Reply to this email directly or view it on GitHub
https://github.com/kangax/fabric.js/issues/1621#issuecomment-53605934.

—
Reply to this email directly or view it on GitHub
https://github.com/kangax/fabric.js/issues/1621#issuecomment-53607381.

[Bnaya]

I mean to create all the possible accsessors in a mixin, but to extend from
it selectively only the needed one's using loop over the state props
On Aug 27, 2014 8:20 PM, "Juriy Zaytsev" [email protected] wrote:

You mean to make new Function -based accessors optional? For user to
chose to "turn on"?

On Wed, Aug 27, 2014 at 7:15 PM, Bnaya Peretz [email protected]
wrote:

Hmm i might have another idea:
A poll of all the possible accsesors and assign then where needed using
the
loop.
On Aug 27, 2014 8:05 PM, "Juriy Zaytsev" [email protected]
wrote:

I've heard about few cases of Fabric being used for games. But any
frequent
action — usually animation or dragging — that uses getters/setters
will
suffer (we just need to find out to which degree).

Oh, also rendering lots of objects — particles, etc.

On Tue, Aug 26, 2014 at 8:47 PM, Bnaya Peretz <
[email protected]>

wrote:

@kangax https://github.com/kangax do you know about someone that
using
fabric as/part of game engine?
is game engines such opt can matters if the function is part of
repetitive
loops

—
Reply to this email directly or view it on GitHub
<
https://github.com/kangax/fabric.js/issues/1621#issuecomment-53469986>.

—
Reply to this email directly or view it on GitHub
https://github.com/kangax/fabric.js/issues/1621#issuecomment-53605934.

—
Reply to this email directly or view it on GitHub
https://github.com/kangax/fabric.js/issues/1621#issuecomment-53607381.

—
Reply to this email directly or view it on GitHub
https://github.com/kangax/fabric.js/issues/1621#issuecomment-53607980.

[Bnaya]

Thats what i meant:
https://github.com/Bnaya/fabric.js/compare/accessors-refactor?expand=1

[Bnaya]

@kangax What do you say?

[Bnaya]

Pull request #1674

[francislavoie]

Sorry, I did a search for issues but I didn't find this one.

As you've said here https://github.com/kangax/fabric.js/pull/1674#issuecomment-101068804, I'm interested in this. I need to use fabric to implement image cropping in a Chrome App. I'm using darkroom.js which uses fabric.

Thanks

[kangax]

@francislavoie have you seen https://developer.chrome.com/extensions/sandboxingEval

[francislavoie]

I was not aware, thanks for pointing it out. Enabling sandboxing broke some other stuff (webfonts wouldn't load) but I fixed those by loading those as base64 data URIs instead.

For reference, I ended up sandboxing the entire index.html page by adding the following to my manifest:

"sandbox": {
  "pages": ["index.html"]
},

I'm not seeing any negative side effects from this yet, so it should be good.

Thanks! :)

[francislavoie]

@kangax Hmm, I didn't realize that sandboxing would block me from using the chrome.* APIs (aside from messaging). For sandboxing to work for me I'd need to put fabric in an iframe, which would take a good amount of refactoring to work. I'll just stick with modifying the chunk in of code in createAccessors() which seems to work for me.

// using `new Function` for better introspection
if (!proto[getterName]) {
  proto[getterName] = (function(property) {
    return function() {return this.get(property)};
    //return new Function('return this.get("' + property + '")');
  })(propName);
}
if (!proto[setterName]) {
  proto[setterName] = (function(property) {
    return function(value) {return this.set(property, value)};
    //return new Function('value', 'return this.set("' + property + '", value)');
  })(propName);
}

[Bnaya]

@francislavoie you can also relax the default chrome app CSP
https://developer.chrome.com/extensions/contentSecurityPolicy#relaxing

[francislavoie]

@Bnaya unfortunately that's for extensions only. Chrome apps don't allow relaxing 'unsafe-eval'. See here: https://developer.chrome.com/apps/contentSecurityPolicy#but

[Bnaya]

Oh, well i understand them. i wish all web pages were strict CSP compliant
imo this feature (named accessors) is a nice sugar but i'm not sure if its realy needed

[off-border]

I'm trying to convert my single-page app that's using fabric.js into Chrome App. And faced with the 'unsafe-eval' error.

Temporarily, I solved the problem this way:


// using new Function for better introspection

if (!proto[getterName]) {

      proto[getterName] = (function(property) {

        //return new Function('return this.get("' + property + '")');
        return function() {return this.get( property )};

      })(propName);

    }

    if (!proto[setterName]) {

      proto[setterName] = (function(property) {

        //return new Function('value', 'return this.set("' + property + '", value)');
        return function(value) { return this.set( property , value) };

      })(propName);

    }

[oreoshake]

Any movement here? Will https://github.com/kangax/fabric.js/compare/master...Bnaya:accessors-refactor?expand=1 work?

It's a bummer that sites can't fully lock down their apps if they use this library. In one case, this is the final blocker.

However... I'd love to see the numbers on how much exactly we're saving here.

Perhaps there could be an "optimized" and "slow" option for sites that value security over micro-optimization?

EDIT: sorry, looks like there's a different problem I'm noticing. With eval:

https://github.com/kangax/fabric.js/blob/30c64ec9c115276f14fd83e76ca800002d8f31c1/src/node.js#L125 and https://github.com/kangax/fabric.js/blob/30c64ec9c115276f14fd83e76ca800002d8f31c1/lib/json2.js#L474

[kangax]

Neil, if you can send a PR, I think I can get this merged.

On Fri, Nov 20, 2015 at 6:55 PM, Neil Matatall [email protected]
wrote:

Any movement here? Will
https://github.com/kangax/fabric.js/compare/master...Bnaya:accessors-refactor?expand=1
work?

It's a bummer that sites can't fully lock down their apps if they use this
library. In one case, this is the final blocker.

However... I'd love to see the numbers on how much exactly we're saving
here.

Perhaps there could be an "optimized" and "slow" option for sites that
value security over micro-optimization?

—
Reply to this email directly or view it on GitHub
https://github.com/kangax/fabric.js/issues/1621#issuecomment-158579236.

[Bnaya]

I think pre create accessors should be even more performant than new Function, and also CSP compliant, but we can put it behind build flag to save lib size.

The thing with precleared accessors is that if someone add new stateProperties you also need to manually add accessor.
And we can fallback to closure accessor thats CSP, less size, but less performant

[asturur]

i would choose one way. i would not add extra complexity, there are lot lot of options already.

[Bnaya]

I've updated
https://github.com/kangax/fabric.js/compare/master...Bnaya:accessors-refactor?expand=1
And now its include all of the current accessors in use and pass the tests
I'm generating the accessors code using:
http://jsbin.com/tazuq/edit?html,js,output

If this is an acceptable solution i can make a PR

And performance notice:
It might be better to make the accessors directly modify the stateProperty than using this.set/get,
set/get have variable arguments types what can them not to be optimized/or get deoptimizations all the time

[kangax]

I wonder if we can generate that file instead :) Not to spell everything out like that.

[Bnaya]

you need plain js array/object with all the possible stateProps to generate it on build time, or to find a way to collect is from the various classes

[pfafman]

I would put in a vote here for solving this issue. I use fabric for a site that has to pass security audits and it will not pass if eval is allowed, so I have been branching and merging in the changes that @Bnaya has in his PR.

[asturur]

anyone still interested here? I would like to close this issue once for all.
I'm scared at the idea of mantaining another file.

I would rather try to build a script that updates it cycling trought a list of known classes and theyr stateProperties array. What is out of state properties does not have the accessors.

OR

remove accessors in 2.0 version of api and leave .set and .get

[pfafman]

I am still interested. I have to hack this into fabric every time you do an update and I want to use it on my sites. I would vote to remove the wrapper accessors. Also safer for you site to have eval turned off.

[mobidev111]

@asturur any progress here? I'm still interested

[asturur]

yes. today i release 1.7.4
last of planned 1.x releasese ( bugfix will be made if possible )

i save everything in the 1.x branch and in the master branch i start to do the big breaking changes.
one of this is remove from default build the accessors, so that fabric comes out without the extended setThis and setThat. who wants to mantain compability with old code will have a way to build them maybe.

[asturur]

If someone think that extended accessors are very important to have, if he can share some reasons here so that if i m about to do a wrong move, i see why.

[Bnaya]

One reason i can think of

obj.set(propName: string, value: string | number)

If the type of value is changing every x calls, v8 might optimize and deopt it a lot of times

So its better to keep arguments types stable.
But it might be irrelevant, i don't know

[asturur]

not sure about how hard this optimization is.

the slow part here is rendering

[ansuz]

:+1: for not using new Function, I used fabricjs to make a prototype of a zero knowledge whiteboard app in cryptpad, but since we started making heavy use of CSP it stopped working.

I'm developing with a patched version, but I'd much rather see this get merged.

[asturur]

i would like to ditch full accessors completely in 2.0 as a default and leave a method to create them if someone for some reasons wants to use them.
(i m about to release 2.0 alpha but the api change is not there yet).

[cjdelisle]

Was this optimization the result of profiling ?
I'm curious about it because generally I understand eval() to be the enemy of performance because it forces the js engine back to interpreted mode.
I made a jsperf to check this and at least in chrome, the function template seems to be faster: https://jsperf.com/eval-accessor/1

[asturur]

@Bnaya if you still remember, which is the difference between your proposed solution and calling set(pro, value) manually?

Would v8 make any difference?

[Bnaya]

First

With my abit outdated knowledge of v8, The current accessors are inefficient and might cause v8 de-opt
Why?
The named accessors are using o.set
The signature of set looks like:
o.set(propName: string, value: string | number)
And when you call the same function with different args type, v8 might choose not to optimize it, or worse, optimize and de-optimize

But this kind of considerations are relevant only if you are developing a game and have aloot of objects.
And on that case, you shouldn't use the accessors at all of their current form.

The most optimzied way would be to have an accessor per value type.
setString, setNumber, setWhatever

new Function vs closure

If we compare the current new Function impl vs using a closure, performance wise:
The overhead of the closure is only adding another lookup step for the prop name.
And eval/new Function has it own overheads and considered harmful nowdays
There's no memory overhead compare to new Function because both of the cases creating new function for each of the possible props for each object type (which is a different issue).

The holy grail of accessors would be something different from the proposed and current state, and would be more similar to what i did here:
https://github.com/kangax/fabric.js/compare/master...Bnaya:accessors-refactor?expand=1

[asturur]

i was referring exactly to that code and im asking you, what deoptimization differences can you see between creating a function

setSkewX: function (value) {this.set("skewX", value); return this;},

that calls this.set with the second argument that change in type ( value ), and not creating it at all and let the user call set by its own with an argument that change in type ( the user will have just this.set() )

[Bnaya]

As far as i understand, it will be the same.
EDIT:
Just that there are 2 functions involved and not only o.set

[asturur]

i think i will just trash this accessors... really undecided.

[Bnaya]

I see no reason why not if you are pushing breaking changes build anyway.
If someone will be missing it he can add it himself

[asturur]

in the unperforming/unsecure/uncomfortable way he/she prefers

[ansuz]

thanks so much for your responses! I'm looking forward to being able to integrate this again.

[cjdelisle]

I've created a fork called secure-fabric.js which fixes the CSP problem and 2 others (although they are overridable by the user if they would like to specify a function as a string in the configuration).

You can upgrade to secure-fabric.js easily with bower:

bower uninstall --save fabric.js`
bower install --save secure-fabric.js#secure-v1.7.9`

I'm happy to take other pull requests here: https://github.com/cjdelisle/secure-fabric.js

[mobidev111]

@cjdelisle I would be great to have your changes as a pull request to this fabric.js repository here.

what are the reasons for the fork? the fork has a high risk of being outdated while at the same time the fabric.js repository is maintained very well.

[cjdelisle]

@mobidev111 as you can see in https://github.com/kangax/fabric.js/issues/1621#issuecomment-55881277 @Bnaya has made a PR already which fixes fabric.js security but @kangax judged it as not interesting to accept so my understanding is that CSP is not a priority for fabric.js

[mobidev111]

This pull request dates way back to 2014.

My understanding is that @asturur is working currently to remove the cause for the CSP exception by removing the accessors. If you already did that, then this saves time for @asturur..

[cjdelisle]

The issue is already fixed ( https://github.com/cjdelisle/secure-fabric.js/commit/284d9f04e34e7d5a6aaa2074ceb069966a453439 ) and changes to the API are not necessary, although they may be desirable. The fact that this issue has been ongoing since 2014 is the reason why I chose to fork rather than create (yet another) pull request.

[asturur]

i would not agree with // The user passed a string as the options.source value so they don't care about CSP. Looks like that having new function in the source code is a reason to be declared non safe.

[asturur]

apart from that, the pr from @Bnaya was not considered not interesting, there was an ongoing discussion about performance and manual manteinence of the file with the accessor proposed.

No CSP is not a priority at all, that is true.

Fixing accessors will not fix pattern and clipTo.
ClipTo is another thing i want to remove since is both hard to use and at the end is always clipping with either a rect or a shape. At this point better have a property called clipWith and passing in a shape/rect/circle/polygon. ( does not work with text nor now nor later )

[cjdelisle]

That's a fair point, it seems that in most cases people pass in a function which is then stringified and then re-parsed to a function (for reasons I do not understand). But looking at the code I can see that it will work with a string representation of a function so I chose to preserve this behavior in order to be API-identical while still being CSP compliant (where we recognize the risk with CSP is that it will just fail)

Thinking about it, I'd be happy to take a PR to secure-fabric.js which removes this option. Come be the first contributor 😄

[mobidev111]

pull requests are clearly welcomed in this repo, even this issue mentions this (https://github.com/kangax/fabric.js/issues/1621#issuecomment-158584980) .

in this repo your contribution benefits directly the community - and it will be maintained in the long run. this IMHO is the cool thing about open source - proposing a solution and iterating until it serves the requirements of the overall project.

[asturur]

the point of passing functions in properties that must be stored and restored( clipTo or patternSource ) is that is easy to pass wrong functions with references to vars that are not available after restoring.

I'm inclined to remove them all. same for patterns.

Now i m more interested in understanding how people will suffer from missing a part of the full-named-accessors.

Let's take the text example.
In Text there are 6 or 7 properties that if set with set trigger a dimension calculation ( sort of expensive ). If you set them with named accessors you trigger one calculation per property.

If you set with set({ object with many prop }) you can trigger one ( after a code update ).

Being generated from props names, named accessors do not give an better mnemonic name to the situation

named accessors make docs longer, i prefere a longer jsdoc over the property itself.

Positive things of named accessor? can someone help me finding them out?

[asturur]

so i moved the accessors in a separate files that you can add at build time. the module in excluded by default in 2.0 ( for now )

If the module gets included the accessors get created.
Let's see what kind of riot comes out for the 2.0 upgrade, and maybe it will be included by default and excludible.

I prefer not to have them.

This is not closing the issue since clipTo and patterns are still there playing with functions.

[Bnaya]

Where is the clipTo/patterns problematic code?

[asturur]

well is not problematic but patterns can have the property source as a function and clipTo is a function.

So when we are doing toJSON/fromJSON a parsing of the function body needs to be done, and i bet is not safe.

Those are 2 things i want to remove anyway.

pattern: line 56 of pattern.calss.js
clipTo: line 53 of share_methods.mixin.js

[spand]

Is this (still?) slated for the 2.0 release ?

[XhmikosR]

@kangax: can't you do something about this? It's a pretty important issue.

[Webbanditten]

We have a very strict content security policy and I was initially going to go with fabricjs, however now I cant because of this issue.

[asturur]

clipTo is gone, so one issue less.
And since you remind me of this, pattern function evaluation can go away since fabric4 has breaking changes.

So if you want, you can re-evaluate.

[Webbanditten]

Just tried version 4.0.0-beta.7 - Still experiencing the same issue.

[asturur]

i m not sure what the issue is. fabric 4 it does still make use of creating functions from text.
The last part of code that does it is the pattern code, that could be eliminated before version 4 stable.

[ansuz]

FYI It was trivial to patch to be compliant with strict CSP. We forked the project three years ago for this patch so that we could use the library within CryptPad.

[asturur]

I would prefer not restoring functions from json at all.
Remove the problem at the root.
Many devs also have difficulties understanding that references to global vars are hard to restore and also fabricjs is more oriented to store data that can be displayed back and not functions.

This is still useful for patterns but since i would like to have patterns defined as groups more than functions, maybe we should let it go