Encrypt and sign cookies in the CookieJar.#2026
Merged
Conversation
jwoertink
commented
Apr 3, 2026
There was a problem hiding this comment.
Pull request overview
This pull request updates Lucky’s cookie encryption to include signature verification during decryption, addressing the rare (≈1/256) flaky failure caused by unauthenticated CBC decryption accepting random valid padding (Issue #2025).
Changes:
- Switch
Lucky::CookieJarto encrypt cookies viaMessageEncryptor#encrypt_and_signand decrypt via#verify_and_decrypt. - Add a legacy fallback path intended to support pre-signature (unsigned) cookies.
- Update padding-oracle reference links in
MessageEncryptordocumentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/lucky/support/message_encryptor.cr |
Documentation reference update for padding oracle attacks. |
src/lucky/cookies/cookie_jar.cr |
Moves cookie encryption to “encrypt + HMAC sign” format, and attempts to support legacy unsigned cookies during decryption. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
akadusei
reviewed
Apr 4, 2026
…ce the previous version was technically a vulnerability. Applying some suggestions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes #2025
Description
Warning
This is a breaking change because any cookies out there that were encrypted before this change won't decrypt properly.
I've seen this flaky spec pop up a few times in the past, but it's pretty rare... In fact, it's a 1/256 chance rare lol. This appears to be an oversight that's just never been caught. We've been missing the whole verification part of the encryption.
I'd like to get your opinion on this, @akadusei if you get some time
Checklist
crystal tool format spec src./script/setup./script/test