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

[PM-5744] Adjust Fido2 Content Script Injection to Meet mv3 Requirements #8222

Merged

Conversation

cagonzalezcs
Copy link
Contributor

@cagonzalezcs cagonzalezcs commented Mar 6, 2024

Type of change

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

Objective

The changes in this PR ensure that we can inject the FIDO2 content scripts used for the Passkeys feature in a manner than is allowed within manifest v3. It also addresses a couple of side concerns that ensure we inject our content scripts for this feature in a manner that is respectful of user settings while ensuring we can inject them ASAP.

With these changes, we are introducing a polyfill that facilitates the browser.contentScripts.register method within browsers that do not support this feature (Chrome and Safari). This allows us to dynamically register content scripts such that we can handle injecting the FIDO2 content scripts without any delay, allowing us to more effectively introduce the WebAuthn overrides to facilitate Bitwarden passkeys behavior. With this added polyfill, we can also disable the content scripts entirely for users who do not wish to use the feature. This ensures that we do not inject these scripts at all if the user does not want them injected into a tab.

Another improvement introduced by this PR is the lightening of the page-script.js file that gets injected to facilitate Passkeys behavior. We've reworked the imports used in this script to have the script go from ~650kb to ~35kb after compilation.

Finally, we've implemented a fix that ensures we dynamically inject or disable the FIDO2 content scripts after a user has set/unset the setting for passkeys. This ensures that users do not need to reload their browser in order to use the feature after the setting has been turned on. It also disables the feature in all existing browser tabs if the user has turned the feature off.

Code changes

I'm going to reference files that directly affect the implementation, anything not referenced either represents testing files, abstractions, or other small changes that do not affect the implementation in a significant manner.

  • apps/browser/src/autofill/content/autofill-init.ts: Fixing a small issue here where can get a Extension context invalidated error due to a delayed capture of autofill page content if the user reloads the extension
  • apps/browser/src/background/main.background.ts: Adjusting the implementation of the main background to rename all references to the Fido2Service class such that the referenced the new Fido2Background class used to handle background interactions for Fido2 behavior.
  • apps/browser/src/background/runtime.background.ts: Adjusting the implementation of RuntimeBackground to bring all Fido2 related messages to the Fido2Background. This will centralize expected logic triggered by the message within Fido2Background.
  • apps/browser/src/manifest.json: Removing the set trigger-fido2-content-script-inject.js static content script in favor of the dynamic registration approach we now use. Also adding the webNavigation permission to facilitate dynamic registration of these scripts in all frames.
  • apps/browser/src/manifest.v3.json: Removing the set trigger-fido2-content-script-inject.js static content script in favor of the dynamic registration approach we now use. Also adding the webNavigation permission to facilitate dynamic registration of these scripts in all frames.
  • apps/browser/src/platform/browser/browser-api.register-content-scripts-polyfill.ts: This is a newly introduced polyfill that allows us to dynamically register static content scripts within the extension. We use this to ensure we do not inject the Fido2 content scripts into the tabs of users who do not wish to use the feature. The reason we are incorporating this directly rather than through the npm package is due to the structure of the package. It throws several errors during compilation and when jest testing the implementation. As a result, it makes sense to "fork" the package into our implementation and maintain it ourselves. I've added typing information for this, as well as refactored the implementation to ensure we do not enable it's logic unless necessary.
  • apps/browser/src/platform/browser/browser-api.ts: Adding methods that allow us to dynamically register static content scripts using either the chrome.scripting API, the browser.contentScripts API, or the content script polyfill depending on what is available to use in the browser scope.
  • apps/browser/src/vault/fido2/background/fido2.background.ts: This is a rework of the Fido2Service class that now facilitates injection of the Fido2 content scripts and updating behavior related to Fido2 script injection. This class now handles all Fido2 related extension messages, and triggers behavior as necessary based on those messages.
  • apps/browser/src/vault/fido2/content/content-script.ts: Refactoring the implementation of the core Fido2 content script and ensuring that it can tear down its logic if necessary.
  • apps/browser/src/vault/fido2/content/messaging/messenger.ts: Refactoring small elements in this implementation to ensure that our Fido2 implementation follows the WebAuthn specifications more closely.
  • apps/browser/src/vault/fido2/content/page-script-append.mv2.ts: This is a content script used specifically for manifest v2 which facilitates injection of the fido2 content script through appending the script to the DOM. This approach will not be allowed in mv3, but is the only way to trigger the page-script.js injection in mv2.
  • apps/browser/src/vault/fido2/content/page-script.ts: Refactoring the implementation to ensure that the script can tear down its logic in the case that it needs to do so.
  • apps/browser/webpack.config.js: Updating the webpack implementation to ensure that we conditionally create the content script that appends page-script.js to the DOM.
  • libs/common/src/vault/services/fido2/fido2-client.service.ts: Updating the implementation of isFido2FeatureEnabled to ensure we disqualify usage of the feature as soon as possible.
  • libs/common/src/vault/services/fido2/fido2-utils.ts: Removing the reference to the platform Utils lib and pulling used methods from that lib into this file. This is done to lighten the load of page-script.ts

cagonzalezcs and others added 30 commits February 1, 2024 16:12
…mv2 side script for `lp-suppress-import-download.mv2.ts` if building the extension for mv3
…f the Fileless Importer CSV download supression script
coroiu
coroiu previously approved these changes Apr 17, 2024
Copy link
Contributor

@coroiu coroiu left a comment

Choose a reason for hiding this comment

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

🔥

@cagonzalezcs cagonzalezcs removed the needs-qa Marks a PR as requiring QA approval label Apr 18, 2024
@cagonzalezcs cagonzalezcs enabled auto-merge (squash) April 18, 2024 16:04
@cagonzalezcs cagonzalezcs merged commit 9277465 into main Apr 18, 2024
62 of 65 checks passed
@cagonzalezcs cagonzalezcs deleted the autofill/pm-5744-adjust-fido2-page-script-injection-for-mv3 branch April 18, 2024 16:05
MGibson1 pushed a commit that referenced this pull request Apr 22, 2024
…nts (#8222)

* [PM-5876] Adjust LP Fileless Importer to Suppress Download with DOM Append in Manifest v3

* [PM-5876] Incorporating jest tests for affected logic

* [PM-5876] Fixing jest test that leverages rxjs

* [PM-5876] Updating documentation within BrowserApi.executeScriptInTab

* [PM-5876] Implementing jest tests for the new LP suppress download content scripts

* [PM-5876] Adding a change to webpack to ensure we do not package the mv2 side script for `lp-suppress-import-download.mv2.ts` if building the extension for mv3

* [PM-5876] Implementing changes based on feedback during code review

* [PM-5876] Implementing changes based on feedback during code review

* [PM-5876] Implementing changes based on feedback during code review

* [PM-5876] Implementing changes based on feedback during code review

* [PM-5876] Implementing a configuration to feed the script injection of the Fileless Importer CSV download supression script

* [PM-5744] Adjust injection of `page-script.ts` within FIDO 2 implementation to ensure mv3 compatibility

* [PM-5744] Adjusting structure of manifest.json to clean up implementation and ensure consistency between mv2 and mv3

* [PM-5744] Reverting inclusion of the ConsoleLogService

* [PM-4791] Injected content scripts prevent proper XML file display and disrupt XML responses

* [PM-5744] Adjust FIDO2 content script injection methodology to be compatible with manifest v3

* [PM-5744] Adjusting references to Fido2Service to mirror change of name to Fido2Background

* [PM-5744] Migrating runtime background messages that are associated with Fido2 into Fido2Background

* [PM-5744] Fixing named reference within Fido2Background

* [PM-5744] Migrating all Fido2 messages from the runtime.background.ts script to the fido2.background.ts script

* [PM-5744] Removing unnecessary dependency from runtime background

* [PM-5744] Removing unnecessary dependency from runtime background

* [PM-5744] Reworking how we handle init of Fido2Background

* [PM-5744] Reworking page-script.ts to ensure that it can destory its global values on unload

* [PM-5744] Reworking page-script.ts to ensure that it can destory its global values on unload

* [PM-5744] Implementing separated injection methodology between manifest v2 and v3

* [PM-4791] Adjsuting reference for Fido2 script injection to ensure it only triggers on https protocol types

* [PM-5744] Removing unnecessary message and handling reload of content scripts based on updates on observable

* [PM-5744] Refactoring content-script implementation for fido2

* [PM-5744] Refactoring content-script implementation for fido2

* [PM-5744] Reworking implementation to avoid having multiple contenType checks within the FIDO2 content scripts

* [PM-5744] Re-implementing the messageWithResponse within runtime.background.ts

* [PM-5744] Reverting change to autofill.service.ts

* [PM-5744] Removing return value from runtime.background.ts process message call

* [PM-5744] Reworking how we handle injection of the fido2 page and content script elements

* [PM-5744] Adjusting how we override the navigator.credentials request/reponse structure

* [PM-5744] Working through jest tests for the fido2Background implementation

* [PM-5744] Finalizing jest tests for the Fido2Background implementation

* [PM-5744] Stubbing out jest tests for content-script and page-script

* [PM-5744] Implementing a methodology that allows us to dynamically set and unset content scripts

* [PM-5744] Applying cleanup to page-script.ts to lighten the footprint of the script

* [PM-5744] Further simplifying page-script implementation

* [PM-5744] Reworking Fido2Utils to remove references to the base Utils methods to allow the page-script.ts file to render at a lower file size

* [PM-5744] Reworking Fido2Utils to remove references to the base Utils methods to allow the page-script.ts file to render at a lower file size

* [PM-5744] Implementing the `RegisterContentScriptPolyfill` as a separately compiled file as opposed to an import

* [PM-5744] Implementing methodology to ensure that the RegisterContentScript polyfill is not built in cases where it is not needed

* [PM-5744] Implementing methodology to ensure that the RegisterContentScript polyfill is not built in cases where it is not needed

* [PM-5744] Reverting package-lock.json

* [PM-5744] Implementing a methodology to ensure we can instantiate the RegisterContentScript polyfill in a siloed manner

* [PM-5744] Migrating chrome extension api calls to the BrowserApi class

* [PM-5744] Implementing typing information within the RegisterContentScriptsPolyfill

* [PM-5744] Removing any eslint-disable references within the RegisterContentScriptsPolyfill

* [PM-5744] Refactoring polyfill implementation

* [PM-5744] Refactoring polyfill implementation

* [PM-5744] Fixing an issue where Safari was not resolving the await chrome proxy

* [PM-5744] Fixing jest tests for the page-script append method

* [PM-5744] Fixing an issue found where collection of page details can trigger a context invalidated message when the extension is refreshed

* [PM-5744] Implementing jest tests for the added BrowserApi methods

* [PM-5744] Refactoring Fido2Background implementation

* [PM-5744] Refactoring Fido2Background implementation

* [PM-5744] Adding enums to the implementation for the Fido2 Content Scripts and working through jest tests for the BrowserApi and Fido2Background classes

* [PM-5744] Adding comments to the FIDO2 content-script.ts file

* [PM-5744] Adding jest tests for the Fido2 content-script.ts

* [PM-5744] Adding jest tests for the Fido2 content-script.ts

* [PM-5744] Adding jest tests for the Fido2 page-script.ts

* [PM-5744] Working through an attempt to jest test the page-script.ts file

* [PM-5744] Finalizing jest tests for the page-script.ts implementation

* [PM-5744] Applying stricter type information for the passed params within fido2-testing-utils.ts

* [PM-5744] Adjusting documentation

* [PM-5744] Adjusting implementation of jest tests to use mock proxies

* [PM-5744] Adjusting jest tests to simply implementation

* [PM-5744] Adjust jest tests based on code review feedback

* [PM-5744] Adjust jest tests based on code review feedback

* [PM-5744] Adjust jest tests based on code review feedback

* [PM-5744] Adjusting jest tests to based on feedback

* [PM-5744] Adjusting jest tests to based on feedback

* [PM-5744] Adjusting jest tests to based on feedback

* [PM-5744] Adjusting conditional within page-script.ts

* [PM-5744] Removing unnecessary global reference to the messager

* [PM-5744] Updating jest tests

* [PM-5744] Updating jest tests

* [PM-5744] Updating jest tests

* [PM-5744] Updating jest tests

* [PM-5744] Updating how we export the Fido2Background class

* [PM-5744] Adding duplciate jest tests to fido2-utils.ts to ensure we maintain functionality for utils methods pulled from platform utils

* [PM-5189] Addressing code review feedback

* [PM-5744] Applying code review feedback, reworking obserable subscription within fido2 background

* [PM-5744] Reworking jest tests to avoid mocking `firstValueFrom`

* [PM-5744] Reworking jest tests to avoid usage of private methods

* [PM-5744] Reworking jest tests to avoid usage of private methods

* [PM-5744] Implementing jest tests for the ScriptInjectorService and updating references within the Browser Extension to use the new service

* [PM-5744] Converting ScriptInjectorService to a dependnecy instead of a static class

* [PM-5744] Reworking typing for the ScriptInjectorService

* [PM-5744] Adjusting implementation based on feedback provided during code review

* [PM-5744] Adjusting implementation based on feedback provided during code review

* [PM-5744] Adjusting implementation based on feedback provided during code review

* [PM-5744] Adjusting implementation based on feedback provided during code review

* [PM-5744] Adjusting how the ScriptInjectorService accepts the config to simplify the data structure

* [PM-5744] Updating jest tests to cover edge cases within ScriptInjectorService

* [PM-5744] Updating jest tests to reference the ScriptInjectorService directly rather than the underlying ExecuteScript api call

* [PM-5744] Updating jest tests to reflect provided feedback during code review

* [PM-5744] Updating jest tests to reflect provided feedback during code review

* [PM-5744] Updating documentation based on code review feedback

* [PM-5744] Updating how we extend the abstract ScriptInjectorService

* [PM-5744] Updating reference to the frame property on the ScriptInjectionConfig
lorenz added a commit to lorenz/clients that referenced this pull request Oct 29, 2024
The original implementation of bufferSourceToUint8Array was incorrect as
it did not consider that TypedArray instances represent a view of the
underlying ArrayBuffer which does not necessarily cover the entire
backing ArrayBuffer. This resulted in the output of this function
containing data which would not be logically contained in the input.

This was partially fixed by bitwarden#8787 for the common case of the input
already being an Uint8Array, but it was still broken for any other
TypedArrays. But bitwarden#8222 introduced another copy of the original broken
code, breaking the Uint8Array case again.

Fix this once and hopefully for the last time with a correct
implementation of bufferSourceToUint8Array and using that in the
appropriate places instead of open-coding it. In addition there are now
tests which exercise most edge cases with regards to ArrayBuffer and
TypedArrays.
lorenz added a commit to lorenz/clients that referenced this pull request Oct 29, 2024
The original implementation of bufferSourceToUint8Array was incorrect as
it did not consider that TypedArray instances represent a view of the
underlying ArrayBuffer which does not necessarily cover the entire
backing ArrayBuffer. This resulted in the output of this function
containing data which would not be logically contained in the input.

This was partially fixed by bitwarden#8787 for the common case of the input
already being an Uint8Array, but it was still broken for any other
TypedArrays. But bitwarden#8222 introduced another copy of the original broken
code, breaking the Uint8Array case again.

Fix this once and hopefully for the last time with a correct
implementation of bufferSourceToUint8Array and using that in the
appropriate places instead of open-coding it. In addition there are now
tests which exercise most edge cases with regards to ArrayBuffer and
TypedArrays.
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.

7 participants