Commit 16431a9d authored by Stan Hu's avatar Stan Hu

Don't send SameSite=None to incompatible browsers

We set `SameSite=None` in GitLab 12.10 via
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28205 because
Chrome v80, rolled out in March 2020, treats any cookies without the
`SameSite` directive set as though they are `SameSite=Lax`
(https://www.chromestatus.com/feature/5088147346030592). This is a
breaking change from the previous default behavior, which was to treat
those cookies as `SameSite=None`.

However, older browsers (e.g. MacOS 10.14 on Safari 13.0.3) may
interpret the `None` as `Strict`, which causes users that click on
gitlab.com links from third-party sites (e.g. Gmail, Slack) to log in
again.

https://www.chromium.org/updates/same-site/incompatible-clients
recommends a set of regular expressions to determine whether to send
this. This commit implements most of the logic but skips one case since
this doesn't seem common: macOS 10.14 with an embedded WebKit
browser. This is also what
https://rubygems.org/gems/rails_same_site_cookie does.

I considered using that gem
(https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40663), but I
didn't like how it added another dependency (`user_agent_parser`) that
loads a large YAML database
(https://github.com/ua-parser/uap-ruby#the-pattern-database).
parent 4e12f87c
---
title: Don't send SameSite=None to incompatible browsers
merge_request: 40667
author:
type: fixed
......@@ -33,6 +33,8 @@ module Gitlab
cookies = set_cookie.split(COOKIE_SEPARATOR)
return result if same_site_none_incompatible?(headers['User-Agent'])
cookies.each do |cookie|
next if cookie.blank?
......@@ -58,6 +60,23 @@ module Gitlab
def ssl?
Gitlab.config.gitlab.https
end
# Taken from https://www.chromium.org/updates/same-site/incompatible-clients
def same_site_none_incompatible?(user_agent)
return false unless user_agent.present?
browser = Browser.new(user_agent)
has_webkit_same_site_bug?(browser) || drops_unrecognized_same_site_cookies?(browser)
end
def has_webkit_same_site_bug?(browser)
browser.platform.ios?(12) ||
(browser.platform.mac?("~> 10.14") && browser.safari?)
end
def drops_unrecognized_same_site_cookies?(browser)
browser.uc_browser?("<12.13.2") || browser.chrome?([">= 51", "< 67"])
end
end
end
end
......@@ -3,23 +3,30 @@
require 'spec_helper'
RSpec.describe Gitlab::Middleware::SameSiteCookies do
using RSpec::Parameterized::TableSyntax
include Rack::Test::Methods
let(:user_agent) { nil }
let(:mock_app) do
Class.new do
attr_reader :cookies
attr_reader :cookies, :user_agent
def initialize(cookies)
def initialize(cookies, user_agent)
@cookies = cookies
@user_agent = user_agent
end
def call(env)
[200, { 'Set-Cookie' => cookies }, ['OK']]
[
200,
{ 'Set-Cookie' => cookies, 'User-Agent' => user_agent }.compact,
['OK']
]
end
end
end
let(:app) { mock_app.new(cookies) }
let(:app) { mock_app.new(cookies, user_agent) }
subject do
described_class.new(app)
......@@ -63,6 +70,42 @@ RSpec.describe Gitlab::Middleware::SameSiteCookies do
end
end
context 'with different browsers' do
where(:description, :user_agent, :expected) do
"iOS 12" | "Mozilla/5.0 (iPhone; CPU iPhone OS 12_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Mobile/15E148 Safari/604.1" | false
"macOS 10.14 + Safari" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Safari/605.1.15" | false
"macOS 10.14 + Opera" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.78 Safari/537.36 OPR/47.0.2631.55" | false
"macOS 10.14 + Chrome v80" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.87 Safari/537.36" | true
"Chrome v41" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2227.1 Safari/537.36" | true
"Chrome v50" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2348.1 Safari/537.36" | true
"Chrome v51" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2718.15 Safari/537.36" | false
"Chrome v66" | "Mozilla/5.0 (Linux; Android 4.4.2; Avvio_793 Build/KOT49H) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.126 Mobile Safari/537.36" | false
"Chrome v67" | "Mozilla/5.0 (Linux; Android 7.1.1; SM-J510F Build/NMF26X) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3371.0 Mobile Safari/537.36" | true
"Chrome v85" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36" | true
"Chromium v66" | "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/66.0.3359.181 HeadlessChrome/66.0.3359.181 Safari/537.36" | false
"Chromium v85" | "Mozilla/5.0 (X11; Linux aarch64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/85.0.4183.59 Chrome/85.0.4183.59 Safari/537.36" | true
"UC Browser 12.0.4" | "Mozilla/5.0 (Linux; U; Android 4.4.4; zh-CN; A31 Build/KTU84P) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.0.4.986 Mobile Safari/537.36" | false
"UC Browser 12.13.0" | "Mozilla/5.0 (Linux; U; Android 7.1.1; en-US; SM-C9000 Build/NMF26X) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.13.0.1207 Mobile Safari/537.36" | false
"UC Browser 12.13.2" | "Mozilla/5.0 (Linux; U; Android 9; en-US; Redmi Note 7 Build/PQ3B.190801.002) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.13.2.1208 Mobile Safari/537.36" | true
"UC Browser 12.13.5" | "Mozilla/5.0 (Linux; U; Android 5.1.1; en-US; PHICOMM C630 (CLUE L) Build/LMY47V) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.13.5.1209 Mobile Safari/537.36" | true
"Playstation" | "Mozilla/5.0 (PlayStation 4 2.51) AppleWebKit/537.73 (KHTML, like Gecko)" | true
end
with_them do
let(:cookies) { "thiscookie=12345" }
it 'returns expected SameSite status' do
response = do_request
if expected
expect(response['Set-Cookie']).to include('SameSite=None')
else
expect(response['Set-Cookie']).not_to include('SameSite=None')
end
end
end
end
context 'with single cookie' do
let(:cookies) { "thiscookie=12345" }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment