Skip to content

fix: shorten email otp length #513

Merged
kangmingtay merged 10 commits intomasterfrom
km/fix-email-otp-length
Jul 5, 2022
Merged

fix: shorten email otp length #513
kangmingtay merged 10 commits intomasterfrom
km/fix-email-otp-length

Conversation

@kangmingtay
Copy link
Member

@kangmingtay kangmingtay commented Jun 25, 2022

What kind of change does this PR introduce?

  • Addresses concerns in fix: send otp in email link #379
  • Shorten email otp length to range between 6-10 digits
  • Email link token param is now a hash(email + otp)
  • Values stored in xxx_token in the db is now hash(email + otp) instead of just the otp
  • Email link verification works by matching the token param with the token field in the db (since both are hashed, we can just compare the hashes)
  • Email otp works by taking hash(email + otp) and comparing the value with the token field in the db (for sms otps, the hash will be hash(phone + otp))
  • support old otp format (to be deprecated within a week - current otps only last for 24 hrs so users will have to regenerate new ones regardless)
  • Potentially a breaking change if user has already built a UI to account for the old token format (xxxxx-xxxxx-xxxxx-xxxxx)

To-Dos

  • Fix tests
  • Generate link endpoint should also return the raw unhashed otp

@kangmingtay kangmingtay requested review from J0, hf and mansueli June 25, 2022 09:17
@kangmingtay kangmingtay self-assigned this Jun 25, 2022
@kangmingtay kangmingtay changed the title [WIP] fix: shorten email otp length fix: shorten email otp length Jun 28, 2022
Copy link
Contributor

@hf hf left a comment

Choose a reason for hiding this comment

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

Hey generally looks good, but I'd suggest using crypto/sha256 instead of crypto/md. It's quite old, and not particularly secure for this use case. It's only acceptable for computing message digests and not as a one-way function.

@hf
Copy link
Contributor

hf commented Jul 1, 2022

Actually for even more security you could use Sum224 which is also resistant to padding attacks.

@hf
Copy link
Contributor

hf commented Jul 1, 2022

One other remark, and this is really a nit, is to use encoding/hex as it expresses intent more clearly than fmt.Sprintf("%x", ...). I'd be OK with Base64 too.

@kangmingtay kangmingtay requested a review from hf July 1, 2022 18:37
@kangmingtay
Copy link
Member Author

One other remark, and this is really a nit, is to use encoding/hex as it expresses intent more clearly than fmt.Sprintf("%x", ...). I'd be OK with Base64 too.

decided not to use the encoding/hex package and stuck to fmt.Sprintf("%x", ...) instead because EncodeToString takes in a byte slice while sha256.Sum224 returns a [28]byte array

Copy link
Contributor

@hf hf left a comment

Choose a reason for hiding this comment

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

LGTM!

@hf
Copy link
Contributor

hf commented Jul 5, 2022

One other remark, and this is really a nit, is to use encoding/hex as it expresses intent more clearly than fmt.Sprintf("%x", ...). I'd be OK with Base64 too.

decided not to use the encoding/hex package and stuck to fmt.Sprintf("%x", ...) instead because EncodeToString takes in a byte slice while sha256.Sum224 returns a [28]byte array

Usually you can just do something like:

hex.EncodeToString(array[:])

To convert the array to a slice...

No need to block the PR on this though, if you want you can change it in another PR -- do it, if not, c'est la vie. 😸

Copy link
Contributor

@hf hf left a comment

Choose a reason for hiding this comment

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

Actually. This seems to not be working very well: https://go.dev/play/p/yKmLeyU2vmi

@hf
Copy link
Contributor

hf commented Jul 5, 2022

Actually. This seems to not be working very well: https://go.dev/play/p/yKmLeyU2vmi

LOL. No this uses Println not Printf. This is good to go.

@kangmingtay kangmingtay merged commit 397c949 into master Jul 5, 2022
@kangmingtay kangmingtay deleted the km/fix-email-otp-length branch July 5, 2022 09:23
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2022

🎉 This PR is included in version 2.7.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
* fix: add migrations to hash email

* add email otp length to config

* remove email hash migration

* send email hash & otp in email link

* verify post should check for token hash

* fix verify tests

* fix tests

* update generate_link endpoint

* remove magic number

* use sum224 instead of md5
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
* fix: add migrations to hash email

* add email otp length to config

* remove email hash migration

* send email hash & otp in email link

* verify post should check for token hash

* fix verify tests

* fix tests

* update generate_link endpoint

* remove magic number

* use sum224 instead of md5
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
* fix: add migrations to hash email

* add email otp length to config

* remove email hash migration

* send email hash & otp in email link

* verify post should check for token hash

* fix verify tests

* fix tests

* update generate_link endpoint

* remove magic number

* use sum224 instead of md5
cemalkilic pushed a commit that referenced this pull request Aug 7, 2025
* fix: add migrations to hash email

* add email otp length to config

* remove email hash migration

* send email hash & otp in email link

* verify post should check for token hash

* fix verify tests

* fix tests

* update generate_link endpoint

* remove magic number

* use sum224 instead of md5
xeladotbe pushed a commit to xeladotbe/supabase-auth that referenced this pull request Sep 27, 2025
* fix: add migrations to hash email

* add email otp length to config

* remove email hash migration

* send email hash & otp in email link

* verify post should check for token hash

* fix verify tests

* fix tests

* update generate_link endpoint

* remove magic number

* use sum224 instead of md5
fadymak pushed a commit that referenced this pull request Sep 30, 2025
* fix: add migrations to hash email

* add email otp length to config

* remove email hash migration

* send email hash & otp in email link

* verify post should check for token hash

* fix verify tests

* fix tests

* update generate_link endpoint

* remove magic number

* use sum224 instead of md5
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.

3 participants