Use base 35 for generating secure tokens #24005
Conversation
The current implementation uses base58, which has issues with uniqueness on database tables with case insensitive collation (MySQL default). This uses base 35 to avoid that issue. Additionally, it does not use 0 and it uses only uppercase letters to avoid visually similar characters such as O and 0 or l and 1 * Use SecureRandom.random_number for token generation * Remove base58 method from SecureRandom (no longer used)
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
r? @matthewd |
|
LGTM, why the increase from 24 to 25 chars? |
I felt 128 bits was a reasonable amount of randomness, similar to uuid, and 128 bit numbers turned out to encode into 25 characters when using base 35. We can change it if people feel strongly about it. The current base58 implementation provides about 140 bits of randomness. A multiple of 8 bits does have the advantage that you can optimize performance by using something like |
|
I recently encountered another reason to use case insensitive tokens: QR codes are case insensitive (or rather, QR codes only support upper case letters when using alphanumeric encoding, which they do for urls). |
|
@HermanHiddema thank you for this PR, this was super helpful! @rafaelfranca @matthewd Can this be merged? |
|
@oyeanuj not as is, we don't remove old changelog and release note entries after of releases and we can't just remove |
| * Add `SecureRandom.base58` for generation of random base58 strings. | ||
|
|
||
| *Matthew Draper*, *Guillermo Iguaran* | ||
|
|
There was a problem hiding this comment.
Can leave the previous release's changelog as-is. It was already released.
| BASE58_ALPHABET[idx] | ||
| end.join | ||
| end | ||
| end |
There was a problem hiding this comment.
This is public API. Removing it means going through a deprecation cycle.
| @@ -732,9 +732,6 @@ Please refer to the [Changelog][active-support] for detailed changes. | |||
| `#tomorrow` for `Date`, `Time`, and `DateTime`. | |||
| ([Pull Request](httpshttps://github.com/rails/rails/pull/18335)) | |||
|
|
|||
| * Added `SecureRandom.base58` for generation of random base58 strings. | |||
| ([commit](https://github.com/rails/rails/commit/b1093977110f18ae0cafe56c3d99fc22a7d54d1b)) | |||
|
|
|||
There was a problem hiding this comment.
Ditto. Changing current behavior doesn't change the facts of a previous release.
| define_method("regenerate_#{attribute}") { update! attribute => self.class.generate_unique_secure_token } | ||
| before_create { self.send("#{attribute}=", self.class.generate_unique_secure_token) unless self.send("#{attribute}?")} | ||
| end | ||
|
|
||
| def generate_unique_secure_token | ||
| SecureRandom.base58(24) | ||
| SecureRandom.random_number(1<<128).to_s(35).upcase.tr('0','Z').rjust(25,'Z') |
There was a problem hiding this comment.
Moving the base35 implementation into this one usage means apps lose a convenient public way to generate a compact URL/QR/etc-safe token.
Perhaps it could be exposed as something like SecureRandom.random_token(bits, alphabet: …)
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Summary
The has_secure_token implementation currently uses base58 for its token generation, but as issue #20133 notes this can cause issues in databases with case insensitive collations. This PR fixes that issue, while keeping bits per character higher than hex and while keeping the advantage of base58 that there are no visually similar characters
Rationale
The original implementation (#18217) used
SecureRandom.hex, but the discussion already noted that it might be preferable to use base62 to increase the bit count per character, making for shorter tokens. This was done in a separate PR (#18347) which ultimately chose base58. Base58 removes visually similar characters (O and 0, l and 1) to allow for visual inspection of matching tokens. As #20133 notes, some databases use case insensitive collations (notably MySQL), which means that tokens that are different only in their case may be considered identical. The suggested fix to go back to hex seems suboptimal, as that increases the token length a lot.Implementation
This implementation uses base 35 (0..9A..Y), for 5.13 bits per character. It generates a 128 bit random number using
SecureRandom.random_numberand encodes it as a 25 character token string. The character0(zero) is replaced byZto avoid confusion withO, and the string is upcased to avoid confusion between1andl(lowercase L). The removal of 0 has the additional advantage that the token never has leading zeroes (some software, such as excel, could remove those if the whole token is numeric)This PR removes the base58 method from SecureRandom, as the token generation was the only place that method was used, so this has the advantage of not polluting that namespace.