Fix OTP type

[Sneezry]

Fix #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]

I feel like there are other places where this change needs to be made. E.g.

Authenticator/src/definitions/module-interface.d.ts

Line 65 in c8a651e

OTPType: number;

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]

Also fixes: #1302 #1292 #1291

@mymindstorm we should have a hotfix ASAP.

[mymindstorm]

found a bug:

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

the hotp code becomes a totp code

[mymindstorm]

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

[mymindstorm]

This has a bug in it.

[mymindstorm]

This line is causing the bug. This if statement is not considering if decryptedData.type is a valid OTPType already and will always return OTPType.totp when decryptedData.type is a string.

Recommend making a type guard for OTPType and using it whenever we need to check if a type is valid, such as in storage.ts and elsewhere. Plus another shared function to convert from legacy otptype to new version. That would help prevent subtle mistakes like this and help remove all this casting to unknown.

[mymindstorm]

move this into the case statement or otherwise refactor so we don't have to cast to unknown below. we can set type: unknown in otp.d.ts/RawOTPStorage

[olfek]

I recommend keeping numeric enums where you get the benefit of reverse mappings so you don't have to do this ugly thing.

[olfek]

(2) I recommend keeping numeric enums where you get the benefit of reverse mappings so you don't have to do this ugly thing.

[olfek]

Error OTP type entries would only ever be 1, there is no need to check the other numbers.

See my comment below

[olfek]

(2) Error OTP type entries would only ever be 1, there is no need to check the other numbers.

See my comment below

[olfek]

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:

Authenticator/src/models/storage.ts

Line 481 in 6511920

type: (parseInt(data[hash].type) as OTPType) || OTPType[OTPType.totp],

(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:

Authenticator/src/models/otp.ts

Lines 156 to 160 in 6511920

	if (this.type === OTPType.totp && entry.period) {
	this.period = entry.period;
	} else {
	this.period = 30;
	}

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

Authenticator/src/models/storage.ts

Lines 221 to 227 in 6511920

	const storageItem: RawOTPStorage = {
	encrypted,
	hash: entry.hash,
	index: entry.index,
	type: OTPType[entry.type],
	secret,
	};

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

Authenticator/src/models/storage.ts

Lines 624 to 638 in 6511920

	switch (entryData.type) {
	case "totp":
	case "hotp":
	case "battle":
	case "steam":
	case "hex":
	case "hhex":
	type = OTPType[entryData.type];
	break;
	default:
	// we need correct the type here
	// and save it
	type = OTPType.totp;
	entryData.type = OTPType[OTPType.totp];
	}

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]

The cause of this type bug:

Authenticator/src/models/storage.ts

Line 481 in 6511920

type: (parseInt(data[hash].type) as OTPType) || OTPType[OTPType.totp],

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

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