Skip to content
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

Downgrade onfido UI version to fix vulnerability #14414

Merged
merged 1 commit into from
Jan 21, 2023

Conversation

tylerkaraszewski
Copy link
Contributor

@tylerkaraszewski tylerkaraszewski commented Jan 19, 2023

Details

Followup to here: #14353
That PR was accidentally merged with a failing security test. It specified onfido version 10.3.0, but too loosely. This change is just to specify the version more tightly.

Fixed Issues

$ #13262
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Tests copied from the original PR:

  1. Open a workspace
  2. Navigate to the "Connect Bank Account" step
  3. Complete all necessary steps until you reach the personal information page (choose "connect manually")
  4. Look for the "issuing country" dropdown menu
  5. Verify that the list of countries in the dropdown is visible.

Note:
For IOS and Android native, the screen that shows the offending dropdown box is skipped, and thus this change (and bug) have no effect there.

  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
web.mov
Mobile Web - Chrome
android-web.mov
Mobile Web - Safari
ios-web.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov

@tylerkaraszewski tylerkaraszewski requested a review from a team as a code owner January 19, 2023 19:52
@melvin-bot melvin-bot bot requested review from PauloGasparSv and removed request for a team January 19, 2023 19:53
@melvin-bot
Copy link

melvin-bot bot commented Jan 19, 2023

@PauloGasparSv 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]

@tylerkaraszewski tylerkaraszewski self-assigned this Jan 19, 2023
@tylerkaraszewski tylerkaraszewski changed the title [WIP] Downgrade onfido UI version to fix vulnerability Downgrade onfido UI version to fix vulnerability Jan 19, 2023
@PauloGasparSv
Copy link
Contributor

PauloGasparSv commented Jan 19, 2023

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Web.mov
Mobile Web - Chrome
Android.Web.mov
Mobile Web - Safari
ios.Web.mov
Desktop Desktop
iOS
ios.mov
Android
Android.mov

@PauloGasparSv
Copy link
Contributor

Hey @tylerkaraszewski will start testing now.
I think you are missing the Web evidences, but that's ok by me!

@tylerkaraszewski
Copy link
Contributor Author

@PauloGasparSv - added the web recording. I must have just forgot to attach it.

@PauloGasparSv
Copy link
Contributor

PauloGasparSv commented Jan 19, 2023

Also @tylerkaraszewski , I'm a bit confused with the test steps and the evidences. Do I need to go to the page with the country dropdown or can I just test if the Onfido screen opens correctly?

I'm not sure how to get to the country dropdown page, can you help me on that?

@tylerkaraszewski
Copy link
Contributor Author

yes, you need to go to the page with the country dropdown.

Here's a video showing all the steps from the beginning:

full.mov

Copy link
Contributor

@PauloGasparSv PauloGasparSv left a comment

Choose a reason for hiding this comment

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

LGTM so I'm already approving.

Couldn't get to the Country page on the iOS and Android native tests but I checked and both this P.R. and the original only went to the Onfido camera screen so I did the same!

I'm still adding evidence for Android mWeb, for some reason it is failing on all my emulators saying the Chrome version is outdated so I'm creating a new device to test

image

@PauloGasparSv
Copy link
Contributor

@tylerkaraszewski can you test this on Chrome mobile Android? Spent some time on it but I still can't get to the country page there

image

@syedsaroshfarrukhdot
Copy link
Contributor

syedsaroshfarrukhdot commented Jan 20, 2023

@tylerkaraszewski can you test this on Chrome mobile Android? Spent some time on it but I still can't get to the country page there

image

@PauloGasparSv Please refer to this comment here

#13262 (comment)

You need to run 'adb reverse tcp:8080 tcp:8080'

And access it using localhost:8080 on mWeb/Chrome to test it locally

@PauloGasparSv
Copy link
Contributor

Thks @tylerkaraszewski! I only looked in the original P.R. but not the issue for help on that, will try it here!

@PauloGasparSv PauloGasparSv merged commit e43bab5 into main Jan 21, 2023
@PauloGasparSv PauloGasparSv deleted the tyler-fix-onfido-version branch January 21, 2023 19:47
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
Open Search Page TTI 601.251 ms → 610.991 ms (+9.739 ms, +1.6%)
App start TTI 686.388 ms → 693.701 ms (+7.313 ms, +1.1%)
App start nativeLaunch 19.586 ms → 21.625 ms (+2.039 ms, +10.4%)
App start regularAppStart 0.014 ms → 0.015 ms (+0.001 ms, +9.3%)
App start runJsBundle 193.313 ms → 192.063 ms (-1.250 ms, -0.6%)
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 601.251 ms
Stdev: 19.669 ms (3.3%)
Runs: 559.5275480002165 574.0209150016308 575.9753820002079 579.266561999917 580.7929280009121 581.8709310013801 584.628824999556 586.8732090014964 587.2351890001446 589.5702320002019 589.7030849996954 593.2021479997784 593.437093000859 595.4539800006896 600.0863040015101 601.7119960002601 602.8954680003226 603.2859300002456 604.2903239987791 606.0627450011671 606.5650639999658 608.422933999449 608.6213380005211 612.3844810016453 614.5413009990007 615.7126470003277 619.9941819999367 623.8056239988655 625.6907150000334 627.640746999532 627.8371179997921 658.9362789988518

