stacks-archive / blockstack-explorer

A block explorer for Blockstack
61 stars 43 forks source link

Fix security alerts #159

Closed diwakergupta closed 4 years ago

diwakergupta commented 5 years ago

I've enabled automated PRs for future security alerts, but we have a bunch of prior ones: https://github.com/blockstack/blockstack-explorer/network/alerts

I could generate PRs for all of them, but would prefer someone from the team go through that dependency by dependency, just to be safe that we're not going to break anything horribly as we update dependencies.

At the very least we should address the critical and high-severity alerts.

@hstove , maybe @kyranjamie can also help.

hstove commented 5 years ago

FYI - I've merged & tested all the automated security fixes on our current staging branch. I'll update this when we merge to master.

diwakergupta commented 5 years ago

Thanks @hstove , reminder to close this out when staging is merged into master.

cc/ @reedrosenbluth

diwakergupta commented 5 years ago

The alerts still show up even after @hstove 's PR was merged: https://github.com/blockstack/blockstack-explorer/network/alerts

@reedrosenbluth would you be able to investigate, and make sure the critical / high-severity imports are addressed? Thanks.

reedrosenbluth commented 5 years ago

@diwakergupta I can't seem to access that link. Can you grant me permission?

hstove commented 5 years ago

I merged all of the automated pull requests before we deployed - the new PRs are from yesterday.

Fixing these security alerts is suprisingly non-trivial, as they're usually dependencies multiple levels down the dependency tree, and there is no way to "hard-lock" sub-dependencies at a certain version. You have to wait for the top-level dependency to update (which sometimes takes time, if you google some of the issues), or manually change the yarn.lock file (which requires creating signatures and manually editing a file that is meant to be automatically generated). You can view the automated PRs to see what that looks like.

I'd recommend we determine which alerts are for dependencies that are only used in local development (which is many of them, like anything relating to ESLint) and timeboxing these alerts one by one to see what can be resolved vs. the amount of impact not updating them could have.

diwakergupta commented 5 years ago

@hstove that sounds reasonable -- I agree we shouldn't spend too much time fixing nested or test-only deps, especially if it requires a lot of work. The automated PRs should do the heavy lifting here and if we're confident in our automated tests catching regressions, hopefully that takes care of most of these.

@reedrosenbluth yeah, @zone117x also ran into the same thing -- apparently it's only visible to repo / org admins. Let me check if there's a better way to expose these alerts to a subset of folks.

diwakergupta commented 5 years ago

Also, to clarify, the PRs @hstove merged and the new ones are all after I enabled automated PRs. We have a few outstanding alerts from before I enabled automated PRs, those are the ones I'm flagging for this. Here's a screenshot of what I see:

Screenshot 2019-11-14 12 26 56
hstove commented 5 years ago

FYI - you can also view these alerts with yarn audit - they use the same source for issues.

reedrosenbluth commented 5 years ago

Seems like Yarn hasn't implemented yarn audit fix yet, like NPM has

https://github.com/yarnpkg/yarn/issues/7075

However, they do have a mechanism for fixing these issues manually: https://yarnpkg.com/lang/en/docs/selective-version-resolutions/