Skip to content

Commit

Permalink
Don't write out secure cookies unless the request is secure
Browse files Browse the repository at this point in the history
  • Loading branch information
pixeltrix committed Oct 25, 2010
1 parent cdce5fc commit 2d5a12a
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 3 deletions.
14 changes: 11 additions & 3 deletions actionpack/lib/action_dispatch/middleware/cookies.rb
Expand Up @@ -98,17 +98,19 @@ class CookieJar < Hash #:nodoc:
def self.build(request)
secret = request.env[TOKEN_KEY]
host = request.host
secure = request.ssl?

new(secret, host).tap do |hash|
new(secret, host, secure).tap do |hash|
hash.update(request.cookies)
end
end

def initialize(secret = nil, host = nil)
def initialize(secret = nil, host = nil, secure = false)
@secret = secret
@set_cookies = {}
@delete_cookies = {}
@host = host
@secure = secure

super()
end
Expand Down Expand Up @@ -193,9 +195,15 @@ def signed
end

def write(headers)
@set_cookies.each { |k, v| ::Rack::Utils.set_cookie_header!(headers, k, v) }
@set_cookies.each { |k, v| ::Rack::Utils.set_cookie_header!(headers, k, v) if write_cookie?(v) }
@delete_cookies.each { |k, v| ::Rack::Utils.delete_cookie_header!(headers, k, v) }
end

private

def write_cookie?(cookie)
@secure || !cookie[:secure] || Rails.env.development?

This comment has been minimized.

Copy link
@josevalim

josevalim Oct 27, 2010

Contributor

We should check first if Rails is defined. "defined?(Rails.env) && Rails.env.development?"

This comment has been minimized.

Copy link
@pixeltrix

pixeltrix Oct 27, 2010

Author Contributor

Done here and here.

This comment has been minimized.

Copy link
@hlcfan

hlcfan Mar 8, 2019

Why it can write cookie under development environment? Under what kind of situation? Thanks.

This comment has been minimized.

Copy link
@pixeltrix

pixeltrix Mar 8, 2019

Author Contributor

@hlcfan generally people don't run their local development environment on SSL but if they configure the cookie with the secure flag then they can't run their app in development if they need the cookie set (e.g. sessions, flash, CSRF and form-based login gems like devise).

This comment has been minimized.

Copy link
@hlcfan

hlcfan Mar 8, 2019

Thanks for the reply. If run app with ssl disabled and set cookie with secure flag, then the cookie cannot be set in browser, what's the purpose of writing the cookie in development? Please correct me if I'm not getting you right :-)

This comment has been minimized.

Copy link
@pixeltrix

pixeltrix Mar 8, 2019

Author Contributor

I think I understand what you're saying - that setting the cookie on an non-ssl connection in development won't send the cookie on a following page. That may be the case now (I haven't checked) but IIRC, 9 years ago it wasn't - I think it just ignored secure if it was over a non-ssl connection.

This comment has been minimized.

Copy link
@hlcfan

hlcfan Mar 8, 2019

Yea, the secure flag only shows in the response but not in browser. I'm thinking maybe remove this Rails.env.development? check and keep config.always_write_cookie which was introduced in https://github.com/rails/rails/pull/3739/files. Having always_write_cookie to be true only in development is a bit sneaky to me. Maybe set config.always_write_cookie to false by default and add it into Rails configuring documentation. What's your idea?

This comment has been minimized.

Copy link
@pixeltrix

pixeltrix Mar 8, 2019

Author Contributor

If there's an issue that needs addressing please open a ticket and we can discuss it there - a commit comment isn't the right place for it since we can't track it's resolution.

This comment has been minimized.

Copy link
@hlcfan

hlcfan Mar 9, 2019

It's not a bug, just the current implementation is a bit confusing.

In development env, it allows to write cookie, while no in other environments. Upon writing cookies - such general behavior, discrepancy across environments should be better smoothed. If we want to control the behavior of writing cookie, use this configuration directive config.always_write_cookie, however it's not showing up in any docs.

Just take a look at the contributing guidelines, the "Issues" are for bugs. I can put my thought in https://groups.google.com/forum/?fromgroups#!forum/rubyonrails-core, please advise.

This comment has been minimized.

Copy link
@hlcfan

hlcfan Mar 9, 2019

Created post https://groups.google.com/forum/?fromgroups&hl=en#!topic/rubyonrails-core/bNd_QfZJ6_8, please let me know your thoughts. Thanks.

end
end

class PermanentCookieJar < CookieJar #:nodoc:
Expand Down
5 changes: 5 additions & 0 deletions actionpack/test/abstract_unit.rb
Expand Up @@ -47,6 +47,11 @@
require 'pp' # require 'pp' early to prevent hidden_methods from not picking up the pretty-print methods until too late

module Rails
class << self
def env
@_env ||= ActiveSupport::StringInquirer.new(ENV["RAILS_ENV"] || ENV["RACK_ENV"] || "test")
end
end
end

ActiveSupport::Dependencies.hook!
Expand Down
23 changes: 23 additions & 0 deletions actionpack/test/dispatch/cookies_test.rb
Expand Up @@ -135,11 +135,25 @@ def test_setting_cookie_with_http_only
end

def test_setting_cookie_with_secure
@request.env["HTTPS"] = "on"
get :authenticate_with_secure
assert_cookie_header "user_name=david; path=/; secure"
assert_equal({"user_name" => "david"}, @response.cookies)
end

def test_setting_cookie_with_secure_in_development
Rails.env.stubs(:development?).returns(true)
get :authenticate_with_secure
assert_cookie_header "user_name=david; path=/; secure"
assert_equal({"user_name" => "david"}, @response.cookies)
end

def test_not_setting_cookie_with_secure
get :authenticate_with_secure
assert_not_cookie_header "user_name=david; path=/; secure"
assert_not_equal({"user_name" => "david"}, @response.cookies)
end

def test_multiple_cookies
get :set_multiple_cookies
assert_equal 2, @response.cookies.size
Expand Down Expand Up @@ -286,4 +300,13 @@ def assert_cookie_header(expected)
assert_equal expected.split("\n"), header
end
end

def assert_not_cookie_header(expected)
header = @response.headers["Set-Cookie"]
if header.respond_to?(:to_str)
assert_not_equal expected.split("\n").sort, header.split("\n").sort
else
assert_not_equal expected.split("\n"), header
end
end
end

0 comments on commit 2d5a12a

Please sign in to comment.