Merge pull request #116 from Shopify/save_signature_cookies

Save signature OAuth cookies when using setcookie
Shopify · Nov 30, 2021 · a5238ef · a5238ef
2 parents 104d520 + a9e0093
commit a5238ef
Show file tree

Hide file tree

Showing 4 changed files with 141 additions and 46 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
 - [#117](https://github.com/Shopify/shopify-php-api/pull/117) Handle float `Retry-After` headers
 - [#114](https://github.com/Shopify/shopify-php-api/pull/114) Update session cookie expiration after OAuth
+- [#116](https://github.com/Shopify/shopify-php-api/pull/116) Save signature OAuth cookies when using the fallback function for frameworkless apps
 
 ### Added
 ### Fixed
diff --git a/src/Auth/OAuth.php b/src/Auth/OAuth.php
@@ -25,6 +25,7 @@
 class OAuth
 {
     public const SESSION_ID_COOKIE_NAME = 'shopify_session_id';
+    public const SESSION_ID_SIG_COOKIE_NAME = 'shopify_session_id_sig';
     public const ACCESS_TOKEN_POST_PATH = '/admin/oauth/access_token';
 
     /**
@@ -260,7 +261,18 @@ public static function getCurrentSessionId(array $rawHeaders, array $cookies, bo
      */
     private static function getCookieSessionId(array $cookies): string
     {
-        $sessionId = $cookies[self::SESSION_ID_COOKIE_NAME] ?? null;
+        $signature = $cookies[self::SESSION_ID_SIG_COOKIE_NAME] ?? null;
+        $cookieId = $cookies[self::SESSION_ID_COOKIE_NAME] ?? null;
+
+        $sessionId = null;
+        if ($signature && $cookieId) {
+            $expectedSignature = hash_hmac('sha256', $cookieId, Context::$API_SECRET_KEY);
+
+            if ($signature === $expectedSignature) {
+                $sessionId = $cookieId;
+            }
+        }
+
         if (!$sessionId) {
             throw new CookieNotFoundException("Could not find the current session id in the cookies");
         }
@@ -279,14 +291,27 @@ private static function getCookieSessionId(array $cookies): string
      */
     private static function setCookieSessionId(?callable $setCookieFunction, $sessionId, $expiration): bool
     {
+        $signature = hash_hmac('sha256', $sessionId, Context::$API_SECRET_KEY);
+        $signatureCookie = new OAuthCookie($signature, self::SESSION_ID_SIG_COOKIE_NAME, $expiration, true, true);
         $cookie = new OAuthCookie($sessionId, self::SESSION_ID_COOKIE_NAME, $expiration, true, true);
 
         if ($setCookieFunction) {
-            $cookieSet = $setCookieFunction($cookie);
+            $cookieSet = $setCookieFunction($signatureCookie);
+            $cookieSet = $cookieSet && $setCookieFunction($cookie);
         } else {
             // @codeCoverageIgnoreStart
             // cannot mock setcookie() function
             $cookieSet = setcookie(
+                $signatureCookie->getName(),
+                $signatureCookie->getValue(),
+                $signatureCookie->getExpire(),
+                "",
+                "",
+                $signatureCookie->isSecure(),
+                $signatureCookie->isHttpOnly(),
+            );
+
+            $cookieSet = $cookieSet && setcookie(
                 $cookie->getName(),
                 $cookie->getValue(),
                 $cookie->getExpire(),

diff --git a/tests/Auth/OAuthTest.php b/tests/Auth/OAuthTest.php
@@ -62,12 +62,11 @@ final class OAuthTest extends BaseTestCase
      */
     public function testValidBegin($isOnline)
     {
-        $wasCallbackCalled = false;
-        $testCookieId = '';
-        $cookieCallback = function ($cookie) use (&$wasCallbackCalled, &$testCookieId) {
-            $wasCallbackCalled = true;
-            $testCookieId = $cookie->getValue();
-            return isset($testCookieId);
+        /** @var OAuthCookie[] */
+        $cookiesSet = [];
+        $cookieCallback = function (OAuthCookie $cookie) use (&$cookiesSet) {
+            $cookiesSet[$cookie->getName()] = $cookie;
+            return !empty($cookie->getValue());
         };
 
         $returnUrl = OAuth::begin(
@@ -76,15 +75,20 @@ public function testValidBegin($isOnline)
             $isOnline,
             $cookieCallback,
         );
-        $this->assertTrue($wasCallbackCalled);
-        $this->assertNotEmpty($testCookieId);
+        $this->assertNotEmpty($cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getValue());
+        $this->assertEquals(
+            hash_hmac('sha256', $cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getValue(), Context::$API_SECRET_KEY),
+            $cookiesSet[OAuth::SESSION_ID_SIG_COOKIE_NAME]->getValue()
+        );
 
         if ($isOnline) {
             $grantOptions = 'per-user';
         } else {
             $grantOptions = '';
         }
-        $generatedState = Context::$SESSION_STORAGE->loadSession($testCookieId)->getState();
+
+        $cookieSessionId = $cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getValue();
+        $generatedState = Context::$SESSION_STORAGE->loadSession($cookieSessionId)->getState();
         $this->assertEquals(
             // phpcs:ignore
             "https://shopname.myshopify.com/admin/oauth/authorize?client_id=ash&scope=sleepy%2Ckitty&redirect_uri=https%3A%2F%2Fwww.my-friends-cats.com%2Fredirect&state={$generatedState}&grant_options%5B%5D=$grantOptions",
@@ -107,18 +111,11 @@ public function testValidCallback($isOnline, $isEmbedded)
     {
         Context::$IS_EMBEDDED_APP = $isEmbedded;
 
-        $wasCallbackCalled = false;
-        $testCookieId = '';
-        $testCookieExpiration = null;
-        $cookieCallback = function (OAuthCookie $cookie) use (
-            &$wasCallbackCalled,
-            &$testCookieId,
-            &$testCookieExpiration
-        ) {
-            $wasCallbackCalled = true;
-            $testCookieExpiration = $cookie->getExpire();
-            $testCookieId = $cookie->getValue();
-            return isset($testCookieId);
+        /** @var OAuthCookie[] */
+        $cookiesSet = [];
+        $cookieCallback = function (OAuthCookie $cookie) use (&$cookiesSet) {
+            $cookiesSet[$cookie->getName()] = $cookie;
+            return !empty($cookie->getValue());
         };
 
         $this->createTestSession($isOnline);
@@ -134,16 +131,22 @@ public function testValidCallback($isOnline, $isEmbedded)
             ),
         ]);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'state' => '1234',
             'code' => 'real_code',
             'hmac' => '0b19b6077391191829e442a97aafd7730323041e585f738415a77894c41c0a5b',
         ];
         $actualSession = OAuth::callback($mockCookies, $mockQuery, $cookieCallback);
-        $this->assertTrue($wasCallbackCalled);
-        $this->assertEquals($this->oauthSessionId, $testCookieId);
+        $this->assertEquals($this->oauthSessionId, $cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getValue());
+        $this->assertEquals(
+            hash_hmac('sha256', $cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getValue(), Context::$API_SECRET_KEY),
+            $cookiesSet[OAuth::SESSION_ID_SIG_COOKIE_NAME]->getValue()
+        );
 
         $jwtSessionId = OAuth::getJwtSessionId($this->domain, '1');
 
@@ -160,12 +163,13 @@ public function testValidCallback($isOnline, $isEmbedded)
         $expectedSession = $this->buildExpectedSession($expectedSessionId, $isOnline);
         $this->assertEquals($expectedSession, $actualSession);
 
+        $cookieExpiration = $cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getExpire();
         if ($isEmbedded) {
-            $this->assertLessThanOrEqual(1, abs(time() - $testCookieExpiration)); // 1 second grace period
+            $this->assertLessThanOrEqual(1, abs(time() - $cookieExpiration)); // 1 second grace period
         } elseif ($isOnline) {
-            $this->assertEquals($expectedSession->getExpires()->format('U'), $testCookieExpiration);
+            $this->assertEquals($expectedSession->getExpires()->format('U'), $cookieExpiration);
         } else {
-            $this->assertNull($testCookieExpiration);
+            $this->assertNull($cookieExpiration);
         }
     }
 
@@ -188,11 +192,28 @@ public function testCallbackFailsWithoutCookie()
         OAuth::callback([], []);
     }
 
+    public function testCallbackFailsWithInvalidSignature()
+    {
+        $this->createTestSession(false);
+
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => "Not the right signature",
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
+        $this->expectException(\Shopify\Exception\CookieNotFoundException::class);
+        $this->expectExceptionMessage('Could not find the current session id in the cookies');
+        OAuth::callback($mockCookies, []);
+    }
+
     public function testCallbackFailsWithoutSession()
     {
         $this->createTestSession(false);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => "👋 This is not the session you're looking for"];
+        $sessionId = "👋 This is not the session you're looking for";
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $sessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $sessionId,
+        ];
         $this->expectException(OAuthSessionNotFoundException::class);
         $this->expectExceptionMessage(
             'You may have taken more than 60 seconds to complete the OAuth process and the session cannot be found'
@@ -204,7 +225,10 @@ public function testCallbackFailsWithMissingHMAC()
     {
         $this->createTestSession(false);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'state' => '1234',
@@ -219,7 +243,10 @@ public function testCallbackFailsWithInvalidHMAC()
     {
         $this->createTestSession(false);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'state' => '1234',
@@ -235,7 +262,10 @@ public function testCallbackFailsWithMissingShop()
     {
         $this->createTestSession(false);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'state' => '1234',
             'hmac' => '0b19b6077391191829e442a97aafd7730323041e585f738415a77894c41c0a5b',
@@ -249,7 +279,10 @@ public function testCallbackFailsWithInvalidShop()
     {
         $this->createTestSession(false);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => 'not-a-valid.domain',
             'state' => '1234',
@@ -265,7 +298,10 @@ public function testCallbackFailsWithMissingState()
     {
         $this->createTestSession(false);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'code' => 'real_code',
@@ -280,7 +316,10 @@ public function testCallbackFailsWithInvalidState()
     {
         $this->createTestSession(false);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'state' => '4321',
@@ -296,7 +335,10 @@ public function testCallbackFailsWithMissingCode()
     {
         $this->createTestSession(false);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'state' => '1234',
@@ -311,7 +353,10 @@ public function testCallbackFailsWithInvalidCode()
     {
         $this->createTestSession(false);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'state' => '1234',
@@ -329,7 +374,10 @@ public function testFailsForPrivateApp()
 
         Context::$IS_PRIVATE_APP = true;
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'state' => '1234',
@@ -358,7 +406,10 @@ public function testThrowsIfSessionDeleteFails()
             ),
         ]);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'state' => '1234',
@@ -390,7 +441,10 @@ public function testThrowsIfSessionStoreFails()
             ),
         ]);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'state' => '1234',
@@ -419,7 +473,10 @@ public function testFailsIfTokenFetchFails()
             ),
         ]);
 
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
         $mockQuery = [
             'shop' => $this->domain,
             'state' => '1234',
@@ -498,7 +555,10 @@ public function testGetCurrentSessionIdRaisesCookieNotFoundException()
     public function testGetCurrentSessionIdNonEmbeddedApp()
     {
         Context::$IS_EMBEDDED_APP = false;
-        $mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
+        $mockCookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
+        ];
 
         $currentSessionId = OAuth::getCurrentSessionId([], $mockCookies, true);
         $this->assertEquals('test_oauth_session', $currentSessionId);

diff --git a/tests/UtilsTest.php b/tests/UtilsTest.php
@@ -220,7 +220,10 @@ public function testGraphqlProxyFailsWithJWTForNonEmbeddedApps()
 
         $token = $this->encodeJwtPayload();
         $headers = ['Authorization' => "Bearer $token"];
-        $cookies = [OAuth::SESSION_ID_COOKIE_NAME => 'cookie_id'];
+        $cookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', 'cookie_id', Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => 'cookie_id',
+        ];
 
         // The session is valid and can be loaded from the headers
         Context::$IS_EMBEDDED_APP = true;
@@ -241,7 +244,10 @@ public function testGraphqlProxyFailsWithCookiesForEmbeddedApps()
 
         $token = $this->encodeJwtPayload();
         $headers = ['Authorization' => "Bearer $token"];
-        $cookies = [OAuth::SESSION_ID_COOKIE_NAME => 'cookie_id'];
+        $cookies = [
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', 'cookie_id', Context::$API_SECRET_KEY),
+            OAuth::SESSION_ID_COOKIE_NAME => 'cookie_id',
+        ];
 
         // The session is valid and can be loaded from the cookies
         Context::$IS_EMBEDDED_APP = false;
@@ -311,7 +317,10 @@ public function testGraphqlProxyFetchesDataWithCookies()
             )
         ]);
 
-        $cookies = [OAuth::SESSION_ID_COOKIE_NAME => $sessionId];
+        $cookies = [
+            OAuth::SESSION_ID_COOKIE_NAME => $sessionId,
+            OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $sessionId, Context::$API_SECRET_KEY),
+        ];
         $response = Utils::graphqlProxy([], $cookies, $this->testGraphqlQuery);
 
         $this->assertThat($response, new HttpResponseMatcher(200, [], $this->testGraphqlResponse));