fix: ensured that generateTOTP converts the digits and period params into numbers. (#25)

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>

Author: szabolcsnagy

index.js

@@ -56,22 +56,44 @@ async function generateHOTP(
 	)
 	const signature = await crypto.subtle.sign('HMAC', key, byteCounter)
 	const hashBytes = new Uint8Array(signature)
-
-	// Use more bytes for longer OTPs
-	const bytesNeeded = Math.ceil((digits * Math.log2(charSet.length)) / 8)
+	// offset is always the last 4 bits of the signature; its value: 0-15
 	const offset = hashBytes[hashBytes.length - 1] & 0xf
 
-	// Convert bytes to BigInt for larger numbers
 	let hotpVal = 0n
-	for (let i = 0; i < Math.min(bytesNeeded, hashBytes.length - offset); i++) {
-		hotpVal = (hotpVal << 8n) | BigInt(hashBytes[offset + i])
+	// the original specification allows any amount of digits between 4 and 10,
+	// so stay on the 32bit number if the digits are less then or equal to 10.
+	if (digits <= 10) {
+		// stay compatible with the authenticator apps and only use the bottom 32 bits of BigInt
+		hotpVal =
+			0n |
+			(BigInt(hashBytes[offset] & 0x7f) << 24n) |
+			(BigInt(hashBytes[offset + 1]) << 16n) |
+			(BigInt(hashBytes[offset + 2]) << 8n) |
+			BigInt(hashBytes[offset + 3])
+	} else {
+		// otherwise create a 64bit value from the hashBytes
+		hotpVal =
+			0n |
+			(BigInt(hashBytes[offset] & 0x7f) << 56n) |
+			(BigInt(hashBytes[offset + 1]) << 48n) |
+			(BigInt(hashBytes[offset + 2]) << 40n) |
+			(BigInt(hashBytes[offset + 3]) << 32n) |
+			(BigInt(hashBytes[offset + 4]) << 24n) |
+			// we have only 20 hashBytes; if offset is 15 these indexes are out of the hashBytes
+			// fallback to the bytes at the start of the hashBytes
+			(BigInt(hashBytes[(offset + 5) % 20]) << 16n) |
+			(BigInt(hashBytes[(offset + 6) % 20]) << 8n) |
+			BigInt(hashBytes[(offset + 7) % 20])
 	}
 
 	let hotp = ''
 	const charSetLength = BigInt(charSet.length)
 	for (let i = 0; i < digits; i++) {
 		hotp = charSet.charAt(Number(hotpVal % charSetLength)) + hotp
-		hotpVal = hotpVal / charSetLength
+
+		// Ensures hotpVal decreases at a fixed rate, independent of charSet length.
+		// 10n is compatible with the original TOTP algorithm used in the authenticator apps.
+		hotpVal = hotpVal / 10n
 	}
 
 	return hotp
@@ -149,8 +171,8 @@ export async function generateTOTP({
 	charSet = DEFAULT_CHAR_SET,
 } = {}) {
 	const otp = await generateHOTP(base32Decode(secret, 'RFC4648'), {
-		counter: getCounter(period),
-		digits,
+		counter: getCounter(Number(period)),
+		digits: Number(digits),
 		algorithm,
 		charSet,
 	})

index.test.js

@@ -165,3 +165,41 @@ test('generating a auth uri can be used to generate a otp that can be verified',
 	const result = await verifyTOTP({ otp, ...totpConfig })
 	assert.deepStrictEqual(result, { delta: 0 })
 })
+
+test('20 digits OTP should not pad with first character of charSet regardless of the charSet length', async () => {
+	const longCharSet = 'ABCDEFGHIJKLMNPQRSTUVWXYZ123456789'
+	const shortCharSet = 'ABCDEFGHIJK'
+
+	async function generate20DigitCodeWithCharSet(charSet) {
+		const iterations = 100
+		let allOtps = []
+
+		for (let i = 0; i < iterations; i++) {
+			const { otp } = await generateTOTP({
+				algorithm: 'SHA-256',
+				charSet,
+				digits: 20,
+				period: 60 * 30,
+			})
+			allOtps.push(otp)
+
+			// Verify the OTP only contains characters from the charSet
+			assert.match(
+				otp,
+				new RegExp(`^[${charSet}]{20}$`),
+				'OTP should be 20 characters from the charSet'
+			)
+
+			// The first 6 characters should not all be 'A' (first char of charSet)
+			const firstSixChars = otp.slice(0, 6)
+			assert.notStrictEqual(
+				firstSixChars,
+				'A'.repeat(6),
+				'First 6 characters should not all be A'
+			)
+		}
+	}
+
+	await generate20DigitCodeWithCharSet(shortCharSet)
+	await generate20DigitCodeWithCharSet(longCharSet)
+})