-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[$250] Fix security vulnerabilities in Expensify App #48327
Comments
|
@CyberAndrii wow, nice. So does it mean when your PR to |
Yes I guess. Thanks for pointing this out |
Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Hey I’m Szymon from Callstack - can take this over from Hur! |
📣 @szymonrybczak! 📣
|
Ty. Can you update the PR with latest main? |
#48532 was merged, and currently, there are no security vulnerabilities on
|
@grgia PR went to prod 2 weeks ago, can you add a BZ to handle payment here? Ty |
Triggered auto assignment to @zanyrenney ( |
Job added to Upwork: https://www.upwork.com/jobs/~021843732810666984794 |
Current assignee @hungvu193 is eligible for the External assigner, not assigning anyone new. |
Contributor+: @hungvu193 due $250 via NewDot. Do we need/want any regression tests here? My gut says no but I'm new to the issue so erring on the safe side by asking. |
Yes, I don't think we need regression test here, as we only see the security vulnerabilities through the command line. |
i agree we don't need a regression test here. |
Payment summary $250 owed to @hungvu193 for this work. @hungvu193 is paid via ND requests so closing this issue and they can request payment via ND. Thanks! |
@zanyrenney for auditing purposes, so we know who's paid/due payment and where they're paid, can you plz use the format from the payment SO? Thx.
|
$250 approved for @hungvu193 |
Problem
Currently in Expensify App we have around 34 vulnerabilities in our dependencies of Critical, High and Moderate priorities. Even though, our App works fine but there's always a chance of some exploit in the future. Since Expensify is a FinTech App, the security vulnerabilities should be avoided and mitigated as much as possible.
Some examples of the vulnerabilities found via
npm audit
are:Solution
The links for the vulnerabilities also suggests the version that we can bump for that dependency in order to fix the vulnerability. Since most of the vulnerabilities are originating from transitive dependencies, we can use
overrides
property inpackage.json
to use that pinned version for the specific dependency. For example, considerws
transitive dependency which can be bumped to8.17.1
in order to fix the vulnerability. Below is how we can achieve this:Once we add the patched versions of the security vulnerabilities in
overrides
property, we need to do a QA to make sure there are no regressions introduced by the patched versions.Apart from this, upgrading to newer versions can sometimes be useful as they come with bug fixes and performance improvements. For example,
react-pdf
comes with lots of improvements like optimized CPU and memory usage, see here. There's one edge case that I am not aware of and would like to discuss:pdfjs-dist/legacy/build
instead we havepdfjs-dist/build
asreact-pdf
has dropped support for older browsers. Inreact-fast-pdf
, which is maintained byExpensify
here and in Expensfiy App, we have references topdfjs-dist/legacy/build
, which we can just change topdfjs-dist/build
. After this change, on my testing, I was able to send and view the PDF correctly, so everything looks correct but a QA in this area might come handy.Edit:
react-pdf
update is already being tracked here. We have to make sure to verify updates toreact-fast-pdf
Edit: I missed it the first time but we do have
pdfjs-dist/legacy/build
but we have.mjs
instead of.js
, so we still have to updatereact-fast-pdf
to include the.mjs
extension.Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @The text was updated successfully, but these errors were encountered: