algolia / instantsearch

⚡️ Libraries for building performant and instant search and recommend experiences with Algolia. Compatible with JavaScript, TypeScript, React and Vue.
https://www.algolia.com/doc/guides/building-search-ui/what-is-instantsearch/js/
MIT License
3.67k stars 515 forks source link

"eval" in 2.6.3 requires the use of an unsafe-eval CSP directive #2868

Open borisschapira opened 6 years ago

borisschapira commented 6 years ago

First of all, thank you for your solution which is really great and quite incredible in terms of performance!

Feature request

Remove the use of eval (found in …Function("return this")()||(0,eval)("this")… in https://cdn.jsdelivr.net/npm/instantsearch.js@2.6.3) so that we could use a script-src directive in our Content Securities Policies that would not be forced to unsafe-eval.

Use case

I have a pretty restrictive CSP on borisschapira.com and don't want to authorize all my third-party domains to execute eval in order to authorize Algolia Instant Search on my search page.

What will fix this (added by @haroenv)

Hogan.js templates are now deprecated, and will be removed completely in the next major version of InstantSearch. To fix the error in the mean time, alias the hogan.js package to an empty module and use only html templates.

Haroenv commented 6 years ago

I guess this has been added by babel or webpack. Any chance you resolved a similar thing already?

Haroenv commented 6 years ago

full chunk responsible is:

/***/ }),
/* 38 */
/***/ (function(module, exports) {

var g;

// This works in non-strict mode
g = (function() {
    return this;
})();

try {
    // This works if eval is allowed (see CSP)
    g = g || Function("return this")() || (1,eval)("this");
} catch(e) {
    // This works if the window reference is available
    if(typeof window === "object")
        g = window;
}

// g can still be undefined, but nothing to do about it...
// We return undefined, instead of nothing here, so it's
// easier to handle this case. if(!global) { ...}

module.exports = g;

not sure yet which module it is

EDIT: I think it's a webpack handling of global usage

Haroenv commented 6 years ago

confirmed, the code in question is https://github.com/webpack/webpack/blob/fde018300aa52262c384e937c408d5dd97d62951/buildin/global.js#L10

https://github.com/webpack/webpack/issues/5627

Haroenv commented 6 years ago

@borisschapira note that you can still have a safe, non-breaking CSP with strict-dynamic which will work

Haroenv commented 6 years ago

This has been discussed in Webpack with the suggested solution of the strict-dynamic CSP directive, which is the recommended value. There's nothing we can really do except suggest a change in Webpack. If this issue is very important for you, I suggest you go into the Webpack threads and ask how you can help or how it can change there.

borisschapira commented 6 years ago

The 'strict-dynamic' source expression allows script loaded via nonce- or hash-based whitelists to load other script, simplifying the requirements for deployment.

I don't see how this could workaround the issue, insofar as it does not concern a prohibition to load a script from a domain, but a prohibition to execute it because of its content (an eval). The issue is still open, so this one should be too: https://github.com/webpack/webpack/issues/5627

I'm gonna try @torarnv's proposition, if I can understand how webpack configuration works…

Edit: Using webpack 4.1.1 with an explicit override of devtool: 'inline-source-map' to disable the default eval source-map for the development environment, I'm successfully able to produce an eval-less output for both production and development environments.

borisschapira commented 6 years ago

Ok, this would need to modify the Webpack version, and that's far beyond my reach :'(

Haroenv commented 6 years ago

If this is the case, then the eval will be gone in a future version when we upgrade Webpack. Not currently on the planning yet, since we had some incompatible plugins last we checked. I'll reopen so we remember to change to inline-source-map then. For now nothing can be done except changing the CSP

borisschapira commented 6 years ago

Just to be clear for future readers (or ourselves in several months), changing the CSP to accept the eval would mean adding unsafe-eval. In most cases, the common security recommendation is to avoid eval() at all costs, as it can be a vector for Cross-Site Scripting attacks (if you want to know more, learn about the script-src directive in a Content Security Policy).

Spone commented 6 years ago

I currently have a related issue with Instantsearch.js 2.10: Error: "call to Function() blocked by CSP"

It's coming from Hogan.makeTemplate https://github.com/twitter/hogan.js/blob/7e340e9e4dde8faebd1ff34e62abc1c5dd8adb55/lib/compiler.js#L293

I understand that Hogan is used internally by Instantsearch.js to handle templates. I should probably open an issue there. But is there a way to still use Instantsearch.js without Hogan in the meantime?

Haroenv commented 6 years ago

@spone, not directly, since templates are used internally too. What you can do is forking hogan (it’s not accepting PRs currently), making the change, and then we can backport it for the whole library, makes sense?

Spone commented 6 years ago

I'm not sure what is the change to make on Hogan.

new Function is used twice:

But I don't see an easy way to eval the JS code safely in those cases...

heiskr commented 3 years ago

Hello hello 👋🏼 Sorry for bringing up an old issue. We're using instantsearch.js and we're wanting to remove the need for unsafe-eval in our content security policy headers. I'm looking through some issues and pull requests in this repository, and the status of Hogan for this project is a bit unclear to me. It looks like there's been attempts to replace Hogan with template literals or another mustache library. However, it looks like as of the current version, 4.9.0, that Hogan is still in place.

Should we consider this project to be permanently on Hogan and needing unsafe-eval indefinitely, or is replacement still possible? We'd be open to making a pull request to another templating option if time/effort is the blocker. I can understand though if it's too large of a breaking change for this project to consider.

Thanks :)

Haroenv commented 3 years ago

Thanks for your message @heiskr. For the moment it's too large of a breaking change for us to consider seriously, however if you find a good alternative that's:

We'd love a PR for this. Note that since it's so big, we can't guarantee that it'll be merged right away, but we'll keep it in close mind to either enable a drop-in replacement for the templating system (allowing yours to replace it, before we ship it as stable), or replacing it with a new API.

In the mean time you can alias hogan to a module with the same API, but which does much less as long as you only use the bare minimum of hogan should be all right. If you only use the connectors for example you won't even need it anymore at all.

If you're very interested in this, you can schedule a call with me (haroen@algolia.com) to hash out some details :)

heiskr commented 3 years ago

Awesome @Haroenv, nice to meet you! Thanks for the tip about aliasing, I totally forgot about it. I used webpack to alias out Hogan. I realized we're actually not using any of the templating built in as we do all our own templates, so I made a quick shim to fill Hogan so it doesn't error out.

I added this to our webpack config:

  resolve: {
    alias: {
      'hogan.js': path.resolve(__dirname, 'javascripts/fake-hogan.js')
    }
  }

And then my fake-hogan.js file is just:

export default {
  compile (template) {
    return {
      render (data) {
        return ''
      }
    }
  }
}

So that gets us to be able to remove unsafe-eval 🎉


While I was figuring this out, I looked briefly at some Mustache alternatives to Hogan.

Hogan mustache.js micromustache Handlebars
Size, minified non-zip 9KB 12KB 6KB 80KB
Eval Yes No No No
Interpolation Yes Yes Yes Yes
Conditionals/Loops Yes Yes No Yes
Helpers Yes No No Yes

There are some API differences as well. micromustache is very similar. mustache.js doesn't have a compile step. Handlebars' compile returns a function and not an object with render on it.

Haroenv commented 3 years ago

micromustache looks very interesting, but due to it only doing string variable interpolation it might be a little too minimalistic for the use cases we have. Personally I've also evaluated these:

htm

nanohtml

xPaw commented 2 years ago

Is IE support still a requirement? It would be good to get rid of eval.

Haroenv commented 2 years ago

At the moment yes, sorry. If you do not use the templates by overriding all default templates you use with function templates (not string templates), you could alias the hogan.js module to nothing/empty module.

We are working on making the default templating system replaceable without much more complexity to the default case, so if you have any ideas on how you'd like to see this evolve, we'd love to hear them @xPaw!

Haroenv commented 1 year ago

Update here, the next major version of InstantSearch will no longer have Hogan, and you can already safely alias it to an empty module to not have this code in your bundle anymore (note that you need to use html instead of hogan templates then of course)

sabatale commented 1 year ago

We just faced this issue while running instantsearch.js against a strong CSP.

Just to clarify, do you mean the unsafe-eval behaviour will not be a problem anymore once v5 is out?

        S.template = S.Template,
        S.makeTemplate = function(e, t, n) {
            var r = this.makePartials(e);
            return r.code = new Function("c","p","i",this.wrapMain(e.code)),
            new this.template(r,t,this,n)
        }
        ,
        S.makePartials = function(e) {
            var t, n = {
                subs: {},
                partials: e.partials,
                name: e.name
            };
            for (t in n.partials)
                n.partials[t] = this.makePartials(n.partials[t]);
            for (t in e.subs)
                n.subs[t] = new Function("c","p","t","i",e.subs[t]);
            return n
        }
        ,
Haroenv commented 1 year ago

Yes, once we build v5 (which isn't yet planned today, FYI), this will be fixed, but for the time being you can alias the hogan.js package to an empty package as the code paths won't be used if you use only html templates