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

Migrate to MV3 (Chrome, Firefox, Edge) #1009

Merged
merged 42 commits into from
May 27, 2024
Merged

Migrate to MV3 (Chrome, Firefox, Edge) #1009

merged 42 commits into from
May 27, 2024

Conversation

Sneezry
Copy link
Member

@Sneezry Sneezry commented Jan 31, 2023

Fixes #565

  • Move to chrome.storage.session to cache password
  • Move to chrome.scripting to inject scan QR script
  • Replace content_security_policy with content_security_policy.extension_pages or content_security_policy.sandbox as appropriate.
  • Replace setTimeout with chrome.alarms API for auto-lock
  • Replace XMLHttpRequest with fetch in backend agent
  • Migrate LocalStorage to chrome.storage.local
  • Migrate Image and canvas to OffscreenCanvas(?) for service worker use Move QR decode to content script
  • E2E test
    • Chrome
      • Can create new accounts
      • Can create new accounts by scanning QR codes
      • Can import existing accounts
      • Can migrate data with existing installed MV2 extension
      • Can set password
      • Can click to copy
      • Can click to auto fill
      • Can auto backup to 3rd party storage
      • Auto-lock works as expected
      • Force lock works as expected
      • Context menu works as expected
      • Policy configuration works as expected
      • Storage area migration works as expected
      • TBD
    • Firefox
      • Can create new accounts
      • Can create new accounts by scanning QR codes
      • Can import existing accounts
      • Can migrate data with existing installed MV2 extension
      • Can set password
      • Can click to copy
      • Can click to auto fill
      • Can auto backup to 3rd party storage
      • Auto-lock works as expected
      • Force lock works as expected
      • Context menu works as expected
      • Policy configuration works as expected
      • Storage area migration works as expected
      • TBD
    • Edge
      • Can create new accounts
      • Can create new accounts by scanning QR codes
      • Can import existing accounts
      • Can migrate data with existing installed MV2 extension
      • Can set password
      • Can click to copy
      • Can click to auto fill
      • Can auto backup to 3rd party storage
      • Auto-lock works as expected
      • Force lock works as expected
      • Context menu works as expected
      • Policy configuration works as expected
      • Storage area migration works as expected
      • TBD

Known issues

  • Backup export failed when setting a password
  • Browser sync is disabled by default, need to double confirm if it is as expected

@Sneezry Sneezry changed the title WIP: Migrate to MV3 WIP: Migrate to Chrome MV3 Feb 1, 2023
@Sneezry
Copy link
Member Author

Sneezry commented Feb 2, 2023

Some scenario test is still on the way, but the code should be ready for review.

@Sneezry Sneezry marked this pull request as ready for review February 2, 2023 20:47
@Sneezry Sneezry changed the title WIP: Migrate to Chrome MV3 Migrate to Chrome MV3 Feb 2, 2023
@Sneezry
Copy link
Member Author

Sneezry commented Feb 2, 2023

This PR only contains Chrome parts. Firefox and Edge MV3 migration should be in other PRs.

@Sneezry
Copy link
Member Author

Sneezry commented Feb 2, 2023

@rebornix Chrome, Firefox, and Edge are starting to migrate to MV3, MV2 will stop working in the next few months. There are many breaking changes in MV3, Safari may also have some updates. Once this PR has been merged, the Safari build may be broken.

@mymindstorm
Copy link
Member

Just FYI, I might not be able to get to this until next Wednesday.

@mymindstorm
Copy link
Member

mymindstorm commented Feb 21, 2023

I'm getting

/home/mymindstorm/Projects/Authenticator-1/node_modules/loader-runner/lib/LoaderRunner.js:133
                if(isError) throw e;
                            ^

Error: error:0308010C:digital envelope routines::unsupported
    at new Hash (node:internal/crypto/hash:71:19)
    at Object.createHash (node:crypto:133:10)
    at module.exports (/home/mymindstorm/Projects/Authenticator-1/node_modules/webpack/lib/util/createHash.js:135:53)
    at NormalModule._initBuildHash (/home/mymindstorm/Projects/Authenticator-1/node_modules/webpack/lib/NormalModule.js:417:16)
    at handleParseError (/home/mymindstorm/Projects/Authenticator-1/node_modules/webpack/lib/NormalModule.js:471:10)
    at /home/mymindstorm/Projects/Authenticator-1/node_modules/webpack/lib/NormalModule.js:503:5
    at /home/mymindstorm/Projects/Authenticator-1/node_modules/webpack/lib/NormalModule.js:358:12
    at /home/mymindstorm/Projects/Authenticator-1/node_modules/loader-runner/lib/LoaderRunner.js:373:3
    at iterateNormalLoaders (/home/mymindstorm/Projects/Authenticator-1/node_modules/loader-runner/lib/LoaderRunner.js:214:10)
    at iterateNormalLoaders (/home/mymindstorm/Projects/Authenticator-1/node_modules/loader-runner/lib/LoaderRunner.js:221:10)
    at /home/mymindstorm/Projects/Authenticator-1/node_modules/loader-runner/lib/LoaderRunner.js:236:3
    at context.callback (/home/mymindstorm/Projects/Authenticator-1/node_modules/loader-runner/lib/LoaderRunner.js:111:13)
    at makeSourceMapAndFinish (/home/mymindstorm/Projects/Authenticator-1/node_modules/ts-loader/dist/index.js:90:5)
    at successLoader (/home/mymindstorm/Projects/Authenticator-1/node_modules/ts-loader/dist/index.js:68:9)
    at Object.loader (/home/mymindstorm/Projects/Authenticator-1/node_modules/ts-loader/dist/index.js:22:12)
    at LOADER_EXECUTION (/home/mymindstorm/Projects/Authenticator-1/node_modules/loader-runner/lib/LoaderRunner.js:119:14)
    at runSyncOrAsync (/home/mymindstorm/Projects/Authenticator-1/node_modules/loader-runner/lib/LoaderRunner.js:120:4)
    at iterateNormalLoaders (/home/mymindstorm/Projects/Authenticator-1/node_modules/loader-runner/lib/LoaderRunner.js:232:2)
    at Array.<anonymous> (/home/mymindstorm/Projects/Authenticator-1/node_modules/loader-runner/lib/LoaderRunner.js:205:4)
    at Storage.finished (/home/mymindstorm/Projects/Authenticator-1/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:55:16)
    at /home/mymindstorm/Projects/Authenticator-1/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:91:9
    at /home/mymindstorm/Projects/Authenticator-1/node_modules/graceful-fs/graceful-fs.js:123:16
    at FSReqCallback.readFileAfterClose [as oncomplete] (node:internal/fs/read_file_context:68:3) {
  opensslErrorStack: [ 'error:03000086:digital envelope routines::initialization error' ],
  library: 'digital envelope routines',
  reason: 'unsupported',
  code: 'ERR_OSSL_EVP_UNSUPPORTED'
}

Node.js v18.12.1

when building. It goes away with export NODE_OPTIONS=--openssl-legacy-provider.

I think this might be fixed if we use a newer version of loader-runner (seems to be a dependency of webpack). Would that be easy to do here?


The service worker isn't registering properly on my Chrome: Service worker registration failed. Status code: 15.

image


Keyboard shortcuts seem to not be working due to manifest issue:

image

@Sneezry
Copy link
Member Author

Sneezry commented Feb 21, 2023

Can you load this build locally?
chrome.zip

@mymindstorm
Copy link
Member

mymindstorm commented Feb 21, 2023

That build has same issues with the service worker and keyboard commands. (Chromium Version 110.0.5481.77)

@Sneezry
Copy link
Member Author

Sneezry commented Feb 21, 2023

Interesting... It works fine on my machine though...

image

image

image

@mymindstorm
Copy link
Member

mymindstorm commented Feb 21, 2023 via email

@mymindstorm
Copy link
Member

oops, I see that you've attached it.

@Sneezry
Copy link
Member Author

Sneezry commented Mar 9, 2023

I might find the root cause of the service work register issue.

Lots of API inside chrome are present only if your extension's manifest.json lists its associated permission name in "permissions" or in a special key
https://stackoverflow.com/a/73789284

The contextMenu is in optional permission, which means chrome.contextMenu should be undefined before the permission granted.

We can move it to required permission since I see no security risk about that.

@mymindstorm do you have any concern?

@mymindstorm
Copy link
Member

Sorry for the lateness on this. I will get to it as soon as I can.

@mymindstorm
Copy link
Member

I agree, I don't see a security concern there.

@mymindstorm
Copy link
Member

I think I've resolved most of the issues I found. Please make sure to test on your end too to make sure I didn't miss anything.

@mymindstorm
Copy link
Member

I am leaving the tests broken, I ran out of time to fix them.

@Sneezry
Copy link
Member Author

Sneezry commented Apr 10, 2023

I think I've resolved most of the issues I found. Please make sure to test on your end too to make sure I didn't miss anything.

Awesome! It works w/o problems on my side.

this prevents a dependency issue in the tests
mv3 makes code cov w/ istanbul virtually impossible due to restrictions on unsafe-eval
@mymindstorm
Copy link
Member

Tests work again! We lost the ability for code coverage reporting, but that was half broken due to vue anyway.

image

Copy link
Member

@mymindstorm mymindstorm left a comment

Choose a reason for hiding this comment

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

I really don't like how LocalStorage works.

  • Putting it as a global everywhere makes it easy to misuse (e.g. forget to load/store data from/to chrome.storage again). This should all be hidden behind a class. It's too easy to make mistakes with the current way.
  • Using the global var as a sort-of cache introduces the possibility of race conditions
  • We should name is something more descriptive, like userSettings
  • We know exactly what should be in LocalStorage/userSettings. There should be an interface for it instead of any.

For example, something like:

// UserSettings.d.ts
declare type StorageLocation = "sync" | "local";

interface UserSettings {
    storageLocation?: StorageLocation;
}

// LocalUserSettings.ts
export class LocalUserSettings {
  static async getUserSettings(): Promise<UserSettings> {
    return (await chrome.storage.local.get("userSettings"))?.userSettings || {};
  }

  static async setUserSettings(userSettings: UserSettings) {
    let mergedSettings = await getUserSettings();
    Object.assign(mergedSettings, userSettings);
    await chrome.storage.local.set({ userSettings: mergedSettings });
  }
}

What are your thoughts on this?

src/models/storage.ts Outdated Show resolved Hide resolved
src/models/storage.ts Outdated Show resolved Hide resolved
@mymindstorm
Copy link
Member

Also, the migration for localStorage broke in Firefox. Will investigate when I have more time.

@Sneezry
Copy link
Member Author

Sneezry commented Mar 18, 2024

I really don't like how LocalStorage works.

  • Putting it as a global everywhere makes it easy to misuse (e.g. forget to load/store data from/to chrome.storage again). This should all be hidden behind a class. It's too easy to make mistakes with the current way.
  • Using the global var as a sort-of cache introduces the possibility of race conditions
  • We should name is something more descriptive, like userSettings
  • We know exactly what should be in LocalStorage/userSettings. There should be an interface for it instead of any.

For example, something like:

// UserSettings.d.ts
declare type StorageLocation = "sync" | "local";

interface UserSettings {
    storageLocation?: StorageLocation;
}

// LocalUserSettings.ts
export class LocalUserSettings {
  static async getUserSettings(): Promise<UserSettings> {
    return (await chrome.storage.local.get("userSettings"))?.userSettings || {};
  }

  static async setUserSettings(userSettings: UserSettings) {
    let mergedSettings = await getUserSettings();
    Object.assign(mergedSettings, userSettings);
    await chrome.storage.local.set({ userSettings: mergedSettings });
  }
}

What are your thoughts on this?

That makes sense. I will update the code later.

@mymindstorm
Copy link
Member

Ok. I will be unavailable for the next week and a half, but can help out after that.

@Sneezry
Copy link
Member Author

Sneezry commented May 13, 2024

@mymindstorm I have updated the PR. Let's complete this PR before end of the next week (5/26).

@Sneezry Sneezry requested a review from mymindstorm May 13, 2024 08:56
@Sneezry
Copy link
Member Author

Sneezry commented May 20, 2024

Also, the migration for localStorage broke in Firefox. Will investigate when I have more time.

I didn't meet any error when migrating from MV2. Could you test the latest code to verify if the issue has been resolved?

@mymindstorm
Copy link
Member

mymindstorm commented May 20, 2024 via email

@Sneezry
Copy link
Member Author

Sneezry commented May 20, 2024 via email

@Sneezry
Copy link
Member Author

Sneezry commented May 24, 2024

Considering Google is required all extensions to migrate to MV3 by June, and leaving a week for CWS review, I will complete this PR by 5/26 anyway.

@mymindstorm
Copy link
Member

Just FYI, starting to work on this. Will probably push something within 3-6 hours.

Copy link
Member

@mymindstorm mymindstorm left a comment

Choose a reason for hiding this comment

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

From what I've seen it appears to work fine, outside of that crash I found. Although, I want to start automating E2E tests at some point.

@Sneezry Sneezry merged commit df0b859 into dev May 27, 2024
10 checks passed
Sneezry added a commit that referenced this pull request May 27, 2024
* add compact theme

* Add new strings

This commit was automatically made by run 1641549669

* add icon for shortcut (#820)

* strict ts

* managed storage not available in safari.

* use blob url to replace data url (#817)

* Add flat theme.

* add permissions management (#827)

* add permissions management

* hide required permission by default

* update permission description

* update strings

Co-authored-by: Brendan Early <[email protected]>

* Add new strings

This commit was automatically made by run 1704482678

* Bump pathval from 1.1.0 to 1.1.1 (#852)

Bumps [pathval](https://github.com/chaijs/pathval) from 1.1.0 to 1.1.1.
- [Release notes](https://github.com/chaijs/pathval/releases)
- [Changelog](https://github.com/chaijs/pathval/blob/master/CHANGELOG.md)
- [Commits](chaijs/pathval@v1.1.0...v1.1.1)

---
updated-dependencies:
- dependency-name: pathval
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump node-fetch from 2.6.1 to 2.6.7 (#853)

Bumps [node-fetch](https://github.com/node-fetch/node-fetch) from 2.6.1 to 2.6.7.
- [Release notes](https://github.com/node-fetch/node-fetch/releases)
- [Commits](node-fetch/node-fetch@v2.6.1...v2.6.7)

---
updated-dependencies:
- dependency-name: node-fetch
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump urijs from 1.19.7 to 1.19.10 (#873)

Bumps [urijs](https://github.com/medialize/URI.js) from 1.19.7 to 1.19.10.
- [Release notes](https://github.com/medialize/URI.js/releases)
- [Changelog](https://github.com/medialize/URI.js/blob/gh-pages/CHANGELOG.md)
- [Commits](medialize/URI.js@v1.19.7...v1.19.10)

---
updated-dependencies:
- dependency-name: urijs
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump minimist from 1.2.5 to 1.2.6 (#884)

Bumps [minimist](https://github.com/substack/minimist) from 1.2.5 to 1.2.6.
- [Release notes](https://github.com/substack/minimist/releases)
- [Commits](https://github.com/substack/minimist/compare/1.2.5...1.2.6)

---
updated-dependencies:
- dependency-name: minimist
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump ansi-regex from 3.0.0 to 3.0.1 (#885)

Bumps [ansi-regex](https://github.com/chalk/ansi-regex) from 3.0.0 to 3.0.1.
- [Release notes](https://github.com/chalk/ansi-regex/releases)
- [Commits](chalk/ansi-regex@v3.0.0...v3.0.1)

---
updated-dependencies:
- dependency-name: ansi-regex
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump urijs from 1.19.10 to 1.19.11 (#886)

Bumps [urijs](https://github.com/medialize/URI.js) from 1.19.10 to 1.19.11.
- [Release notes](https://github.com/medialize/URI.js/releases)
- [Changelog](https://github.com/medialize/URI.js/blob/gh-pages/CHANGELOG.md)
- [Commits](medialize/URI.js@v1.19.10...v1.19.11)

---
updated-dependencies:
- dependency-name: urijs
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* add autofill command (#831)

* add autofill command

* address comment

* Add 'wasm-unsafe-eval' to custom CSP (fix #906)

It seems that some dependencies (at least argon2-browser) require allowing WebAssembly at the CSP level.
Recent Firefox allows that by default, but having your own CSP means this extension won't be able to benefit from this automatically.

* revert unnecessary change

* Update readme

* Browser componnet

* isFirefox

* isEdge.

* 💄

* TypeScript does not like type annotation for catch.

* 💄

* fix build

* update theme for safari

* fix local download

* update to latest

* minimal change

* 💄

* fix redundant theme option

* Update translations from crowdin

* Update readme for safari.

* Use `&&`

* Add new strings

This commit was automatically made by run 2680600562

* Bump terser from 4.8.0 to 4.8.1 (#929)

Bumps [terser](https://github.com/terser/terser) from 4.8.0 to 4.8.1.
- [Release notes](https://github.com/terser/terser/releases)
- [Changelog](https://github.com/terser/terser/blob/master/CHANGELOG.md)
- [Commits](https://github.com/terser/terser/commits)

---
updated-dependencies:
- dependency-name: terser
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump loader-utils from 1.4.0 to 1.4.1 (#970)

Bumps [loader-utils](https://github.com/webpack/loader-utils) from 1.4.0 to 1.4.1.
- [Release notes](https://github.com/webpack/loader-utils/releases)
- [Changelog](https://github.com/webpack/loader-utils/blob/v1.4.1/CHANGELOG.md)
- [Commits](webpack/loader-utils@v1.4.0...v1.4.1)

---
updated-dependencies:
- dependency-name: loader-utils
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump qs from 6.9.4 to 6.11.0 (#985)

Bumps [qs](https://github.com/ljharb/qs) from 6.9.4 to 6.11.0.
- [Release notes](https://github.com/ljharb/qs/releases)
- [Changelog](https://github.com/ljharb/qs/blob/main/CHANGELOG.md)
- [Commits](ljharb/qs@v6.9.4...v6.11.0)

---
updated-dependencies:
- dependency-name: qs
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump json5 from 1.0.1 to 1.0.2 (#993)

Bumps [json5](https://github.com/json5/json5) from 1.0.1 to 1.0.2.
- [Release notes](https://github.com/json5/json5/releases)
- [Changelog](https://github.com/json5/json5/blob/main/CHANGELOG.md)
- [Commits](json5/json5@v1.0.1...v1.0.2)

---
updated-dependencies:
- dependency-name: json5
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump cookiejar from 2.1.2 to 2.1.4 (#1006)

Bumps [cookiejar](https://github.com/bmeck/node-cookiejar) from 2.1.2 to 2.1.4.
- [Release notes](https://github.com/bmeck/node-cookiejar/releases)
- [Commits](https://github.com/bmeck/node-cookiejar/commits)

---
updated-dependencies:
- dependency-name: cookiejar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix disable backup policy not working issue (#1050)

* 6.3.5

* typo

* fix typo (#1144)

* add import QR images/OTP URLs buttons to add account page

* Add new strings

This commit was automatically made by run 8076854721

* Migrate to MV3 (Chrome, Firefox, Edge) (#1009)

* Migrate to MV3

* fix entry type

* fix default storage area

* fix mv3 migration errors

* further CI fixes

* Fix disable backup policy not working issue (#1050)

* 6.3.5

* typo

* Fix bad practice with argon2-browser

* Remove 'offline_enabled' from manifest

* update test runner and coverage for mv3

* don't use dev config by default

* fix typo (#1144)

* add import QR images/OTP URLs buttons to add account page

* Migrate to MV3

* fix entry type

* fix default storage area

* fix mv3 migration errors

* further CI fixes

* Fix bad practice with argon2-browser

* Remove 'offline_enabled' from manifest

* update test runner and coverage for mv3

* don't use dev config by default

* dump version

* update manifest

* fix ff csp

* remove artifact

* rename test files

* move syncTimeWithGoogle out of popup.ts

this prevents a dependency issue in the tests

* fix the tests, remove code coverage

mv3 makes code cov w/ istanbul virtually impossible due to restrictions on unsafe-eval

* remove testing code

* refactor user settings (#1191)

* remove out-of-date eslint comments

* fix user settings migration issue

* fix user setting migration issue

* fix edge errors

* fix edge issues

* update firefox permissions

* remove all_urls permission since Firefox has supported activeTab

* fix firefox crash due to functions getting added to usersettings object

---------

Co-authored-by: Brendan Early <[email protected]>
Co-authored-by: Zhe Li <[email protected]>
Co-authored-by: spaette <[email protected]>
Co-authored-by: vuittont60 <[email protected]>

* Add new strings

This commit was automatically made by run 9249787733

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Zhe Li <[email protected]>
Co-authored-by: rebornix <[email protected]>
Co-authored-by: Brendan Early <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Frederik Braun <[email protected]>
Co-authored-by: Peng Lyu <[email protected]>
Co-authored-by: spaette <[email protected]>
Co-authored-by: vuittont60 <[email protected]>
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.

MV3 Compatibility
6 participants