Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: set_cookie not using default value of $secure from config #6540

Closed
hamzawain7 opened this issue Sep 14, 2022 · 6 comments · Fixed by #6544
Closed

Bug: set_cookie not using default value of $secure from config #6540

hamzawain7 opened this issue Sep 14, 2022 · 6 comments · Fixed by #6544
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@hamzawain7
Copy link

hamzawain7 commented Sep 14, 2022

PHP Version

7.4

CodeIgniter4 Version

4.2.6

CodeIgniter4 Installation Method

Manual (zip or tar.gz)

Which operating systems have you tested for this bug?

macOS

Which server did you use?

apache

Database

No response

What happened?

set_cookie function in Helpers/cookie_helper.php is not using the default value $secure i specified in app/Config/Cookie.php.

This used to work in CI3 because I think the default value of $secure param of set_cookie function used to be null which later on fallback to default.****

Steps to Reproduce

  1. Set public $secure = true in /app/Config/Cookie.php'
  2. Call the helper function set_cookie
         helper('cookie');
         set_cookie([
             'name' => 'pmw',
             'value' => 2,
             'samesite' => 'None',
         ]);
  3. CookieException::forInvalidSameSiteNone() is thrown although $secure was suppose to be true

Expected Output

No exception

Anything else?

No response

@hamzawain7 hamzawain7 added the bug Verified issues on the current code behavior or pull requests that will fix them label Sep 14, 2022
@ddevsr
Copy link
Collaborator

ddevsr commented Sep 15, 2022

You must set variable secure to true, by default function set_cookie secure = false

set_cookie([
         'name' => 'pmw',
         'value' => 2,
         'secure' => true,
         'samesite' => 'None',
]);

If you want create cookie with default config (see https://codeigniter.com/user_guide/libraries/cookies.html#creating-cookies) or you can using set_cookie helper

$cookieConfig = config('cookie');

 set_cookie([
           'name'     => 'pmw',
           'value'    => 2,
           'expire'   => $cookieConfig->expires,
           'domain'   => $cookieConfig->domain,
           'path'     => $cookieConfig->path,
           'prefix'   => $cookieConfig->prefix,
           'secure'   => $cookieConfig->secure,
           'httponly' => $cookieConfig->httponly,
           'samesite' => $cookieConfig->samesite,
]);

@hamzawain7
Copy link
Author

set_cookie function uses new Cookie in its implementation which makes it get the default value of "domain", "samesite" and other non bool config values, but doesn't for "secure". Isn't that inconsistency? Won't it better for all of them to fallback to values in config?

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them and removed bug Verified issues on the current code behavior or pull requests that will fix them labels Sep 15, 2022
@kenjis
Copy link
Member

kenjis commented Sep 15, 2022

When constructing the Cookie object, only the name attribute is required. All other else are optional. If the optional attributes are not modified, their values will be filled up by the default values saved in the Cookie class. To override the defaults currently stored in the class, you can pass a Config\Cookie instance or an array of defaults to the static Cookie::setDefaults() method.
https://codeigniter4.github.io/CodeIgniter4/libraries/cookies.html#creating-cookies

Reading the user guide, I think the values in the Config file should be used for unspecified items.

@kenjis
Copy link
Member

kenjis commented Sep 15, 2022

This used to work in CI3 because I think the default value of $secure param of set_cookie function used to be null which later on fallback to default.

Exactly.
https://github.com/bcit-ci/CodeIgniter/blob/9b8f2b7a8405acd1b8ad5956ada3d84472b1e8ae/system/helpers/cookie_helper.php#L71

Ref,

function set_cookie(
$name,
string $value = '',
string $expire = '',
string $domain = '',
string $path = '/',
string $prefix = '',
bool $secure = false,
bool $httpOnly = false,
?string $sameSite = null
) {

@kenjis
Copy link
Member

kenjis commented Sep 15, 2022

This seems the first commit: https://github.com/codeigniter4/CodeIgniter4/pull/241/files#diff-1ee301297e39aa12b421d2ad72d266269f4fc8fe56c01c366fbcdf16056a942dR82
However, no reason is given for the change in behavior from CI3.

@kenjis
Copy link
Member

kenjis commented Sep 15, 2022

I sent a PR: #6544

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants