Skip to content

Commit 04b1f5d

Browse files
evansimsevansims
authored
[SDK-3646] Reliability and performance improvements to CookieStore (#649)
* Improve CookieStore reliability under some circumstances * Adjust tests to reflect CookieStore changes * Ignore code coverage on untestable circumstances * Remove leftover debug code * Temporarily lock dependency due to break upstream * Code quality improvements * Additional improvements * Expand tests
1 parent 1e34266 commit 04b1f5d

File tree

5 files changed

+181
-110
lines changed

5 files changed

+181
-110
lines changed

‎composer.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@
5050
"pestphp/pest": "^1.21",
5151
"php-http/mock-client": "^1.5",
5252
"phpstan/phpstan": "^1.7",
53-
"phpstan/phpstan-strict-rules": "^1.3",
53+
"phpstan/phpstan-strict-rules": "1.4.3",
5454
"phpunit/phpunit": "^9.5",
5555
"rector/rector": "^0.13.6",
5656
"squizlabs/php_codesniffer": "^3.7",

‎src/Store/CookieStore.php

+121-71
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,8 @@
1414
*/
1515
final class CookieStore implements StoreInterface
1616
{
17-
public const USE_CRYPTO = true;
1817
public const KEY_HASHING_ALGO = 'sha256';
19-
public const KEY_CHUNKING_THRESHOLD = 3072;
18+
public const KEY_CHUNKING_THRESHOLD = 2048;
2019
public const KEY_SEPARATOR = '_';
2120
public const VAL_CRYPTO_ALGO = 'aes-128-gcm';
2221

@@ -48,6 +47,16 @@ final class CookieStore implements StoreInterface
4847
*/
4948
private bool $deferring = false;
5049

50+
/**
51+
* Determine if changes have been made since the last setState.
52+
*/
53+
private bool $dirty = false;
54+
55+
/**
56+
* Determine if changes have been made since the last setState.
57+
*/
58+
private bool $encrypt = true;
59+
5160
/**
5261
* CookieStore constructor.
5362
*
@@ -68,13 +77,7 @@ public function __construct(
6877

6978
$this->configuration = $configuration;
7079
$this->namespace = (string) $namespace;
71-
72-
// @phpstan-ignore-next-line
73-
if (self::USE_CRYPTO) {
74-
$this->namespace = hash(self::KEY_HASHING_ALGO, $this->namespace);
75-
}
76-
77-
$this->threshold = self::KEY_CHUNKING_THRESHOLD - strlen($this->namespace) + 2;
80+
$this->threshold = self::KEY_CHUNKING_THRESHOLD - strlen($this->namespace);
7881

7982
$this->getState();
8083
}
@@ -95,6 +98,25 @@ public function getThreshold(): int
9598
return $this->threshold;
9699
}
97100

101+
/**
102+
* Returns the current encryption state
103+
*/
104+
public function getEncrypted(): bool
105+
{
106+
return $this->encrypt;
107+
}
108+
109+
/**
110+
* Toggle the encryption state
111+
*
112+
* @param bool $encrypt Enable or disable cookie encryption.
113+
*/
114+
public function setEncrypted(bool $encrypt = true): self
115+
{
116+
$this->encrypt = $encrypt;
117+
return $this;
118+
}
119+
98120
/**
99121
* Defer saving state changes to destination to improve performance during blocks of changes.
100122
*
@@ -103,14 +125,13 @@ public function getThreshold(): int
103125
public function defer(
104126
bool $deferring
105127
): void {
128+
$this->deferring = $deferring;
129+
106130
// If we were deferring state saving and we've been asked to cancel that deference
107-
if ($this->deferring && ! $deferring) {
131+
if (! $deferring) {
108132
// Immediately push the state to the host device.
109133
$this->setState();
110134
}
111-
112-
// Update our deference state.
113-
$this->deferring = $deferring;
114135
}
115136

116137
/**
@@ -125,6 +146,10 @@ public function getState(
125146
): array {
126147
// Overwrite our internal state with one passed (presumably during unit tests.)
127148
if ($state !== null) {
149+
if ($this->store !== $state) {
150+
$this->dirty = true;
151+
}
152+
128153
return $this->store = $state;
129154
}
130155

@@ -151,7 +176,7 @@ public function getState(
151176
}
152177

153178
// If no cookies were found, set an empty state and continue.
154-
if (mb_strlen($data) === 0) {
179+
if ($data === '') {
155180
return $this->store = [];
156181
}
157182

@@ -171,9 +196,16 @@ public function getState(
171196

172197
/**
173198
* Push our storage state to the source for persistence.
199+
*
200+
* @psalm-suppress UnusedFunctionCall
174201
*/
175-
public function setState(): self
176-
{
202+
public function setState(
203+
bool $force = false
204+
): self {
205+
if (!$this->dirty && !$force) {
206+
return $this;
207+
}
208+
177209
$setOptions = $this->getCookieOptions();
178210
$deleteOptions = $this->getCookieOptions(-1000);
179211
$existing = [];
@@ -212,7 +244,7 @@ public function setState(): self
212244
// @codeCoverageIgnoreStart
213245
if (! defined('AUTH0_TESTS_DIR')) {
214246
/** @var array{expires?: int, path?: string, domain?: string, secure?: bool, httponly?: bool, samesite?: 'Lax'|'lax'|'None'|'none'|'Strict'|'strict', url_encode?: int} $setOptions */
215-
setcookie($cookieName, $chunk, $setOptions);
247+
setrawcookie($cookieName, $chunk, $setOptions);
216248
}
217249
// @codeCoverageIgnoreEnd
218250

@@ -233,14 +265,15 @@ public function setState(): self
233265
// @codeCoverageIgnoreStart
234266
if (! defined('AUTH0_TESTS_DIR')) {
235267
/** @var array{expires?: int, path?: string, domain?: string, secure?: bool, httponly?: bool, samesite?: 'Lax'|'lax'|'None'|'none'|'Strict'|'strict', url_encode?: int} $deleteOptions */
236-
setcookie($cookieName, '', $deleteOptions);
268+
setrawcookie($cookieName, '', $deleteOptions);
237269
}
238270
// @codeCoverageIgnoreEnd
239271

240272
// Clear PHP's internal COOKIE global of the orphaned cookie.
241273
unset($_COOKIE[$cookieName]);
242274
}
243275

276+
$this->dirty = false;
244277
return $this;
245278
}
246279

@@ -260,7 +293,10 @@ public function set(
260293
[$key, \Auth0\SDK\Exception\ArgumentException::missing('key')],
261294
])->isString();
262295

263-
$this->store[(string) $key] = $value;
296+
if (! isset($this->store[(string) $key]) || $this->store[(string) $key] !== $value) {
297+
$this->store[(string) $key] = $value;
298+
$this->dirty = true;
299+
}
264300

265301
if (! $this->deferring) {
266302
$this->setState();
@@ -303,7 +339,10 @@ public function delete(
303339
[$key, \Auth0\SDK\Exception\ArgumentException::missing('key')],
304340
])->isString();
305341

306-
unset($this->store[(string) $key]);
342+
if (isset($this->store[(string) $key])) {
343+
unset($this->store[(string) $key]);
344+
$this->dirty = true;
345+
}
307346

308347
if (! $this->deferring) {
309348
$this->setState();
@@ -315,7 +354,10 @@ public function delete(
315354
*/
316355
public function purge(): void
317356
{
318-
$this->store = [];
357+
if ($this->store !== []) {
358+
$this->store = [];
359+
$this->dirty = true;
360+
}
319361

320362
if (! $this->deferring) {
321363
$this->setState();
@@ -351,10 +393,9 @@ public function getCookieOptions(
351393
$options['samesite'] = 'Lax';
352394
}
353395

354-
$domain = $this->configuration->getCookieDomain() ?? $_SERVER['HTTP_HOST'] ?? null;
396+
$domain = $this->configuration->getCookieDomain() ?? null;
355397

356-
if ($domain !== null) {
357-
/** @var string $domain */
398+
if ($domain !== null && $domain !== $_SERVER['HTTP_HOST']) {
358399
$options['domain'] = $domain;
359400
}
360401

@@ -364,47 +405,70 @@ public function getCookieOptions(
364405
/**
365406
* Encrypt data for safe storage format for a cookie.
366407
*
367-
* @param array<mixed> $data Data to encrypt.
408+
* @param array<mixed> $data Data to encrypt.
409+
* @param array<mixed> $options Additional configuration options.
368410
*
369411
* @psalm-suppress TypeDoesNotContainType
370412
*/
371-
private function encrypt(
372-
array $data
413+
public function encrypt(
414+
array $data,
415+
array $options = []
373416
): string {
374-
// @codeCoverageIgnoreStart
375-
// @phpstan-ignore-next-line
376-
if (! self::USE_CRYPTO) {
377-
return base64_encode(json_encode(serialize($data), JSON_THROW_ON_ERROR));
417+
if (! $this->encrypt) {
418+
$data = $options['encoded1'] ?? json_encode($data);
419+
420+
if (! is_string($data)) {
421+
return '';
422+
}
423+
424+
return rawurlencode($data);
378425
}
379-
// @codeCoverageIgnoreEnd
380426

381427
$secret = $this->configuration->getCookieSecret();
382-
$ivLen = openssl_cipher_iv_length(self::VAL_CRYPTO_ALGO);
428+
$ivLen = $options['ivLen'] ?? openssl_cipher_iv_length(self::VAL_CRYPTO_ALGO);
429+
$tag = null;
383430

384431
if ($secret === null) {
385432
throw \Auth0\SDK\Exception\ConfigurationException::requiresCookieSecret();
386433
}
387434

388-
// @codeCoverageIgnoreStart
389-
if ($ivLen === false) {
435+
if (! is_int($ivLen)) {
436+
return '';
437+
}
438+
439+
$iv = $options['iv'] ?? openssl_random_pseudo_bytes($ivLen);
440+
441+
if (! is_string($iv)) {
442+
return '';
443+
}
444+
445+
$data = $options['encoded1'] ?? json_encode($data);
446+
447+
if (! is_string($data)) {
390448
return '';
391449
}
392-
// @codeCoverageIgnoreEnd
393450

394-
$iv = openssl_random_pseudo_bytes($ivLen);
451+
// Encrypt the PHP array.
452+
$encrypted = $options['encrypted'] ?? openssl_encrypt($data, self::VAL_CRYPTO_ALGO, $secret, 0, $iv, $tag);
453+
$iv = $options['iv'] ?? $iv;
454+
$tag = $options['tag'] ?? $tag;
395455

396-
// @codeCoverageIgnoreStart
397-
// @phpstan-ignore-next-line
398-
if ($iv === false) {
456+
if (! is_string($encrypted)) {
399457
return '';
400458
}
401-
// @codeCoverageIgnoreEnd
402459

403-
// Encrypt the serialized PHP array.
404-
$encrypted = openssl_encrypt(serialize($data), self::VAL_CRYPTO_ALGO, $secret, 0, $iv, $tag);
460+
if (! is_string($tag)) {
461+
return '';
462+
}
405463

406464
// Return a JSON encoded object containing the crypto tag and iv, and the encrypted data.
407-
return json_encode(serialize(['tag' => base64_encode($tag), 'iv' => base64_encode($iv), 'data' => $encrypted]), JSON_THROW_ON_ERROR);
465+
$encoded = $options['encoded2'] ?? json_encode(['tag' => base64_encode($tag), 'iv' => base64_encode($iv), 'data' => $encrypted]);
466+
467+
if (is_string($encoded)) {
468+
return rawurlencode($encoded);
469+
}
470+
471+
return '';
408472
}
409473

410474
/**
@@ -416,30 +480,19 @@ private function encrypt(
416480
*
417481
* @psalm-suppress TypeDoesNotContainType
418482
*/
419-
private function decrypt(
483+
public function decrypt(
420484
string $data
421485
) {
422-
// @codeCoverageIgnoreStart
423-
// @phpstan-ignore-next-line
424-
if (! self::USE_CRYPTO) {
425-
$returns = [];
426-
$decoded = base64_decode($data, true);
427-
428-
if (is_string($decoded)) {
429-
$decoded = json_decode($decoded, true, 512, JSON_THROW_ON_ERROR);
430-
}
431-
432-
if (is_string($decoded)) {
433-
$decoded = unserialize($decoded);
434-
}
486+
if (! $this->encrypt) {
487+
$decoded = rawurldecode($data);
488+
$decoded = json_decode($decoded, true);
435489

436490
if (is_array($decoded)) {
437-
$returns = $decoded;
491+
return $decoded;
438492
}
439493

440-
return $returns;
494+
return [];
441495
}
442-
// @codeCoverageIgnoreEnd
443496

444497
[$data] = Toolkit::filter([$data])->string()->trim();
445498

@@ -453,14 +506,11 @@ private function decrypt(
453506
throw \Auth0\SDK\Exception\ConfigurationException::requiresCookieSecret();
454507
}
455508

456-
$data = json_decode((string) $data, true);
509+
$decoded = rawurldecode((string) $data);
510+
$stripped = stripslashes($decoded);
511+
$data = json_decode($stripped, true, 512);
457512

458-
if (! is_string($data)) {
459-
return null;
460-
}
461-
462-
/** @var array{iv?: int|string|null, tag?: int|string|null, data: string} */
463-
$data = unserialize($data);
513+
/** @var array{iv?: int|string|null, tag?: int|string|null, data: string} $data */
464514

465515
if (! isset($data['iv']) || ! isset($data['tag']) || ! is_string($data['iv']) || ! is_string($data['tag'])) {
466516
return null;
@@ -469,17 +519,17 @@ private function decrypt(
469519
$iv = base64_decode($data['iv'], true);
470520
$tag = base64_decode($data['tag'], true);
471521

472-
if ($iv === false || $tag === false) {
522+
if (! is_string($iv) || ! is_string($tag)) {
473523
return null;
474524
}
475525

476526
$data = openssl_decrypt($data['data'], self::VAL_CRYPTO_ALGO, $secret, 0, $iv, $tag);
477527

478-
if ($data === false) {
528+
if (! is_string($data)) {
479529
return null;
480530
}
481531

482-
$data = unserialize($data);
532+
$data = json_decode($data, true);
483533

484534
/** @var array<mixed> $data */
485535
return $data;

0 commit comments

Comments
 (0)