fix(Cookie): validate cookie path and domain attributes - #10527
Conversation
b5e5f79 to
a3ad401
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Non-raw prefixes still bypass validation and can cause setcookie() to throw a ValueError.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds cookie attribute validation to prevent header injection and native PHP ValueErrors. Review assumes the develop base branch.
Changes:
- Validates cookie paths, domains, and prefixes.
- Defaults nullable cookie options safely.
- Adds tests, messages, and documentation.
File summaries
| File | Description |
|---|---|
system/Cookie/Cookie.php |
Implements normalization and validation. |
system/Cookie/Exceptions/CookieException.php |
Adds validation exceptions. |
system/Language/en/Cookie.php |
Adds error messages. |
tests/system/Cookie/CookieTest.php |
Adds regression coverage. |
user_guide_src/source/libraries/cookies.rst |
Documents attribute validation. |
user_guide_src/source/changelogs/v4.7.5.rst |
Records the fix. |
Review details
Suppressed comments (1)
system/Cookie/Cookie.php:458
withPrefix()has the same non-raw bypass: an invalid prefix is stored unchanged and later causes nativesetcookie()to throw because prefixes are not URL-encoded bygetPrefixedName(). Prefix character validation must run regardless of the cookie's raw mode.
$this->validateName($prefix, $this->raw);
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
NUL-containing prefixes still bypass validation and may reach PHP’s native cookie functions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| $this->validateName($name, $raw); | ||
| if ($prefix !== '') { | ||
| $this->validateName($prefix, true); |
| } | ||
|
|
||
| /** | ||
| * Validates the cookie path per RFC 6265 and PHP setcookie() constraints. |
There was a problem hiding this comment.
These checks do not fully implement RFC 6265 validation. For this PHP-compatible fix, please describe them as reserved-character checks and adjust validateDomain() PHPDoc similarly.
| Validating the Path Attribute | ||
| ============================= | ||
|
|
||
| The cookie path must not contain control characters, spaces, tabs, or separator characters | ||
| (``, ;``) as `setcookie() <https://www.php.net/manual/en/function.setcookie.php>`_ and | ||
| `setrawcookie() <https://www.php.net/manual/en/function.setrawcookie.php>`_ will reject them. |
There was a problem hiding this comment.
"Control characters" implies all control bytes, but this check allows \x01 and \x7F. Please list the exact rejected characters here and in the domain section to reflect this PR PHP-compatible validation scope.
| The cookie prefix must not contain control characters, spaces, tabs, or separator characters | ||
| (``= , ; \t \r \n \v \f ( ) < > @ : \" / [ ] ? { }``) as `setcookie()` and | ||
| `setrawcookie() <https://www.php.net/manual/en/function.setrawcookie.php>`_ will reject them. | ||
|
|
There was a problem hiding this comment.
PHP accepts some listed separators, including : and / (https://github.com/php/php-src/blob/PHP-8.2/ext/standard/head.c#L93). Rejecting them in non-raw prefixes introduces a behavior change beyond the PHP error fix. For this develop PR, please preserve that existing behavior while rejecting PHP-prohibited characters and NUL regardless of raw mode, and update this explanation accordingly.
| - **CodeIgniter:** Fixed a bug where ``gatherOutput()`` could be called twice when ``startController()`` returned a ``ResponseInterface`` (e.g., from filter attributes or closure routes). | ||
| - **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. | ||
| - **Cookie:** Fixed a bug where ``Cookie`` instances created with ``raw: true`` allowed invalid characters in cookie values rejected by ``setrawcookie()``. | ||
| - **Cookie:** Fixed a bug where ``Cookie`` instances allowed invalid characters in cookie values (with ``raw: true``), path, domain, and prefix attributes rejected by ``setcookie()``, ``setrawcookie()``, and RFC 6265. |
There was a problem hiding this comment.
Please remove the RFC 6265 claim: PHP and the RFC have different restrictions, and these checks do not provide full RFC validation. Please keep the existing raw-value fix separate and describe the new path/domain/prefix checks.
- Prohibit only PHP-rejected characters and NUL in cookie prefixes to preserve compatibility with separators like ':' and '/' - Update cookie prefix validation documentation in User Guide - Separate Cookie raw value fix and path/domain/prefix validation entries in changelog and remove RFC 6265 reference - Add test coverage for PHP-prohibited prefix characters and allowed separators
6ed2d2f to
ba58d55
Compare
Description
Validates cookie
path,domain, andprefixattributes against reserved characters (",; \t\r\n\v\f\0") to prevent CRLF injection in raw cookies and avoid fatalValueErrors from PHP's nativesetcookie()andsetrawcookie(). Also ensuresnullattribute options safely fall back to defaults in the constructor.Checklist: