CSP issue

[phaistonian]

🐛 Bug Report

As of the this commit React Spring won't work whenever a CSP without 'unsafe-eval' directive is active.

To Reproduce

Set content security policy (CSP) without 'unsafe-eval' in connect-src directive.

[joshuaellis]

Thanks for the report! Can you please provide a repo so we can test against?

[phaistonian]

I am afraid not. This happened in our main repo and it's not one I can not share.

It's really easy to repro, though.

All you need is to set a CSP and load RS.

Btw, this is the culprit.

Apparently CSP treats super('return arguments.callee._call.apply(arguments.callee, arguments)') as eval.

[joshuaellis]

@dbismut I haven't looked at this properly but do you have any immediate thoughts on this?

[dbismut]

@joshuaellis I guess we can try the Proxy method in this article instead https://hackernoon.com/creating-callable-objects-in-javascript-d21l3te1.

[steponas]

I agree that this is an issue. The "unsafe eval" issue prevents the upgraded plugin from running in environments which have some thought put into their CSP rules.

[joshuaellis]

An update, we're currently looking into how viable it is to use the Proxy method. We understand there comes some overhead and we want to gauge how much that overhead it is and how it impacts the library.

If anyone has any suggestions how else we might tackle the issue, we'd love to hear it.

[crux153]

Using Proxy will cause some issue with old browsers without ES6 support, such as IE11. While ES6 Proxy isn't fully polyfillable, I think this specific functionality can be supported as it only uses apply method of Proxy, which is supported by limited polyfill like proxy-polyfill. Maybe it would be best option to figure out how proxy-polyfill handles it and embed it directly in SpringRef so that it can work without using Proxy.

[joshuaellis]

As a rule we don't officially support IE11. AFAIK if you need to, you should run this module through babel but i've never tried it if I'm honest.

I'll be looking to implement this in a beta this week and see how it feels from there.

[joshuaellis]

This has been released in 9.2.0-beta.5, it'd be really helpful if someone could confirm our change has solved the issue 😄

[phaistonian]

@joshuaellis Will upgrade and get back to you soon enough :)

[phaistonian]

It looks/works fine.

Can't test it on an IE (at the moment), though.

[joshuaellis]

As a rule we don't officially support IE11.

See above comment, this originally to do with a package rafz but now Proxy will be an issue too, to resolve this i'd imagine you need to run react-spring through babel and polyfill yourself. I'm glad to see theres no CSP issue though.

[piotrblasiak]

I'm not an expert but I know instantiating Function is banned, so instantiating anything extending Function is probably as well.

[joshuaellis]

I'm not an expert but I know instantiating Function is banned, so instantiating anything extending Function is probably as well.

You may not be an expert, but you've definitely taught me something! ⭐

[joshuaellis]

I've opened a new PR that completely basically reverts the state of SpringRef to what i was before (no extending Function). but if anyones got a moment, it'd be good to see if you find being able to call start() like you did in v8 useful or not really anymore. See the PR for details.

[chris-armstrong]

This has been causing me grief as well, and to be honest, I have a small amount of code that I want to move to v9, and the calling pattern is trivial to change as part of the API update, compared to the execution environment headaches that come from the implicit eval() used to maintain backwards compatibility.

[joshuaellis]

@chris-armstrong thanks! To be honest I really did think the same, but the amount of people who seemed bewildered by the change at the time was crazy! Hopefully though, the PR will resolve this, I don't normally work in strict environments so wasn't aware of this, nor is there a way to test this yet!

[crux153]

Currently I'm just patching react-spring to remove the callable behavior (which is deprecated and not being used in my codebase).

{
  test: /node_modules\/@react-spring\/core/,
  loader: "string-replace-loader",
  options: {
    multiple: [
      {
        search: "class SpringRef extends Function",
        replace: "class SpringRef",
        strict: true,
      },
      {
        search: `super("return arguments.callee._call.apply(arguments.callee, arguments)");`,
        replace: "",
        strict: true,
      },
    ],
  },
}

I used webpack string-replace-loader but something like patch-package would also work fine.

How about offering a flag to disable the callable behavior? I think we can do something like below.

export class SpringRef<State extends Lookup = Lookup> {
  static disableCallable: boolean = false;
  readonly current: Controller<State>[] = []

