Make all HTTPS cookies set SameSite to none

Some users reported being logged out quite frequently, and we suspect
a change in Chrome caused this.

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.

To fix this, we add a middleware that tags all cookies with the Secure
and SameSite=None headers. This middleware is needed until we upgrade to
Rack v2.1.0+
(https://github.com/rack/rack/commit/c859bbf7b53cb59df1837612a8c330dfb4147392)
and a version of Rails that has native support
(https://github.com/rails/rails/commit/7ccaa125ba396d418aad1b217b63653d06044680).

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/212551

changelogs/unreleased/andr3-rails-session-samesite-strict.yml

+---
+title: Make all HTTPS cookies set SameSite to none
+merge_request: 28205
+author:
+type: fixed

config/application.rb

@@ -24,6 +24,7 @@ class Application < Rails::Application
     require_dependency Rails.root.join('lib/gitlab/current_settings')
     require_dependency Rails.root.join('lib/gitlab/middleware/read_only')
     require_dependency Rails.root.join('lib/gitlab/middleware/basic_health_check')
+    require_dependency Rails.root.join('lib/gitlab/middleware/same_site_cookies')
     require_dependency Rails.root.join('lib/gitlab/runtime')
 
     # Settings in config/environments/* take precedence over those specified here.
@@ -231,6 +232,8 @@ class Application < Rails::Application
 
     config.middleware.insert_after Warden::Manager, Rack::Attack
 
+    config.middleware.insert_before ActionDispatch::Cookies, ::Gitlab::Middleware::SameSiteCookies
+
     # Allow access to GitLab API from other domains
     config.middleware.insert_before Warden::Manager, Rack::Cors do
       headers_to_expose = %w[Link X-Total X-Total-Pages X-Per-Page X-Page X-Next-Page X-Prev-Page X-Gitlab-Blob-Id X-Gitlab-Commit-Id X-Gitlab-Content-Sha256 X-Gitlab-Encoding X-Gitlab-File-Name X-Gitlab-File-Path X-Gitlab-Last-Commit-Id X-Gitlab-Ref X-Gitlab-Size]

lib/gitlab/middleware/same_site_cookies.rb

+# frozen_string_literal: true
+
+# This middleware sets the SameSite directive to None on all cookies.
+# It also adds the Secure directive if HTTPS is enabled.
+#
+# 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.
+#
+# This middleware is needed until we upgrade to Rack v2.1.0+
+# (https://github.com/rack/rack/commit/c859bbf7b53cb59df1837612a8c330dfb4147392)
+# and a version of Rails that has native support
+# (https://github.com/rails/rails/commit/7ccaa125ba396d418aad1b217b63653d06044680).
+#
+module Gitlab
+  module Middleware
+    class SameSiteCookies
+      COOKIE_SEPARATOR = "\n".freeze
+
+      def initialize(app)
+        @app = app
+      end
+
+      def call(env)
+        status, headers, body = @app.call(env)
+        result = [status, headers, body]
+
+        set_cookie = headers['Set-Cookie']&.strip
+
+        return result if set_cookie.blank? || !ssl?
+
+        cookies = set_cookie.split(COOKIE_SEPARATOR)
+
+        cookies.each do |cookie|
+          next if cookie.blank?
+
+          # Chrome will drop SameSite=None cookies without the Secure
+          # flag. If we remove this middleware, we may need to ensure
+          # that all cookies set this flag.
+          if ssl? && !(cookie =~ /;\s*secure/i)
+            cookie << '; Secure'
+          end
+
+          unless cookie =~ /;\s*samesite=/i
+            cookie << '; SameSite=None'
+          end
+        end
+
+        headers['Set-Cookie'] = cookies.join(COOKIE_SEPARATOR)
+
+        result
+      end
+
+      private
+
+      def ssl?
+        Gitlab.config.gitlab.https
+      end
+    end
+  end
+end

spec/lib/gitlab/middleware/same_site_cookies_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Middleware::SameSiteCookies do
+  include Rack::Test::Methods
+
+  let(:mock_app) do
+    Class.new do
+      attr_reader :cookies
+
+      def initialize(cookies)
+        @cookies = cookies
+      end
+
+      def call(env)
+        [200, { 'Set-Cookie' => cookies }, ['OK']]
+      end
+    end
+  end
+
+  let(:app) { mock_app.new(cookies) }
+
+  subject do
+    described_class.new(app)
+  end
+
+  describe '#call' do
+    let(:request) { Rack::MockRequest.new(subject) }
+
+    def do_request
+      request.post('/some/path')
+    end
+
+    context 'without SSL enabled' do
+      before do
+        allow(Gitlab.config.gitlab).to receive(:https).and_return(false)
+      end
+
+      context 'with cookie' do
+        let(:cookies) { "thiscookie=12345" }
+
+        it 'does not add headers to cookies' do
+          response = do_request
+
+          expect(response['Set-Cookie']).to eq(cookies)
+        end
+      end
+    end
+
+    context 'with SSL enabled' do
+      before do
+        allow(Gitlab.config.gitlab).to receive(:https).and_return(true)
+      end
+
+      context 'with no cookies' do
+        let(:cookies) { nil }
+
+        it 'does not add headers' do
+          response = do_request
+
+          expect(response['Set-Cookie']).to be_nil
+        end
+      end
+
+      context 'with single cookie' do
+        let(:cookies) { "thiscookie=12345" }
+
+        it 'adds required headers' do
+          response = do_request
+
+          expect(response['Set-Cookie']).to eq("#{cookies}; Secure; SameSite=None")
+        end
+      end
+
+      context 'multiple cookies' do
+        let(:cookies) { "thiscookie=12345\nanother_cookie=56789" }
+
+        it 'adds required headers' do
+          response = do_request
+
+          expect(response['Set-Cookie']).to eq("thiscookie=12345; Secure; SameSite=None\nanother_cookie=56789; Secure; SameSite=None")
+        end
+      end
+
+      context 'multiple cookies with some missing headers' do
+        let(:cookies) { "thiscookie=12345; SameSite=None\nanother_cookie=56789; Secure" }
+
+        it 'adds missing headers' do
+          response = do_request
+
+          expect(response['Set-Cookie']).to eq("thiscookie=12345; SameSite=None; Secure\nanother_cookie=56789; Secure; SameSite=None")
+        end
+      end
+
+      context 'multiple cookies with all headers present' do
+        let(:cookies) { "thiscookie=12345; Secure; SameSite=None\nanother_cookie=56789; Secure; SameSite=None" }
+
+        it 'does not add new headers' do
+          response = do_request
+
+          expect(response['Set-Cookie']).to eq(cookies)
+        end
+      end
+    end
+  end
+end