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] httpOnly flag is ignored in set() for cookies #13464

Closed
mohahn opened this Issue Aug 15, 2018 · 5 comments

Comments

Projects
5 participants
@mohahn
Copy link

mohahn commented Aug 15, 2018

System:

$ phalcon info

Phalcon DevTools (3.4.0)

Environment:
  OS: Linux vmv 4.9.0-6-amd64 #1 SMP Debian 4.9.88-1+deb9u1 (2018-05-07) x86_64
  PHP Version: 7.0.30-0+deb9u1
  PHP SAPI: cli
  PHP Bin: /usr/bin/php7.0
  PHP Extension Dir: /usr/lib/php/20151012
  PHP Bin Dir: /usr/bin
  Loaded PHP config: /etc/php/7.0/cli/php.ini
Versions:
  Phalcon DevTools Version: 3.4.0
  Phalcon Version: 3.4.0
  AdminLTE Version: 2.3.6

By default httpOnly seems to be active (see). In API documentation one can see, that with the last parameter of method set this flag can be set.
But, regardless of value I'm using, the cookie is sent with active httpOnly to browser, always.

Ex. in controller:

$this->cookies->set(
    'uid', // name
    $id, // value
    time() + 86400, // expire
    '/', // path
    false, // secure
    $domain, // domain
    false // http only
);

The only way I get httpOnly removed is by calling following statements right after set():

$cookie = $this->cookies->get('uid');
$cookie->setHttpOnly(false);

So the flag in set() seems to be ignored (all other parameters are working correctly, only httpOnly is set to true always). That's not very comfortable. This is urgent to me, so to be sure I fall back to native setcookie() now.
What I do not understand is, that the flag httpOnly is optional and declared with null when calling set(). But as property in class Cookie it's initialized with true, see here.

@mohahn mohahn changed the title httpOnly flag is ignored in set() for cookies [BUG] httpOnly flag is ignored in set() for cookies Aug 18, 2018

@stamster

This comment has been minimized.

Copy link
Contributor

stamster commented Aug 20, 2018

	/**
	 * Returns if the cookie is accessible only through the HTTP protocol
	 */
public function getHttpOnly() -> boolean

what do you get when you invoke this method?

It is clear that default property value is set to true:
protected _httpOnly = true;

as you already outlined.

So $cookie->setHttpOnly(false); seems like a solution to the problem, if this is a problem at all.

@mohahn

This comment has been minimized.

Copy link
Author

mohahn commented Aug 20, 2018

I'm executing the following code

$this->cookies->set(
    'uid', // name
    $id, // value
    time() + 86400, // expire
    '/', // path
    false, // secure
    $domain, // domain
    false // http only
);
$check = $this->cookies->get('uid');
var_dump($check->getHttpOnly());

which gives me true. Seems like an internal bug.
BTW: It's a feature request, but I don't think it's correct to enable httpOnly as default, when native PHP function setcookie expects this parameter as NULL if not set.

@avionwd

This comment has been minimized.

Copy link

avionwd commented Oct 16, 2018

I have the same issue! Phalcon version 3.4.1 (migrated from 3.3.x).
So, our application sets a cookie at the backend, but frontend (JS) cannot get it because it has flag HttpOnly.
Pls, fix this.

@antsokol

This comment has been minimized.

Copy link

antsokol commented Dec 13, 2018

I have the same. Strange, because source code looks like right.

class ExtraCookie extends \Phalcon\Http\Cookie
{
	public function __construct($name, $value = null, $expire = 0, $path = "/", $secure = null, $domain = null, $httpOnly = null)
	{
		parent::__construct($name, $value, $expire, $path, $secure, $domain, $httpOnly);

		var_dump($httpOnly);
		var_dump($this->_httpOnly);
	}
}

$cookie = new ExtraCookie('test-cookie', 'some value', time() + 8400, '/', false, null, false);

bool(false) bool(true)

Version => 3.4.2
Build Date => Dec 2 2018 17:24:20
Powered by Zephir => Version 0.10.14

@niden niden added the Bug - High label Dec 23, 2018

@niden niden self-assigned this Dec 23, 2018

@niden niden added this to To do in 4.0 Release via automation Dec 23, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Dec 24, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Dec 24, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Dec 27, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Dec 27, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Jan 7, 2019

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Jan 7, 2019

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Jan 12, 2019

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Jan 12, 2019

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Jan 25, 2019

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Jan 25, 2019

niden added a commit that referenced this issue Jan 25, 2019

@niden

This comment has been minimized.

Copy link
Member

niden commented Jan 25, 2019

Thsi has been resolved

@niden niden closed this Jan 25, 2019

4.0 Release automation moved this from To do to Done Jan 25, 2019

niden added a commit to niden/cphalcon that referenced this issue Jan 25, 2019

[phalcon#13060] - Merge branch '4.0.x' into T13060-filter-service
* 4.0.x:
  _httpOnly has a default value of false
  Updated Tests
  Update tests/unit/Http/Cookie/CookieCest.php
  Created test for Issue phalcon#13464
  Fixes phalcon#13464: httpOnly is no longer initialised with a value
  Regenerated build
  Use latest Zephir
  phalcon#13749: Removed Phalcon\Mvc\User\* (phalcon#13775)
  phalcon#13717: Cleaned up the `Phalcon\Mvc\Model\Metadata\Redis` constructor (phalcon#13774)
  phalcon#13758: `Phalcon\Exception` implements `\Throwable` (phalcon#13776)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment