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

Allow opting out of the SameSite cookie attribute when setting a cookie #45501

Merged
merged 1 commit into from
Jul 5, 2022
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
10 changes: 10 additions & 0 deletions actionpack/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
* Allow opting out of the `SameSite` cookie attribute when setting a cookie.

You can opt out of `SameSite` by passing `same_site: nil`.

`cookies[:foo] = { value: "bar", same_site: nil }`

Previously, this incorrectly set the `SameSite` attribute to the value of the `cookies_same_site_protection` setting.

*Alex Ghiculescu*

* Allow using `helper_method`s in `content_security_policy` and `permissions_policy`

Previously you could access basic helpers (defined in helper modules), but not
Expand Down
5 changes: 3 additions & 2 deletions actionpack/lib/action_dispatch/middleware/cookies.rb
Original file line number Diff line number Diff line change
Expand Up @@ -453,8 +453,9 @@ def handle_options(options)

options[:path] ||= "/"

cookies_same_site_protection = request.cookies_same_site_protection
options[:same_site] ||= cookies_same_site_protection.call(request)
unless options.key?(:same_site)
options[:same_site] = request.cookies_same_site_protection.call(request)
end

if options[:domain] == :all || options[:domain] == "all"
# If there is a provided tld length then we use it otherwise default domain regexp.
Expand Down
44 changes: 39 additions & 5 deletions actionpack/test/dispatch/cookies_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,16 @@ def rails_5_2_stable_signed_cookie_with_authenticated_encryption_flag_off

head :ok
end

def set_same_site_strict
cookies["user_name"] = { value: "david", same_site: :strict }
head :ok
end

def set_same_site_nil
cookies["user_name"] = { value: "david", same_site: nil }
head :ok
end
end

tests TestController
Expand Down Expand Up @@ -362,15 +372,15 @@ def setup
@request.host = "www.nextangle.com"
end

def test_setting_cookie_with_no_protection
def test_setting_cookie_with_no_same_site_protection
@request.env["action_dispatch.cookies_same_site_protection"] = proc { :none }

get :authenticate
assert_cookie_header "user_name=david; path=/; SameSite=None"
assert_equal({ "user_name" => "david" }, @response.cookies)
end

def test_setting_cookie_with_protection_proc_normal_user_agent
def test_setting_cookie_with_same_site_protection_proc_normal_user_agent
@request.env["action_dispatch.cookies_same_site_protection"] = Proc.new do |request|
:strict unless request.user_agent == "spooky browser"
end
Expand All @@ -380,7 +390,7 @@ def test_setting_cookie_with_protection_proc_normal_user_agent
assert_equal({ "user_name" => "david" }, @response.cookies)
end

def test_setting_cookie_with_protection_proc_special_user_agent
def test_setting_cookie_with_same_site_protection_proc_special_user_agent
@request.env["action_dispatch.cookies_same_site_protection"] = Proc.new do |request|
:strict unless request.user_agent == "spooky browser"
end
Expand All @@ -391,7 +401,7 @@ def test_setting_cookie_with_protection_proc_special_user_agent
assert_equal({ "user_name" => "david" }, @response.cookies)
end

def test_setting_cookie_with_misspelled_protection_raises
def test_setting_cookie_with_misspelled_same_site_protection_raises
@request.env["action_dispatch.cookies_same_site_protection"] = proc { :funky }

error = assert_raise ArgumentError do
Expand All @@ -400,14 +410,38 @@ def test_setting_cookie_with_misspelled_protection_raises
assert_match "Invalid SameSite value: :funky", error.message
end

def test_setting_cookie_with_strict
def test_setting_cookie_with_same_site_strict
@request.env["action_dispatch.cookies_same_site_protection"] = proc { :strict }

get :authenticate
assert_cookie_header "user_name=david; path=/; SameSite=Strict"
assert_equal({ "user_name" => "david" }, @response.cookies)
end

def test_setting_cookie_with_same_site_nil
@request.env["action_dispatch.cookies_same_site_protection"] = proc { nil }

get :authenticate
assert_cookie_header "user_name=david; path=/"
assert_equal({ "user_name" => "david" }, @response.cookies)
end

def test_setting_cookie_with_specific_same_site_strict
@request.env["action_dispatch.cookies_same_site_protection"] = proc { :lax }
Copy link
Member

Choose a reason for hiding this comment

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

This is already set in setup, so we could possibly omit. I do see an argument for being explicit here, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is better to be explicit since all the tests around it have the same pattern.


get :set_same_site_strict
assert_cookie_header "user_name=david; path=/; SameSite=Strict"
assert_equal({ "user_name" => "david" }, @response.cookies)
end

def test_setting_cookie_with_specific_same_site_nil
@request.env["action_dispatch.cookies_same_site_protection"] = proc { :lax }

get :set_same_site_nil
assert_cookie_header "user_name=david; path=/"
assert_equal({ "user_name" => "david" }, @response.cookies)
end

def test_setting_cookie
get :authenticate
assert_cookie_header "user_name=david; path=/; SameSite=Lax"
Expand Down