downgrade regenerator-runtime due to unsafe eval usages #1062
Conversation
|
ci isn't working for me for some reason (couldn't figure out why so filed a support ticket), manually ran npm run build and npm test |
oshi97
force-pushed
the
dev/remove-use-strict
branch
from
25cf110
to
a291373
Compare
oshi97
changed the title
By default, rollup bundles js in strict mode.
When regenerator-runtime is included in use-strict
mode, they use a Function() call (which is like
a slightly safer eval()) to initialize things.
Specifically
```js
Function("r", "regeneratorRuntime = r")(runtime);
```
Removing use-strict mode lets regenerator runtime init
without any unsafe evals. Because strict mode
javascript is a subset of regular javascript, this
should not break any code. AFAIK there is no way
to require a script tag to be in 'use strict' mode.
T=https://yextops.zendesk.com/agent/tickets/347915
TEST=manual
tested with a local apache httpd server
added below line to .htaccess, which sets the CSP to
the same as Syncreon
also test csp with a `<meta>` tag
tested both answers.js and answers.min.js
oshi97
force-pushed
the
dev/remove-use-strict
branch
from
a291373
to
1e3df7c
Compare
conf/gulp-tasks/library.gulpfile.js
Outdated
| @@ -68,7 +68,8 @@ function legacyBundleIIFE () { | |||
| return legacyBundle({ | |||
| format: 'iife', | |||
| name: NAMESPACE, | |||
| sourcemap: true | |||
| sourcemap: true, | |||
There was a problem hiding this comment.
Do we need to fix this for the modern bundle as well?
There was a problem hiding this comment.
nope! just checked, it's not an issue for the modern bundle. I believe it's because the modern bundle doesn't have the regenerator-runtime polyfill (or any of the polyfill-prefix stuff)?
There was a problem hiding this comment.
updated the test desc
There was a problem hiding this comment.
I'm a bit wary of just disabling strict mode. How much research have we done into it? I took a quick look at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode. It sounds like strict mode has some benefits:
- Can produce more optimized JS.
- Can help produce more secure JS.
Are we sure we want to get rid of this? Is it standard for libraries such as ours to use strict mode? Others have encountered this same CSP issue: mozilla/pdf.js#11036. One recommendation was to downgrade regenerator-runtime (apparently an older version(s) did not have this problem with strict mode). Is that a viable option here?
I'm not sure that we're getting much performance benefit from strict mode. It sounds like strict mode supports better compiler optimization. Uglify is a compiler of sorts. Is the bundle size different at all between strict and sloppy? |
The bundle size is identical. Since the SDK doesn't have regenerator runtime in develop anymore, we don't need this change in future versions, at least right now. I do want to add an acceptance test for no unsafe-eval csp on a searchbar only page, though (created slap-619 last week for this) here are the two builds I looked at |
oshi97
changed the title
This commit downgrades regenerator-runtime from ^0.13.3 (currently 0.13.7) to being pinned to exactly 0.13.1. This is because there is a change in 0.13.2 which adds back in a Function() constructor to define regenerator runtime globally. See #369 https://github.com/facebook/regenerator/commits/master/packages/regenerator-runtime T=https://yextops.zendesk.com/agent/tickets/347915 TEST=manual tested with a local apache httpd server on a searchbar only page added below line to .htaccess, which sets the CSP to the same as Syncreon also test csp with a `<meta>` tag tested both answers.js, answers.min.js, answers-modern.js, and answers-modern.min.js double checked that our code still runs correctly in ie11, for both a searchbar only page with no unsafe-eval csp, and a regular page with facets and results that allows unsafe eval
This commit downgrades regenerator-runtime from
^0.13.3 (currently 0.13.7) to being pinned to exactly
0.13.1. This is because there is a change in 0.13.2 which
adds back in a Function() constructor to define
regenerator runtime globally.
See #369 https://github.com/facebook/regenerator/commits/master/packages/regenerator-runtime
T=https://yextops.zendesk.com/agent/tickets/347915
TEST=manual
tested with a local apache httpd server on a searchbar only page
added below line to .htaccess, which sets the CSP to
the same as Syncreon
also test csp with a
<meta>tagtested both answers.js, answers.min.js, answers-modern.js, and answers-modern.min.js
double checked that our code still runs correctly in ie11, for both a searchbar only
page with no unsafe-eval csp, and a regular page with facets and results
that allows unsafe eval