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.

Sign up for GitHub

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

Jump to bottom

Add Documentation For Duration Support & Expiry Meta Data Added to Signed / Encrypted Cookies #30407

Merged
merged 1 commit into from
Sep 4, 2017
Merged

Add Documentation For Duration Support & Expiry Meta Data Added to Signed / Encrypted Cookies #30407

merged 1 commit into from
Sep 4, 2017

Conversation

assain
Copy link
Contributor

@assain assain commented Aug 25, 2017 •
edited

This PR is an addendum to #30121 with the following additions:

  • Documentation for duration support added to :expires option of cookies.
  • Changelog entries for the duration support and expiry meta data added to signed/encrypted cookies.

@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 Aug 25, 2017

r? @kaspth

@rails-bot rails-bot assigned kaspth and unassigned georgeclaghorn Aug 25, 2017
assain
assain commented Aug 25, 2017
View reviewed changes
actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -100,7 +103,7 @@ def cookies_digest
# cookies.permanent[:login] = "XJ-122"
#
# # You can also chain these methods:
# cookies.permanent.signed[:login] = "XJ-122"
# cookies.signed.permanent[:login] = "XJ-122"
#
Copy link
Contributor Author

@assain assain Aug 25, 2017 •
edited

Choose a reason for hiding this comment

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

@kaspth Is it alright if I make this change? 😄

kaspth
kaspth reviewed Aug 28, 2017
View reviewed changes
actionpack/CHANGELOG.md Outdated
@@ -1,3 +1,29 @@
* Cookies `:expires` option supports `ActiveSupport::Duration`

Earlier, we had to specify the cookie expiry using a time object.
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have to mention "Earlier", that's implied by us saying we've added something.

actionpack/CHANGELOG.md Outdated
But now, you can now specify when the cookie expires using an `ActiveSupport::Duration`
object as well.

For example,
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to say for example, just let the text flow.

actionpack/CHANGELOG.md Outdated
For example,

cookies[:user_name] = { value: "assain", expires: 1.hour }
cookies[:oreo] = { value: "chocolate cookies", expires: 6.months }
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent by 4 more spaces to highlight it as code.

actionpack/CHANGELOG.md Outdated
@@ -1,3 +1,29 @@
* Cookies `:expires` option supports `ActiveSupport::Duration`
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a period here.

actionpack/CHANGELOG.md Outdated

Earlier, we had to specify the cookie expiry using a time object.
But now, you can now specify when the cookie expires using an `ActiveSupport::Duration`
object as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also just leave this out. It's clear enough from the example.

actionpack/CHANGELOG.md Outdated

*Assain Jaleel*

* Cookie expiry is enforced for signed/encrypted cookies
Copy link
Contributor

Choose a reason for hiding this comment

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

Enforce signed/encrypted cookie expiry server side.

actionpack/CHANGELOG.md Outdated

* Cookie expiry is enforced for signed/encrypted cookies

By tacking on expiry meta data to the cookie value and verifying them on receipt,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's reverse the 2 paragraphs to start with the security boost.

Rails can thwart attacks by malicious clients that don't honor a cookie's expiry.

It does so by stashing the expiry within the written cookie and relying on the
signing/encrypting to vouch that it hasn't been tampered with. Then on a
server-side read, the expiry is verified and any expired cookie is discarded.

This shit is a challenge to explain clearly in few words! So sorry for throwing your bit, but I really want us to be clear about this 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! 😄

actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
# cookies[:login] = { value: "XJ-122", expires: 1.hour.from_now }
# cookies[:login] = { value: "XJ-122", expires: 1.hour }
#
# # Sets a cookie that expires at a specific time
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a period.

actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
# cookies[:login] = { value: "XJ-122", expires: 1.hour }
#
# # Sets a cookie that expires at a specific time
# cookies[:oreo] = { value: "chocolate cookies", expires: Time.utc(2020, 10, 15, 5) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we're allowed to reference a trademark in documentation/changelogs. While cute, I'd avoid it.

Copy link
Contributor Author

@assain assain Sep 4, 2017 •
edited

Choose a reason for hiding this comment

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

😅 Got this inspiration from the Python community. I'll change that! 😄

getimage 2

actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -144,7 +147,7 @@ def cookies_digest
# * <tt>:tld_length</tt> - When using <tt>:domain => :all</tt>, this option can be used to explicitly
# set the TLD length when using a short (<= 3 character) domain that is being interpreted as part of a TLD.
# For example, to share cookies between user1.lvh.me and user2.lvh.me, set <tt>:tld_length</tt> to 1.
# * <tt>:expires</tt> - The time at which this cookie expires, as a \Time object.
# * <tt>:expires</tt> - The time at which this cookie expires, as a \Time or ActiveSupport::Duration object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this link to the duration class when the doc is generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the generated docs do indeed link to the duration class 😄

getimage 1

@assain
This commit adds:
9f0b3b5 
* Documentation for Duration support added  to  signed/encrypted cookies

* Changelog entries for the duration support and expiry metadata added to cookies

[ci skip]
@assain
Copy link
Contributor Author

assain commented Sep 4, 2017

@kaspth 😄
Here are the suggested changes!

@kaspth kaspth merged commit a917b59 into rails:master Sep 4, 2017
@kaspth
Copy link
Contributor

kaspth commented Sep 4, 2017

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspth kaspth kaspth left review comments

Assignees

@kaspth kaspth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@assain @rails-bot @kaspth @georgeclaghorn