Allowing clearing of cookies with options. #966

russ · 2019-11-15T18:31:47Z

Fixes #965

I ran into a bug where I couldn't clear cookies with a specific domain
being set. After looking into it, I figured out that the browser does
not pass along any option information when making a request. It's up to
the server side to match the headers correctly.

So when clearing cookies, the Set-Cookie header does not match the
original cookie options. That leaves the cookies you "thought" you were
clearing in place.

This fixes that by allowing clear to take a block. That block passes
down the cookies where you can modify the path, domain, etc.

Breakdown

This PR adds in cookie_jar.clear with a block, and cookie_jar.delete(key) with a block.

# if your cookies have the whatever.com domain, they will be deleted
cookies.clear do |cookie|
  cookie.domain("whatever.com")
end

# Delete the snickerdoodle cookie with the domain of whatever.com
cookies.delete(:snickerdoodle) do |cookie|
  cookie.domain("whatever.com")
end

Checklist

- An issue already exists detailing the issue/or feature request that this PR fixes
- All specs are formatted with crystal tool format spec src
- Inline documentation has been added and/or updated
- Lucky builds on docker with ./script/setup
- All builds and specs pass on docker with ./script/test

I ran into a bug where I couldn't clear cookies with a specific domain being set. After looking into it, I figured out that the browser does not pass along any option information when making a request. It's up to the server side to match the headers correctly. So when clearing cookies, the Set-Cookie header does not match the original cookie options. That leaves the cookies you "thought" you were clearing in place. This fixes that by allowing clear to take a block. That block passes down the cookies where you can modify the path, domain, etc.

paulcsmith

Cookies are still a bit voodoo to me so wondering a bit about the fix

paulcsmith · 2019-11-15T18:59:52Z

spec/lucky/cookies/cookie_jar_spec.cr

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


paulcsmith · 2019-11-15T19:01:42Z

src/lucky/cookies/cookie_jar.cr

+  #         .http_only(true)
+  #         .secure(true)
+  # end
+  def clear(&block : HTTP::Cookie ->) : Void


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?

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.

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 :)

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.

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

Make it so you can pass a block to delete like Rails has there

Document that in website https://luckyframework.org/guides/http-and-routing/sessions-and-cookies

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

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

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

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

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.

jwoertink · 2019-11-15T18:34:36Z

spec/lucky/cookies/cookie_jar_spec.cr

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


SUPERFREAK!

jwoertink · 2020-10-30T22:17:58Z

I decided to revive this to see what is left, and after digging around, I'm not sure if we can auto-set the cookie info for users.

@paulcsmith says:

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

I don't think this will be possible though. From what I can tell, we create a new cookie jar on every request. We fill this jar with cookie data sent back to us. When we go to delete a cookie, we don't know what the domain was originally set 4 cookie jars ago because the content sent back to us only seems to know about the name and the path.

So in the case of the code example above:

cookies.each do |cookie|
  cookies.delete cookie.name do |delete_cookie|
    # both are blank here
    delete_cookie.domain = cookie.domain
  end
end

To better show how this works in a Lucky app, here's how it can be tested:

# In this example, we set a simple cookie, delete on the next request, and it's gone on the third
class Tests::One < BrowserAction
  get "/one" do
    cookies.set_raw(:test, "arosa")
    html OnePage
  end
end

class Tests::Two < BrowserAction
  get "/two" do
    cookies.delete(:test)
    html TwoPage
  end
end

class Tests::Three < BrowserAction
  get "/three" do
    c = cookies.get_raw?(:test)
    # c is nil here because it's been deleted
    raise c.inspect
  end
end

# In this example, we set a cookie with a domain, simple delete on the next request, and it's still there on the third
class Tests::One < BrowserAction
  get "/one" do
    c = cookies.set_raw(:test, "arosa")
    c.domain("cars.com")
    html OnePage
  end
end

class Tests::Two < BrowserAction
  get "/two" do
    c = cookies.get_raw(:test)
    puts c.domain # it's nil at this point 
    cookies.delete(:test)
    html TwoPage
  end
end

class Tests::Three < BrowserAction
  get "/three" do
    c = cookies.get_raw?(:test)
    # c is for cookie here because when we tried to delete, the domain wasn't set
    raise c.inspect
  end
end

I've also encountered another bug while digging in to this, but I'll open a separate issue. As for this one, and taking a look at what rails does, our only real option, that I see, is to merge this as is with the block.

…ookie and specify the additional options

matthewmcgarvey · 2020-10-30T23:29:33Z

Why is it that you have to set the properties on the cookie again to delete it? Do those fields not stay on the cookie on subsequent requests?

jwoertink · 2020-10-31T01:12:36Z

Right. It works like this:

# request comes in
cookie_jar # is empty

# we send this to the browser
response.headers["Set-Cookie"] = "name=whatever; value=1; domain=whatever.com; path=/"

cookie_jar # has 1 cookie in it

# the browser goes, cool, I've saved it, then sends us back a new request
cookie_jar # is empty again ... <-- this is where we lost all the data

request.headers["Cookie"] == "name=whatever; path=/"

# our cookie jar now has 1 cookie with the name "whatever" and path "/", and that's it
# we send this back to the browser to "delete" it
response.headers["Set-Cookie"] = "name=whatever; value= ; expires=last year"

# but the browser sees a cookie called "whatever" on the current domain, and the cookie we sent
# doesn't have that domain, so it's not deleted.

Basically, the browser keeps a ton of extra info that it keeps secret on all future requests. Since it doesn't send us back the original domain, we have no clue. Now, it does send us the path, but the thing I didn't test was if that path it sends is of the cookie, or the current path 🤔 But in any case, the issue is still the same.

TL;DR: we set it manually because we don't have the info.

matthewmcgarvey

Thanks for that extra info. I didn't need it, of course, but I'm sure there's someone out there would might not know these things... not me, though 😄

paulcsmith

Makes sense and looks good!

russ changed the title ~~fAllowing clearing of cookies with options.~~ Allowing clearing of cookies with options. Nov 15, 2019

paulcsmith reviewed Nov 15, 2019

View reviewed changes

jwoertink reviewed Nov 15, 2019

View reviewed changes

Merge branch 'master' into cookie-clear-options

4061dab

Adding in cookie jar delete with a block so you can delete a single c…

301f5c5

…ookie and specify the additional options

jwoertink requested a review from matthewmcgarvey October 30, 2020 23:14

matthewmcgarvey approved these changes Oct 31, 2020

View reviewed changes

paulcsmith approved these changes Nov 4, 2020

View reviewed changes

jwoertink merged commit c094df3 into luckyframework:master Nov 4, 2020

jwoertink mentioned this pull request Nov 4, 2020

Document new cookie clearing methods luckyframework/website#478

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allowing clearing of cookies with options. #966

Allowing clearing of cookies with options. #966

russ commented Nov 15, 2019 •

edited by jwoertink

paulcsmith left a comment

paulcsmith Nov 15, 2019

paulcsmith Nov 15, 2019

russ Nov 15, 2019

paulcsmith Nov 15, 2019

russ Nov 18, 2019

paulcsmith Nov 18, 2019

jwoertink Feb 10, 2020

paulcsmith Feb 11, 2020

jwoertink Feb 11, 2020

paulcsmith Feb 11, 2020

jwoertink Apr 24, 2020

jwoertink Nov 15, 2019

jwoertink commented Oct 30, 2020

matthewmcgarvey commented Oct 30, 2020

jwoertink commented Oct 31, 2020

matthewmcgarvey left a comment

paulcsmith left a comment

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

Allowing clearing of cookies with options. #966

Allowing clearing of cookies with options. #966

Conversation

russ commented Nov 15, 2019 • edited by jwoertink

Breakdown

Checklist

paulcsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwoertink commented Oct 30, 2020

matthewmcgarvey commented Oct 30, 2020

jwoertink commented Oct 31, 2020

matthewmcgarvey left a comment

Choose a reason for hiding this comment

paulcsmith left a comment

Choose a reason for hiding this comment

russ commented Nov 15, 2019 •

edited by jwoertink