Cookie Helper | CodeIgniter Forums

1 of 3

Cookie Helper

ksheurs

Posted: 25 March 2007 06:02 PM

[ Ignore ]

Grad Student

Total Posts: 66

Joined 07-12-2006

I opened up the cookie helper and noticed 2 weird things going within the set_cookie() function.

Starting on line 61:

if ($prefix == '' AND $CI->config->item('cookie_prefix') != '')
{
    $CI->config->item('cookie_prefix');
}
if ($domain == '' AND $CI->config->item('cookie_domain') != '')
{
    $CI->config->item('cookie_domain');
}
if ($prefix == '/' AND $CI->config->item('cookie_path') != '/')
{
    $CI->config->item('cookie_path');
} 

What exactly is this doing? Nothing? Shouldn’t it be:

if ($prefix == '' AND $CI->config->item('cookie_prefix') != '')
{
    $prefix = $CI->config->item('cookie_prefix');
}
if ($domain == '' AND $CI->config->item('cookie_domain') != '')
{
    $domain = $CI->config->item('cookie_domain');
}
if ($path == '/' AND $CI->config->item('cookie_path') != '/')
{
    $path = $CI->config->item('cookie_path');
} 

Notice that the values are now being set so they can be used in the setcookie() function below. Also ... the last if statement was using if ($prefix ==). Shouldn’t that be $path.

Profile

Oscar Bajner

Posted: 27 March 2007 01:04 PM

[ Ignore ] [ # 1 ]

Research Assistant

Total Posts: 472

Joined 09-26-2006

This does look wrong.
I never noticed before because I either use setcookie, or explicitly set discrete parameters with set_cookie()

Will bash it about a bit, but your change looks good.

Signature

Old programmers never die, they just parse away.

Profile

Oscar Bajner

Posted: 18 April 2007 12:28 PM

[ Ignore ] [ # 2 ]

Research Assistant

Total Posts: 472

Joined 09-26-2006

I’m just bumping this!
This bug can be replicated / demonstrated as follows:

With a vanilla CI 1.5.3 installation:
Config change:

$config['cookie_prefix']    = "my_"; // By default we want ALL cookie names prefixed with 'my_' 

In a controller, set the cookie, using default values for: prefix, domain, path

$cookie = array(
    'name'   => 'cookiejar',
    'value'  => 'Who Put the Cookie',
    'expire' => '60',
);
set_cookie($cookie); 

Result: A cookie is set called “cookiejar”, BUG, It should pick up the config and call it “my_cookiejar”.

Fix: As per the code above in ksheurs original post.

Signature

Old programmers never die, they just parse away.

Profile

Derek Allard

Posted: 19 April 2007 04:53 AM

[ Ignore ] [ # 3 ]

Administrator

Total Posts: 7321

Joined 03-23-2006

ksheurs, thanks for your contribution.

Oscar, Have you tested this?

If I can get 1 more person to confirm that they implemented the patch and it works, I’ll add it in.

Signature

DerekAllard.com - CodeIgniter, ExpressionEngine, and the World of Web Design
BambooInvoice - Open Source, CodeIgniter powered invoicing.

Profile

MSG

Oscar Bajner

Posted: 20 April 2007 01:30 PM

[ Ignore ] [ # 4 ]

Research Assistant

Total Posts: 472

Joined 09-26-2006

Yup, I have tested it. It works, (With one reservation)

If the patch is applied, and your default cookie config is as follows:

$config['cookie_prefix']    = "his_"; // By default All cookies prefixed with 'his_' 

Then in a controller you want to set a cookie called “her_cookiejar”. So you

$cookie = array('name'=> 'cookiejar', 'value' => 'The Cookie', 'expire' => '60',);               set_cookie($cookie); 

Result: A Cookie called “her_cookiejar” is set as expected

Now, you want to set a cookie with the name “cookiejar”, so you

$cookie = array('name'=> 'cookiejar', 'value' => 'The Cookie', 'expire' => '60', 'prefix' => '');               set_cookie($cookie); 

Result: A Cookie called “his_cookiejar” is set somewhat unexpectedly
I say “somewhat”, because, If you don’t supply a parameter, the function must get the default from the config, so this is correct.
But you might really expect it to use the parameter, ‘’ that was specified.

I’m not sure now, what people regard as “expected” in this case.

Signature

Old programmers never die, they just parse away.

Profile

Derek Allard

Posted: 21 April 2007 07:13 AM

[ Ignore ] [ # 5 ]

Administrator

Total Posts: 7321

Joined 03-23-2006

OK, I guess we should also modify the get_cookie function as well.

Oscar,ksheurs and anyone else (coolfactor?) could you test this out just for completeness, and I’ll commit.

/**
 * Fetch an item from the COOKIE array
 *
 * @access    public
 * @param    string
 * @param    bool
 * @param    string    the cookie prefix
 * @return    mixed
 */
function get_cookie($index = '', $xss_clean = FALSE, $prefix = '')
{
    $CI =& get_instance();

    if ($prefix == '' AND $CI->config->item('cookie_prefix') != '')
    {
        $prefix = $CI->config->item('cookie_prefix');
    }

    return $CI->input->cookie($prefix.$index, $xss_clean);
} 

Signature

DerekAllard.com - CodeIgniter, ExpressionEngine, and the World of Web Design
BambooInvoice - Open Source, CodeIgniter powered invoicing.

Profile

MSG

coolfactor

Posted: 21 April 2007 07:31 AM

[ Ignore ] [ # 6 ]

Lab Technician

Total Posts: 1747

Joined 06-23-2006

Has my vote.

Signature

Mac OS X 10.4.10, Apache 1.3.3, PHP 5.2.3, CodeIgniter 1.5.x., baby!

Profile

Oscar Bajner

Posted: 22 April 2007 04:45 AM

[ Ignore ] [ # 7 ]

Research Assistant

Total Posts: 472

Joined 09-26-2006

Derek, it’s not just the prefix, but also the domain and path defaults which are affected. (Sorry, I should have made that clear)

This is working for me (with no unexpected behaviours)
Change line 45 in cookie_helper from:

function set_cookie($name = '', $value = '', $expire = '', $domain = '', $path = '/', $prefix = '') 

TO :

function set_cookie($name = '', $value = '', $expire = '', $domain = null, $path = null, $prefix = null) 

Change lines 58 - 72 from :

// Set the config file options
    $CI =& get_instance();
    
    if ($prefix == '' AND $CI->config->item('cookie_prefix') != '')
    {
        $CI->config->item('cookie_prefix');
    }
    if ($domain == '' AND $CI->config->item('cookie_domain') != '')
    {
        $CI->config->item('cookie_domain');
    }
    if ($prefix == '/' AND $CI->config->item('cookie_path') != '/')
    {
        $CI->config->item('cookie_path');
    } 

TO:

// Set the config file options
    $CI =& get_instance();
    
    if ($prefix === null AND $CI->config->item('cookie_prefix') != '')
    {
        $prefix = $CI->config->item('cookie_prefix');
    }
    if ($domain === null AND $CI->config->item('cookie_domain') != '')
    {
        $domain = $CI->config->item('cookie_domain');
    }
    if ($path === null AND $CI->config->item('cookie_path') != '')
    {
        $path = $CI->config->item('cookie_path');
    } 

This ensures that the function will behave correctly when you pass a parameter like “prefix=’’”

I don’t believe the get_cookie() needs to change really. If we do, then the same thing applies:
Eg. If you have a default like “prefix=‘her_’, but you want to get a cookie called “cookiejar”, you
will have to pass a $prefix of ‘’, which will just get you the default “her_”.
This should work: (I’d prefer not to change the get_cookie tho)

function get_cookie($index = '', $xss_clean = FALSE, $prefix = null)
{
    $CI =& get_instance();

    if ($prefix === null AND $CI->config->item('cookie_prefix') != '')
    {
        $prefix = $CI->config->item('cookie_prefix');
    }

    return $CI->input->cookie($prefix.$index, $xss_clean);
} 

It would be good if a couple of people could also test drive all this, my tests were ok.

Signature

Old programmers never die, they just parse away.

Profile

coolfactor

Posted: 22 April 2007 06:56 AM

[ Ignore ] [ # 8 ]

Lab Technician

Total Posts: 1747

Joined 06-23-2006

I support Oscar’s use of NULL, as it means “no value”, which is exactly what is being tested for.

Signature

Mac OS X 10.4.10, Apache 1.3.3, PHP 5.2.3, CodeIgniter 1.5.x., baby!

Profile

Derek Allard

Posted: 22 April 2007 05:57 PM

[ Ignore ] [ # 9 ]

Administrator

Total Posts: 7321

Joined 03-23-2006

good call oscar.

I’ll put this into practice shortly, but as you said, the more to test drive it, the faster it makes it into the codebase.

Signature

DerekAllard.com - CodeIgniter, ExpressionEngine, and the World of Web Design
BambooInvoice - Open Source, CodeIgniter powered invoicing.

Profile

MSG

waldemarbautista

Posted: 25 April 2007 03:02 AM

[ Ignore ] [ # 10 ]

Summer Student

Total Posts: 7

Joined 04-25-2007

Hi!

I would also like to point out this block of code in set_cookie.

if ( ! is_numeric($expire))
    {
        $expire = time() - 86500;
    } 

I believe 86500 should be added to time() and not subtracted.

Other than that, the revised code is working properly on my side.

Profile

JAAulde

Posted: 27 April 2007 10:04 AM

[ Ignore ] [ # 11 ]

Lab Assistant

Total Posts: 128

Joined 04-06-2007

No, wabautista, it should be subtracted.

The documentation says that if you pass set_cookie() no expiration, the cookie will be deleted. So if the expire value is not numeric, the 86500 gets subtracted, which sets the cookies expiration into the past, causing the browser to delete it.

As for the rest of the stuff mentioned in this thread…has it been committed to SVN for the next release?

Thanks,
Jim

Profile

waldemarbautista

Posted: 27 April 2007 10:14 AM

[ Ignore ] [ # 12 ]

Summer Student

Total Posts: 7

Joined 04-25-2007

JAAulde, if that is the case, then the documentation is incorrect when it says that “Only the name and value are required.”.

I tried only the name and value in set_cookie() and it did not work because of that. So either the documentation is incorrect or the code is.

Profile

JAAulde

Posted: 27 April 2007 11:31 AM

[ Ignore ] [ # 13 ]

Lab Assistant

Total Posts: 128

Joined 04-06-2007

wabautista - 27 April 2007 10:14 AM
So either the documentation is incorrect or the code is.

I think there are problems with both the code and the documentation. Their is definitely a disagreement between them, and worse I think their are some minor issues in the code which don’t line up with fundamental cookie concepts.

I am working on a replacement cookie helper which I will definitely be using and will be posting to this board for others if they want it. I learned a lot about cookie practices when I wrote my JavaScript Cookie Handling Library.

Profile

Oscar Bajner

Posted: 27 April 2007 12:14 PM

[ Ignore ] [ # 14 ]

Research Assistant

Total Posts: 472

Joined 09-26-2006

I’m having a look at this.
(Derek, do you feel comfortable with committing two changes, or prefer one at
a time? The helper should allow a secure parameter, as pointed out by Jim)
I guess I’m saying I would be prepared to make the (extra) change and test and whathaveyou…

The code and the docs are not One (how very unzen) To delete a cookie, you need to set it with exactly
the same parameters it was set with, excepting, value must be an empty string (Signals, delete this cookie),
and expiry can be a date in the past (Signals to browser, delete this cookie now!)

Signature

Old programmers never die, they just parse away.

Profile

JAAulde

Posted: 27 April 2007 12:22 PM

[ Ignore ] [ # 15 ]

Lab Assistant

Total Posts: 128

Joined 04-06-2007

I have a complete replacement to cookie helper now. Some of the arguments have changed a bit and the secure flag is in place.

First, my config area for cookies:

/*
|--------------------------------------------------------------------------
| Cookie Related Variables
|--------------------------------------------------------------------------
| These are default values.  Each can be changed at will during a cookie operation
|
| 'cookie_prefix' = Set a prefix if you need to avoid collisions
| 'cookie_lifetime' = Set length of time (seconds) cookies should live in browser.
|                      0 means until browser closes.
| 'cookie_domain' = Set to .your-domain.com for site-wide cookies
| 'cookie_path'   =  Typically will be a forward slash
|
*/
$config['cookie_prefix']    = 'CI_';
$config['cookie_lifetime']    = 0;
$config['cookie_domain']    = $_SERVER['HTTP_HOST'];
$config['cookie_path']        = '/'; 

Now the new helper and user guide page:
http://www.jaaulde.com/test_bed/CodeIgniter/modCookieHelper/

Profile

1 of 3

‹‹ Session Class Requires ActiveRecord Bug Avoidance --> Loader::_ci_init_class() ››

Username		Remember Me?
Password		forgot password?

Post Marker Legend
	New posts	Hot Topic with new posts	New Poll	Moved Topic	Sticky topic
	No new posts	Hot Topic with no new posts	Old Poll	Closed Topic	Announcements

Visitor Statistics
The most visitors ever was 721, on January 06, 2010 09:38 AM
Total Registered Members:	114968	Total Logged-in Users:	55
Total Topics:	122423	Total Anonymous Users:	1
Total Replies:	647238	Total Guests:	466
Total Posts:	769661