Merge branch 'sh-same-site-none-fix-try2' into 'master'

Don't send SameSite=None to incompatible browsers

See merge request gitlab-org/gitlab!40667

changelogs/unreleased/sh-same-site-none-fix-try2.yml

+---
+title: Don't send SameSite=None to incompatible browsers
+merge_request: 40667
+author:
+type: fixed

lib/gitlab/middleware/same_site_cookies.rb

@@ -30,6 +30,7 @@ module Gitlab
         set_cookie = headers['Set-Cookie']&.strip
 
         return result if set_cookie.blank? || !ssl?
+        return result if same_site_none_incompatible?(headers['User-Agent'])
 
         cookies = set_cookie.split(COOKIE_SEPARATOR)
 
@@ -39,11 +40,11 @@ module Gitlab
           # 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)
+          unless SECURE_REGEX.match?(cookie)
             cookie << '; Secure'
           end
 
-          unless cookie =~ /;\s*samesite=/i
+          unless SAME_SITE_REGEX.match?(cookie)
             cookie << '; SameSite=None'
           end
         end
@@ -55,9 +56,93 @@ module Gitlab
 
       private
 
+      # Taken from https://www.chromium.org/updates/same-site/incompatible-clients
+      # We use RE2 instead of the browser gem for performance.
+      IOS_REGEX = RE2('\(iP.+; CPU .*OS (\d+)[_\d]*.*\) AppleWebKit\/')
+      MACOS_REGEX = RE2('\(Macintosh;.*Mac OS X (\d+)_(\d+)[_\d]*.*\) AppleWebKit\/')
+      SAFARI_REGEX = RE2('Version\/.* Safari\/')
+      CHROMIUM_REGEX = RE2('Chrom(e|ium)')
+      CHROMIUM_VERSION_REGEX = RE2('Chrom[^ \/]+\/(\d+)')
+      UC_BROWSER_REGEX = RE2('UCBrowser\/')
+      UC_BROWSER_VERSION_REGEX = RE2('UCBrowser\/(\d+)\.(\d+)\.(\d+)')
+
+      SECURE_REGEX = RE2(';\s*secure', case_sensitive: false)
+      SAME_SITE_REGEX = RE2(';\s*samesite=', case_sensitive: false)
+
       def ssl?
         Gitlab.config.gitlab.https
       end
+
+      def same_site_none_incompatible?(user_agent)
+        return false if user_agent.blank?
+
+        has_webkit_same_site_bug?(user_agent) || drops_unrecognized_same_site_cookies?(user_agent)
+      end
+
+      def has_webkit_same_site_bug?(user_agent)
+        ios_version?(12, user_agent) ||
+          (macos_version?(10, 14, user_agent) && safari?(user_agent))
+      end
+
+      def drops_unrecognized_same_site_cookies?(user_agent)
+        if uc_browser?(user_agent)
+          return !uc_browser_version_at_least?(12, 13, 2, user_agent)
+        end
+
+        chromium_based?(user_agent) && chromium_version_between?(51, 66, user_agent)
+      end
+
+      def ios_version?(major, user_agent)
+        m = IOS_REGEX.match(user_agent)
+
+        return false if m.nil?
+
+        m[1].to_i == major
+      end
+
+      def macos_version?(major, minor, user_agent)
+        m = MACOS_REGEX.match(user_agent)
+
+        return false if m.nil?
+
+        m[1].to_i == major && m[2].to_i == minor
+      end
+
+      def safari?(user_agent)
+        SAFARI_REGEX.match?(user_agent)
+      end
+
+      def chromium_based?(user_agent)
+        CHROMIUM_REGEX.match?(user_agent)
+      end
+
+      def chromium_version_between?(from_major, to_major, user_agent)
+        m = CHROMIUM_VERSION_REGEX.match(user_agent)
+
+        return false if m.nil?
+
+        version = m[1].to_i
+        version >= from_major && version <= to_major
+      end
+
+      def uc_browser?(user_agent)
+        UC_BROWSER_REGEX.match?(user_agent)
+      end
+
+      def uc_browser_version_at_least?(major, minor, build, user_agent)
+        m = UC_BROWSER_VERSION_REGEX.match(user_agent)
+
+        return false if m.nil?
+
+        major_version = m[1].to_i
+        minor_version = m[2].to_i
+        build_version = m[3].to_i
+
+        return major_version > major if major_version != major
+        return minor_version > minor if minor_version != minor
+
+        build_version >= build
+      end
     end
   end
 end

spec/lib/gitlab/middleware/same_site_cookies_spec.rb

@@ -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" }