ci - install deps - limit install scripts to whitelist #7208

kumavis · 2019-09-23T03:23:01Z

moving in parallel to SESify/lavamoat, I'm doing some research to reduce our exposure to 3rd party code.

There's 3 main phases we need to concern ourselves with:

Dep Install time via pre/post-install scripts (related to this PR)
Build time via build tools (lavamoat the build system...? lavamoat node loader?)
Run time via deps in bundle (related to SESify/lavamoat)

metamaskbot · 2019-09-23T03:36:59Z

Builds ready [bc45ed0]

builds: chrome, firefox, opera, edge
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

.circleci/scripts/deps-install.sh

Co-Authored-By: Mark Stacey <markjstacey@gmail.com>

kumavis · 2019-09-23T04:07:27Z

aha test failures, thats more what I was expecting

Gudahtt · 2019-09-23T04:11:54Z

This seems pretty reasonable, though from the looks of CI there is at least one package missing from the list.

I'm a bit concerned that it might be difficult to determine if we've missed one, as our test coverage does have some significant gaps. So it's possible we're missing an important build step that'll result in a subtle bug somewhere. I'm also a bit concerned about how we're to maintain this list over time.

Maybe we could recursively scan all dependencies for build scripts, and compare it to the list we've manually built?

Then there's the question of which scripts we do want to run. It's possible we have packages with optional scripts that we could just ignore.

This package implemented something similar that stored the list in package.json: https://github.com/dominykas/allow-scripts
It might prove useful. Or at least maybe we could take inspiration from it, and store the list in a more machine-readable format to make it easier to maintain.

kumavis · 2019-09-23T04:44:12Z

My first assumption is that nothing that ends up in the bundle actually requires a install script, unless someone is babel compiling at post-install time. My assumption is that its mostly for native deps (bindings to code that needs to be compiled, eg written in c or something) that are part of our build and test systems

nice find with allow-scripts, pretty much what we're doing by hand here. I suppose it does not run its own deps if any under this same restriction, though they are few

kumavis · 2019-09-23T05:28:33Z

wrote a script, these ones have scripts

core-js: /home/xyz/Development/metamask-extension2/node_modules/core-js
core-js-pure: /home/xyz/Development/metamask-extension2/node_modules/core-js-pure
sc-uws: /home/xyz/Development/metamask-extension2/node_modules/sc-uws
tiny-secp256k1: /home/xyz/Development/metamask-extension2/node_modules/tiny-secp256k1
chromedriver: /home/xyz/Development/metamask-extension2/node_modules/chromedriver
electron: /home/xyz/Development/metamask-extension2/node_modules/electron
jpegtran-bin: /home/xyz/Development/metamask-extension2/node_modules/jpegtran-bin
jss: /home/xyz/Development/metamask-extension2/node_modules/jss
optipng-bin: /home/xyz/Development/metamask-extension2/node_modules/optipng-bin
sinon: /home/xyz/Development/metamask-extension2/node_modules/sinon
scrypt: /home/xyz/Development/metamask-extension2/node_modules/scrypt
ursa-optional: /home/xyz/Development/metamask-extension2/node_modules/ursa-optional
leveldown: /home/xyz/Development/metamask-extension2/node_modules/leveldown
fetch-mock: /home/xyz/Development/metamask-extension2/node_modules/fetch-mock
secp256k1: /home/xyz/Development/metamask-extension2/node_modules/secp256k1
@sentry/cli: /home/xyz/Development/metamask-extension2/node_modules/@sentry/cli
assemblyscript: /home/xyz/Development/metamask-extension2/node_modules/assemblyscript
gifsicle: /home/xyz/Development/metamask-extension2/node_modules/gifsicle
keccak: /home/xyz/Development/metamask-extension2/node_modules/keccak
sha3: /home/xyz/Development/metamask-extension2/node_modules/sha3
weak: /home/xyz/Development/metamask-extension2/node_modules/weak
websocket: /home/xyz/Development/metamask-extension2/node_modules/websocket
node-sass: /home/xyz/Development/metamask-extension2/node_modules/node-sass
sqlite3: /home/xyz/Development/metamask-extension2/node_modules/sqlite3
phantomjs-prebuilt: /home/xyz/Development/metamask-extension2/node_modules/phantomjs-prebuilt
gc-stats: /home/xyz/Development/metamask-extension2/node_modules/gc-stats
geckodriver: /home/xyz/Development/metamask-extension2/node_modules/geckodriver

