Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

unset domain for localhost by default and support options when setting cookie #118

Closed
wants to merge 2 commits into from

4 participants

@colinsurprenant

should fix #113 & #115

First, by default unset domain when @request.host == 'localhost'. Setting domain to localhost does not work in particular on Chrome.

I also added the possibility to pass cookie options when setting a cookie.

Added corresponding tests and doc.

@colinsurprenant

just like @rykann in #91 I just realized there is some history (#66) for per cookie options. I too definitely prefer the way I suggested since it is the way response.set_cookie works.

spec/cookies_spec.rb
@@ -130,6 +138,28 @@ def cookies(*set_cookies)
cookies['foo']
end.should be == 'bar'
end
+
+ it 'sets a cookie using hash value' do
+ cookie_route do
+ cookies['foo'] = {:value => 'bar'}
+ response['Set-Cookie'].lines.detect { |l| l.start_with? 'foo=bar' }
+ end
@rykann
rykann added a note

Looks like an assertion is needed here.

true this! will do.

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

These changes look good to me, with an assertion added to the spec I commented on. Thanks for adding the ability to set cookie options. This takes care of #91 as well.

@colinsurprenant colinsurprenant missing assertion per #118
42dd791
@colinsurprenant

a friendly ping!

@TheNeikos

I second this ping :+1:

Ran into this issue today!

@zzak
Owner

Hello!

I'm sorry for the late response!

I will work on a patch to unset the domain for localhost, but will not be able to merge the following spec changes: 'sets a cookie options using hash' as this could lead to other unexpected behavior.

@zzak zzak referenced this pull request from a commit
@zzak zzak Don't set cookie domain on localhost from #118
Closes #113
Closes #115
9b35f60
@zzak zzak closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 6, 2014
  1. @colinsurprenant
Commits on Mar 6, 2014
  1. @colinsurprenant

    missing assertion per #118

    colinsurprenant authored
This page is out of date. Refresh to see the latest.
Showing with 49 additions and 6 deletions.
  1. +18 −5 lib/sinatra/cookies.rb
  2. +31 −1 spec/cookies_spec.rb
View
23 lib/sinatra/cookies.rb
@@ -21,6 +21,21 @@ module Sinatra
# redirect to('/')
# end
#
+ # Globally set default cookies options:
+ #
+ # set :cookie_options,
+ # :domain => "mydomain.com",
+ # :path => "/mypath",
+ # :secure => false,
+ # :httponly => false
+ #
+ # Write cookie with options:
+ #
+ # get '/set' do
+ # cookies[:something] = {:value => 'foobar', :path => "/mypath", :domain => "mydomain.com"}
+ # redirect to('/')
+ # end
+ #
# And generally behaves like a hash:
#
# get '/demo' do
@@ -66,14 +81,12 @@ def initialize(app)
@deleted = []
@options = {
- :path => @request.script_name,
- :domain => @request.host,
+ :path => @request.script_name.to_s.empty? ? '/' : @request.script_name,
+ :domain => @request.host == 'localhost' ? nil : @request.host,
:secure => @request.secure?,
:httponly => true
}
- @options[:path] = '/' if @options[:path].to_s.empty?
-
if app.settings.respond_to? :cookie_options
@options.merge! app.settings.cookie_options
end
@@ -88,7 +101,7 @@ def [](key)
end
def []=(key, value)
- @response.set_cookie key.to_s, @options.merge(:value => value)
+ @response.set_cookie key.to_s, @options.merge(value.is_a?(Hash) ? value : {:value => value})
end
def assoc(key)
View
32 spec/cookies_spec.rb
@@ -9,7 +9,7 @@ def cookie_route(*cookies, &block)
result = instance_eval(&block)
"ok"
end
- get '/'
+ get '/', {}, @headers || {}
last_response.should be_ok
body.should be == "ok"
result
@@ -97,6 +97,14 @@ def cookies(*set_cookies)
end.should include('HttpOnly')
end
+ it 'sets domain to nil if localhost' do
+ @headers = {'HTTP_HOST' => 'localhost'}
+ cookie_route do
+ cookies['foo'] = 'bar'
+ response['Set-Cookie']
+ end.should_not include("domain")
+ end
+
it 'sets the domain' do
cookie_route do
cookies['foo'] = 'bar'
@@ -130,6 +138,28 @@ def cookies(*set_cookies)
cookies['foo']
end.should be == 'bar'
end
+
+ it 'sets a cookie using hash value' do
+ cookie_route do
+ cookies['foo'] = {:value => 'bar'}
+ response['Set-Cookie'].lines.detect { |l| l.start_with? 'foo=' }
+ end.should include('foo=bar')
+ end
+
+ ["baz.com", "localhost"].each do |domain|
+ it 'sets a cookie options using hash' do
+ cookie_route do
+ cookies['foo'] = {
+ :value => 'bar',
+ :path => '/baz',
+ :domain => domain,
+ :secure => true,
+ :httponly => true
+ }
+ response['Set-Cookie'].lines.detect { |l| l.start_with? 'foo=bar' }
+ end.should include("path=/baz;", "domain=#{domain};", "secure;", "HttpOnly")
+ end
+ end
end
describe :assoc do
Something went wrong with that request. Please try again.