add ember-no-implicit-this-codemod #20
Conversation
kellyselden
changed the title
manifest.json
Outdated
| }, | ||
| "projectOptions": ["app", "addon"], | ||
| "nodeVersion": "8.0.0", | ||
| "script": "const cp = require('child_process'); let ps = cp.spawn('ember', ['s']); let url = await new Promise(resolve => { ps.stdout.on('data', data => { let str = data.toString(); let matches = str.match(/^Build successful \\(\\d+ms\\) – Serving on (.*)$/m); if (matches) { resolve(matches[1]); } }); }); cp.execSync(`npx ember-no-implicit-this-codemod ${url} app/`); ps.kill(); await new Promise(resolve => { ps.on('exit', resolve); });" |
There was a problem hiding this comment.
this seems risky :-\
There was a problem hiding this comment.
just that having a bunch of node stored as a string implies that this'll be eval'd somewhere, and I know that eval can lead to vulnerabilities.
Makes me wonder if the codemods should distribute interactive bins that ember-cli-update invokes -- that way each codemod could be responsible for instructing the user how to set things up. idk
There was a problem hiding this comment.
I did this to help protect kellyselden/boilerplate-update#125
But in the long run, there isn't too much difference between eval and npx, both download code and execute.
There was a problem hiding this comment.
Well, in this case it is embedding eval and npx 😸. Two semi-risky things seems strictly worse than one no?
There was a problem hiding this comment.
I'm torn. I know there is risk associated with eval, but I don't think there is much additional risk here. My thoughts:
- The dangers of
evalare mostly for people concatenating strings from user input, much like SQL injection. The strings in this case are under our control and don't take user input. - I use the
Functioncontructor instead ofevaldescribed here. - So the attack vector seems to be a compromised GitHub account, which seems as likely as a compromised NPM account.
Please correct me if I'm wrong. Am I naive?
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
3 times, most recently
from
26a97ce to
1df5c06
Compare
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
from
1df5c06 to
de5900f
Compare
|
I think we should refactor how this works in general, each codemod booting the app is going to get bonkers pretty quickly. The list of codemods that want telemetry data is going to continue growing (its very important for a number of the current/pressing use cases). Imagine your app has a boot time of ~45seconds, you are incurring that cost for each codemod, the overall runtime quickly gets high. We should probably consider grouping the codemods that need a running app in this manifest file, and having the host process that runs through them boot the app for us. Then the only thing that would be needed is to interpolate the port number for invocation. |
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
from
de5900f to
3693727
Compare
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
3 times, most recently
from
447599c to
2cfa118
Compare
kellyselden
changed the title
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
from
2cfa118 to
fc75b4b
Compare
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
from
fc75b4b to
0c3d391
Compare
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
2 times, most recently
from
46e5122 to
00ff5ee
Compare
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
from
00ff5ee to
8266c8d
Compare
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
2 times, most recently
from
5a0dec5 to
6db120c
Compare
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
4 times, most recently
from
89e1ba2 to
91d8cae
Compare
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
2 times, most recently
from
557d7c8 to
24e787a
Compare
kellyselden
force-pushed
the
ember-no-implicit-this-codemod
branch
from
24e787a to
964185a
Compare
Closes #19
cc @NullVoxPopuli