Skip to content

Commit 7222315

Browse files
GirgiasGirgias
committed
Promote warnings to errors for set(raw)cookie()
Closes GH-5819
1 parent 1618997 commit 7222315

File tree

7 files changed

+248
-140
lines changed

7 files changed

+248
-140
lines changed

‎ext/standard/head.c

Lines changed: 61 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -76,36 +76,41 @@ PHPAPI int php_header(void)
7676
}
7777
}
7878

79-
PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires, zend_string *path, zend_string *domain, int secure, int httponly, zend_string *samesite, int url_encode)
79+
#define ILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", or \"\\014\""
80+
PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t expires,
81+
zend_string *path, zend_string *domain, bool secure, bool httponly,
82+
zend_string *samesite, bool url_encode)
8083
{
8184
zend_string *dt;
8285
sapi_header_line ctr = {0};
83-
int result;
86+
zend_result result;
8487
smart_str buf = {0};
8588

8689
if (!ZSTR_LEN(name)) {
87-
zend_error( E_WARNING, "Cookie names must not be empty" );
90+
zend_argument_value_error(1, "cannot be empty");
8891
return FAILURE;
89-
} else if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */
90-
zend_error(E_WARNING, "Cookie names cannot contain any of the following '=,; \\t\\r\\n\\013\\014'" );
92+
}
93+
if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */
94+
zend_argument_value_error(1, "cannot contain \"=\", " ILLEGAL_COOKIE_CHARACTER);
9195
return FAILURE;
9296
}
93-
9497
if (!url_encode && value &&
9598
strpbrk(ZSTR_VAL(value), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */
96-
zend_error(E_WARNING, "Cookie values cannot contain any of the following ',; \\t\\r\\n\\013\\014'" );
99+
zend_argument_value_error(2, "cannot contain " ILLEGAL_COOKIE_CHARACTER);
97100
return FAILURE;
98101
}
99102

100103
if (path && strpbrk(ZSTR_VAL(path), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */
101-
zend_error(E_WARNING, "Cookie paths cannot contain any of the following ',; \\t\\r\\n\\013\\014'" );
104+
zend_value_error("%s(): \"path\" option cannot contain " ILLEGAL_COOKIE_CHARACTER,
105+
get_active_function_name());
102106
return FAILURE;
103107
}
104-
105108
if (domain && strpbrk(ZSTR_VAL(domain), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */
106-
zend_error(E_WARNING, "Cookie domains cannot contain any of the following ',; \\t\\r\\n\\013\\014'" );
109+
zend_value_error("%s(): \"domain\" option cannot contain " ILLEGAL_COOKIE_CHARACTER,
110+
get_active_function_name());
107111
return FAILURE;
108112
}
113+
/* Should check value of SameSite? */
109114

110115
if (value == NULL || ZSTR_LEN(value) == 0) {
111116
/*
@@ -142,7 +147,8 @@ PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires,
142147
if (!p || *(p + 5) != ' ') {
143148
zend_string_free(dt);
144149
smart_str_free(&buf);
145-
zend_error(E_WARNING, "Expiry date cannot have a year greater than 9999");
150+
zend_value_error("%s(): \"expires\" option cannot have a year greater than 9999",
151+
get_active_function_name());
146152
return FAILURE;
147153
}
148154

@@ -186,49 +192,40 @@ PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires,
186192
return result;
187193
}
188194

189-
static void php_head_parse_cookie_options_array(zval *options, zend_long *expires, zend_string **path, zend_string **domain, zend_bool *secure, zend_bool *httponly, zend_string **samesite) {
190-
int found = 0;
195+
static zend_result php_head_parse_cookie_options_array(zval *options, zend_long *expires, zend_string **path,
196+
zend_string **domain, zend_bool *secure, zend_bool *httponly, zend_string **samesite)
197+
{
191198
zend_string *key;
192199
zval *value;
193200

194201
ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(options), key, value) {
195-
if (key) {
196-
if (zend_string_equals_literal_ci(key, "expires")) {
197-
*expires = zval_get_long(value);
198-
found++;
199-
} else if (zend_string_equals_literal_ci(key, "path")) {
200-
*path = zval_get_string(value);
201-
found++;
202-
} else if (zend_string_equals_literal_ci(key, "domain")) {
203-
*domain = zval_get_string(value);
204-
found++;
205-
} else if (zend_string_equals_literal_ci(key, "secure")) {
206-
*secure = zval_is_true(value);
207-
found++;
208-
} else if (zend_string_equals_literal_ci(key, "httponly")) {
209-
*httponly = zval_is_true(value);
210-
found++;
211-
} else if (zend_string_equals_literal_ci(key, "samesite")) {
212-
*samesite = zval_get_string(value);
213-
found++;
214-
} else {
215-
php_error_docref(NULL, E_WARNING, "Unrecognized key '%s' found in the options array", ZSTR_VAL(key));
216-
}
202+
if (!key) {
203+
zend_value_error("%s(): option array cannot have numeric keys", get_active_function_name());
204+
return FAILURE;
205+
}
206+
if (zend_string_equals_literal_ci(key, "expires")) {
207+
*expires = zval_get_long(value);
208+
} else if (zend_string_equals_literal_ci(key, "path")) {
209+
*path = zval_get_string(value);
210+
} else if (zend_string_equals_literal_ci(key, "domain")) {
211+
*domain = zval_get_string(value);
212+
} else if (zend_string_equals_literal_ci(key, "secure")) {
213+
*secure = zval_is_true(value);
214+
} else if (zend_string_equals_literal_ci(key, "httponly")) {
215+
*httponly = zval_is_true(value);
216+
} else if (zend_string_equals_literal_ci(key, "samesite")) {
217+
*samesite = zval_get_string(value);
217218
} else {
218-
php_error_docref(NULL, E_WARNING, "Numeric key found in the options array");
219+
zend_value_error("%s(): option \"%s\" is invalid", get_active_function_name(), ZSTR_VAL(key));
220+
return FAILURE;
219221
}
220222
} ZEND_HASH_FOREACH_END();
221-
222-
/* Array is not empty but no valid keys were found */
223-
if (found == 0 && zend_hash_num_elements(Z_ARRVAL_P(options)) > 0) {
224-
php_error_docref(NULL, E_WARNING, "No valid options were found in the given array");
225-
}
223+
return SUCCESS;
226224
}
227225

228-
/* {{{ setcookie(string name [, string value [, array options]])
229-
Send a cookie */
230-
PHP_FUNCTION(setcookie)
226+
static void php_setcookie_common(INTERNAL_FUNCTION_PARAMETERS, bool is_raw)
231227
{
228+
/* to handle overloaded function array|int */
232229
zval *expires_or_options = NULL;
233230
zend_string *name, *value = NULL, *path = NULL, *domain = NULL, *samesite = NULL;
234231
zend_long expires = 0;
@@ -248,24 +245,27 @@ PHP_FUNCTION(setcookie)
248245
if (expires_or_options) {
249246
if (Z_TYPE_P(expires_or_options) == IS_ARRAY) {
250247
if (UNEXPECTED(ZEND_NUM_ARGS() > 3)) {
251-
php_error_docref(NULL, E_WARNING, "Cannot pass arguments after the options array");
252-
RETURN_FALSE;
248+
zend_argument_count_error("%s(): Expects exactly 3 arguments when argument #3 "
249+
"($expires_or_options) is an array", get_active_function_name());
250+
RETURN_THROWS();
251+
}
252+
if (FAILURE == php_head_parse_cookie_options_array(expires_or_options, &expires, &path,
253+
&domain, &secure, &httponly, &samesite)) {
254+
goto cleanup;
253255
}
254-
php_head_parse_cookie_options_array(expires_or_options, &expires, &path, &domain, &secure, &httponly, &samesite);
255256
} else {
256257
expires = zval_get_long(expires_or_options);
257258
}
258259
}
259260

260-
if (!EG(exception)) {
261-
if (php_setcookie(name, value, expires, path, domain, secure, httponly, samesite, 1) == SUCCESS) {
262-
RETVAL_TRUE;
263-
} else {
264-
RETVAL_FALSE;
265-
}
261+
if (php_setcookie(name, value, expires, path, domain, secure, httponly, samesite, !is_raw) == SUCCESS) {
262+
RETVAL_TRUE;
263+
} else {
264+
RETVAL_FALSE;
266265
}
267266

268267
if (expires_or_options && Z_TYPE_P(expires_or_options) == IS_ARRAY) {
268+
cleanup:
269269
if (path) {
270270
zend_string_release(path);
271271
}
@@ -277,59 +277,20 @@ PHP_FUNCTION(setcookie)
277277
}
278278
}
279279
}
280+
281+
/* {{{ setcookie(string name [, string value [, array options]])
282+
Send a cookie */
283+
PHP_FUNCTION(setcookie)
284+
{
285+
php_setcookie_common(INTERNAL_FUNCTION_PARAM_PASSTHRU, false);
286+
}
280287
/* }}} */
281288

282289
/* {{{ setrawcookie(string name [, string value [, array options]])
283290
Send a cookie with no url encoding of the value */
284291
PHP_FUNCTION(setrawcookie)
285292
{
286-
zval *expires_or_options = NULL;
287-
zend_string *name, *value = NULL, *path = NULL, *domain = NULL, *samesite = NULL;
288-
zend_long expires = 0;
289-
zend_bool secure = 0, httponly = 0;
290-
291-
ZEND_PARSE_PARAMETERS_START(1, 7)
292-
Z_PARAM_STR(name)
293-
Z_PARAM_OPTIONAL
294-
Z_PARAM_STR(value)
295-
Z_PARAM_ZVAL(expires_or_options)
296-
Z_PARAM_STR(path)
297-
Z_PARAM_STR(domain)
298-
Z_PARAM_BOOL(secure)
299-
Z_PARAM_BOOL(httponly)
300-
ZEND_PARSE_PARAMETERS_END();
301-
302-
if (expires_or_options) {
303-
if (Z_TYPE_P(expires_or_options) == IS_ARRAY) {
304-
if (UNEXPECTED(ZEND_NUM_ARGS() > 3)) {
305-
php_error_docref(NULL, E_WARNING, "Cannot pass arguments after the options array");
306-
RETURN_FALSE;
307-
}
308-
php_head_parse_cookie_options_array(expires_or_options, &expires, &path, &domain, &secure, &httponly, &samesite);
309-
} else {
310-
expires = zval_get_long(expires_or_options);
311-
}
312-
}
313-
314-
if (!EG(exception)) {
315-
if (php_setcookie(name, value, expires, path, domain, secure, httponly, samesite, 0) == SUCCESS) {
316-
RETVAL_TRUE;
317-
} else {
318-
RETVAL_FALSE;
319-
}
320-
}
321-
322-
if (expires_or_options && Z_TYPE_P(expires_or_options) == IS_ARRAY) {
323-
if (path) {
324-
zend_string_release(path);
325-
}
326-
if (domain) {
327-
zend_string_release(domain);
328-
}
329-
if (samesite) {
330-
zend_string_release(samesite);
331-
}
332-
}
293+
php_setcookie_common(INTERNAL_FUNCTION_PARAM_PASSTHRU, true);
333294
}
334295
/* }}} */
335296

‎ext/standard/head.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828
extern PHP_RINIT_FUNCTION(head);
2929

3030
PHPAPI int php_header(void);
31-
PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires, zend_string *path, zend_string *domain, int secure, int httponly, zend_string *samesite, int url_encode);
31+
PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t expires,
32+
zend_string *path, zend_string *domain, bool secure, bool httponly,
33+
zend_string *samesite, bool url_encode);
3234

3335
#endif

‎ext/standard/tests/network/bug69523.phpt

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@
22
setcookie() allows empty cookie name
33
--FILE--
44
<?php
5-
setcookie('', 'foo');
5+
try {
6+
setcookie('', 'foo');
7+
} catch (\ValueError $e) {
8+
echo $e->getMessage() . \PHP_EOL;
9+
}
610
?>
7-
--EXPECTF--
8-
Warning: Cookie names must not be empty in %s on line %d
11+
--EXPECT--
12+
setcookie(): Argument #1 ($name) cannot be empty

‎ext/standard/tests/network/bug69948.phpt

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,21 @@
22
Bug #69948 (path/domain are not sanitized for special characters in setcookie)
33
--FILE--
44
<?php
5-
var_dump(
6-
setcookie('foo', 'bar', 0, 'asdf;asdf'),
7-
setcookie('foo', 'bar', 0, '/', 'foobar; secure')
8-
);
5+
try {
6+
var_dump(setcookie('foo', 'bar', 0, 'asdf;asdf'));
7+
} catch (\ValueError $e) {
8+
echo $e->getMessage() . \PHP_EOL;
9+
}
10+
try {
11+
var_dump(setcookie('foo', 'bar', 0, '/', 'foobar; secure'));
12+
} catch (\ValueError $e) {
13+
echo $e->getMessage() . \PHP_EOL;
14+
}
15+
916
?>
1017
===DONE===
1118
--EXPECTHEADERS--
12-
--EXPECTF--
13-
Warning: Cookie paths cannot contain any of the following ',; \t\r\n\013\014' in %s on line %d
14-
15-
Warning: Cookie domains cannot contain any of the following ',; \t\r\n\013\014' in %s on line %d
16-
bool(false)
17-
bool(false)
19+
--EXPECT--
20+
setcookie(): "path" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", or "\014"
21+
setcookie(): "domain" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", or "\014"
1822
===DONE===

‎ext/standard/tests/network/setcookie_array_option_error.phpt

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
--TEST--
2+
setcookie() array variant error tests
3+
--INI--
4+
date.timezone=UTC
5+
--FILE--
6+
<?php
7+
8+
ob_start();
9+
10+
// Unrecognized key and no valid keys
11+
try {
12+
setcookie('name', 'value', ['unknown_key' => 'only']);
13+
} catch (\ValueError $e) {
14+
echo $e->getMessage() . "\n";
15+
}
16+
// Numeric key and no valid keys
17+
try {
18+
setcookie('name2', 'value2', [0 => 'numeric_key']);
19+
} catch (\ValueError $e) {
20+
echo $e->getMessage() . "\n";
21+
}
22+
// Unrecognized key
23+
try {
24+
setcookie('name3', 'value3', ['path' => '/path/', 'foo' => 'bar']);
25+
} catch (\ValueError $e) {
26+
echo $e->getMessage() . "\n";
27+
}
28+
// Invalid expiration date
29+
// To go above year 9999: 60 * 60 * 24 * 365 * 9999
30+
try {
31+
setcookie('name', 'value', ['expires' => 315328464000]);
32+
} catch (\ValueError $e) {
33+
echo $e->getMessage() . "\n";
34+
}
35+
// Invalid path key content
36+
try {
37+
setcookie('name', 'value', ['path' => '/;/']);
38+
} catch (\ValueError $e) {
39+
echo $e->getMessage() . "\n";
40+
}
41+
// Invalid domain key content
42+
try {
43+
setcookie('name', 'value', ['path' => '/path/', 'domain' => 'ba;r']);
44+
} catch (\ValueError $e) {
45+
echo $e->getMessage() . "\n";
46+
}
47+
48+
// Arguments after options array (will not be set)
49+
try {
50+
setcookie('name4', 'value4', [], "path", "domain.tld", true, true);
51+
} catch (\ArgumentCountError $e) {
52+
echo $e->getMessage() . "\n";
53+
}
54+
55+
var_dump(headers_list());
56+
--EXPECTHEADERS--
57+
58+
--EXPECTF--
59+
setcookie(): option "unknown_key" is invalid
60+
setcookie(): option array cannot have numeric keys
61+
setcookie(): option "foo" is invalid
62+
setcookie(): "expires" option cannot have a year greater than 9999
63+
setcookie(): "path" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", or "\014"
64+
setcookie(): "domain" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", or "\014"
65+
setcookie(): Expects exactly 3 arguments when argument #3 ($expires_or_options) is an array
66+
array(1) {
67+
[0]=>
68+
string(%s) "X-Powered-By: PHP/%s"
69+
}

0 commit comments

Comments
 (0)