Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

Bump dot-prop and lodash #1893

Merged
merged 3 commits into from
Aug 24, 2020
Merged

Conversation

lucasponce
Copy link
Contributor

@lucasponce lucasponce commented Aug 21, 2020

Address a vulnerability detected by github in the 3rd party libraries.

I've addressed this in k-charted as well kiali/k-charted#110.

I'd require more testing, smoke tests seem ok, but these changes may be good if are tested by anyone else as they can easily introduce side effects.

Copy link
Contributor

@mtho11 mtho11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments...

package.json Outdated
@@ -72,6 +72,7 @@
"cytoscape-dagre": "2.2.2",
"cytoscape-popper": "1.0.7",
"deep-freeze": "0.0.1",
"dot-prop": "^4.2.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should pin to exact version instead of ^

Suggested change
"dot-prop": "^4.2.1",
"dot-prop": "4.2.1",

I don't think this is the correct approach to add this as a direct dependency since the UI doesn't use it directly. It is only used by some other packages:

=> Found "dot-prop@4.2.0"
info Reasons this module exists
   - "snyk#configstore" depends on it
   - Hoisted from "snyk#configstore#dot-prop"
   - Hoisted from "react-scripts#optimize-css-assets-webpack-plugin#cssnano#cssnano-preset-default#postcss-merge-rules#postcss-selector-parser#dot-prop"
   - Hoisted from "react-scripts#optimize-css-assets-webpack-plugin#cssnano#cssnano-preset-default#postcss-minify-selectors#postcss-selector-parser#dot-prop"
   - Hoisted from "react-scripts#optimize-css-assets-webpack-plugin#cssnano#cssnano-preset-default#postcss-merge-longhand#stylehacks#postcss-selector-parser#dot-prop"

And even the indirect use of the library is only for dev build stuff (non-runtime). So I think the better way is to add it to the resolutions clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, thanks for taking a look

Copy link
Contributor

@mtho11 mtho11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lucasponce
Copy link
Contributor Author

@hhovsepy @skondkar this PR should be safe to merge, but would you mind to perform some smoke test on it ?

Bumping dependencies is a way to introduce side effects, probably it's not necessary to run full regressions, just some smoke test in the features to see if there is something odd.

@hhovsepy
Copy link
Contributor

Smoke test pass.

@lucasponce lucasponce merged commit 92142fe into kiali:master Aug 24, 2020
@ghost ghost added this to the v1.23.0 milestone Aug 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants