search

Authenticator-Extension / Authenticator

Authenticator generates 2-Step Verification codes in your browser.
https://authenticator.cc
MIT License
3.14k stars 743 forks source link

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

Closed Sneezry closed 2 months ago

Sneezry commented 1 year ago

Fixes #565

Known issues

Sneezry commented 1 year ago

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

Sneezry commented 1 year ago

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

Sneezry commented 1 year ago

@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 commented 1 year ago

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

mymindstorm commented 1 year ago

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 commented 1 year ago

Can you load this build locally? chrome.zip

mymindstorm commented 1 year ago

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

Sneezry commented 1 year ago

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

image

image

image

mymindstorm commented 1 year ago

Interesting, I'll investigate further this weekend. What is your chrome version?

-------- Original message -------- From: Zhe Li @.> Date: 2/20/23 9:47 PM (GMT-06:00) To: Authenticator-Extension/Authenticator @.> Cc: Brendan Early @.>, Review requested @.> Subject: Re: [Authenticator-Extension/Authenticator] Migrate to Chrome MV3 (PR #1009)

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

[image]https://user-images.githubusercontent.com/1621293/220242490-8e0dbd82-9afa-483d-b62b-1e09b59de843.png

[image]https://user-images.githubusercontent.com/1621293/220242531-f1ca9d29-bc67-4728-ac96-e99b04c89066.png

[image]https://user-images.githubusercontent.com/1621293/220242564-d99fe754-55a7-483d-97e9-3d7b66cf1b47.png

— Reply to this email directly, view it on GitHubhttps://github.com/Authenticator-Extension/Authenticator/pull/1009#issuecomment-1437819555, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGUAT3TEG4BOBV3HDW2IS7DWYQ3DZANCNFSM6AAAAAAUM4FOEQ. You are receiving this because your review was requested.Message ID: @.***>

mymindstorm commented 1 year ago

oops, I see that you've attached it.

Sneezry commented 1 year ago

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 commented 1 year ago

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

mymindstorm commented 1 year ago

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

mymindstorm commented 1 year ago

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 commented 1 year ago

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

Sneezry commented 1 year ago

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.

mymindstorm commented 6 months ago

I spent a few hours working on tests. They technically work again. Except for the fact that I had to disable the UI ones until I have more time to work out the kinks of MV3 disallowing all unsafe-eval and some weird webpack dependency issues. At least it is in an overall better state than it was previously.

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 5.17241% with 165 lines in your changes are missing coverage. Please review.

Project coverage is 20.56%. Comparing base (d34f98b) to head (6268407). Report is 36 commits behind head on dev.

:exclamation: Current head 6268407 differs from pull request most recent head a5ef095

Please upload reports for the commit a5ef095 to get more accurate results.

Files Patch % Lines
src/models/storage.ts 5.38% 158 Missing :warning:
src/models/otp.ts 0.00% 5 Missing :warning:
src/models/key-utilities.ts 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #1009 +/- ## ========================================== + Coverage 18.13% 20.56% +2.43% ========================================== Files 18 3 -15 Lines 1544 530 -1014 Branches 338 147 -191 ========================================== - Hits 280 109 -171 + Misses 1230 406 -824 + Partials 34 15 -19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Sneezry commented 5 months ago

Firefox should now be operational. However, end-to-end testing has not yet been completed.

WofWca commented 4 months ago

Hey! So, the extension seems to be working alright on Manifest V3, it's only tests (i.e. non-production code) that are missing? Am I reading this correctly?

Sneezry commented 4 months ago

Hey! So, the extension seems to be working alright on Manifest V3, it's only tests (i.e. non-production code) that are missing? Am I reading this correctly?

The end-to-end test is not completed. If all tests pass, we can deliver it to the public.

mymindstorm commented 4 months ago

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

image

mymindstorm commented 4 months ago

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

Sneezry commented 4 months ago

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 commented 4 months ago

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

Sneezry commented 2 months ago

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

Sneezry commented 2 months ago

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 commented 2 months ago

Sure. I've been sick since last week so it might take me some time to get to.

-------- Original message -------- From: Zhe Li @.> Date: 5/19/24 11:47 PM (GMT-06:00) To: Authenticator-Extension/Authenticator @.> Cc: Brendan Early @.>, Mention @.> Subject: Re: [Authenticator-Extension/Authenticator] Migrate to MV3 (Chrome, Firefox, Edge) (PR #1009)

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?

— Reply to this email directly, view it on GitHubhttps://github.com/Authenticator-Extension/Authenticator/pull/1009#issuecomment-2119659227, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGUAT3QA5A42QZRE66CI6GTZDF55BAVCNFSM6AAAAAAUM4FOESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZGY2TSMRSG4. You are receiving this because you were mentioned.Message ID: @.***>

Sneezry commented 2 months ago

Oh, take care and recover soon!

Brendan Early @.***>于2024年5月20日 周一20:46写道：

Sure. I've been sick since last week so it might take me some time to get to.

-------- Original message -------- From: Zhe Li @.> Date: 5/19/24 11:47 PM (GMT-06:00) To: Authenticator-Extension/Authenticator @.> Cc: Brendan Early @.>, Mention @.> Subject: Re: [Authenticator-Extension/Authenticator] Migrate to MV3 (Chrome, Firefox, Edge) (PR #1009)

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?

— Reply to this email directly, view it on GitHub< https://github.com/Authenticator-Extension/Authenticator/pull/1009#issuecomment-2119659227>, or unsubscribe< https://github.com/notifications/unsubscribe-auth/AGUAT3QA5A42QZRE66CI6GTZDF55BAVCNFSM6AAAAAAUM4FOESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZGY2TSMRSG4>.

You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/Authenticator-Extension/Authenticator/pull/1009#issuecomment-2120392085, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAML2LK6LWI4FEUZLYDRO4TZDHWBJAVCNFSM6AAAAAAUM4FOESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRQGM4TEMBYGU . You are receiving this because you authored the thread.Message ID: @.***>

Sneezry commented 2 months ago

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 commented 2 months ago

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