kumavis · 2019-09-23T05:37:09Z

I'm so confused, my tool is reporting that weak has an install script, and its consistent with an error im seeing, but the actual package.json at the reported location does not have an install script..?

kumavis · 2019-09-23T05:40:08Z

oh hell, this is the default value for the install script "install": "node-gyp rebuild" https://docs.npmjs.com/misc/scripts#default-values

kumavis · 2019-09-23T07:04:59Z

for the second phase "build time", we could maybe use a nodejs loader but this suggests it only works with esm
https://nodejs.org/api/esm.html#esm_resolve_hook

metamaskbot · 2019-09-23T07:08:26Z

Builds ready [3428381]

builds: chrome, firefox, opera, edge
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

kumavis · 2019-09-23T07:08:47Z

surprised these weren't needed phantomjs-prebuilt, gc-stats. dead deps? incorrectly configured deep dev deps?

kumavis · 2019-09-23T07:20:20Z

Added my utility script that shows what deps have install scripts. we only run a small subset of them

core-js: node_modules/core-js postinstall
core-js-pure: node_modules/core-js-pure postinstall
sc-uws: node_modules/sc-uws install
tiny-secp256k1: node_modules/tiny-secp256k1 install
chromedriver: node_modules/chromedriver install
electron: node_modules/electron postinstall
jpegtran-bin: node_modules/jpegtran-bin postinstall
jss: node_modules/jss postinstall
optipng-bin: node_modules/optipng-bin postinstall
sinon: node_modules/sinon postinstall
scrypt: node_modules/scrypt preinstall,install
ursa-optional: node_modules/ursa-optional install
leveldown: node_modules/leveldown install
fetch-mock: node_modules/fetch-mock postinstall
secp256k1: node_modules/secp256k1 install
@sentry/cli: node_modules/@sentry/cli install
assemblyscript: node_modules/assemblyscript postinstall
gifsicle: node_modules/gifsicle postinstall
keccak: node_modules/keccak install
sha3: node_modules/sha3 install
weak: node_modules/weak install
websocket: node_modules/websocket install
node-sass: node_modules/node-sass install,postinstall
sqlite3: node_modules/sqlite3 install
phantomjs-prebuilt: node_modules/phantomjs-prebuilt install
gc-stats: node_modules/gc-stats install
geckodriver: node_modules/geckodriver postinstall

metamaskbot · 2019-09-23T08:53:55Z

Builds ready [ddb1a25]

builds: chrome, firefox, opera, edge
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

kumavis · 2019-09-23T09:03:25Z

I'm also a bit concerned about how we're to maintain this list over time.

I don't expect it to need to change very often but as long as CI fails, the list should get updated. If it gets a bit stale, its not too bad, but currently will need some manual review.

anyways im happy with this for now

package.json

metamaskbot · 2019-09-25T03:24:46Z

Builds ready [c9c69be]

builds: chrome, firefox, opera, edge
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Gudahtt

Looks good!

