reworks the message verifier to remove the magic -- character.

luckyframework / lucky

A full-featured Crystal web framework that catches bugs for you, runs incredibly fast, and helps you write code that lasts.

https://luckyframework.org

MIT License

2.59k stars 156 forks source link

reworks the message verifier to remove the magic -- character. #1674

Closed jwoertink closed 2 years ago

jwoertink commented 2 years ago

Purpose

Fixes #1595

Description

We were using a magic -- to split the data in the token generated by the message verifier. In some rare instances, the data on the left or the right of the magic character could also contain -- themselves. This caused issues when splitting on that.

This fix removes the magic character completely and just uses some JSON instead.

/cc @robacarp

Checklist

[x] - An issue already exists detailing the issue/or feature request that this PR fixes
[x] - All specs are formatted with crystal tool format spec src
[x] - Inline documentation has been added and/or updated
[x] - Lucky builds on docker with ./script/setup
[x] - All builds and specs pass on docker with ./script/test

robacarp commented 2 years ago

This is great @jwoertink. I agree with @matthewmcgarvey that detecting and rolling forward auth tokens would smooth out the release. Box it into a method that can be marked as deprecated and not forgotten. I'm sure even then some people will get an invalid token error when it is eventually removed, but that would help a ton.

jwoertink commented 2 years ago

This will now try to decode legacy tokens, and then basically fallback to new tokens. I've added the deprecation notice:

❯ crystal spec/
In src/lucky/support/message_verifier.cr:22:22

 22 | data, digest = legacy_verified(signed_message)
                     ^--------------
Warning: Deprecated Lucky::MessageVerifier#legacy_verified. Legacy token verification will be removed in the next release.

A total of 1 warnings were found.

jwoertink commented 2 years ago

Swapping the logic was a little weird, but specs passed. After we remove the deprecated legacy stuff, we should be able to clean this up more. Let me know if you guys have other suggestions for this.