  constructor() {
    if (!SpringRef.disableCallable) {
      Function.call(this, 'return arguments.callee._call.apply(arguments.callee, arguments)');
    }
  }
}

Then set this flag at the script entry when using csp.

import { SpringRef } from "@react-spring/web";
SpringRef.disableCallable = true;

We might bundle a simple check to automatically set this flag.

try {
  new Function('');
} catch (e) {
  SpringRef.disableCallable = true;
}

[joshuaellis]

Thanks for the suggestion @crux153, would you have to set SpringRef.disableCallable for everytime you try to use it? Because we call it internally on hooks such as useSprings 🤔 it might have to be a Global instead. If you're interested in creating a PR i'd be interested in seeing this!

[crux153]

@joshuaellis SpringRef.disableCallable is a static property and only has to be set once, so there's no need to have a Global.

However, I found that it is not feasible to conditionally inherit from Function. I first thought it should be simple enough with prototype inheritance, but it seems like extending Function with prototype inheritance is massive pain without ES6 class. It IS possible, but Babel involves a lot of tricks to accomplish this and there will be performance penalties.

Most simple approach is to have two copy of SpringRef, one that extends Function and one not callable. Since multiple inheritance is not supported in JavaScript. I haven't found any way to accomplish this without code duplication.

[crux153]

@joshuaellis Ok, I think I found a way of doing this.

function noUnsafeEval() {
  try {
    new Function('')
    return false
  } catch (e) {
    return true
  }
}

const ConditionalCallable = (() => {
  if ((window as any).REACT_SPRING_DISABLE_CALLABLE_REF || noUnsafeEval()) {
    return class {}
  } else {
    return class extends Function {
      constructor() {
        super(
          'return arguments.callee._call.apply(arguments.callee, arguments)'
        )
      }
    }
  }
})()

export class SpringRef<State extends Lookup = Lookup> extends ConditionalCallable {}

This will simply disable the callable behavior in CSP-enabled environments without unsafe-eval.

One can also disable it explicitly with a global variable like

window.REACT_SPRING_DISABLE_CALLABLE_REF = true

Using the same approach, we can also enable the Proxy method only when CSP is enabled.

You can see the full version here, and I'll create a PR if you like it.

[joshuaellis]

Thanks @crux153 i've just released 9.2.0-beta.7 which involved re-writing SpringRef as a function with static methods instead of a class. If this doesn't resolve the issue, I can look at your solution ⭐

It'd be great if anyone could try this and let me know if it's resolved the issue?

@simonauner
@piotrblasiak
@phaistonian

[phaistonian]

@joshuaellis Gave it a go. Did not work ;(

[joshuaellis]

Interesting, I wonder why this would give us said error when we're not extending from Function. Would it be the static methods?

[phaistonian]

@joshuaellis Let me try something else - there's a chance I might be not testing it properly. Will get back a bit later.

[phaistonian]

@joshuaellis it seems it does not work, still. I am sure it will be dealt with eventually, though :)

[joshuaellis]

Can you share any error messages? I'm really not sure why this wouldn't work considering we've removed the extension of Function.

[piotrblasiak]

At first testing it works here! Good job! I'll let you know if I run into any other problem but so far so good.

[phaistonian]

@joshuaellis somehow I still see this - not sure why (considering that you mentioned removing the Function extension).

[joshuaellis]

Odd, I just downloaded the latest beta and it's certainly not there, it should look something like this:

I hate being that guy and suggesting this, but have you tried a clean npm install? 😅

[phaistonian]

@joshuaellis So, for this to work, I had to add a yarn resolution to the latest beta.

Not sure why (yet), but it did the trick.

Thank you - and apologies for the confusion.

[joshuaellis]

It looks like the issue is resolved, it's released on 9.2.0-beta.7 for anyone looking for the fix, although today i'll be releasing 9.2.0 stable. So it'll be on there EOD. I'm going to close this, we can revisit if there are still issues, thank you to everyone who offered help and tested for me you're the real MVP.

[metart43]

Can also confirm that 9.2.0 fixed this issue for me.

[thestephenmarshall]

And another.. 9.2.0 has fixed it for me! 🎉