-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Chore] Security vulnerability audit #1685
Conversation
…version in sitecore-jss packages; this fixes test run in sitecore-jss-angular
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress 👍
Please, see some questions/comments below
package-lock.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this file is committed by mistake, should be removed
@@ -35,7 +35,7 @@ | |||
"@angular/platform-browser-dynamic": "~15.2.9", | |||
"@angular/router": "~15.2.9", | |||
"@types/jasmine": "^3.4.1", | |||
"@types/node": "^14.14.35", | |||
"@types/node": "^16.11.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version should be bumped to 18, and it's already applied in dev: https://github.com/Sitecore/jss/blob/dev/packages/sitecore-jss-angular/package.json#L38
You can pull latest changes from dev
yarn.lock
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, pull latest changes from dev, I see that the versions here are old. As an example, here is Angular 15 instead 16 that we upgraded in dev
@@ -35,7 +35,9 @@ | |||
"typescript": "~4.7.4" | |||
}, | |||
"resolutions": { | |||
"eslint-plugin-jsx-a11y": "6.7.1" | |||
"eslint-plugin-jsx-a11y": "6.7.1", | |||
"@types/react-native/@types/react": "17.0.34", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you, please, explain the problem here? How does it fix security vulnerabilities? If it's something related to react-native, should it be under sitecore-jss-react-native package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this changes is that the direct dependency @types/react-native was installing its own dependency @types/react at ver 18 and this caused type discrepancies and build failing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern here, is that you are adding it to package.json in the root (it's a monorepo config), so for the end customer, it doesn't change anything, it will only affect local development
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right. i added overrides in react-native's package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yavorsk Should it be removed from here in a such case?
…r dependency issue
…in react-native package.json
@@ -35,7 +35,9 @@ | |||
"typescript": "~4.7.4" | |||
}, | |||
"resolutions": { | |||
"eslint-plugin-jsx-a11y": "6.7.1" | |||
"eslint-plugin-jsx-a11y": "6.7.1", | |||
"@types/react-native/@types/react": "17.0.34", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yavorsk Should it be removed from here in a such case?
@@ -64,8 +64,13 @@ | |||
"ts-jest": "~26.0.0", | |||
"typescript": "~4.3.5" | |||
}, | |||
"resolutions": { | |||
"babel-core": "6.26.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
babel-core is not needed anymore? was it here because of security vulnerability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, babel-core was here before that. I removed it because 'resolutions' work only in root package.json, see https://yarnpkg.com/configuration/manifest : "The resolutions field can only be set at the root of the project, and will generate a warning if used in any other workspace."
Description / Motivation
Fix dependency vulnerabilities.
Testing Details
Types of changes