Current
Mean: 610.991 ms
Stdev: 22.155 ms (3.6%)
Runs: 572.0616870000958 576.7290859986097 577.2186279986054 583.0830490011722 588.1669110003859 588.6169029995799 589.0019949991256 597.2291260007769 597.737997001037 599.9610190000385 599.96883200109 602.4690350014716 602.596883000806 606.7678229995072 607.1226400006562 610.5860199984163 611.3001310005784 611.3755290005356 613.7204180005938 614.813354998827 618.2368169985712 618.5213619992137 618.7379960007966 619.2732349988073 621.4278159998357 628.2645670007914 629.7452799994498 640.8873290009797 644.9132899995893 646.0225419998169 648.1321220006794 667.0148120000958
App start TTI Baseline
Mean: 686.388 ms
Stdev: 23.645 ms (3.4%)
Runs: 643.0929790008813 644.413327999413 653.0569119993597 657.9973269999027 660.5890670008957 662.8951309993863 664.3800499998033 669.4392409995198 671.3780940007418 671.629247000441 672.1645270008594 676.7286339998245 679.0734400004148 680.5979520007968 682.2119360007346 686.6696579996496 687.0796089991927 689.7457289993763 693.0202730000019 694.774443000555 694.8452470004559 700.7213780004531 701.3774069994688 702.5465469993651 710.8539070002735 710.9317150004208 716.0553789995611 721.9502440001816 722.3383120000362 727.4832349997014 727.9831489995122

Current
Mean: 693.701 ms
Stdev: 32.728 ms (4.7%)
Runs: 650.4183399993926 651.4842959996313 651.6601999998093 652.8013480007648 654.6350180003792 662.473384000361 663.4569680001587 669.6282770000398 669.865993000567 673.7827560007572 674.9073329996318 676.5420750007033 676.9867339991033 680.5136270001531 682.0042209997773 685.1752690002322 687.8136330004781 695.4232100006193 695.7568399999291 703.6198549997061 703.9968270007521 704.5196509994566 716.2345039993525 719.2577219996601 722.5665160007775 724.872532999143 741.5924999993294 748.9478489998728 749.8477880004793 755.608703000471 758.3447660002857
App start nativeLaunch Baseline
Mean: 19.586 ms
Stdev: 1.274 ms (6.5%)
Runs: 17 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 21 21 21 21 22 22 22

Current
Mean: 21.625 ms
Stdev: 2.945 ms (13.6%)
Runs: 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 21 21 21 22 22 23 23 23 24 24 24 24 25 26 27 27 29
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.9%)
Runs: 0.012492001056671143 0.012653999030590057 0.013020999729633331 0.013062000274658203 0.013101998716592789 0.013183999806642532 0.013387000188231468 0.013508999720215797 0.013549000024795532 0.01355000026524067 0.013671999797224998 0.013672001659870148 0.013996999710798264 0.014037998393177986 0.014038000255823135 0.01407800056040287 0.014078998938202858 0.014159999787807465 0.014201000332832336 0.014281999319791794 0.0143630001693964 0.014649000018835068 0.014649000018835068 0.01469000056385994 0.01476999931037426 0.014932999387383461 0.015015000477433205 0.015095999464392662 0.015380999073386192 0.015420999377965927 0.01603199914097786

Current
Mean: 0.015 ms
Stdev: 0.001 ms (5.0%)
Runs: 0.013712000101804733 0.014403998851776123 0.014444999396800995 0.014485999941825867 0.014649000018835068 0.014689000323414803 0.014689000323414803 0.014973999932408333 0.015013998374342918 0.01517699845135212 0.015177000313997269 0.015177000313997269 0.015217998996376991 0.01521800085902214 0.015259001404047012 0.015300000086426735 0.01533999852836132 0.01534000039100647 0.015421001240611076 0.015543000772595406 0.015869000926613808 0.01590999960899353 0.015991000458598137 0.01603100076317787 0.01607299968600273 0.016234999522566795 0.016316000372171402 0.016478998586535454 0.01664300076663494 0.017375001683831215
App start runJsBundle Baseline
Mean: 193.313 ms
Stdev: 20.371 ms (10.5%)
Runs: 161 164 164 167 167 168 169 174 181 182 182 186 188 188 188 191 197 198 198 200 201 203 203 205 206 210 215 218 221 225 228 238

Current
Mean: 192.063 ms
Stdev: 24.386 ms (12.7%)
Runs: 158 160 163 168 169 170 171 172 173 175 177 177 182 185 186 186 188 188 191 194 194 195 198 202 212 216 217 217 224 242 243 253

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/PauloGasparSv in version: 1.2.58-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.2.58-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants