Skip to content

Commit a3ad401

Browse files
gr8mangr8man
committed
fix(Cookie): validate cookie path and domain attributes
1 parent cc882d6 commit a3ad401

6 files changed

Lines changed: 261 additions & 7 deletions

File tree

‎system/Cookie/Cookie.php‎

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -232,11 +232,11 @@ public static function fromHeaderString(string $cookie, bool $raw = false)
232232
* @param string $name The cookie's name
233233
* @param string $value The cookie's value
234234
* @param array{
235-
* prefix?: string,
235+
* prefix?: string|null,
236236
* max-age?: int|numeric-string,
237237
* expires?: DateTimeInterface|int|string,
238-
* path?: string,
239-
* domain?: string,
238+
* path?: string|null,
239+
* domain?: string|null,
240240
* secure?: bool,
241241
* httponly?: bool,
242242
* samesite?: string,
@@ -258,9 +258,9 @@ final public function __construct(string $name, string $value = '', array $optio
258258
}
259259

260260
// to preserve backward compatibility with array-based cookies in previous CI versions
261-
$prefix = ($options['prefix'] === '') ? self::$defaults['prefix'] : $options['prefix'];
262-
$path = ($options['path'] === '') ? self::$defaults['path'] : $options['path'];
263-
$domain = ($options['domain'] === '') ? self::$defaults['domain'] : $options['domain'];
261+
$prefix = in_array($options['prefix'], [null, ''], true) ? self::$defaults['prefix'] : $options['prefix'];
262+
$path = in_array($options['path'], [null, '', '0'], true) ? self::$defaults['path'] : $options['path'];
263+
$domain = in_array($options['domain'], [null, ''], true) ? self::$defaults['domain'] : $options['domain'];
264264

265265
// empty string SameSite should use the default for browsers
266266
$samesite = ($options['samesite'] === '') ? self::$defaults['samesite'] : $options['samesite'];
@@ -270,7 +270,12 @@ final public function __construct(string $name, string $value = '', array $optio
270270
$httponly = $options['httponly'];
271271

272272
$this->validateName($name, $raw);
273+
if ($prefix !== '') {
274+
$this->validateName($prefix, $raw);
275+
}
273276
$this->validateValue($value, $raw);
277+
$this->validatePath($path);
278+
$this->validateDomain($domain);
274279
$this->validatePrefix($prefix, $secure, $path, $domain);
275280
$this->validateSameSite($samesite, $secure);
276281

@@ -449,6 +454,9 @@ public function getOptions(): array
449454
public function withPrefix(string $prefix = '')
450455
{
451456
$this->validatePrefix($prefix, $this->secure, $this->path, $this->domain);
457+
if ($prefix !== '') {
458+
$this->validateName($prefix, $this->raw);
459+
}
452460

453461
$cookie = clone $this;
454462

@@ -515,6 +523,7 @@ public function withExpired()
515523
public function withPath(?string $path)
516524
{
517525
$path = in_array($path, [null, '', '0'], true) ? self::$defaults['path'] : $path;
526+
$this->validatePath($path);
518527
$this->validatePrefix($this->prefix, $this->secure, $path, $this->domain);
519528

520529
$cookie = clone $this;
@@ -530,6 +539,7 @@ public function withPath(?string $path)
530539
public function withDomain(?string $domain)
531540
{
532541
$domain ??= self::$defaults['domain'];
542+
$this->validateDomain($domain);
533543
$this->validatePrefix($this->prefix, $this->secure, $this->path, $domain);
534544

535545
$cookie = clone $this;
@@ -586,6 +596,9 @@ public function withSameSite(string $samesite)
586596
public function withRaw(bool $raw = true)
587597
{
588598
$this->validateName($this->name, $raw);
599+
if ($this->prefix !== '') {
600+
$this->validateName($this->prefix, $raw);
601+
}
589602
$this->validateValue($this->value, $raw);
590603

591604
$cookie = clone $this;
@@ -790,6 +803,30 @@ protected function validateValue(string $value, bool $raw): void
790803
}
791804
}
792805

806+
/**
807+
* Validates the cookie path per RFC 6265 and PHP setcookie() constraints.
808+
*
809+
* @throws CookieException
810+
*/
811+
protected function validatePath(string $path): void
812+
{
813+
if (strpbrk($path, self::$reservedValueCharsList) !== false) {
814+
throw CookieException::forInvalidCookiePath();
815+
}
816+
}
817+
818+
/**
819+
* Validates the cookie domain per RFC 6265 and PHP setcookie() constraints.
820+
*
821+
* @throws CookieException
822+
*/
823+
protected function validateDomain(string $domain): void
824+
{
825+
if ($domain !== '' && strpbrk($domain, self::$reservedValueCharsList) !== false) {
826+
throw CookieException::forInvalidCookieDomain();
827+
}
828+
}
829+
793830
/**
794831
* Validates the special prefixes if some attribute requirements are met.
795832
*

‎system/Cookie/Exceptions/CookieException.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,26 @@ public static function forInvalidCookieValue()
7070
return new static(lang('Cookie.invalidCookieValue'));
7171
}
7272

73+
/**
74+
* Thrown when the cookie path contains invalid characters.
75+
*
76+
* @return static
77+
*/
78+
public static function forInvalidCookiePath()
79+
{
80+
return new static(lang('Cookie.invalidCookiePath'));
81+
}
82+
83+
/**
84+
* Thrown when the cookie domain contains invalid characters.
85+
*
86+
* @return static
87+
*/
88+
public static function forInvalidCookieDomain()
89+
{
90+
return new static(lang('Cookie.invalidCookieDomain'));
91+
}
92+
7393
/**
7494
* Thrown when using the `__Secure-` prefix but the `Secure` attribute
7595
* is not set to true.

‎system/Language/en/Cookie.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
'invalidExpiresValue' => 'The cookie expiration time is not valid.',
1818
'invalidCookieName' => 'The cookie name "{0}" contains invalid characters.',
1919
'invalidCookieValue' => 'The cookie value contains invalid characters.',
20+
'invalidCookiePath' => 'The cookie path contains invalid characters.',
21+
'invalidCookieDomain' => 'The cookie domain contains invalid characters.',
2022
'emptyCookieName' => 'The cookie name cannot be empty.',
2123
'invalidSecurePrefix' => 'Using the "__Secure-" prefix requires setting the "Secure" attribute.',
2224
'invalidHostPrefix' => 'Using the "__Host-" prefix must be set with the "Secure" flag, must not have a "Domain" attribute, and the "Path" is set to "/".',

‎tests/system/Cookie/CookieTest.php‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -443,4 +443,183 @@ public function testNonRawCookieSafelyEncodesCRLF(): void
443443
$this->assertStringNotContainsString("\r", $result);
444444
$this->assertStringNotContainsString("\n", $result);
445445
}
446+
447+
#[DataProvider('provideValidationOfCookiePath')]
448+
public function testValidationOfCookiePath(string $path): void
449+
{
450+
$this->expectException(CookieException::class);
451+
$this->expectExceptionMessage(lang('Cookie.invalidCookiePath'));
452+
new Cookie('test', 'value', ['path' => $path]);
453+
}
454+
455+
#[DataProvider('provideValidationOfCookiePath')]
456+
public function testValidationOfCookiePathInWithPath(string $path): void
457+
{
458+
$this->expectException(CookieException::class);
459+
$this->expectExceptionMessage(lang('Cookie.invalidCookiePath'));
460+
$cookie = new Cookie('test', 'value');
461+
$cookie->withPath($path);
462+
}
463+
464+
/**
465+
* @return iterable<string, array{string}>
466+
*/
467+
public static function provideValidationOfCookiePath(): iterable
468+
{
469+
yield 'comma' => ['/path,comma'];
470+
471+
yield 'semicolon' => ['/path;semicolon'];
472+
473+
yield 'space' => ['/path with space'];
474+
475+
yield 'tab' => ["/path\twith_tab"];
476+
477+
yield 'carriage return' => ["/path\rcarriage"];
478+
479+
yield 'newline' => ["/path\nnewline"];
480+
481+
yield 'vertical tab' => ["/path\vvertical_tab"];
482+
483+
yield 'form feed' => ["/path\fform_feed"];
484+
485+
yield 'null byte' => ["/path\0null_byte"];
486+
487+
yield 'CRLF' => ["/path\r\nwith_crlf"];
488+
}
489+
490+
#[DataProvider('provideFromHeaderStringValidationOfCookiePath')]
491+
public function testFromHeaderStringValidationOfCookiePath(string $path): void
492+
{
493+
$this->expectException(CookieException::class);
494+
$this->expectExceptionMessage(lang('Cookie.invalidCookiePath'));
495+
Cookie::fromHeaderString("test=value; Path={$path}");
496+
}
497+
498+
/**
499+
* @return iterable<string, array{string}>
500+
*/
501+
public static function provideFromHeaderStringValidationOfCookiePath(): iterable
502+
{
503+
foreach (self::provideValidationOfCookiePath() as $name => $case) {
504+
if ($name === 'semicolon') {
505+
continue;
506+
}
507+
508+
yield $name => $case;
509+
}
510+
}
511+
512+
#[DataProvider('provideValidationOfCookieDomain')]
513+
public function testValidationOfCookieDomain(string $domain): void
514+
{
515+
$this->expectException(CookieException::class);
516+
$this->expectExceptionMessage(lang('Cookie.invalidCookieDomain'));
517+
new Cookie('test', 'value', ['domain' => $domain]);
518+
}
519+
520+
#[DataProvider('provideValidationOfCookieDomain')]
521+
public function testValidationOfCookieDomainInWithDomain(string $domain): void
522+
{
523+
$this->expectException(CookieException::class);
524+
$this->expectExceptionMessage(lang('Cookie.invalidCookieDomain'));
525+
$cookie = new Cookie('test', 'value');
526+
$cookie->withDomain($domain);
527+
}
528+
529+
/**
530+
* @return iterable<string, array{string}>
531+
*/
532+
public static function provideValidationOfCookieDomain(): iterable
533+
{
534+
yield 'comma' => ['domain,comma.com'];
535+
536+
yield 'semicolon' => ['domain;semicolon.com'];
537+
538+
yield 'space' => ['domain with space.com'];
539+
540+
yield 'tab' => ["domain\twith_tab.com"];
541+
542+
yield 'carriage return' => ["domain\rcarriage.com"];
543+
544+
yield 'newline' => ["domain\nnewline.com"];
545+
546+
yield 'vertical tab' => ["domain\vvertical_tab.com"];
547+
548+
yield 'form feed' => ["domain\fform_feed.com"];
549+
550+
yield 'null byte' => ["domain\0null_byte.com"];
551+
552+
yield 'CRLF' => ["domain\r\nwith_crlf.com"];
553+
}
554+
555+
#[DataProvider('provideFromHeaderStringValidationOfCookieDomain')]
556+
public function testFromHeaderStringValidationOfCookieDomain(string $domain): void
557+
{
558+
$this->expectException(CookieException::class);
559+
$this->expectExceptionMessage(lang('Cookie.invalidCookieDomain'));
560+
Cookie::fromHeaderString("test=value; Domain={$domain}");
561+
}
562+
563+
/**
564+
* @return iterable<string, array{string}>
565+
*/
566+
public static function provideFromHeaderStringValidationOfCookieDomain(): iterable
567+
{
568+
foreach (self::provideValidationOfCookieDomain() as $name => $case) {
569+
if ($name === 'semicolon') {
570+
continue;
571+
}
572+
573+
yield $name => $case;
574+
}
575+
}
576+
577+
public function testNullPathAndDomainDefaultProperly(): void
578+
{
579+
$cookie = new Cookie('test', 'val', ['path' => null, 'domain' => null, 'prefix' => null]);
580+
581+
$this->assertSame('/', $cookie->getPath());
582+
$this->assertSame('', $cookie->getDomain());
583+
$this->assertSame('', $cookie->getPrefix());
584+
585+
$cookie2 = $cookie->withPath(null)->withDomain(null)->withPrefix('');
586+
$this->assertSame('/', $cookie2->getPath());
587+
$this->assertSame('', $cookie2->getDomain());
588+
$this->assertSame('', $cookie2->getPrefix());
589+
}
590+
591+
public function testValidCookiePathAndDomain(): void
592+
{
593+
$cookie = new Cookie('test', 'val', ['path' => '/sub/dir/', 'domain' => 'example.com']);
594+
$this->assertSame('/sub/dir/', $cookie->getPath());
595+
$this->assertSame('example.com', $cookie->getDomain());
596+
597+
$cookie2 = $cookie->withPath('/another/path')->withDomain('.example.com');
598+
$this->assertSame('/another/path', $cookie2->getPath());
599+
$this->assertSame('.example.com', $cookie2->getDomain());
600+
601+
$cookie3 = new Cookie('test', 'val', ['path' => '/', 'domain' => '']);
602+
$this->assertSame('/', $cookie3->getPath());
603+
$this->assertSame('', $cookie3->getDomain());
604+
}
605+
606+
public function testValidationOfRawCookiePrefix(): void
607+
{
608+
$this->expectException(CookieException::class);
609+
new Cookie('test', 'val', ['prefix' => "bad\r\n", 'raw' => true]);
610+
}
611+
612+
public function testValidationOfRawCookiePrefixInWithPrefix(): void
613+
{
614+
$this->expectException(CookieException::class);
615+
$cookie = new Cookie('test', 'val', ['raw' => true]);
616+
$cookie->withPrefix("bad\r\n");
617+
}
618+
619+
public function testValidationOfRawCookiePrefixInWithRaw(): void
620+
{
621+
$this->expectException(CookieException::class);
622+
$cookie = new Cookie('test', 'val', ['prefix' => "bad\r\n", 'raw' => false]);
623+
$cookie->withRaw(true);
624+
}
446625
}

‎user_guide_src/source/changelogs/v4.7.5.rst‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ Message Changes
1919
***************
2020

2121
- Added the ``Cookie.invalidCookieValue`` language string.
22+
- Added the ``Cookie.invalidCookiePath`` language string.
23+
- Added the ``Cookie.invalidCookieDomain`` language string.
2224

2325
*******
2426
Changes
@@ -41,7 +43,7 @@ Bugs Fixed
4143
- **CLIRequest:** Fixed a bug where ``parseCommand()`` could throw a TypeError when ``argv`` is missing.
4244
- **CodeIgniter:** Fixed a bug where ``gatherOutput()`` could be called twice when ``startController()`` returned a ``ResponseInterface`` (e.g., from filter attributes or closure routes).
4345
- **Content Security Policy:** Fixed a bug where empty ``Content-Security-Policy``, ``Content-Security-Policy-Report-Only``, and ``Reporting-Endpoints`` response headers were generated when no corresponding values existed.
44-
- **Cookie:** Fixed a bug where ``Cookie`` instances created with ``raw: true`` allowed invalid characters in cookie values rejected by ``setrawcookie()``.
46+
- **Cookie:** Fixed a bug where ``Cookie`` instances allowed invalid characters in cookie values (with ``raw: true``), path, and domain attributes rejected by ``setrawcookie()`` and RFC 6265.
4547
- **Database:** Fixed a bug where rebuilding a SQLite3 table (e.g., ``Forge::dropColumn()``, ``Forge::modifyColumn()``, ``Forge::dropForeignKey()`` and ``Forge::dropPrimaryKey()``) corrupted the table names referenced by its foreign keys when ``DBPrefix`` was set.
4648
- **Files:** Fixed a bug where ``File::move()`` and ``UploadedFile::move()`` set executable and overly permissive file permissions (``0777 & ~umask()`` instead of ``0666 & ~umask()``), and ``UploadedFile::move()`` targeted the parent directory instead of the destination file for ``chmod()``.
4749
- **Helpers:** Fixed a bug where ``get_dir_file_info()`` returned incomplete entries for subdirectories and missing files instead of omitting them.

‎user_guide_src/source/libraries/cookies.rst‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,20 @@ If setting the ``$raw`` parameter to ``true``, the cookie value will also be val
104104
It must not contain control characters, spaces, tabs, or separator characters
105105
(``, ;``) as `setrawcookie() <https://www.php.net/manual/en/function.setrawcookie.php>`_ will reject them.
106106

107+
Validating the Path Attribute
108+
=============================
109+
110+
The cookie path must not contain control characters, spaces, tabs, or separator characters
111+
(``, ;``) as `setcookie() <https://www.php.net/manual/en/function.setcookie.php>`_ and
112+
`setrawcookie() <https://www.php.net/manual/en/function.setrawcookie.php>`_ will reject them.
113+
114+
Validating the Domain Attribute
115+
===============================
116+
117+
The cookie domain must not contain control characters, spaces, tabs, or separator characters
118+
(``, ;``) as `setcookie() <https://www.php.net/manual/en/function.setcookie.php>`_ and
119+
`setrawcookie() <https://www.php.net/manual/en/function.setrawcookie.php>`_ will reject them.
120+
107121
Validating the Prefix Attribute
108122
===============================
109123

0 commit comments

Comments
 (0)