Skip to content
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 Purpose Metadata to Cookies #32937

Merged
merged 1 commit into from Aug 12, 2018
Merged

Conversation

assain
Copy link
Contributor

@assain assain commented May 20, 2018

Summary
This PR adds purpose metadata to cookies so that the value of one cookie cannot be copied and used as the value of another cookie.

Other Information
Turn on config.action_dispatch.use_cookies_with_metadata to embed expiry and purpose metadata inside signed/encrypted cookies, and to verify if signed/encrypted cookie values have been copy-pasted. Previously set cookies (i.e. without purpose or expiry metadata) will continue to be honored.

@rails-bot
Copy link

r? @georgeclaghorn

(@rails-bot has picked a reviewer for you, use r? to override)

@assain
Copy link
Contributor Author

assain commented May 20, 2018

r? @kaspth
@guilleiguaran

@rails-bot rails-bot assigned kaspth and unassigned georgeclaghorn May 20, 2018
@@ -1300,6 +1300,22 @@ def test_vanilla_cookie_with_expires_set_relatively
end
end

def test_purpose_metadata_for_signed_cookies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want the two new tests to set the ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata config to true

Copy link
Contributor Author

@assain assain May 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests related to purpose and expiry metadata are failing because I can't switch on - ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata properly, which is set to false by default.

I tried setting ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata config to true within the new tests, but don't know why it breaks a lot of other tests. Would it be because of the way the tests are run ?

I tried switching it to true in the metadata tests like this -

  def test_purpose_metadata_for_encrypted_cookies
    ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata = true
    cookies.encrypted[:user_id] = 50
    cookies.encrypted[:discount_percentage] = 10

    cookies[:discount_percentage] = cookies[:user_id]
    assert_nil cookies.encrypted[:discount_percentage]
    # ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata = false
  end

I also tried creating a subclass of CookiesTest with setup and teardown methods. But it screws up everything and other kinds of experimentation.

Could you suggest me the right way to do this? 😕
@Edouard-chin @kaspth @guilleiguaran

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably because ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata = false was never run since the execution stoped when the assertion assert_nil cookies.encrypted[:discount_percentage] failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had tried both. Are tests run parallelly ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not, I tried running the tests and they work fine with something like

Diff
diff --git a/actionpack/test/dispatch/cookies_test.rb b/actionpack/test/dispatch/cookies_test.rb
index aa9e4dac42..9a822c4f44 100644
--- a/actionpack/test/dispatch/cookies_test.rb
+++ b/actionpack/test/dispatch/cookies_test.rb
@@ -1301,19 +1301,27 @@ def test_vanilla_cookie_with_expires_set_relatively
   end

   def test_purpose_metadata_for_signed_cookies
+    previous_config = ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata
+    ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata = true
     cookies.signed[:user_id] = 50
     cookies.signed[:discount_percentage] = 10

     cookies[:discount_percentage] = cookies[:user_id]
     assert_nil cookies.signed[:discount_percentage]
+  ensure
+    ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata = previous_config
   end

   def test_purpose_metadata_for_encrypted_cookies
+    previous_config = ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata
+    ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata = true
     cookies.encrypted[:user_id] = 50
     cookies.encrypted[:discount_percentage] = 10

     cookies[:discount_percentage] = cookies[:user_id]
     assert_nil cookies.encrypted[:discount_percentage]
+  ensure
+    ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata = previous_config
   end

Only two tests are failing related to expiry, but it makes sense since the expiry option is now set solely when use_cookies_with_metadata is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Edouard-chin for helping me out with your expertise. Got to make a few more tweaks to the PR 😄

cookies.signed[:discount_percentage] = 10

cookies[:discount] = cookies[:user_id]
assert_nil cookies.signed[:discount_percentage]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be assert_nil cookies.signed[:discount] instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Edouard-chin Yeah, my bad 😅

@@ -1318,4 +1334,4 @@ def assert_not_cookie_header(expected)
assert_not_equal expected.split("\n"), header
end
end
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Edouard-chin I've made some more changes 😄

cookies[:discount] = cookies[:user_id]
assert_nil cookies.encrypted[:discount_percentage]
end

Copy link
Contributor Author

@assain assain May 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests related to purpose and expiry metadata are failing, because I can't switch on - ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata which is set to false by default.
I tried turning it to true in the metadata tests like this -

  def test_purpose_metadata_for_encrypted_cookies
    ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata = true
    cookies.encrypted[:user_id] = 50
    cookies.encrypted[:discount_percentage] = 10

    cookies[:discount] = cookies[:user_id]
    assert_nil cookies.encrypted[:discount_percentage]
    # ActionDispatch::Cookies::AbstractCookieJar.use_cookies_with_metadata = false
  end

I also tried creating a subclass of CookiesTest with setup and teardown methods. But it screws up everything and other kinds of experimentation.

Could you suggest me the right way to do this? 😕

@assain assain force-pushed the add-purpose-to-cookies branch 2 times, most recently from 290e675 to d898409 Compare June 8, 2018 18:31
@assain assain changed the title Add purpose metadata to cookies. Add Purpose Metadata to Cookies Jun 8, 2018
@assain assain force-pushed the add-purpose-to-cookies branch 3 times, most recently from 6819834 to a8c88cd Compare June 10, 2018 07:04
mjc-gh
mjc-gh approved these changes Jun 10, 2018
Copy link
Contributor

@mjc-gh mjc-gh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall! Nice work!!

@@ -489,11 +497,30 @@ def []=(name, options)
def request; @parent_jar.request; end

private
def expiry_options(options)
def fetch_metadata(options)
metadata = Hash.new
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could make this a bit more concise with tap and a Hash literal:

{ purpose: options[:cookie_name] }.tap do |metadata|
  # set expires_in or expires_at...
end

metadata[:expires_at] = options[:expires]
end
end
end
Copy link
Contributor Author

@assain assain Jun 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikeycgto Was this what you had suggested? 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, exactly!

# for how long those cookies should be honored as a Time or ActiveSupport::Duration object.

# Also, setting this value to nil or to a date in the past rejects the existing cookies upfront.
Rails.application.config.action_dispatch.fail_cookies_without_metadata_after = <%= 1.month.from_now %>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikeycgto @kaspth @guilleiguaran
Travis CI's GEM = aj:integration fails with a syntax error because of the output of <%= 1.month.from_now %>. Should I convert <%= 1.month.from_now %> into a string or any other format? Or am I missing anything?

@assain assain force-pushed the add-purpose-to-cookies branch 3 times, most recently from aa4cd7e to cf88f3b Compare June 11, 2018 10:12
# for how long those cookies should be honored as a Time or ActiveSupport::Duration object.

# Also, setting this value to nil or to a date in the past rejects the existing cookies upfront.
Rails.application.config.action_dispatch.fail_cookies_without_metadata_after = <%= ActiveSupport::JSON.encode 1.month.from_now %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilleiguaran Yes, that's the place we should put this.

Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial pass! Looks like this mostly gets the job done, but the flow and readability isn't quite where we need it. But that's good! Now we have a base to expand on.

Left lots of questions to hopefully guide you in your further decision making.

cookies.signed[:user_name] = { value: "assain", expires: 2.hours }

travel 1.hour
assert_equal "assain", cookies.signed[:user_name]

travel 2.hours
assert_nil cookies.signed[:user_name]

ensure
switch_off_cookies_with_metadata
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having these ensures spread throughout is a bad practice.

Switch to:

with_cookie_metadata_enabled do
  # …
end

Remember to capture the old value in a local variable too and reassign it back.

# for how long those cookies should be honored as a Time or ActiveSupport::Duration object.

# Also, setting this value to nil or to a date in the past rejects the existing cookies upfront.
Rails.application.config.action_dispatch.fail_cookies_without_metadata_after = <%= ActiveSupport::JSON.encode 1.month.from_now %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilleiguaran Yes, that's the place we should put this.

# for how long those cookies should be honored as a Time or ActiveSupport::Duration object.

# Also, setting this value to nil or to a date in the past rejects the existing cookies upfront.
Rails.application.config.action_dispatch.fail_cookies_without_metadata_after = <%= ActiveSupport::JSON.encode 1.month.from_now %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you make this a JSON value here? What's the benefit of this way, as opposed to deferring it? How does this choice mesh with our goal of programmer happiness and readability?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is use_cookies_with_metadata set to true? Seems like we'd ship something that would never actually use metadata? And is it the best approach to split this over 2 options? E.g. is the mental overhead of 2 toggles worth it for end users?

