Authenticator icon indicating copy to clipboard operation
Authenticator copied to clipboard

Fix OTP type

Open Sneezry opened this issue 1 year ago • 6 comments

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.

Sneezry avatar Aug 26 '24 18:08 Sneezry

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.

mymindstorm avatar Sep 03 '24 02:09 mymindstorm

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.

Sneezry avatar Sep 11 '24 12:09 Sneezry

found a bug:

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

the hotp code becomes a totp code

mymindstorm avatar Sep 15 '24 00:09 mymindstorm

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

mymindstorm avatar Sep 15 '24 00:09 mymindstorm

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 avatar Sep 26 '24 23:09 olfek

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 ?

olfek avatar Sep 30 '24 10:09 olfek