Authenticator-Extension / Authenticator

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

Fix OTP type #1283

Open Sneezry opened 2 months ago

Sneezry commented 2 months ago

Fix https://github.com/Authenticator-Extension/Authenticator/issues/1271

We have both OTPType and string definitions for OTP entry types. Previously, OTPType was a numeric enum. When importing OTP URLs, OTP entries were generated with a numeric type value, causing an issue where the period parameter was ignored. This happened because we were comparing OTPType.totp (which has a numeric value of 1) with the string "totp", leading to a false condition. As a result, the system incorrectly identified the entry as not time-based, causing the period parameter to be disregarded. This fix ensures all OTP types are now aligned with the new OTPType string enum, resolving the issue.

mymindstorm commented 2 months ago

I feel like there are other places where this change needs to be made. E.g. https://github.com/Authenticator-Extension/Authenticator/blob/c8a651e52b597a16d2182d7c7b7844ef549375a1/src/definitions/module-interface.d.ts#L65

We really need to spend some time writing E2E tests. This change is scary to me because this enum is used in a lot of places where type checking is half broken due to vuex.

Sneezry commented 2 months ago

Also fixes: https://github.com/Authenticator-Extension/Authenticator/issues/1302 https://github.com/Authenticator-Extension/Authenticator/issues/1292 https://github.com/Authenticator-Extension/Authenticator/issues/1291

@mymindstorm we should have a hotfix ASAP.

mymindstorm commented 2 months ago

found a bug:

  1. add a HOTP code
  2. set an encryption password

the hotp code becomes a totp code

mymindstorm commented 2 months ago

this also occurs if you add a hotp when encryption is on and reload the extension

olfek commented 1 month ago

Flow in question:

  1. src\background.ts > getTotp()
  2. src\models\storage.ts > EntryStorage > import()
  3. src\models\otp.ts > OTPEntry
  4. src\models\storage.ts > EntryStorage > getOTPStorageFromEntry()
  5. src\models\storage.ts > EntryStorage > get()

So in the currently deployed extension we have this line:

https://github.com/Authenticator-Extension/Authenticator/blob/6511920bac98576e3c97fe2d213b23e9dc899422/src/models/storage.ts#L481

(parseInt(data[hash].type) as OTPType) is always NaN because data[hash].type is a TEXT string.

OTPType[OTPType.totp] gives the string totp but the type of type is the enum OTPType.

From this point onwards, all equality checks on type fail and default type with relevant code is always in use. For example:

https://github.com/Authenticator-Extension/Authenticator/blob/6511920bac98576e3c97fe2d213b23e9dc899422/src/models/otp.ts#L156-L160

Input period is lost and entry is saved with the default value of 30. Other properties may be affected too.

https://github.com/Authenticator-Extension/Authenticator/blob/6511920bac98576e3c97fe2d213b23e9dc899422/src/models/storage.ts#L221-L227

Type of type is string, entry.type is a string, OTPType[entry.type] gives a number, a number is stored.

https://github.com/Authenticator-Extension/Authenticator/blob/6511920bac98576e3c97fe2d213b23e9dc899422/src/models/storage.ts#L624-L638

Now when loading an entry back in, the number type doesn't match any string type so the switch goes to default.

Pretty significant bug I think, this PR is a high priority one for sure.

olfek commented 1 month ago

The cause of this type bug:

https://github.com/Authenticator-Extension/Authenticator/blob/6511920bac98576e3c97fe2d213b23e9dc899422/src/models/storage.ts#L481

https://www.typescriptlang.org/play/?#code/FAehAIGUDsEsAd4FMAu4CMxhOgVwLbgBiA9icAN7Dg3gBCAggErAC+WAxidAM5ooBPZAC5iZcAF5wACngBDAE48kASWgppAImgk0c8HnwAjJAs0BKcHJ5iSlgD73bAbVIkAdIyYBdANxYwKDhEVHAAJiwcAlsIqlp6ZkkMNk5uPnBBZDDRNzCk2UVlNQ1tXSsDAhMzS2sYhydc1zIwz2Y-AIhAXg3AVD2gA

Do you think this is a TypeScript bug? @mymindstorm @Sneezry ?