npm audit fix - Githubissues

DAVFoundation / dav-js

Enable integration of JavaScript, TypeScript, and Node.js code with the DAV Network

https://developers.dav.network/

MIT License

76 stars 51 forks source link

npm audit fix #135

Closed debragail closed 5 years ago

debragail commented 5 years ago

Updated package.json to fix security vulnerabilities.

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[x ] Enhancement that improves upon existing functionality

Checklist:

[x ] My code follows the code style of this project.
[x ] My change requires a change to the documentation.
[x ] I have read the CONTRIBUTING document.
[x ] I have added tests to cover my changes.
[x ] All new and existing tests passed.

mariolo1985 commented 5 years ago

Hi @debragail thanks for finding and fixing these vulnerabilities. It looks like writing Gulp tasks has changed. Running the test script npm run jest and other build scripts are not completing. Could you resolve these related to the upgrade? Thank you.

debragail commented 5 years ago

@mariolo1985 I believe I need to roll back my npm audit fix changes because they negate the audit that was done on the source code. Is that correct @TalAter ?

TalAter commented 5 years ago

@debragail Not in this case as your commit doesn't touch any of the audited files (anything that has to do with DAVToken or DAVCrowdsale).

However the changes you introduced here don't work. We cannot trust npm audit fix blindly as it sometime updates major versions of packages which introduce breaking bugs. This is the case here with gulp version 4 which introduces breaking changes to the gulp API. You can see how I handled this in another project: https://github.com/DAVFoundation/missioncontrol/commit/5576ed81abe9c79e72b181ece1c210cbf6b6b045

mariolo1985 commented 5 years ago

hey @debragail you can probably keep the upgrade as-is and handle the tasks as @TalAter has suggested.