Skip to content

Commit a5238ef

Browse files
paulomargpaulomarg
authored
Merge pull request #116 from Shopify/save_signature_cookies
Save signature OAuth cookies when using setcookie
2 parents 104d520 + a9e0093 commit a5238ef

4 files changed

Lines changed: 141 additions & 46 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
99

1010
- [#117](https://github.com/Shopify/shopify-php-api/pull/117) Handle float `Retry-After` headers
1111
- [#114](https://github.com/Shopify/shopify-php-api/pull/114) Update session cookie expiration after OAuth
12+
- [#116](https://github.com/Shopify/shopify-php-api/pull/116) Save signature OAuth cookies when using the fallback function for frameworkless apps
1213

1314
### Added
1415
### Fixed

‎src/Auth/OAuth.php‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
class OAuth
2626
{
2727
public const SESSION_ID_COOKIE_NAME = 'shopify_session_id';
28+
public const SESSION_ID_SIG_COOKIE_NAME = 'shopify_session_id_sig';
2829
public const ACCESS_TOKEN_POST_PATH = '/admin/oauth/access_token';
2930

3031
/**
@@ -260,7 +261,18 @@ public static function getCurrentSessionId(array $rawHeaders, array $cookies, bo
260261
*/
261262
private static function getCookieSessionId(array $cookies): string
262263
{
263-
$sessionId = $cookies[self::SESSION_ID_COOKIE_NAME] ?? null;
264+
$signature = $cookies[self::SESSION_ID_SIG_COOKIE_NAME] ?? null;
265+
$cookieId = $cookies[self::SESSION_ID_COOKIE_NAME] ?? null;
266+
267+
$sessionId = null;
268+
if ($signature && $cookieId) {
269+
$expectedSignature = hash_hmac('sha256', $cookieId, Context::$API_SECRET_KEY);
270+
271+
if ($signature === $expectedSignature) {
272+
$sessionId = $cookieId;
273+
}
274+
}
275+
264276
if (!$sessionId) {
265277
throw new CookieNotFoundException("Could not find the current session id in the cookies");
266278
}
@@ -279,14 +291,27 @@ private static function getCookieSessionId(array $cookies): string
279291
*/
280292
private static function setCookieSessionId(?callable $setCookieFunction, $sessionId, $expiration): bool
281293
{
294+
$signature = hash_hmac('sha256', $sessionId, Context::$API_SECRET_KEY);
295+
$signatureCookie = new OAuthCookie($signature, self::SESSION_ID_SIG_COOKIE_NAME, $expiration, true, true);
282296
$cookie = new OAuthCookie($sessionId, self::SESSION_ID_COOKIE_NAME, $expiration, true, true);
283297

284298
if ($setCookieFunction) {
285-
$cookieSet = $setCookieFunction($cookie);
299+
$cookieSet = $setCookieFunction($signatureCookie);
300+
$cookieSet = $cookieSet && $setCookieFunction($cookie);
286301
} else {
287302
// @codeCoverageIgnoreStart
288303
// cannot mock setcookie() function
289304
$cookieSet = setcookie(
305+
$signatureCookie->getName(),
306+
$signatureCookie->getValue(),
307+
$signatureCookie->getExpire(),
308+
"",
309+
"",
310+
$signatureCookie->isSecure(),
311+
$signatureCookie->isHttpOnly(),
312+
);
313+
314+
$cookieSet = $cookieSet && setcookie(
290315
$cookie->getName(),
291316
$cookie->getValue(),
292317
$cookie->getExpire(),

‎tests/Auth/OAuthTest.php‎

Lines changed: 101 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,11 @@ final class OAuthTest extends BaseTestCase
6262
*/
6363
public function testValidBegin($isOnline)
6464
{
65-
$wasCallbackCalled = false;
66-
$testCookieId = '';
67-
$cookieCallback = function ($cookie) use (&$wasCallbackCalled, &$testCookieId) {
68-
$wasCallbackCalled = true;
69-
$testCookieId = $cookie->getValue();
70-
return isset($testCookieId);
65+
/** @var OAuthCookie[] */
66+
$cookiesSet = [];
67+
$cookieCallback = function (OAuthCookie $cookie) use (&$cookiesSet) {
68+
$cookiesSet[$cookie->getName()] = $cookie;
69+
return !empty($cookie->getValue());
7170
};
7271

7372
$returnUrl = OAuth::begin(
@@ -76,15 +75,20 @@ public function testValidBegin($isOnline)
7675
$isOnline,
7776
$cookieCallback,
7877
);
79-
$this->assertTrue($wasCallbackCalled);
80-
$this->assertNotEmpty($testCookieId);
78+
$this->assertNotEmpty($cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getValue());
79+
$this->assertEquals(
80+
hash_hmac('sha256', $cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getValue(), Context::$API_SECRET_KEY),
81+
$cookiesSet[OAuth::SESSION_ID_SIG_COOKIE_NAME]->getValue()
82+
);
8183

8284
if ($isOnline) {
8385
$grantOptions = 'per-user';
8486
} else {
8587
$grantOptions = '';
8688
}
87-
$generatedState = Context::$SESSION_STORAGE->loadSession($testCookieId)->getState();
89+
90+
$cookieSessionId = $cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getValue();
91+
$generatedState = Context::$SESSION_STORAGE->loadSession($cookieSessionId)->getState();
8892
$this->assertEquals(
8993
// phpcs:ignore
9094
"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)
107111
{
108112
Context::$IS_EMBEDDED_APP = $isEmbedded;
109113

110-
$wasCallbackCalled = false;
111-
$testCookieId = '';
112-
$testCookieExpiration = null;
113-
$cookieCallback = function (OAuthCookie $cookie) use (
114-
&$wasCallbackCalled,
115-
&$testCookieId,
116-
&$testCookieExpiration
117-
) {
118-
$wasCallbackCalled = true;
119-
$testCookieExpiration = $cookie->getExpire();
120-
$testCookieId = $cookie->getValue();
121-
return isset($testCookieId);
114+
/** @var OAuthCookie[] */
115+
$cookiesSet = [];
116+
$cookieCallback = function (OAuthCookie $cookie) use (&$cookiesSet) {
117+
$cookiesSet[$cookie->getName()] = $cookie;
118+
return !empty($cookie->getValue());
122119
};
123120

124121
$this->createTestSession($isOnline);
@@ -134,16 +131,22 @@ public function testValidCallback($isOnline, $isEmbedded)
134131
),
135132
]);
136133

137-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
134+
$mockCookies = [
135+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
136+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
137+
];
138138
$mockQuery = [
139139
'shop' => $this->domain,
140140
'state' => '1234',
141141
'code' => 'real_code',
142142
'hmac' => '0b19b6077391191829e442a97aafd7730323041e585f738415a77894c41c0a5b',
143143
];
144144
$actualSession = OAuth::callback($mockCookies, $mockQuery, $cookieCallback);
145-
$this->assertTrue($wasCallbackCalled);
146-
$this->assertEquals($this->oauthSessionId, $testCookieId);
145+
$this->assertEquals($this->oauthSessionId, $cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getValue());
146+
$this->assertEquals(
147+
hash_hmac('sha256', $cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getValue(), Context::$API_SECRET_KEY),
148+
$cookiesSet[OAuth::SESSION_ID_SIG_COOKIE_NAME]->getValue()
149+
);
147150

148151
$jwtSessionId = OAuth::getJwtSessionId($this->domain, '1');
149152

@@ -160,12 +163,13 @@ public function testValidCallback($isOnline, $isEmbedded)
160163
$expectedSession = $this->buildExpectedSession($expectedSessionId, $isOnline);
161164
$this->assertEquals($expectedSession, $actualSession);
162165

166+
$cookieExpiration = $cookiesSet[OAuth::SESSION_ID_COOKIE_NAME]->getExpire();
163167
if ($isEmbedded) {
164-
$this->assertLessThanOrEqual(1, abs(time() - $testCookieExpiration)); // 1 second grace period
168+
$this->assertLessThanOrEqual(1, abs(time() - $cookieExpiration)); // 1 second grace period
165169
} elseif ($isOnline) {
166-
$this->assertEquals($expectedSession->getExpires()->format('U'), $testCookieExpiration);
170+
$this->assertEquals($expectedSession->getExpires()->format('U'), $cookieExpiration);
167171
} else {
168-
$this->assertNull($testCookieExpiration);
172+
$this->assertNull($cookieExpiration);
169173
}
170174
}
171175

@@ -188,11 +192,28 @@ public function testCallbackFailsWithoutCookie()
188192
OAuth::callback([], []);
189193
}
190194

195+
public function testCallbackFailsWithInvalidSignature()
196+
{
197+
$this->createTestSession(false);
198+
199+
$mockCookies = [
200+
OAuth::SESSION_ID_SIG_COOKIE_NAME => "Not the right signature",
201+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
202+
];
203+
$this->expectException(\Shopify\Exception\CookieNotFoundException::class);
204+
$this->expectExceptionMessage('Could not find the current session id in the cookies');
205+
OAuth::callback($mockCookies, []);
206+
}
207+
191208
public function testCallbackFailsWithoutSession()
192209
{
193210
$this->createTestSession(false);
194211

195-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => "👋 This is not the session you're looking for"];
212+
$sessionId = "👋 This is not the session you're looking for";
213+
$mockCookies = [
214+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $sessionId, Context::$API_SECRET_KEY),
215+
OAuth::SESSION_ID_COOKIE_NAME => $sessionId,
216+
];
196217
$this->expectException(OAuthSessionNotFoundException::class);
197218
$this->expectExceptionMessage(
198219
'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()
204225
{
205226
$this->createTestSession(false);
206227

207-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
228+
$mockCookies = [
229+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
230+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
231+
];
208232
$mockQuery = [
209233
'shop' => $this->domain,
210234
'state' => '1234',
@@ -219,7 +243,10 @@ public function testCallbackFailsWithInvalidHMAC()
219243
{
220244
$this->createTestSession(false);
221245

222-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
246+
$mockCookies = [
247+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
248+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
249+
];
223250
$mockQuery = [
224251
'shop' => $this->domain,
225252
'state' => '1234',
@@ -235,7 +262,10 @@ public function testCallbackFailsWithMissingShop()
235262
{
236263
$this->createTestSession(false);
237264

238-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
265+
$mockCookies = [
266+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
267+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
268+
];
239269
$mockQuery = [
240270
'state' => '1234',
241271
'hmac' => '0b19b6077391191829e442a97aafd7730323041e585f738415a77894c41c0a5b',
@@ -249,7 +279,10 @@ public function testCallbackFailsWithInvalidShop()
249279
{
250280
$this->createTestSession(false);
251281

252-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
282+
$mockCookies = [
283+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
284+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
285+
];
253286
$mockQuery = [
254287
'shop' => 'not-a-valid.domain',
255288
'state' => '1234',
@@ -265,7 +298,10 @@ public function testCallbackFailsWithMissingState()
265298
{
266299
$this->createTestSession(false);
267300

268-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
301+
$mockCookies = [
302+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
303+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
304+
];
269305
$mockQuery = [
270306
'shop' => $this->domain,
271307
'code' => 'real_code',
@@ -280,7 +316,10 @@ public function testCallbackFailsWithInvalidState()
280316
{
281317
$this->createTestSession(false);
282318

283-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
319+
$mockCookies = [
320+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
321+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
322+
];
284323
$mockQuery = [
285324
'shop' => $this->domain,
286325
'state' => '4321',
@@ -296,7 +335,10 @@ public function testCallbackFailsWithMissingCode()
296335
{
297336
$this->createTestSession(false);
298337

299-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
338+
$mockCookies = [
339+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
340+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
341+
];
300342
$mockQuery = [
301343
'shop' => $this->domain,
302344
'state' => '1234',
@@ -311,7 +353,10 @@ public function testCallbackFailsWithInvalidCode()
311353
{
312354
$this->createTestSession(false);
313355

314-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
356+
$mockCookies = [
357+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
358+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
359+
];
315360
$mockQuery = [
316361
'shop' => $this->domain,
317362
'state' => '1234',
@@ -329,7 +374,10 @@ public function testFailsForPrivateApp()
329374

330375
Context::$IS_PRIVATE_APP = true;
331376

332-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
377+
$mockCookies = [
378+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
379+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
380+
];
333381
$mockQuery = [
334382
'shop' => $this->domain,
335383
'state' => '1234',
@@ -358,7 +406,10 @@ public function testThrowsIfSessionDeleteFails()
358406
),
359407
]);
360408

361-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
409+
$mockCookies = [
410+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
411+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
412+
];
362413
$mockQuery = [
363414
'shop' => $this->domain,
364415
'state' => '1234',
@@ -390,7 +441,10 @@ public function testThrowsIfSessionStoreFails()
390441
),
391442
]);
392443

393-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
444+
$mockCookies = [
445+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
446+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
447+
];
394448
$mockQuery = [
395449
'shop' => $this->domain,
396450
'state' => '1234',
@@ -419,7 +473,10 @@ public function testFailsIfTokenFetchFails()
419473
),
420474
]);
421475

422-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
476+
$mockCookies = [
477+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
478+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
479+
];
423480
$mockQuery = [
424481
'shop' => $this->domain,
425482
'state' => '1234',
@@ -498,7 +555,10 @@ public function testGetCurrentSessionIdRaisesCookieNotFoundException()
498555
public function testGetCurrentSessionIdNonEmbeddedApp()
499556
{
500557
Context::$IS_EMBEDDED_APP = false;
501-
$mockCookies = [OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId];
558+
$mockCookies = [
559+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $this->oauthSessionId, Context::$API_SECRET_KEY),
560+
OAuth::SESSION_ID_COOKIE_NAME => $this->oauthSessionId,
561+
];
502562

503563
$currentSessionId = OAuth::getCurrentSessionId([], $mockCookies, true);
504564
$this->assertEquals('test_oauth_session', $currentSessionId);

‎tests/UtilsTest.php‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,10 @@ public function testGraphqlProxyFailsWithJWTForNonEmbeddedApps()
220220

221221
$token = $this->encodeJwtPayload();
222222
$headers = ['Authorization' => "Bearer $token"];
223-
$cookies = [OAuth::SESSION_ID_COOKIE_NAME => 'cookie_id'];
223+
$cookies = [
224+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', 'cookie_id', Context::$API_SECRET_KEY),
225+
OAuth::SESSION_ID_COOKIE_NAME => 'cookie_id',
226+
];
224227

225228
// The session is valid and can be loaded from the headers
226229
Context::$IS_EMBEDDED_APP = true;
@@ -241,7 +244,10 @@ public function testGraphqlProxyFailsWithCookiesForEmbeddedApps()
241244

242245
$token = $this->encodeJwtPayload();
243246
$headers = ['Authorization' => "Bearer $token"];
244-
$cookies = [OAuth::SESSION_ID_COOKIE_NAME => 'cookie_id'];
247+
$cookies = [
248+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', 'cookie_id', Context::$API_SECRET_KEY),
249+
OAuth::SESSION_ID_COOKIE_NAME => 'cookie_id',
250+
];
245251

246252
// The session is valid and can be loaded from the cookies
247253
Context::$IS_EMBEDDED_APP = false;
@@ -311,7 +317,10 @@ public function testGraphqlProxyFetchesDataWithCookies()
311317
)
312318
]);
313319

314-
$cookies = [OAuth::SESSION_ID_COOKIE_NAME => $sessionId];
320+
$cookies = [
321+
OAuth::SESSION_ID_COOKIE_NAME => $sessionId,
322+
OAuth::SESSION_ID_SIG_COOKIE_NAME => hash_hmac('sha256', $sessionId, Context::$API_SECRET_KEY),
323+
];
315324
$response = Utils::graphqlProxy([], $cookies, $this->testGraphqlQuery);
316325

317326
$this->assertThat($response, new HttpResponseMatcher(200, [], $this->testGraphqlResponse));

0 commit comments

Comments
 (0)