* origin/develop: (56 commits) Add advanced setting to enable editing nonce on confirmation screens (MetaMask#7089) Add migration on 3box imports and remove feature flag (MetaMask#7209) ci - install deps - limit install scripts to whitelist (MetaMask#7208) Add a/b test for full screen transaction confirmations (MetaMask#7162) Update minimum Firefox verison to 56.0 (MetaMask#7213) mesh-testing - submit infura rpc requests to mesh-testing container (MetaMask#7031) obs-store/local-store should upgrade webextension error to real error (MetaMask#7207) sesify-viz - bump dep for visualization enhancement (MetaMask#7175) address book entries by chainId (MetaMask#7205) Optimize images only during production build (MetaMask#7194) Use common test build during CI (MetaMask#7196) Report missing `en` locale messages to Sentry (MetaMask#7197) Verify locales on CI (MetaMask#7199) updated ganache and addons-linter (MetaMask#7204) fixup! add user rejected errors add user rejected errors update json-rpc-engine use eth-json-rpc-errors Remove unused locale messages (MetaMask#7190) Remove unused components (MetaMask#7191) ...

ci - install deps - limit install scripts to those needed for build

bc45ed0

kumavis changed the title ~~ci - install deps - limit install scripts to those needed for build~~ ci - install deps - limit install scripts to whitelist Sep 23, 2019

kumavis marked this pull request as ready for review September 23, 2019 03:46

Gudahtt reviewed Sep 23, 2019

View reviewed changes

.circleci/scripts/deps-install.sh Outdated Show resolved Hide resolved

Update .circleci/scripts/deps-install.sh

8c8d145

Co-Authored-By: Mark Stacey <markjstacey@gmail.com>

kumavis added 2 commits September 23, 2019 13:52

ci - install deps - expand install scripts needed for tests

c235b83

ci - install deps - expand install scripts needed for integration tests

3428381

kumavis added 3 commits September 23, 2019 15:10

ci - install deps - fix node-sass script ref

b5278c9

github - set codeowners for scripts/deps-install

affebaf

development - add utility to show deps with install scripts

8e7dbca

kumavis requested review from danjm and whymarrh as code owners September 23, 2019 07:19

lint fix

ddb1a25

kumavis mentioned this pull request Sep 23, 2019

CI - create archive of dependency sources #7153

Closed

Gudahtt requested changes Sep 24, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

deps - move read-installed to devDeps

c9c69be

Gudahtt approved these changes Sep 25, 2019

View reviewed changes

kumavis merged commit e1efb4d into develop Sep 25, 2019

kumavis deleted the sec-limit-install-scripts branch September 25, 2019 12:01

whymarrh mentioned this pull request Feb 7, 2020

Add husky pre-commit hook, add lint-staged #8013

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci - install deps - limit install scripts to whitelist #7208

ci - install deps - limit install scripts to whitelist #7208

kumavis commented Sep 23, 2019 •

edited

Loading

metamaskbot commented Sep 23, 2019

kumavis commented Sep 23, 2019

Gudahtt commented Sep 23, 2019

kumavis commented Sep 23, 2019

kumavis commented Sep 23, 2019

kumavis commented Sep 23, 2019

kumavis commented Sep 23, 2019

kumavis commented Sep 23, 2019

metamaskbot commented Sep 23, 2019

kumavis commented Sep 23, 2019

kumavis commented Sep 23, 2019

metamaskbot commented Sep 23, 2019

kumavis commented Sep 23, 2019

metamaskbot commented Sep 25, 2019

Gudahtt left a comment

ci - install deps - limit install scripts to whitelist #7208

ci - install deps - limit install scripts to whitelist #7208

Conversation

kumavis commented Sep 23, 2019 • edited Loading

metamaskbot commented Sep 23, 2019

kumavis commented Sep 23, 2019

Gudahtt commented Sep 23, 2019

kumavis commented Sep 23, 2019

kumavis commented Sep 23, 2019

kumavis commented Sep 23, 2019

kumavis commented Sep 23, 2019

kumavis commented Sep 23, 2019

metamaskbot commented Sep 23, 2019

kumavis commented Sep 23, 2019

kumavis commented Sep 23, 2019

metamaskbot commented Sep 23, 2019

kumavis commented Sep 23, 2019

metamaskbot commented Sep 25, 2019

Gudahtt left a comment

Choose a reason for hiding this comment

kumavis commented Sep 23, 2019 •

edited

Loading