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
Add expiry metadata to Cookies and freshen expires option to support duration #30121
Add expiry metadata to Cookies and freshen expires option to support duration #30121
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (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. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
:expires_in
& :expires_at
elsif expires_in | ||
options[:expires] = expires_in.from_now | ||
end | ||
end |
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.
Rack::Utils.add_cookie_to_header uses the :expires option to set the expiration date, so I decided to set expiration using it.
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.
This makes sense to me. The expiration set for a cookie can match the expiration for the underlying Verified/Encrypted message. This would definitely mitigate the "valid forever" cookie problem as the messages will now have their own expiration builtin.
@@ -569,7 +580,7 @@ def parse(name, signed_message) | |||
end | |||
|
|||
def commit(options) | |||
options[:value] = @verifier.generate(serialize(options[:value])) | |||
options[:value] = @verifier.generate(serialize(options[:value]), expires_at: options[:expires_at], expires_in: options[:expires_in]) | |||
|
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.
Currently, just the value gets serialized not the wrapped message with metadata since it uses NullSerailizer
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.
@kaspth
Does the following sound reasonable?
Set the verifier to use the serializer used by cookies instead of NullSerializer, this way the wrapped message could be serialized with the cookie_serializer.
To preserve backwards compatibility we could have a switch for it.
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.
The verifier/encryptor is unaware of the serialization for cookies since custom serializers could be used for cookies. This method here is where a completely custom serializer
could come into play: https://github.com/assain/rails/blob/931257defaecd16212d3b6853d5ca2177f8d0700/actionpack/lib/action_dispatch/middleware/cookies.rb#L547-L557
def test_cookie_expiration_using_expires_in | ||
freeze_time do | ||
get :set_expiration_using_expires_in | ||
assert_cookie_header "user_name=assain; path=/; expires=#{1.hour.from_now.rfc2822}" |
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.
What about timezones here in this test case? Should the 1.hour.from_now
be converted to UTC first?
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.
Not liking the dynamic interpolated time here. We should use a fixed time like the other tests do. E.g. travel_to Time.new(2017, …) do
@@ -458,6 +460,15 @@ def make_set_cookie_header(header) | |||
def write_cookie?(cookie) | |||
request.ssl? || !cookie[:secure] || always_write_cookie | |||
end | |||
|
|||
def set_expires_attribute(options) | |||
expires_at, expires_in = options[:expires_at], options[:expires_in] |
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.
These should probably delete
.
Might be worth asserting that only one of :expires
, :expires_at
, :expires_in
is set?
I do think that, even if we add these friendlier aliases, :expires
should remain fully supported. In fact, alternative API suggestion: only use :expires
, but make both expires: DateTime
and expires: Duration
DWIM.
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.
only use :expires, but make both expires: DateTime and expires: Duration DWIM.
Ah yeah, let's just go with that here
Then we only have to convert expires: 1.hour
into an absolute time via options[:expires] = options[:expires].from_now if options[:expires].respond_to?(:from_now)
.
And pass it in to the verifier/encryptor via expires_at: options[:expires]
.
end | ||
|
||
def set_expiration_using_expires_at | ||
cookies["user_name"] = { value: "assain", expires_at: Time.now.advance(years: 1)} |
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.
Needs a space before }
|
||
def set_signed_cookie_expiration_using_expires_in | ||
cookies.signed["user_name"] = { value: "assain", expires_in: 1.hour } | ||
head :ok |
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.
Don't controllers respond with head :no_content
by default? So that these head
lines can be omitted?
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.
When switching to expires
supporting both durations and time we only need to test signed cookies and how their expiry is enforced.
E.g. do as this test but add travel_to <after_expiry>
and assert_nil
on the signed cookie.
def test_cookie_expiration_using_expires_in | ||
freeze_time do | ||
get :set_expiration_using_expires_in | ||
assert_cookie_header "user_name=assain; path=/; expires=#{1.hour.from_now.rfc2822}" |
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.
Not liking the dynamic interpolated time here. We should use a fixed time like the other tests do. E.g. travel_to Time.new(2017, …) do
|
||
expected_time = 2.hours.from_now.utc.rfc2822 | ||
assert_cookie_header "user_name=assain; path=/; expires=#{expected_time}" | ||
end |
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.
@kaspth
Previously you'd admonished that interpolating was a bad idea. However, I couldn't figure out the best way to test this yet
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.
Don't use freeze_time
then. Maybe travel_to Time.parse("some httpdate formatted time with timezone")
, then put that into the httpdate
time into the expires=
like some of the other tests do.
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.
I think this test can be written like the two above it. That is, we can travel within the expiration window, assert we can read it, then travel to outside the expiration window and assert it's nil.
options[:value] = @encryptor.encrypt_and_sign(serialize(options[:value])) | ||
if options[:expires].respond_to?(:from_now) | ||
options[:expires] = options[:expires].from_now | ||
end |
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.
@kaspth
Should we extract this into a method?
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 can't share that implementation between the general cookie jar and the abstract cookie jar. So maybe we should put a handle_options
before commit
is called.
Something like:
private
delegate :handle_options, to: :@parent_jar
Then we'll eventually hit the CookieJar
handle_options
. Though it might be better to define a preprocess_options
that specifically does the from_now
handling.
Shows another fault in the cookies code, that we still have duplicated and split options processing, but I guess that's for later.
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.
There's a failing test that now has it's signed data changed because we're verifying the integrity of the expiry.
|
||
def cookie_expires_in_two_hours | ||
cookies[:user_name] = { value: "assain", expires: 2.hours } | ||
head :ok |
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.
Why do we need the head :ok
don't these default to head :no_content
?
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.
@kaspth
But, deleting the line throws ActionController::UnknownFormat:
ActionController::UnknownFormat: CookiesTest::TestController#cookie_expires_in_two_hours is missing a template for this request format and variant.
|
||
expected_time = 2.hours.from_now.utc.rfc2822 | ||
assert_cookie_header "user_name=assain; path=/; expires=#{expected_time}" | ||
end |
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.
Don't use freeze_time
then. Maybe travel_to Time.parse("some httpdate formatted time with timezone")
, then put that into the httpdate
time into the expires=
like some of the other tests do.
options[:value] = @encryptor.encrypt_and_sign(serialize(options[:value])) | ||
if options[:expires].respond_to?(:from_now) | ||
options[:expires] = options[:expires].from_now | ||
end |
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 can't share that implementation between the general cookie jar and the abstract cookie jar. So maybe we should put a handle_options
before commit
is called.
Something like:
private
delegate :handle_options, to: :@parent_jar
Then we'll eventually hit the CookieJar
handle_options
. Though it might be better to define a preprocess_options
that specifically does the from_now
handling.
Shows another fault in the cookies code, that we still have duplicated and split options processing, but I guess that's for later.
@@ -488,6 +497,8 @@ def []=(name, options) | |||
def request; @parent_jar.request; end | |||
|
|||
private | |||
delegate :preprocess_options, to: :@parent_jar |
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.
@kaspth
This way handle_options work is not duplicated.
@@ -378,6 +378,12 @@ def handle_options(options) #:nodoc: | |||
end | |||
end | |||
|
|||
def preprocess_options(options) |
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.
Could this method's logic just be moved to the handle_options
method?
@@ -480,6 +487,8 @@ def []=(name, options) | |||
options = { value: options } | |||
end | |||
|
|||
preprocess_options(options) |
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.
Two lines after this the call @parent_jar[name]
will lead to preprocess_options
being invoked in the @parent_jar
. Thus, I think this call maybe unnecessary...
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.
@mikeycgto
Correct me if I'm wrong:
We now want :expires
to support duration, i.e. we want to specify the duration for which the cookie is valid like this:
cookies.signed[:user_name] = { value: "bob", expires: 2.hours }
And since we're tacking on the value of options[:expires]
to :expires_at
in case of signed / encrypted cookies, isn't it necessary to do preprocess_options
before commit method is called, since @parent_jar[name] = options
is only called later on.
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.
If I'm following this code correctly, I think calling @parent_jar[name] = options
here will lead to preprocess_options
being called again on line 398 in the parent jar. Thus, maybe we only need to call it once in the parent jar itself?
You are right that we do need the options to be "converted" before the cookie is actually written.
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.
The tough part is that we need preprocess_options
to happen before commit(options)
(so options[:expires]
can be passed to :expires_at
. When @parent_jar[name] = options
comes along we'll hit preprocess_options
again, yes.
Perhaps there's two things going on here that happen to look similar.
E.g. there's the duration support in :expires
, but then there's the conversion that Messages::Metadata
handles.
We could stash the from_now
thing in handle_options
and then have a separate method to handle the chained cookie jar cases.
def expiry_options
if options[:expires].is_a?(ActiveSupport::Duration)
{ expires_in: options[:expires] }
else
{ expires_at: options[:expires] }
end
end
That would spare us the complex delegation up the chained jars but cost us a type check. We could perhaps use respond_to?(:from_now)
in both cases.
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 could perhaps use respond_to?(:from_now) in both cases"
@kaspth are you suggesting to stick onto delegation? Or make the changes using ActiveSupport::Duration?
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.
No, I meant respond_to?(:from_now)
in lieu of is_a?
in my proposed expiry_options
.
@@ -378,6 +378,12 @@ def handle_options(options) #:nodoc: | |||
end | |||
end | |||
|
|||
def preprocess_options(options) |
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.
Remember to # :nodoc:
this.
@@ -389,6 +395,7 @@ def []=(name, options) | |||
options = { value: value } | |||
end | |||
|
|||
preprocess_options(options) | |||
handle_options(options) |
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 should probably just call preprocess_options
in handle_options
as well.
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.
Of course, if you know why it isn't necessary for delete
and deleted?
do explain and we'll just leave this as is.
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 should probably just call preprocess_options in handle_options as well." Did you mean to say: "delete the function call here and add it to handle_options?"
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.
That was what I meant then… but now I'm more interested in #30121 (comment) and #30121 (comment)
def test_vanilla_cookie_with_expires_set_relatively | ||
travel_to Time.utc(2017, 8, 15) do | ||
get :cookie_expires_in_two_hours | ||
assert_cookie_header "user_name=assain; path=/; expires=Tue, 15 Aug 2017 02:00:00 -0000" |
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.
Won't this return a different time zone depending on where you run it?
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.
Should I change this?
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.
Only if it needs to change, which is what I'm asking you about
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.
I used Time.utc
method, since there were other tests using it while setting the :expires
option. Was that method the root of your concern?
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.
Ah, read utc
as new
, which isn't the same. But now I'm a little curious why the time is "02:00" and not "00:00".
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.
In get :cookie_expires_in_two_hours
the cookie is being set to expire in two hours and since:
Time.utc(2017, 8, 15) => 2017-08-15 00:00:00 UTC
Two hours from_now in rfc 2822:
Tue, 15 Aug 2017 02:00:00 -0000
Hope, I got your question right
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.
Ah right, all good
{ expires_at: options[:expires] } | ||
end | ||
end | ||
|
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.
@kaspth
Here's the suggested changes!
@@ -292,7 +297,7 @@ def test_session_store_with_expire_after | |||
end | |||
|
|||
# Second request does not access the session | |||
time = Time.local(2008, 4, 25) | |||
time = Time.local(2008, 4, 24) | |||
Time.stub :now, time do |
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.
Are these changes to the test okay?
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.
One final comment, otherwise I think it's there codewise.
We should update the documentation as well.
We'll also need entries in the Action Pack changelog:
- That
:expires
supportsActiveSupport::Duration
s - That cookie expiry integrity is now enforced for signed/encrypted cookies
Write a title and short description for each.
Later we'll add something to the upgrading guide.
@@ -292,7 +297,7 @@ def test_session_store_with_expire_after | |||
end | |||
|
|||
# Second request does not access the session | |||
time = Time.local(2008, 4, 25) | |||
time = Time.local(2008, 4, 24) |
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.
Why do we have to change the date here?
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.
cookie_body
at line 294 contains the message with expiry metadata time = Time.local(2008, 4, 24)
five hours from this time.
At line 307, the cookie_body
contains the value from 294, but its expected expiry is changed by 1 day. Consequently, the assertion fails because the time is stubbed at: Time.local(2008, 4, 25)
.
Hope I didn't goof up again
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.
But then why did it pass before?
Just being frank: I'm not convinced you have a sufficient understanding of what's going on here, to allow this change to happen. It's your job to prove you know why changes are required.
I think it is also worth mentioning the expiry integrity in the security
guide. Definitely a major step forward for Rails' security overall
|
@@ -299,7 +304,7 @@ def test_session_store_with_expire_after | |||
get "/no_session_access" | |||
assert_response :success | |||
|
|||
assert_equal "_myapp_session=#{cookie_body}; path=/; expires=#{expected_expiry}; HttpOnly", | |||
assert_equal "_myapp_session=#{cookies[SessionKey]}; path=/; expires=#{expected_expiry}; HttpOnly", | |||
headers["Set-Cookie"] | |||
end |
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.
Here's the suggested changes @kaspth
@kaspth, The checks have passed |
Indeed they have! |
Great addition! |
@kaspth
This PR adds:
:expires
option, i.e. you can specify when the cookie should expire relatively.e.g.
cookies.signed[:user_name] = { value: "bob", expires: 2.hours }