-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Update cookie_store_test to use encrypted cookies #31146
Update cookie_store_test to use encrypted cookies #31146
Conversation
r? @kamipo (@rails-bot has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch 👍
Some minor issues, then we're good to go.
Rotations = ActiveSupport::Messages::RotationConfiguration.new | ||
|
||
Verifier = ActiveSupport::MessageVerifier.new(SessionSecret, digest: "SHA1") | ||
SignedBar = Verifier.generate(foo: "bar", session_id: SecureRandom.hex(16)) | ||
Encryptor = ActiveSupport::MessageEncryptor.new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's skip the parens with:
Encryptor = ActiveSupport::MessageEncryptor.new \
Generator…
@@ -106,8 +120,12 @@ def test_getting_session_id | |||
|
|||
def test_disregards_tampered_sessions | |||
with_test_route_set do | |||
cookies[SessionKey] = "BAh7BjoIZm9vIghiYXI%3D--123456780" | |||
encryptor = ActiveSupport::MessageEncryptor.new('A' * 32, cipher: "aes-256-gcm", serializer: Marshal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'A'
needs to use double quotes.
session_value = parse_cookie_from_header | ||
session_data = Encryptor.decrypt_and_verify(Rack::Utils.unescape(session_value)) rescue nil | ||
|
||
refute_nil session_data, "session failed to decrypt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer assert_not_nil
over refute_*
(can't remember why though).
7efa044
to
a774050
Compare
Looks like there's merge conflicts 😊 |
Rotations = ActiveSupport::Messages::RotationConfiguration.new | ||
|
||
Verifier = ActiveSupport::MessageVerifier.new(SessionSecret, digest: "SHA1") | ||
SignedBar = Verifier.generate(foo: "bar", session_id: SecureRandom.hex(16)) | ||
Encryptor = ActiveSupport::MessageEncryptor.new \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use parentheses here instead of \
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: c8f014f
This now modernizes these tests to use encrypted cookies instead of using secret_token HMACs. This commit also adds a tests to ensure session cookies with :expires_after set are invalidated and no longer accepted when the time has elapsed.
a774050
to
68b2573
Compare
Summary
This now modernizes these tests to use encrypted cookies instead of using secret_token HMACs. This commit also adds a test to ensure session cookies with
:expires_after
set are invalidated and no longer accepted when the time has elapsed./cc @assain @kaspth