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

Bnaya@08de3c3

[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 notifications@github.com
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
#1621 (comment).

[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" notifications@github.com 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 notifications@github.com
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
#1621 (comment).

—
Reply to this email directly or view it on GitHub
#1621 (comment).

[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 notifications@github.com
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" notifications@github.com
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 notifications@github.com

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
#1621 (comment).

—
Reply to this email directly or view it on GitHub
#1621 (comment).

—
Reply to this email directly or view it on GitHub
#1621 (comment).

[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" notifications@github.com 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 notifications@github.com
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" notifications@github.com
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 <
notifications@github.com>

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
#1621 (comment).

—
Reply to this email directly or view it on GitHub
#1621 (comment).

—
Reply to this email directly or view it on GitHub
#1621 (comment).

[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 #1674 (comment), 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 notifications@github.com
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
#1621 (comment).

[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

[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 #1621 (comment) @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 ( cjdelisle@284d9f0 ) 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 (#1621 (comment)) .

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