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.

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

Implement charSet options #4

Merged
merged 3 commits into from
Sep 13, 2023
Merged

Conversation

moishinetzer
Copy link
Contributor

This allows the user to define different charsets for the OTP to be in. This is incredibly useful for increasing the entropy and time-to-crack when dealing with brute force attacks.

When using a substantially larger character set, the increased complexity means, in theory, you could use this as a main form of authentication always rather than relying on a 2FA setup.

e.g. 10 digits at length 6 = 1 Million possibilities

However,

26 letters + 10 digits at length 6 = 36^6 = 2.1 Billion options.

With a rate limitting system, this is practically uncrackable.

This allows the user to define different charsets for the OTP to be in. This is incredibly useful for increasing the entropy and time-to-crack when dealing with brute force attacks.

When using a substantially larger character set, the increased complexity means, in theory, you could use this as a main form of authentication always rather than relying on a 2FA setup.

e.g. 10 digits at length 6 = 1 Million possibilities

However,

26 letters + 10 digits at length 6 = 36^6 = 2.1 Billion options.

With a rate limitting system, this is practically uncrackable.
@@ -17,7 +17,7 @@ test('options can be customized', () => {
algorithm: 'SHA256',
period: 60,
digits: 8,
secret: base32.encode(Math.random().toString(16).slice(2)),
secret: base32.encode(Math.random().toString(16).slice(2)).toString(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is put to a string because base32.encode() returns a Buffer but the official type signature is a string

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This looks awesome! Thank you for working on it!

I think this should get a special call out in the README. Basically how you described it in this PR would be a good addition to the README. Could you do that?

@kentcdodds
Copy link
Member

Looks like the charset needs to be returned as well

@@ -1,6 +1,6 @@
import assert from 'node:assert'
import { test, afterEach } from 'node:test'
import * as base32 from 'thirty-two'
import { test } from 'node:test'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

afterEach is unused

import { test, afterEach } from 'node:test'
import * as base32 from 'thirty-two'
import { test } from 'node:test'
import base32 from 'thirty-two'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kentcdodds can you confirm this works for you locally? I found that it fixed some type errors in my project when I imported it this way. Happy to revert if it breaks on your side

Copy link
Member

Choose a reason for hiding this comment

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

If it works for you and CI then it's good enough for me :)

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Solid. Thank you!

import { generateTOTP, verifyTOTP } from '@epic-web/totp'

const { otp, secret, period, digits, algorithm, charSet } = generateTOTP({
charSet: 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789', // custom character set
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
charSet: 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789', // custom character set
charSet: 'ABCDEFGHIJKLMNPQRSTUVWXYZ123456789', // custom character set

Comment on lines +205 to +206
Just as an aside, you probably want to exclude the letter O and the number 0 to
make it easier for users to enter the code.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Just as an aside, you probably want to exclude the letter O and the number 0 to
make it easier for users to enter the code.
Just as an aside, you probably want to exclude the letter O and the number 0 as
we do above to make it easier for users to enter the code.

import { test, afterEach } from 'node:test'
import * as base32 from 'thirty-two'
import { test } from 'node:test'
import base32 from 'thirty-two'
Copy link
Member

Choose a reason for hiding this comment

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

If it works for you and CI then it's good enough for me :)

@kentcdodds kentcdodds merged commit 05d468b into epicweb-dev:main Sep 13, 2023
2 checks passed
@moishinetzer
Copy link
Contributor Author

Pleasure, thanks for your work on creating this package!

@github-actions
Copy link

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants