Bug fix for incorrect SameSite=none behaviour on some platforms. (#386)

* Attempt to fix incorrect samesite none compatibility for some user agents. Instead of manually generating the major and minor numbers of a version, to rely on the Agent library's built in tools. * UCBrowser 12.14 should be compatible (incompatible version is 12.3 and below) * Fixes to platform checker since it's all checking the float version now. * Updated version constraint for Laravel 6 in travis since Laravel 6 is now using Semver. previously, it would only receive bug-fixes to 6.0, but now minor releases are in 6.* as well. * Added a compatible user agent that was previously reported as failing * Flipped the samesite=none logic from a whitelist to a blacklist. This means that bad agents will be treated as compatible by default. * Changed browser detection to be more explicit to prevent errors caused by user-agents with more than 1 keyword. * Reset the session config to laravel default before each test. * Added an incompatible version of Chrome to the test. * Added some additional user agents that were reported as failing in the existing code. * Removed the try catch since there is no longer an expected scenario where an exception would occur
gnikyt · Feb 6, 2020 · dfd8b6b · dfd8b6b
1 parent 2f6f1d2
commit dfd8b6b
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 44 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -11,13 +11,13 @@ env:
   - LARAVEL_VERSION=5.6.*
   - LARAVEL_VERSION=5.7.*
   - LARAVEL_VERSION=5.8.*
-  - LARAVEL_VERSION=6.0.*
+  - LARAVEL_VERSION=^6.0
 
 matrix:
   fast_finish: true
   exclude:
   - php: 7.1
-    env: LARAVEL_VERSION=6.0.*
+    env: LARAVEL_VERSION=^6.0
 
 cache:
   directories:

diff --git a/src/ShopifyApp/Services/ShopSession.php b/src/ShopifyApp/Services/ShopSession.php
@@ -287,39 +287,35 @@ public function setCookiePolicy()
      */
     private function checkSameSiteNoneCompatible()
     {
-        $compatible = false;
+        $compatible = true;
 
         $this->agent = new Agent();
 
-        try {
-            $browser = $this->getBrowserDetails();
-            $platform = $this->getPlatformDetails();
-
-            if ($this->agent->is('Chrome') && $browser['major'] >= 67) {
-                $compatible = true;
-            }
-
-            if ($this->agent->is('iOS') && $platform['major'] > 12) {
-                $compatible = true;
-            }
-
-            if ($this->agent->is('OS X') &&
-                ($this->agent->is('Safari') && !$this->agent->is('iOS')) &&
-                $platform['float'] > 10.14
-            ) {
-                $compatible = true;
-            }
-
-            if ($this->agent->is('UCBrowser') &&
-                $browser['float'] > 12.13
-            ) {
-                $compatible = true;
-            }
-
-            return $compatible;
-        } catch (\Exception $e) {
-            return false;
+        $browser = $this->getBrowserDetails();
+        $platform = $this->getPlatformDetails();
+
+        if ($this->agent->browser() == 'Chrome' && $browser['float'] < 67) {
+            $compatible = false;
+        }
+
+        if ($this->agent->is('iOS') && $platform['float'] < 13) {
+            $compatible = false;
+        }
+
+        if ($this->agent->is('OS X') &&
+            ($this->agent->browser() == 'Safari' && !$this->agent->is('iOS')) &&
+            $platform['float'] < 10.15
+        ) {
+            $compatible = false;
+        }
+
+        if ($this->agent->browser() == 'UCBrowser' &&
+            $browser['float'] < 12.132
+        ) {
+            $compatible = false;
         }
+
+        return $compatible;
     }
 
     /**
@@ -329,13 +325,10 @@ private function checkSameSiteNoneCompatible()
      */
     private function getBrowserDetails()
     {
-        $version = $this->agent->version($this->agent->browser());
-        $pieces = explode('.', str_replace('_', '.', $version));
+        $version = $this->agent->version($this->agent->browser(), Agent::VERSION_TYPE_FLOAT);
 
         return [
-            'major' => $pieces[0],
-            'minor' => $pieces[1],
-            'float' => (float) sprintf('%s.%s', $pieces[0], $pieces[1]),
+            'float' => ($version ?: 0),
         ];
     }
 
@@ -346,13 +339,10 @@ private function getBrowserDetails()
      */
     private function getPlatformDetails()
     {
-        $version = $this->agent->version($this->agent->platform());
-        $pieces = explode('.', str_replace('_', '.', $version));
+        $version = $this->agent->version($this->agent->platform(), Agent::VERSION_TYPE_FLOAT);
 
         return [
-            'major' => $pieces[0],
-            'minor' => $pieces[1],
-            'float' => (float) sprintf('%s.%s', $pieces[0], $pieces[1]),
+            'float' => ($version ?: 0),
         ];
     }
 }
diff --git a/tests/Services/ShopSessionTest.php b/tests/Services/ShopSessionTest.php
@@ -122,22 +122,28 @@ public function testCanForget()
     public function testSameSiteCookie()
     {
         $incompatibleUserAgents = [
+            'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.78 Safari/537.36',
             'Mozilla/5.0 (iPhone; CPU iPhone OS 12_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) GSA/87.0.279142407 Mobile/15E148 Safari/605.1',
             'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1.2 Safari/605.1.15',
             'Mozilla/5.0 (Linux; U; Android 7.0; en-US; SM-G935F Build/NRD90M) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 UCBrowser/11.3.8.976 U3/0.8.0 Mobile Safari/534.30',
             'UCWEB/2.0 (Java; U; MIDP-2.0; en-US; generic) U2/1.0.0 UCBrowser/9.5.0.449 U2/1.0.0 Mobile',
-            'Mozilla/5.0 (Linux; U; Android 9; zh-CN; Nokia X7 Build/PPR1.180610.011) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.14.0.1020 Mobile Safari/537.36',
             'Mozilla/5.0 (iPod; CPU iPhone OS 12_0 like macOS) AppleWebKit/602.1.50 (KHTML, like Gecko) Version/12.0 Mobile/14A5335d Safari/602.1.50',
         ];
 
         $compatibleUserAgents = [
+            'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.78 Safari/537.36',
             'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.117 Safari/537.36',
             'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:72.0) Gecko/20100101 Firefox/72.0',
             'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_2) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0.4 Safari/605.1.15',
             'Mozilla/5.0 (Linux; U; Android 9; zh-CN; Nokia X7 Build/PPR1.180610.011) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.15.0.1020 Mobile Safari/537.36',
             'Mozilla/5.0 (iPhone; CPU iPhone OS 13_0 like Mac OS X) AppleWebKit/602.1.38 (KHTML, like Gecko) Version/66.6 Mobile/14A5297c Safari/602.1',
             'Mozilla/5.0 (Linux; U; Android 7.1.2; en-US; GT-N5110 Build/NJH47F) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.14.0.1221 Mobile Safari/537.36',
+            'Mozilla/5.0 (Linux; U; Android 9; zh-CN; Nokia X7 Build/PPR1.180610.011) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.14.0.1020 Mobile Safari/537.36',
             'UCWEB/2.0 (Java; U; MIDP-2.0; en-US; generic) U2/1.0.0 UCBrowser/12.15.0.449 U2/2.0.0 Mobile',
+            'Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0',
+            'Mozilla/5.0 zgrab/0.x',
+            'AWS Security Scanner',
+            'Mozilla/5.0 (compatible; Baiduspider/2.0; +http://www.baidu.com/search/spider.html)',
         ];
 
         $badUserAgents = [
@@ -151,14 +157,26 @@ public function testSameSiteCookie()
         $shop = factory(Shop::class)->create();
 
         foreach ($badUserAgents as $agent) {
+            // reset sessions before each test
+            config([
+                'session.secure'    => false,
+                'session.same_site' => null,
+            ]);
+
             $_SERVER['HTTP_USER_AGENT'] = $agent;
             $response = $this->get('/');
 
-            $this->assertNull($response->baseResponse->headers->getCookies()[0]->getSameSite());
-            $this->assertFalse($response->baseResponse->headers->getCookies()[0]->isSecure());
+            $this->assertEquals('none', $response->baseResponse->headers->getCookies()[0]->getSameSite());
+            $this->assertTrue($response->baseResponse->headers->getCookies()[0]->isSecure());
         }
 
         foreach ($incompatibleUserAgents as $agent) {
+            // reset sessions before each test
+            config([
+                'session.secure'    => false,
+                'session.same_site' => null,
+            ]);
+
             $_SERVER['HTTP_USER_AGENT'] = $agent;
             $response = $this->get('/');
 
@@ -167,6 +185,12 @@ public function testSameSiteCookie()
         }
 
         foreach ($compatibleUserAgents as $agent) {
+            // reset sessions before each test
+            config([
+                'session.secure'    => false,
+                'session.same_site' => null,
+            ]);
+
             $_SERVER['HTTP_USER_AGENT'] = $agent;
             $response = $this->get('/');