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

Allowing clearing of cookies with options. #966

Merged
merged 3 commits into from Nov 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
44 changes: 44 additions & 0 deletions spec/lucky/cookies/cookie_jar_spec.cr
Expand Up @@ -134,6 +134,28 @@ describe Lucky::CookieJar do
jar.get_raw(:rules).expired?.should be_true
jar.get_raw(:rules).value.should eq("")
end

it "deletes a valid cookie with a block" do
jar = Lucky::CookieJar.empty_jar
jar.set(:rules, "no fighting!").domain("brawl.co")

jar.delete(:rules) do |cookie|
cookie.domain("brawl.co")
end

jar.deleted?(:rules).should be_true
end

it "ignores an invalid cookie when trying to delete" do
jar = Lucky::CookieJar.empty_jar
jar.set(:rules, "no fighting!").domain("brawl.co")

jar.delete(:burritos) do |cookie|
cookie.domain("brawl.co")
end

jar.deleted?(:rules).should be_false
end
end

describe "deleted?" do
Expand Down Expand Up @@ -168,5 +190,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
28 changes: 28 additions & 0 deletions src/lucky/cookies/cookie_jar.cr
Expand Up @@ -42,19 +42,47 @@ class Lucky::CookieJar
{% raise "use CookieJar#delete instead of CookieJar#unset" %}
end

# Delete all cookies.
def clear : Void
clear { }
end

# Delete 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.

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

# https://tools.ietf.org/search/rfc6265#page-8
# to remove a cookie, the server returns a Set-Cookie header
# with an expiration date in the past. The server will be successful
# in removing the cookie only if the Path and the Domain attribute in
# the Set-Cookie header match the values used when the cookie was
# created.
def delete(key : Key) : Nil
if cookie = cookies[key.to_s]?
cookie.expires(1.year.ago).value("")
set_cookies[key.to_s] = cookie
end
end

# Delete a specific cookie by name `key`. Yield that cookie
# to the block so you can add additional options like domain, path, etc...
def delete(key : Key) : Nil
if cookie = cookies[key.to_s]?
yield cookie
delete cookie.name
end
end

# Returns `true` if the cookie has been expired, and has no value.
# Will return `false` if the cookie does not exist, or is valid.
def deleted?(key : Key) : Bool
Expand Down