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.

Sign up for GitHub

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

Jump to bottom

[PM-5717] Fix calling methods on undefined in biometrics service #7559

Merged
merged 6 commits into from
Jan 26, 2024
Merged

[PM-5717] Fix calling methods on undefined in biometrics service #7559

merged 6 commits into from
Jan 26, 2024

Conversation

djsmith85
Copy link
Contributor

Type of change

- [X] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Reading a bug report on GH, I saw some console output where the biometrics service was not being able to call init.

(node:71453) UnhandledPromiseRejectionWarning: TypeError: Cannot read properties of undefined (reading 'init')
    at BiometricsService.<anonymous> (/snap/bitwarden/103/resources/app.asar/main.js:49684:55)
    at Generator.next (<anonymous>)
    at /snap/bitwarden/103/resources/app.asar/main.js:49650:71
    at new Promise (<anonymous>)
    at biometrics_service_awaiter (/snap/bitwarden/103/resources/app.asar/main.js:49646:12)
    at BiometricsService.init (/snap/bitwarden/103/resources/app.asar/main.js:49683:16)
    at Main.<anonymous> (/snap/bitwarden/103/resources/app.asar/main.js:55950:46)
    at Generator.next (<anonymous>)
    at fulfilled (/snap/bitwarden/103/resources/app.asar/main.js:55838:58)
    at emitUnhandledRejectionWarning (node:internal/process/promises:200:15)
    at processPromiseRejections (node:internal/process/promises:296:13)
    at process.processTicksAndRejections (node:internal/process/task_queues:96:32)
(node:71453) TypeError: Cannot read properties of undefined (reading 'init')
    at BiometricsService.<anonymous> (/snap/bitwarden/103/resources/app.asar/main.js:49684:55)
    at Generator.next (<anonymous>)
    at /snap/bitwarden/103/resources/app.asar/main.js:49650:71
    at new Promise (<anonymous>)
    at biometrics_service_awaiter (/snap/bitwarden/103/resources/app.asar/main.js:49646:12)
    at BiometricsService.init (/snap/bitwarden/103/resources/app.asar/main.js:49683:16)
    at Main.<anonymous> (/snap/bitwarden/103/resources/app.asar/main.js:55950:46)
    at Generator.next (<anonymous>)
    at fulfilled (/snap/bitwarden/103/resources/app.asar/main.js:55838:58)

Curious to why this might be happening, I had a look at the biometrics-service. The issue, is that we currently do not support biometrics on Linux and no platform-specific instance exists. So we try calling init on undefined.

Code changes

  • Fix calling init() on undefined in biometrics.service.ts - d6ca296
  • Add guard on osSupportsBiometric - e879a35

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

@djsmith85 djsmith85 requested a review from a team as a code owner January 15, 2024 23:06
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Jan 15, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 15, 2024 •
edited
Loading

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (c0e1576) 63.42% compared to head (e0a2f15) 61.04%.

❗ Current head e0a2f15 differs from pull request most recent head 5e878c0. Consider uploading reports for the commit 5e878c0 to get more accurate results

Files Patch % Lines
...src/platform/main/biometric/biometric.noop.main.ts 14.28% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7559      +/-   ##
==========================================
- Coverage   63.42%   61.04%   -2.39%     
==========================================
  Files         797      793       -4     
  Lines       22505    23219     +714     
  Branches     4489     4660     +171     
==========================================
- Hits        14274    14174     -100     
- Misses       7400     8188     +788     
- Partials      831      857      +26

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

willmartian
willmartian previously approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@willmartian willmartian left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks!

@djsmith85
Copy link
Contributor Author

@willmartian: QA found some more calls that failed, due to not being implemented.

Instead of using guard-clauses on each method, I've decided to create a NoopBiometricsService. This makes it easier to implement and replace once biometrics on Linux are supported. It also future-proof the current scenario in case any public methods are added, they'll need to be implemented on the noop too.

@djsmith85
Merge branch 'main' of https://github.com/bitwarden/clients into ps/p…
e0a2f15 
…m-5717/fix-calling-init-on-undefined-in-biometrics-service
@bitwarden-bot
Copy link

bitwarden-bot commented Jan 24, 2024 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – 82863e2b-edc7-4c70-a9e3-fad240751e24

Fixed Issues

Severity Issue Source File / Package
HIGH Client_DOM_Code_Injection /apps/browser/src/autofill/services/collect-autofill-content.service.ts: 1021
HIGH Client_DOM_Stored_XSS /apps/web/src/connectors/duo-redirect.ts: 23
HIGH Client_DOM_Stored_XSS /apps/web/src/connectors/sso.ts: 33
HIGH Client_DOM_XSS /apps/web/src/connectors/common.ts: 2
HIGH Client_DOM_XSS /apps/web/src/connectors/common.ts: 2
HIGH Client_DOM_XSS /apps/web/src/connectors/common.ts: 2
HIGH Client_DOM_XSS /apps/web/src/connectors/sso.ts: 21
HIGH Client_DOM_XSS /apps/web/src/connectors/sso.ts: 19
HIGH Client_DOM_XSS /apps/web/src/connectors/sso.ts: 15
HIGH Missing User Instruction /Dockerfile: 1
MEDIUM Apt Get Install Pin Version Not Defined /Dockerfile: 5
MEDIUM Apt Get Install Pin Version Not Defined /Dockerfile: 5
MEDIUM Client_Potential_XSS /libs/components/src/avatar/avatar.component.ts: 48
MEDIUM Client_Potential_XSS /apps/desktop/src/app/components/avatar.component.ts: 41
MEDIUM Client_Privacy_Violation /apps/browser/src/auth/popup/account-switching/account.component.ts: 12
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 176
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 175
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 174
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 181
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 262
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 254
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 256
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 255
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 93
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 86
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 88
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 87
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/accounts/trial-initiation/billing.component.html: 41
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/accounts/trial-initiation/billing.component.html: 41
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/accounts/trial-initiation/billing.component.html: 41
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/accounts/trial-initiation/billing.component.html: 41
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/accounts/trial-initiation/billing.component.html: 19
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/accounts/trial-initiation/billing.component.html: 19
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/accounts/trial-initiation/billing.component.html: 19
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/accounts/trial-initiation/billing.component.html: 19
MEDIUM Client_Privacy_Violation /bitwarden_license/bit-web/src/app/auth/sso/sso.component.ts: 161
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/reports/pages/breach-report.component.html: 14
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/shared/add-credit.component.html: 46
MEDIUM Client_Privacy_Violation /apps/desktop/src/vault/app/vault/view.component.html: 534
MEDIUM Client_Privacy_Violation /apps/desktop/src/auth/lock.component.html: 32
MEDIUM Client_Privacy_Violation /apps/browser/src/background/runtime.background.ts: 311
MEDIUM Client_Privacy_Violation /bitwarden_license/bit-web/src/app/auth/sso/sso.component.ts: 161
MEDIUM Client_Privacy_Violation /apps/web/src/connectors/webauthn-fallback.ts: 114
MEDIUM Client_Privacy_Violation /apps/desktop/native-messaging-test-runner/src/commands/bw-credential-update.ts: 44
MEDIUM Client_Privacy_Violation /apps/desktop/native-messaging-test-runner/src/commands/bw-credential-update.ts: 44
MEDIUM Client_Privacy_Violation /apps/desktop/native-messaging-test-runner/src/commands/bw-credential-create.ts: 50
MEDIUM Client_Privacy_Violation /libs/components/src/color-password/color-password.component.ts: 14
MEDIUM Client_Privacy_Violation /libs/components/src/color-password/color-password.component.ts: 14
MEDIUM Client_Privacy_Violation /apps/web/src/app/auth/recover-two-factor.component.html: 37
MEDIUM Client_Privacy_Violation /apps/web/src/app/auth/lock.component.html: 18
MEDIUM Client_Privacy_Violation /apps/desktop/src/vault/app/vault/view.component.html: 60
MEDIUM Client_Privacy_Violation /apps/desktop/src/vault/app/vault/view.component.html: 56
MEDIUM Client_Privacy_Violation /apps/browser/src/tools/popup/generator/password-generator-history.component.html: 26
MEDIUM Client_Privacy_Violation /apps/browser/src/vault/popup/components/vault/password-history.component.html: 18
MEDIUM Client_Privacy_Violation /apps/desktop/src/app/tools/password-generator-history.component.html: 15
MEDIUM Client_Privacy_Violation /apps/desktop/src/vault/app/vault/password-history.component.html: 12
MEDIUM Client_Privacy_Violation /apps/desktop/src/vault/app/vault/view.component.html: 50
MEDIUM Container Traffic Not Bound To Host Interface /docker-compose.yml: 7
MEDIUM Host Namespace is Shared /docker-compose.yml: 4
MEDIUM Image Version Not Explicit /Dockerfile: 1
MEDIUM Memory Not Limited /docker-compose.yml: 4
MEDIUM Missing_HSTS_Header /apps/cli/src/auth/commands/login.command.ts: 700
MEDIUM Networks Not Set /docker-compose.yml: 4
MEDIUM Privacy_Violation /apps/desktop/native-messaging-test-runner/src/commands/bw-credential-update.ts: 44
MEDIUM Privacy_Violation /apps/desktop/native-messaging-test-runner/src/commands/bw-credential-update.ts: 44
MEDIUM Security Opt Not Set /docker-compose.yml: 4
MEDIUM Unpinned Actions Full Length Commit SHA /crowdin-pull.yml: 35
MEDIUM Unpinned Actions Full Length Commit SHA /build-cli.yml: 408
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: 1163
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 122
MEDIUM Unpinned Actions Full Length Commit SHA /build-web.yml: 308
MEDIUM Unpinned Actions Full Length Commit SHA /deploy-web.yml: 151
MEDIUM Unpinned Actions Full Length Commit SHA /deploy-web.yml: 170
MEDIUM Unpinned Actions Full Length Commit SHA /crowdin-pull.yml: 41
MEDIUM Unpinned Actions Full Length Commit SHA /staged-rollout-desktop.yml: 29
MEDIUM Unpinned Actions Full Length Commit SHA /deploy-web.yml: 131
MEDIUM Unpinned Actions Full Length Commit SHA /release-cli.yml: 153
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: 1242
MEDIUM Unpinned Actions Full Length Commit SHA /release-web.yml: 147
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop.yml: 70
MEDIUM Unpinned Actions Full Length Commit SHA /release-browser.yml: 106
MEDIUM Unpinned Actions Full Length Commit SHA /build-web.yml: 202
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop-beta.yml: 51
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: 299
MEDIUM Unpinned Actions Full Length Commit SHA /release-web.yml: 44
MEDIUM Unpinned Actions Full Length Commit SHA /release-web.yml: 133
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop-beta.yml: 248
MEDIUM Unpinned Actions Full Length Commit SHA /release-cli.yml: 60
MEDIUM Unpinned Actions Full Length Commit SHA /release-cli.yml: 81
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop.yml: 188
MEDIUM Unpinned Actions Full Length Commit SHA /build-web.yml: 278
MEDIUM Unpinned Actions Full Length Commit SHA /brew-bump-desktop.yml: 26
MEDIUM Unpinned Actions Full Length Commit SHA /build-browser.yml: 432
MEDIUM Unpinned Actions Full Length Commit SHA /brew-bump-cli.yml: 26
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 44
MEDIUM Unpinned Actions Full Length Commit SHA /build-web.yml: 376
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop.yml: 113
MEDIUM Unpinned Actions Full Length Commit SHA /release-desktop.yml: 126
MEDIUM Unpinned Actions Full Length Commit SHA /release-browser.yml: 44
MEDIUM Unpinned Actions Full Length Commit SHA /version-bump.yml: 129
MEDIUM Unpinned Actions Full Length Commit SHA /build-browser.yml: 370
LOW Angular_Usage_of_Unsafe_DOM_Sanitizer /libs/components/src/icon/icon.component.ts: 18
LOW Angular_Usage_of_Unsafe_DOM_Sanitizer /libs/components/src/avatar/avatar.component.ts: 78
LOW Angular_Usage_of_Unsafe_DOM_Sanitizer /apps/desktop/src/app/components/avatar.component.ts: 71
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/common.ts: 2
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/common.ts: 2
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/sso.ts: 21
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/common.ts: 2
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/sso.ts: 19
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/common.ts: 2
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/sso.ts: 15
LOW Client_DOM_Open_Redirect /libs/common/src/auth/iframe-component.ts: 49
LOW Client_DOM_Open_Redirect /apps/browser/src/auth/scripts/duo.js: 277
LOW Client_DOM_Open_Redirect /apps/desktop/src/auth/scripts/duo.js: 277
LOW Client_DOM_Open_Redirect /libs/common/src/auth/webauthn-iframe.ts: 25
LOW Client_DOM_Open_Redirect /apps/desktop/src/auth/scripts/duo.js: 277
LOW Client_DOM_Open_Redirect /apps/browser/src/auth/scripts/duo.js: 277
LOW Client_Hardcoded_Domain /apps/web/src/connectors/captcha.ts: 55
LOW Client_Password_In_Comment /apps/web/src/app/auth/key-rotation/user-key-rotation.service.ts: 42
LOW Client_Password_In_Comment /apps/browser/src/autofill/overlay/pages/list/autofill-overlay-list.ts: 341
LOW Client_Password_In_Comment /apps/browser/src/autofill/overlay/pages/list/autofill-overlay-list.ts: 275
LOW Client_Password_In_Comment /apps/browser/src/autofill/overlay/pages/list/autofill-overlay-list.ts: 388
LOW Client_Password_In_Comment /apps/browser/src/autofill/overlay/pages/list/autofill-overlay-list.ts: 417
LOW Client_Password_In_Comment /apps/browser/src/autofill/background/overlay.background.ts: 216
LOW Client_Password_In_Comment /apps/browser/src/autofill/overlay/pages/list/autofill-overlay-list.ts: 438
LOW Client_Password_In_Comment /apps/browser/src/autofill/overlay/pages/list/autofill-overlay-list.ts: 225
LOW Client_Password_In_Comment /apps/browser/src/autofill/overlay/pages/list/autofill-overlay-list.ts: 247
LOW Client_Password_In_Comment /apps/browser/src/autofill/background/overlay.background.ts: 548
LOW Client_Password_In_Comment /apps/browser/src/autofill/overlay/pages/list/autofill-overlay-list.ts: 456
LOW Client_Password_In_Comment /apps/browser/src/autofill/overlay/pages/list/autofill-overlay-list.ts: 320
LOW Client_Password_In_Comment /libs/angular/src/auth/components/sso.component.ts: 212
LOW Client_Password_In_Comment /apps/browser/src/autofill/services/autofill.service.ts: 393
LOW Client_Password_In_Comment /apps/web/src/app/vault/org-vault/vault.component.ts: 631
LOW Client_Password_In_Comment /apps/web/src/app/vault/individual-vault/vault.component.ts: 609
LOW Client_Password_In_Comment /libs/angular/src/auth/components/sso.component.ts: 236
LOW Client_Password_In_Comment /libs/angular/src/auth/components/two-factor.component.ts: 266
LOW Client_Password_In_Comment /libs/exporter/src/vault-export/bitwarden-json-export-types.ts: 40
LOW Client_Password_In_Comment /apps/web/src/app/vault/individual-vault/add-edit.component.ts: 121
LOW Client_Password_In_Comment /libs/importer/src/importers/onepassword/onepassword-1pux-importer.ts: 336
LOW Client_Password_In_Comment /libs/common/src/auth/login-strategies/password-login.strategy.ts: 130
LOW Client_Password_In_Comment /libs/angular/src/vault/components/add-edit.component.ts: 250
LOW Client_Password_In_Comment /libs/common/src/auth/services/user-verification/user-verification.service.ts: 90
LOW Client_Password_In_Comment /libs/importer/src/importers/bitwarden/bitwarden-password-protected-importer.ts: 51
LOW Client_Password_In_Comment /apps/web/src/app/auth/settings/emergency-access/attachments/emergency-access-attachments.component.ts: 49
LOW Client_Password_In_Comment /libs/importer/spec/test-data/roboform-csv/with-folders.ts: 2
LOW Client_Password_In_Comment /libs/importer/spec/test-data/roboform-csv/empty-folders.ts: 2
LOW Client_Password_In_Comment /libs/common/src/admin-console/enums/policy-type.enum.ts: 10
LOW Client_Use_Of_Iframe_Without_Sandbox /apps/browser/src/autofill/overlay/iframe-content/autofill-overlay-iframe.service.ts: 88
LOW Client_Use_Of_Iframe_Without_Sandbox /apps/browser/src/autofill/content/notification-bar.ts: 873
LOW Client_Use_Of_Iframe_Without_Sandbox /apps/web/src/connectors/duo.ts: 8
LOW Client_Weak_Cryptographic_Hash /libs/common/src/platform/services/web-crypto-function.service.ts: 142
LOW

More results are available on AST platform

willmartian
willmartian approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@willmartian willmartian left a comment

Choose a reason for hiding this comment

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

Great refactor, thank you.

@djsmith85 djsmith85 removed the needs-qa Marks a PR as requiring QA approval label Jan 26, 2024
@djsmith85 djsmith85 enabled auto-merge (squash) January 26, 2024 16:15
@djsmith85 djsmith85 merged commit 53be494 into main Jan 26, 2024
18 of 19 checks passed
@djsmith85 djsmith85 deleted the ps/pm-5717/fix-calling-init-on-undefined-in-biometrics-service branch January 26, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willmartian willmartian willmartian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@djsmith85 @bitwarden-bot @willmartian