Security: npm upgrade packages #22

[sibasankarnayak]

security NPM package upgraded

ticket here https://github.com/mattermost/mattermost-plugin-bitbucket/issues/22

[jfrerich]

Hi, @sibasankar-demansol, would you mind sharing the command you ran to make these changes?

[sibasankarnayak]

Hi, @sibasankar-demansol, would you mind sharing the command you ran to make these changes?

Hi @jfrerich these are the packages which are packages which are not directly used by us and if we want to do it we need to fork the repo and make changes into that , Seems those packages will be handling these issue , to ensure it from our side we tried to install the latest version or the version mentioned to use , sharing those below.

1. npm i elliptic@6.5.4
2. npm i node-fetch@2.6.1
   3. npm i y18n@5.0.5
   4. npm i yargs-parser@18.1.1
   5. npm i ini@2.0.0

[mickmister]

@jfrerich Note that these updates were made with npm 7+, which is why the diff is so big. @sibasankar-demansol Can you please redo the PR with npm 6 running locally? This way the PR's size is appropriate for the purpose of the PR.

[sibasankarnayak]

@jfrerich Note that these updates were made with npm 7+, which is why the diff is so big. @sibasankar-demansol Can you please redo the PR with npm 6 running locally? This way the PR's size is appropriate for the purpose of the PR.

@mickmister can you check now , have tried with npm version 6.14.14

[mickmister]

@jfrerich What command do you usually use when you perform the security updates?

[jfrerich]

@mickmister, I run npm-audit fix for the security updates

[mickmister]

@sibasankar-demansol Sorry about this, but can you start from a clean state again, then run npm-audit fix to automatically make the changes?

[sibasankarnayak]

@sibasankar-demansol Sorry about this, but can you start from a clean state again, then run npm-audit fix to automatically make the changes?

@mickmister Sure, have done with command npm audit fix and updated the PR

[sibasankarnayak]

Actually, the dependencies with vulnerabilities were not updated. The original approach taken in this PR (manually re-installing each of those packages) is undesired because it clutters package.json with dependencies that are not specific to this project. This makes it harder to keep the plugin projects in sync with each other.

Can you try running npm update to see if this fixes this, or rm -rf node_modules && rm package-lock.json && npm i?

Sure let me Try both once and update you

[sibasankarnayak]

Actually, the dependencies with vulnerabilities were not updated. The original approach taken in this PR (manually re-installing each of those packages) is undesired because it clutters package.json with dependencies that are not specific to this project. This makes it harder to keep the plugin projects in sync with each other. Can you try running npm update to see if this fixes this, or rm -rf node_modules && rm package-lock.json && npm i?

Sure let me Try both once and update you

@mickmister Tried both the solution ,

1. npm update update the packages which are directly used
2. removing node_modules/* , package-lock.json - reinstall the packages no change

[mickmister]

@sibasankar-demansol Can you try manually updating the package versions in package-lock.json to avoid adding the dependencies to package.json then? @jfrerich Is there a way we can run the security tool again to verify the problem is fixed?

[sibasankarnayak]

@sibasankar-demansol Can you try manually updating the package versions in package-lock.json to avoid adding the dependencies to package.json then? @jfrerich Is there a way we can run the security tool again to verify the problem is fixed?

@mickmister sure we can do that but after changes in package-locak.json we need to run command npm install ?

[mickmister]

@sibasankar-demansol I'd like to try another strategy. Can we prefix all of the dependency version in package.json with ^? For example, instead of "react": "16.8.6",, we would put "react": "^16.8.6". After doing this, can you try running npm update? This would update all of the packages, which is good housekeeping in general, but may break things.

npm update update the packages which are directly used

When you tried this, did this also update the vulnerable packages to the appropriate versions?

[mickmister]

You can also try npm-audit fix --force to see if that solves the issue

[sibasankarnayak]

@sibasankar-demansol I'd like to try another strategy. Can we prefix all of the dependency version in package.json with ^? For example, instead of "react": "16.8.6",, we would put "react": "^16.8.6". After doing this, can you try running npm update? This would update all of the packages, which is good housekeeping in general, but may break things.

npm update update the packages which are directly used

When you tried this, did this also update the vulnerable packages to the appropriate versions?

@mickmister npm update only fixed the direct packages , those packages we want to upgrade are not used directly

i will try npm audit fix --force and will update you in some time

[mickmister]

@mickmister npm update only fixed the direct packages , those packages we want to upgrade are not used directly

This would also update the second level dependencies that change when the direct dependencies are updated though right? I assume our direct dependencies that import those vulnerable ones have updated their version numbers in their projects in more recent versions.

[sibasankarnayak]

You can also try npm-audit fix --force to see if that solves the issue

@mickmister Out of 5 vulnerable with npm audit fix --force we are able to discard 3 packages listed below 1 . ini

2. y18n
3. yargs-parser

where as for these two the parent package is still using the previous version

1. elliptic
2. node-fetch

but it breaks the plugin while deploying the plugin

[sibasankarnayak]

@mickmister npm update only fixed the direct packages , those packages we want to upgrade are not used directly

This would also update the second level dependencies that change when the direct dependencies are updated though right? I assume our direct dependencies that import those vulnerable ones have updated their version numbers in their projects in more recent versions.

even on trying still can see the packages with version having isssue

[mickmister]

@srkgupta Do you have any guidance here to update the versions of the vulnerable npm dependencies in package-lock.json, without affecting the listed packages in package.json?

[srkgupta]

Hi @mickmister

I am afraid you cannot do this, i.e. update the versions of the vulnerable npm dependencies in package-lock.json, without affecting the listed packages in package.json. There might be a minor version bump in the parent package in package.json which fixes the dependent package versions.

If the parent package of the dependency has not upgraded the version of the affected package, then you may try to contact the maintainer of the parent package and inform them to upgrade the affected package dependencies.

[mickmister]

@srkgupta Sorry I was a bit vague in my question. What I meant to ask by without affecting the listed packages in package.json, I meant that we don't want to add the vulnerable packages to package.json. The parent packages that import these packages should indeed have their versions updated though.

See https://github.com/mattermost/mattermost-plugin-bitbucket/pull/38#issuecomment-893183926, which manually updates the vulnerable packages, but results in adding them to package.json, which pollutes the file with packages we don't import.

[srkgupta]

@jupenur do you have any suggestions to tackle this issue, other than forking the repos until they fix it.

@srkgupta Sorry I was a bit vague in my question. What I meant to ask by without affecting the listed packages in package.json, I meant that we don't want to add the vulnerable packages to package.json. The parent packages that import these packages should indeed have their versions updated though.

See #38 (comment), which manually updates the vulnerable packages, but results in adding them to package.json, which pollutes the file with packages we don't import.

[jupenur]

See #38 (comment), which manually updates the vulnerable packages, but results in adding them to package.json, which pollutes the file with packages we don't import.

That's not actually what it does. The change there just installs additional copies of the dependencies at the top level of node_modules -- but the outdated versions at the lower levels are still what end up being used.

AFAIK with npm there is no way of forcing your transitive dependencies to another version, at least not without actually replacing those dependencies manually with a postinstall script or something similarly hacky.

So if your top-level dependencies haven't been updated, there isn't much you can do other than fork them. But the important question here is, are they even affected by any of the vulnerabilities? Just looking at the names of the dependencies, it doesn't seem likely: E.g. does the project use elliptic-curve cryptography for anything? Probably not. Does it pass in arbitrary user input to y18n? Probably not. Does it parse user-provided ini files? I really don't think so.

So this may require some digging into, but most likely none of the dependencies actually need to be updated, or at least you should be able to trim the list a little bit. The npm audit nag is something you probably just have to live with.

[maisnamrajusingh]

@mickmister @jupenur @srkgupta from the comments above it looks like that we can tag this pr as accepted internally at Demansol.Please let me know otherwise

[mickmister]

@maisnamrajusingh @sibasankarnayak I think we can close this PR, as it seems that none of dependencies actually need to be updated. node-fetch is the one package I was thinking we might use the project, but it isn't used in the project. The vulnerability for node fetch described in https://github.com/mattermost/mattermost-plugin-bitbucket/issues/22 doesn't seem like an issue for this project.

[sibasankarnayak]

@mickmister do you mean we can close this PR and Issue with this conclusion