# If you don't want your existing cookies to be rejected immediately you need to specify a window
# for how long those cookies should be honored as a Time or ActiveSupport::Duration object.

# Also, setting this value to nil or to a date in the past rejects the existing cookies upfront.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put yourself in the shoes of a person who's upgrading their Rails app. What would they want to know here?

def expiry_options(options)
if options[:expires].respond_to?(:from_now)
{ expires_in: options[:expires] }
def fetch_metadata(options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this named fetch? How are we fetching the metadata? And is it the input that's the most important thing here? Do we regularly call methods to pass something in or to get something out?

end
rescue ActiveSupport::MessageEncryptor::InvalidMessage, ActiveSupport::MessageVerifier::InvalidSignature
parse_legacy_signed_message(name, encrypted_message)
end

def commit(options)
options[:value] = @encryptor.encrypt_and_sign(serialize(options[:value]), expiry_options(options))
if cookies_with_metadata?
options[:value] = @encryptor.encrypt_and_sign(serialize(options[:value]), fetch_metadata(options))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this method concerned with cookies_with_metadata?? Especially when there's a method that's called fetch_metadata? Same for the other instances.

@@ -481,6 +487,8 @@ def []=(name, options)
options = { value: options }
end

options[:cookie_name] = name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this break old Rails apps as we're always writing the cookie_name to the cookie? E.g. would a cookie from 5.2 (e.g. no metadata) be readable on 6.0? And would a metadata attached cookie from 6.0 be readable on 5.2? This is one of the most important things we have to keep in mind, so do our tests test these scenarios sufficiently and do they do so in a sufficiently realistic manner?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like assigning straight to the options here. They'll be committed in the cookie body after all. Have you tried passing the name in to commit or moving the metadata construction up to here?

end
value
else
@encryptor.decrypt_and_verify(encrypted_message, on_rotation: rotate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling @encryptor.decrypt_and_verify(encrypted_message, on_rotation: rotate) 3 times here is a smell.

if value.nil? && read_cookies_without_metadata?
value = @verifier.verified(signed_message, on_rotation: rotate)
end
value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also smelly.

if options[:expires].respond_to?(:from_now)
{ expires_in: options[:expires] }
def fetch_metadata(options)
{ purpose: options[:cookie_name] }.tap do |metadata|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think the tap here blurs the flow. Is building the metadata at the same level of abstraction as normalizing the expiry? If not, how can we compose this code better?

@assain assain force-pushed the add-purpose-to-cookies branch 3 times, most recently from 9e9aff9 to 490cfcb Compare June 18, 2018 16:08
@assain
Copy link
Contributor Author

assain commented Jun 18, 2018

@kaspth Here's the suggested changes 😄

config.action_dispatch.cookies_with_purpose_metadata = {
switch_on: false,
honor_cookies_wihout_metadata_till: nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a typo here, I'll fix that...

Copy link
Member

@sikachu sikachu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do feel weird about having a hash ({ switch_on: true }) as a way to enable the feature. Is there a particular reason why we can't make falsy as an off state and any object as an on state?

i.e.

.cookies_with_purpose_metadata = false # off
.cookies_with_purpose_metadata = true # on, `honor_cookies_without_metadata_till` is nil
.cookies_with_purpose_metadata = { honor_cookies_without_metadata_till: 30.days.from_now }

Also, I'm curious about the wording of honor_cookies_without_metadata_till. I think expanding till to until would make it reads better, and maybe we can change honor to allow to, just to make it more inline of other flags that available in Rails (assuming it still conveys the same message):

config.action_dispatch.cookies_with_purpose_metadata = {
  allow_cookies_without_metadata_until: 30.days.from_now
}

Please let me know what you think. Thank you for your patch.

# before they're rejected so that your application behaviour is not broken immediately.
#
# Rails.application.config.action_dispatch.cookies_with_purpose_metadata = {
# switch_on: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind adding some indentation for line 21 and 22 too?

@assain assain force-pushed the add-purpose-to-cookies branch 8 times, most recently from ae121b9 to 5d03ffc Compare August 11, 2018 18:22
Purpose metadata prevents cookie values from being
copy-pasted and ensures that the cookie is used only
for its originally intended purpose.

The Purpose and Expiry metadata are embedded inside signed/encrypted
cookies and will not be readable on previous versions of Rails.

We can switch off purpose and expiry metadata embedded in
signed and encrypted cookies using

	config.action_dispatch.use_cookies_with_metadata = false

if you want your cookies to be readable on older versions of Rails.
kaspth
kaspth approved these changes Aug 12, 2018
Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️👏

Let's clean up the test things and add a changelog in a separate PR. I don't want to hold this up any longer 😄

assert_equal "5-2-Stable Chocolate Cookies", cookies.encrypted[:favorite]

freeze_time do
travel 1001.years
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

freeze_time just does travel_to Time.now, so you could just do travel 1001.years do.

@kaspth kaspth merged commit 14d3c7c into rails:master Aug 12, 2018
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Aug 30, 2018
…"Configuring Rails Applications" guide [ci skip]

Related to rails#32937, rails#33605.
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Aug 30, 2018
kamipo added a commit that referenced this pull request Aug 30, 2018
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Feb 1, 2019
- There is a regression in 6.0 introduced by rails#32937 where cookie
  doesn't expire anymore unless the new `use_cookies_with_metadata`
  configuration is set to `true`.

  This causes issue for app migration from 5.2 to 6.0 because the
  `use_cookies_with_metadata` flag can't be set to true until all
  servers are running on 6.0.

  Here is a small reproduction script that you can run in the console

  ```ruby
  ActionDispatch::Cookies

  request = ActionDispatch::Request.empty
  request.env["action_dispatch.key_generator"] = ActiveSupport::KeyGenerator.new('1234567890')
  request.env["action_dispatch.signed_cookie_salt"] = 'signed cookie'
  request.env["action_dispatch.cookies_rotations"] = ActiveSupport::Messages::RotationConfiguration.new
  request.env["action_dispatch.use_authenticated_cookie_encryption"] = true
  signed_cookie = request.cookie_jar.signed
  signed_cookie[:foobar] = { value: '123', expires: 1.day.ago }
  p signed_cookie[:foobar]
  ```
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Feb 4, 2019
- There is a regression in 6.0 introduced by rails#32937 where cookie
  doesn't expire anymore unless the new `use_cookies_with_metadata`
  configuration is set to `true`.

  This causes issue for app migration from 5.2 to 6.0 because the
  `use_cookies_with_metadata` flag can't be set to true until all
  servers are running on 6.0.

  Here is a small reproduction script that you can run in the console

  ```ruby
  ActionDispatch::Cookies

  request = ActionDispatch::Request.empty
  request.env["action_dispatch.key_generator"] = ActiveSupport::KeyGenerator.new('1234567890')
  request.env["action_dispatch.signed_cookie_salt"] = 'signed cookie'
  request.env["action_dispatch.cookies_rotations"] = ActiveSupport::Messages::RotationConfiguration.new
  request.env["action_dispatch.use_authenticated_cookie_encryption"] = true
  signed_cookie = request.cookie_jar.signed
  signed_cookie[:foobar] = { value: '123', expires: 1.day.ago }
  p signed_cookie[:foobar]
  ```
@rhymes rhymes mentioned this pull request May 23, 2020
12 tasks
vipulnsward pushed a commit that referenced this pull request Feb 6, 2021
The use_cookies_with_metadata option was created in 
#32937 and documented in 
#33757. At the time, the option 
controlled both purpose and expiry metadata. Later 
#35134 changed the option to control 
only the purpose metadata, but the documentation was not updated. I've 
done that here.
brendon pushed a commit to brendon/rails that referenced this pull request Jun 17, 2021
The use_cookies_with_metadata option was created in 
rails#32937 and documented in 
rails#33757. At the time, the option 
controlled both purpose and expiry metadata. Later 
rails#35134 changed the option to control 
only the purpose metadata, but the documentation was not updated. I've 
done that here.
wbotelhos pushed a commit to wbotelhos/rails that referenced this pull request Jul 18, 2021
The use_cookies_with_metadata option was created in 
rails#32937 and documented in 
rails#33757. At the time, the option 
controlled both purpose and expiry metadata. Later 
rails#35134 changed the option to control 
only the purpose metadata, but the documentation was not updated. I've 
done that here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants