-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: fix security vulnerabilities #48532
chore: fix security vulnerabilities #48532
Conversation
…y-App into hur/chore/fix-security-vulnerabilities
@@ -185,7 +185,7 @@ | |||
"react-native-web-linear-gradient": "^1.1.2", | |||
"react-native-web-sound": "^0.1.3", | |||
"react-native-webview": "13.8.6", | |||
"react-pdf": "^7.7.3", | |||
"react-pdf": "9.1.0", |
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.
We can keep or discard the changes related to pdf
. The reason is that here we already have a tracker to update react-pdf
. If we decide to keep the changes, the linked issue probably won't have any affect because it will be deployed in the newer versions, so they can do a bump whenever that issue is fixed.
Once that happens we will have to update react-fast-pdf
as well because it depends on react-pdf
and we no longer have pdfjs-dist/legacy/build
instead we have pdfjs-dist/build
. This is also explained in the issue here. I have added a patch for react-fast-pdf
in this PR thinking that we can first get this PR merged with the patch and then follow up on react-fast-pdf
by fixing the issue there and bumping and removing the patch in Expensify App
.
Edit: We do have pdfjs-dist/legacy/build
but we have .mjs
instead of .js
, so we still have to update react-fast-pdf
. I missed it the first time but now I have updated this PR to include legacy
build of pdfjs-dist
just like we have in main.
@hungvu193 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
package.json
Outdated
"braces": "3.0.3", | ||
"yargs": "17.7.2", | ||
"yargs-parser": "21.1.1", | ||
"@expo/config-plugins": "8.0.4", | ||
"ws": "8.17.1", | ||
"react-pdf": "9.1.0", | ||
"micromatch": "4.0.8", | ||
"json5": "2.2.2", | ||
"loader-utils": "2.0.4", | ||
"follow-redirects": "1.15.6", | ||
"fast-xml-parser": "4.4.1", | ||
"express": "4.19.2", | ||
"elliptic": "6.5.7", | ||
"fast-json-patch": "3.1.1", | ||
"webpack": "^5.94.0", | ||
"@blakeembrey/template": "1.2.0" |
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.
May I know why do we need to add these libs?
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.
@hungvu193 These libraries are transitive dependencies. Which means they are already installed in our project. We are just updating them here and nothing more.
Why do we need to update is explained in the linked issue but TL:DR is to fix the security vulnerabilities in these libraries.
When I tried to upload pdf on mSafari, I got this crash, Can you take a look @hurali97 ? Screen.Recording.2024-09-09.at.16.21.00.mov |
…crashes after selecting to upload
@hungvu193 So it was happening because of a missing polyfill for I have pushed a commit which fixes this. Screen.Recording.2024-09-09.at.4.49.16.PM.mov |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid.movAndroid: mWeb ChromeScreen.Recording.2024-09-10.at.13.40.12.moviOS: NativeIOS.moviOS: mWeb Safariios.safari.movMacOS: Chrome / SafariWeb.chrome.movMacOS: DesktopDesktop.mov |
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.
LGTM
We did not find an internal engineer to review this PR, trying to assign a random engineer to #48327 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
@hurali97 We have conflicts here |
…y-App into hur/chore/fix-security-vulnerabilities
@grgia this PR is ready for your review! |
@hungvu193 After pulling in the latest main, I see 28 vulnerabilities added. Should we fix those in this PR or leave as is? |
Hmm I think Yes |
@hungvu193 so I have it fixed now, below is the diff: diff --git a/package.json b/package.json
index 3f685cc1efe..c636a18aaf8 100644
--- a/package.json
+++ b/package.json
@@ -345,11 +345,14 @@
"loader-utils": "2.0.4",
"follow-redirects": "1.15.6",
"fast-xml-parser": "4.4.1",
- "express": "4.19.2",
+ "express": "4.20.0",
"elliptic": "6.5.7",
"fast-json-patch": "3.1.1",
"webpack": "^5.94.0",
- "@blakeembrey/template": "1.2.0"
+ "@blakeembrey/template": "1.2.0",
+ "body-parser": "1.20.3",
+ "path-to-regexp": "0.1.10",
+ "send": "0.19.0"
},
"expo": {
"autolinking": { I will push it shortly 👍 |
Thanks for your quick work 🚀 |
@hungvu193 After pulling in the latest main CI and local both are failing because of: I don't see any changes in my PR which could affect this, so I am looping in @kirillzyusko as I see some commits from him in the history for |
@hurali97 have you tried to lock the version, i. e. change |
Yeah did that but no luck. The error says that the patch is for the previous version then the one installed. Which means that we maybe missed updating the patch to target this newer version?
From git history, |
@hurali97 there wasn't a difference between I tend to think that the reason why it fails is because you have this lock file content: "node_modules/react-compiler-healthcheck": {
"version": "0.0.0",
"resolved": "https://registry.npmjs.org/react-compiler-healthcheck/-/react-compiler-healthcheck-0.0.0.tgz",
"integrity": "sha512-b+WqemUSkGcAag7+xEEl/pVCULXnP8tWsZsqDx8MgCETkaC+yxPzOwpsScVkTiRJ/Z75e+xdAa9W67S/hx6dSw==",
"dev": true
}, While expected content is: "node_modules/react-compiler-healthcheck": {
"version": "0.0.0-experimental-b130d5f-20240625",
"dev": true,
"license": "MIT",
"dependencies": {
"@babel/core": "^7.24.4",
"@babel/parser": "^7.24.4",
"chalk": "4",
"fast-glob": "^3.3.2",
"ora": "5.4.1",
"yargs": "^17.7.2",
"zod": "^3.22.4",
"zod-validation-error": "^3.0.3"
},
"bin": {
"react-compiler-healthcheck": "dist/index.js"
},
"engines": {
"node": "^14.17.0 || ^16.0.0 || >= 18.0.0"
}
}, So you installed the first published version |
@kirillzyusko Oh man ! Thanks I will check this out 🚀 |
50eade2
to
50fcb26
Compare
…y-App into hur/chore/fix-security-vulnerabilities
@hungvu193 All is green now. |
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.
All yours @grgia
Can you merge main please? @hurali97 😄 |
…y-vulnerabilities # Conflicts: # package-lock.json
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/grgia in version: 9.0.37-0 🚀
|
@hungvu193 Can you please share the QA steps for this or full regression suite is enough for this which we normally do |
@m-natarajan Sure.
|
🚀 Deployed to production by https://github.com/grgia in version: 9.0.38-4 🚀
|
Details
This PR fixes the security issues as reported by
npm audit
. More details in the linked issue.Fixed Issues
$ #48327
PROPOSAL: #48327
Tests
Testing Steps:
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-09-04.at.1.33.38.PM.mov
Android: mWeb Chrome
Screen.Recording.2024-09-04.at.1.50.22.PM.mov
iOS: Native
Screen.Recording.2024-09-04.at.1.32.33.PM.mov
iOS: mWeb Safari
Screen.Recording.2024-09-04.at.1.50.39.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2024-09-04.at.1.53.09.PM.mov
MacOS: Desktop
Screen.Recording.2024-09-04.at.1.56.54.PM.mov