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

Allowing clearing of cookies with options. #966

Merged
merged 3 commits into from Nov 4, 2020
Merged

Allowing clearing of cookies with options. #966

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b09208d
Allowing clearing of cookies with options.
russ Nov 15, 2019
4061dab
Merge branch 'master' into cookie-clear-options
jwoertink Oct 30, 2020
301f5c5
Adding in cookie jar delete with a block so you can delete a single c…
jwoertink Oct 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions spec/lucky/cookies/cookie_jar_spec.cr
View file
Open in desktop
Expand Up @@ -135,5 +135,27 @@ describe Lucky::CookieJar do
name.expired?.should be_true
age.expired?.should be_true
end

it "deletes cookies with options" do
headers = HTTP::Headers.new
headers["Cookie"] = "name=Rick%20James"
Copy link
Member

Choose a reason for hiding this comment

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

SUPERFREAK!

Copy link
Member

Choose a reason for hiding this comment

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

giphy


jar = Lucky::CookieJar.from_request_cookies(
HTTP::Cookies.from_headers(headers))

jar.clear do |cookie|
cookie.path("/")
.http_only(true)
.secure(true)
.domain(".example.com")
end

name = jar.get_raw(:name)
name.value.should eq("")
name.path.should eq("/")
name.domain.should eq(".example.com")
name.secure.should be_true
name.expired?.should be_true
end
end
end
13 changes: 13 additions & 0 deletions src/lucky/cookies/cookie_jar.cr
View file
Open in desktop
Expand Up @@ -42,8 +42,21 @@ class Lucky::CookieJar
{% raise "use CookieJar#delete instead of CookieJar#unset" %}
end

# Clear cookies without any specific options.
def clear : Void
clear { }
end

# Clear cookies with a block to add specific options.
#
# jar.clear do |cookie|
# cookie.path("/")
# .http_only(true)
# .secure(true)
# end
def clear(&block : HTTP::Cookie ->) : Void
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure I understand the use case. Are you saying that clear doesn't actually clear the cookie? If so should we automatically set the options or something rather than making people pass a block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, you think you are clearing the cookies but the browser blissfully ignores it if the options don't match. Can't really guess at the options either as there is no way to tell what they were set to originally.

I learned this the hard way since the cookies weren't being deleted. I figured out this was the case. I'm going to test this in a bit more of a "production" setup early next week. I'll let you know how that goes.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need to do something like Explicitly set the cookie if it is empty?

Right now I think it does nothing if there are no cookies.

lucky/src/lucky/session_handler.cr

Lines 21 to 23 in 956d3ab

context.cookies.updated.each do |cookie|
response.cookies[cookie.name] = cookie
end

So maybe we need to set a @cleared boolean (that gets reset to false if you set more cookies after) and if the cookies are cleared? we write a cookie header that is blank. I think that'll do the trick and make it work without explicitly passing options

But I'm not sure :)

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 found the spot in rails that mentions the domain has to be the same as when it's set and deleted.
https://github.com/rails/rails/blob/f2df77709f7e536aaf4d6f984ff21a49d44d34c1/actionpack/lib/action_dispatch/middleware/cookies.rb#L135

I really think it's a browser "feature". Probably some security related concern.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see! Ok so I think we need a couple things here.

  1. Make it so you can pass a block to delete like Rails has there
  2. Document that in website https://luckyframework.org/guides/http-and-routing/sessions-and-cookies
  3. Do what Rails does for clear so that it works automatically https://github.com/rails/rails/blob/f2df77709f7e536aaf4d6f984ff21a49d44d34c1/actionpack/lib/action_dispatch/middleware/cookies.rb#L395-L397. That way the user does not need to remember the options to clear all cookies.

I'd say the current way is a potential security risk because it'd be easy to call session.clear for example and if you set a domain it would not actually clear it out

Copy link
Member

Choose a reason for hiding this comment

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

ping 😂

Copy link
Member

Choose a reason for hiding this comment

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

This looks close. I think we should also add a delete with a block similar to what Rails has as well.

I also think it'd be cool to read the existing cookies and automatically set the domain option when clearing so that it works as you'd expect, but that could come in a later PR. Here's some rough pseudo code for what I mean

cookies.each do |cookie|
  delete cookie.name do |cookie|
    # Set the domain for the user
    cookie.domain = cookie.domain
  end
end

I don't know if that works as-is, but that's kind of what I'm imagining. Or maybe delete does it instead of clear? If this isn't clear LMK and maybe we can chat it out in Gitter or something

Copy link
Member

Choose a reason for hiding this comment

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

Also along with cookies (maybe in another PR), we should probably think about this whole chrome deal https://adzerk.com/blog/chrome-samesite/

Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure I understand, you're proposing adding a method for setting samesite, right? If so I think that is a good idea. It could accept an enum for the accepted values to keep it nice and safe. I think that should be a separate PR, but yeah this would be a great security feature

Copy link
Member

Choose a reason for hiding this comment

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

Ok, just having come across this in rails... we need to NOT do what rails is doing... unless it's been fixed in 6... I have a signed cookie that no matter what combination I do, I can't get rails to delete it, and it's maddening.

cookies.each do |cookie|
yield cookie
delete cookie.name
end
end
Expand Down