Bug in Javascript Get_Cookie
ThoughtfulCoder
Status: New User - Welcome
Joined: 23 Aug 2007
Posts: 2
Reply Quote
If you have a cookie name that is the end of another cookie name you may get the wrong value.

For example:
If you have a cookie called LoggedVisit and Visit and Visit is after LoggedVisit in document.cookie you'll get the value for LoggedVisit if you try to get the value for Visit.

I came up with the following to get around this issue:

function GetCookie( name )
{
var cookieValue = null;

if ( document.cookie.length > 0 )
{
var extractExp = new RegExp("(?:^|;)\\s*" + name + "=(.*?)(?:$|;)");
var match = document.cookie.match(extractExp);

if ( match != null )
{
cookieValue = unescape(match[1]);
}
}

return cookieValue;
}

I appreciate the posting of the original code as it has been very useful. I hit this problem and it took a bit to figure out why I was getting the wrong value for my cookie :-)
Back to top
techAdmin
Status: Site Admin
Joined: 26 Sep 2003
Posts: 3758
Location: East Coast, West Coast? I know it's one of them.
Reply Quote
I can see the problem, and why it would have been so annoying to resolve. I have to admit, one thing I dislike about these generic scripts is that they tend to have long standiing weaknesses, usually it's better to just write your own, but this one seemed to be so time tested that it seemed safe, but as you have found, it has sloppy coding and testing, which is quite common, sadly.

The solution you posted lacks some elegance and readability, although I assume it works.

Probably better is to simply slice out the cookie names from the full string then do a straight == comparison in a loop, but regex is fine too, that set of functions though I think is best served by being highly readable versus using nifty but to most users very hard to understand methods.

But that doesn't take away from the fact that you found a fairly major weaknesss in that old script, which has seen a LOT of eyes on it since it was released. I'd say your nickname Thoughtful coder is well deserved.

We'll take a look and see if maybe we can't come up with something a bit more elegant, but your solution is of course totally workable, I think anyway, regex tends to make me see double so I'd have to take a close look to see if the patterns are comprehensive.
Back to top
techAdmin
Status: Site Admin
Joined: 26 Sep 2003
Posts: 3758
Location: East Coast, West Coast? I know it's one of them.
Reply Quote
I'm going to try this version for a while and see if there's any issues with it:

:: Code ::
// this fixes an issue with the old method, ambiguous values
// with this test document.cookie.indexOf( name + "=" );
function Get_Cookie( check_name ) {
   // first we'll split this cookie up into name/value pairs
   // note: document.cookie only returns name=value, not the other components
   var a_all_cookies = document.cookie.split( ';' );
   var a_temp_cookie = '';
   var cookie_name = '';
   var cookie_value = '';
   var b_cookie_found = false; // set boolean t/f default f
   
   for ( i = 0; i < a_all_cookies.length; i++ )
   {
      // now we'll split apart each name=value pair, and get rid of any escapes
      a_temp_cookie = a_all_cookies[i].split( '=' );
      // and trim left/right whitespace while we're at it
      cookie_name = a_temp_cookie[0].replace(/^\s+|\s+$/g, '');
      
      // if the extracted name matches passed check_name
      if ( cookie_name == check_name )
      {
         b_cookie_found = true;
         cookie_value = unescape( a_temp_cookie[1].replace(/^\s+|\s+$/g, '') );
         return cookie_value;
         break;
      }
      a_temp_cookie = null;
      cookie_name = '';
   }
   if ( !b_cookie_found )
   {
      return null;
   }
}


I was reading up on cookies, and apparently document.cookie will only return the name=value data, not any of the other stuff like expires etc, so the only real splits you need are the first overall ; split, then the = + trim splits, then you can use a simple equality test, which of course will never result in that sloppy result.

Give the above code a try if you want, I think it should work nicely, and it's slightly more clear and readable than pulling a regex thing, though here because stupid javascript doesn't have a built in trim function, I still have to use a bit of it to clean the whitespace, but cleaning whitespace is easier I think to understand overall than more complex stuff, and less likely to generate error.
Back to top
ThoughtfulCoder
Status: New User - Welcome
Joined: 23 Aug 2007
Posts: 2
Reply Quote
I've used regular expressions very heavily over the years. They seem second nature to me and can save a lot of hassle, especially in more complex validation, search, and replace scenarios. I actually think they can reduce coding errors as they reduce the amount of code written.

But, regular expressions are certainly are not required :-)

I think your solution looks great.

I just wanted to let you know about the bug as others might run into it. Thanks for posting the code on your site, ultimately it helped save me a lot of time.
Back to top
techAdmin
Status: Site Admin
Joined: 26 Sep 2003
Posts: 3758
Location: East Coast, West Coast? I know it's one of them.
Reply Quote
Thanks for pointing this one out, this will get tested for a bit, then get added to the downloadable script.

If you have an active site using cookies a lot I'd appreciate it if you'd give the above block a spin for a few days, just to ensure that there is not logic error, after which of course you could return to the shorter fix you came up with.

The reason, on scripts like this, I avoid complex regex is not because I think it's bad, it's not, but because it becomes harder for the users who download it to understand it. So I'll opt for simpler, easy to comment methods in general, for readability's sake.

Thanks for catching this error, when I looked again at the previous get cookie code, I was really struck by how bad it was. But that's how it goes

I'm going to add the above block to the code page for this javascript cookie script now however before updating the main script.
Back to top
I've modified a little bit
Briganti
Status: New User - Welcome
Joined: 24 Nov 2011
Posts: 1
Location: Cluj-Napoca
Reply Quote
I've made some modifications on the function:

:: Code ::

function GetCookie(name) {
   
    var a_all_cookies = document.cookie.split( ';' );
    var a_temp_cookie = '';
    var cookie_name = '';
    var cookie_value = '';
    var b_cookie_found = false;
   
    for ( i = 0; i < a_all_cookies.length && b_cookie_found === false; i++ ) {
        a_temp_cookie = a_all_cookies[i].split( '=' );
        // trim left/right whitespace
        cookie_name = a_temp_cookie[0].replace(/^\s+|\s+$/g, '');
       
        if ( cookie_name == name ) {
            b_cookie_found = true;
            /* we need to handle case where cookie has no value but exists
            in cases where cookie is initialized but no value, null is returned */
            if ( a_temp_cookie.length > 1 ) {
                cookie_value = unescape( a_temp_cookie[1].replace(/^\s+|\s+$/g, '') );
            }
        }
        a_temp_cookie = null;
        cookie_name = '';
    }
   
    return ( b_cookie_found ) ? cookie_value : false;
}


MOD:
- return false if the cookie is not exist
- small tweaks: 'for' condition, return method[/code]
Back to top
Display posts from previous:   

All times are GMT - 8 Hours