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.

Sign up for GitHub

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

Jump to bottom

Fix OTP type #1283

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
+128 −48
Open

Fix OTP type #1283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef89734
Fix OTP type
Sneezry Aug 26, 2024
cfd775f
convert error otp type
Sneezry Sep 11, 2024
29b2b95
fix otp type for export
Sneezry Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/background.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ async function getTotp(text: string, silent = false) {
}
} else {
let uri = text.split("otpauth://")[1];
let type = uri.substr(0, 4).toLowerCase();
let type = uri.substr(0, 4).toLowerCase() as OTPType;
uri = uri.substr(5);
let label = uri.split("?")[0];
const parameterPart = uri.split("?")[1];
Expand Down Expand Up @@ -193,15 +193,15 @@ async function getTotp(text: string, silent = false) {
if (
!/^[2-7a-z]+=*$/i.test(secret) &&
/^[0-9a-f]+$/i.test(secret) &&
type === "totp"
type === OTPType.totp
) {
type = "hex";
type = OTPType.hex;
} else if (
!/^[2-7a-z]+=*$/i.test(secret) &&
/^[0-9a-f]+$/i.test(secret) &&
type === "hotp"
type === OTPType.hotp
) {
type = "hhex";
type = OTPType.hhex;
}
const entryData: { [hash: string]: RawOTPStorage } = {};
entryData[hash] = {
Expand Down
16 changes: 10 additions & 6 deletions src/components/Popup/BackupPage.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
<script lang="ts">
import Vue from "vue";
import { isSafari } from "../../browser";
import { OTPType } from "../../models/otp";

export default Vue.extend({
data: function () {
Expand Down Expand Up @@ -212,10 +213,13 @@ function getOneLineOtpBackupFile(entryData: { [hash: string]: RawOTPStorage }) {
? otpStorage.issuer + ":" + (otpStorage.account || "")
: otpStorage.account || "";
let type = "";
if (otpStorage.type === "totp" || otpStorage.type === "hex") {
type = "totp";
} else if (otpStorage.type === "hotp" || otpStorage.type === "hhex") {
type = "hotp";
if (otpStorage.type === OTPType.totp || otpStorage.type === OTPType.hex) {
type = OTPType.totp;
} else if (
otpStorage.type === OTPType.hotp ||
otpStorage.type === OTPType.hhex
) {
type = OTPType.hotp;
} else {
continue;
}
Expand All @@ -228,8 +232,8 @@ function getOneLineOtpBackupFile(entryData: { [hash: string]: RawOTPStorage }) {
"?secret=" +
otpStorage.secret +
(otpStorage.issuer ? "&issuer=" + otpStorage.issuer : "") +
(type === "hotp" ? "&counter=" + otpStorage.counter : "") +
(type === "totp" && otpStorage.period
(type === OTPType.hotp ? "&counter=" + otpStorage.counter : "") +
(type === OTPType.totp && otpStorage.period
? "&period=" + otpStorage.period
: "") +
(otpStorage.digits ? "&digits=" + otpStorage.digits : "") +
Expand Down
6 changes: 3 additions & 3 deletions src/components/Popup/EntryComponent.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,10 @@ function getQrUrl(entry: OTPEntry) {
: entry.account;
const type =
entry.type === OTPType.hex
? OTPType[OTPType.totp]
? OTPType.totp
: entry.type === OTPType.hhex
? OTPType[OTPType.hotp]
: OTPType[entry.type];
? OTPType.hotp
: entry.type;
const otpauth =
"otpauth://" +
type +
Expand Down
2 changes: 1 addition & 1 deletion src/definitions/module-interface.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ interface AccountsState {
entries: OTPEntryInterface[];
defaultEncryption: string;
encryption: Map<string, EncryptionInterface>;
OTPType: number;
OTPType: OTPType;
shouldShowPassphrase: boolean;
sectorStart: boolean;
sectorOffset: number;
Expand Down
13 changes: 11 additions & 2 deletions src/definitions/otp.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
declare enum OTPType {
totp = "totp",
hotp = "hotp",
battle = "battle",
steam = "steam",
hex = "hex",
hhex = "hhex",
}

interface OTPEntryInterface {
type: number; // OTPType
type: OTPType;
index: number;
issuer: string;
secret: string | null;
Expand Down Expand Up @@ -42,7 +51,7 @@ interface RawOTPStorage {
index: number;
issuer?: string;
secret: string;
type: string;
type: OTPType;
counter?: number;
period?: number;
digits?: number;
Expand Down
10 changes: 5 additions & 5 deletions src/import.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ export async function getEntryDataFromOTPAuthPerLine(importCode: string) {
}

let uri = item.split("otpauth://")[1];
let type = uri.substr(0, 4).toLowerCase();
let type = uri.substr(0, 4).toLowerCase() as OTPType;
uri = uri.substr(5);
let label = uri.split("?")[0];
const parameterPart = uri.split("?")[1];
Expand Down Expand Up @@ -282,15 +282,15 @@ export async function getEntryDataFromOTPAuthPerLine(importCode: string) {
if (
!/^[2-7a-z]+=*$/i.test(secret) &&
/^[0-9a-f]+$/i.test(secret) &&
type === "totp"
type === OTPType.totp
) {
type = "hex";
type = OTPType.hex;
} else if (
!/^[2-7a-z]+=*$/i.test(secret) &&
/^[0-9a-f]+$/i.test(secret) &&
type === "hotp"
type === OTPType.hotp
) {
type = "hhex";
type = OTPType.hhex;
}

exportData[hash] = {
Expand Down
64 changes: 55 additions & 9 deletions src/models/otp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ import { UserSettings } from "./settings";
import { EntryStorage } from "./storage";

export enum OTPType {
totp = 1,
hotp,
battle,
steam,
hex,
hhex,
totp = "totp",
hotp = "hotp",
battle = "battle",
steam = "steam",
hex = "hex",
hhex = "hhex",
}

export enum CodeState {
Expand Down Expand Up @@ -117,7 +117,30 @@ export class OTPEntry implements OTPEntryInterface {
this.secret = entry.secret;
}

this.type = entry.type;
// We may have already had some error OTP type entries in the storage
// totp = 1
// hotp = 2
// battle = 3
// steam = 4
// hex = 5
// hhex = 6

if ((entry.type as unknown) === 1) {

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

Copy link

Choose a reason for hiding this comment

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

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

See my comment below

this.type = OTPType.totp;
} else if ((entry.type as unknown) === 2) {
this.type = OTPType.hotp;
} else if ((entry.type as unknown) === 3) {
this.type = OTPType.battle;
} else if ((entry.type as unknown) === 4) {
this.type = OTPType.steam;
} else if ((entry.type as unknown) === 5) {
this.type = OTPType.hex;
} else if ((entry.type as unknown) === 6) {
this.type = OTPType.hhex;
} else {
this.type = entry.type;
}

if (entry.issuer) {
this.issuer = entry.issuer;
} else {
Expand Down Expand Up @@ -223,8 +246,31 @@ export class OTPEntry implements OTPEntryInterface {
this.period = decryptedData.period || 30;
this.pinned = decryptedData.pinned || false;
this.secret = decryptedData.secret;
// @ts-expect-error need a better way to do this
this.type = OTPType[decryptedData.type] || OTPType.totp;
this.type = decryptedData.type || OTPType.totp;

// We may have already had some error OTP type entries in the storage
// totp = 1
// hotp = 2
// battle = 3
// steam = 4
// hex = 5
// hhex = 6

if ((decryptedData.type as unknown) === 1) {

This comment was marked as outdated.

Sign in to view

Copy link

Choose a reason for hiding this comment

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

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

See my comment below

this.type = OTPType.totp;
} else if ((decryptedData.type as unknown) === 2) {
this.type = OTPType.hotp;
} else if ((decryptedData.type as unknown) === 3) {
this.type = OTPType.battle;
} else if ((decryptedData.type as unknown) === 4) {
this.type = OTPType.steam;
} else if ((decryptedData.type as unknown) === 5) {
this.type = OTPType.hex;
} else if ((decryptedData.type as unknown) === 6) {
this.type = OTPType.hhex;
} else {
Copy link
Member

@mymindstorm mymindstorm Sep 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

this.type = OTPType.totp;
}

Copy link

Choose a reason for hiding this comment

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

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

if (this.type !== OTPType.hotp && this.type !== OTPType.hhex) {
this.generate();
Expand Down
55 changes: 38 additions & 17 deletions src/models/storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ export class EntryStorage {
encrypted,
hash: entry.hash,
index: entry.index,
type: OTPType[entry.type],
type: entry.type,
secret,
};

Expand Down Expand Up @@ -392,10 +392,7 @@ export class EntryStorage {
}

// remove unnecessary fields
if (
!(entry.type === OTPType[OTPType.hotp]) &&
!(entry.type === OTPType[OTPType.hhex])
) {
if (!(entry.type === OTPType.hotp) && !(entry.type === OTPType.hhex)) {
delete entry.counter;
}

Expand Down Expand Up @@ -478,7 +475,7 @@ export class EntryStorage {
algorithm: OTPAlgorithm;
pinned: boolean;
} = {
type: (parseInt(data[hash].type) as OTPType) || OTPType[OTPType.totp],
type: data[hash].type || OTPType.totp,
index: data[hash].index || 0,
issuer: data[hash].issuer || "",
account: data[hash].account || "",
Expand Down Expand Up @@ -617,29 +614,53 @@ export class EntryStorage {
}

if (!entryData.type) {
entryData.type = OTPType[OTPType.totp];
entryData.type = OTPType.totp;
Copy link
Member

@mymindstorm mymindstorm Sep 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

}

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

// We may have already had some error OTP type entries in the storage
// totp = 1
// hotp = 2
// battle = 3
// steam = 4
// hex = 5
// hhex = 6

if ((entryData.type as unknown) === 1) {
type = OTPType.totp;
} else if ((entryData.type as unknown) === 2) {
type = OTPType.hotp;
} else if ((entryData.type as unknown) === 3) {
type = OTPType.battle;
} else if ((entryData.type as unknown) === 4) {
type = OTPType.steam;
} else if ((entryData.type as unknown) === 5) {
type = OTPType.hex;
} else if ((entryData.type as unknown) === 6) {
type = OTPType.hhex;
} else {
type = OTPType.totp;
}
Copy link

Choose a reason for hiding this comment

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

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


entryData.type = type;
}

let period: number | undefined;
if (
entryData.type === OTPType[OTPType.totp] &&
entryData.type === OTPType.totp &&
entryData.period &&
entryData.period > 0
) {
Expand Down
Loading