Cacherouter.module sets its own DRUPAL_UID cookie, but sets it without specifying the domain and path, potentially causing issues with sites specifying domain-based (as opposed to host-based) cookies, as in sess_destroy_sid().

CommentFileSizeAuthor
#4 cacherouter_cookie.patch1.91 KBfgm
#2 cookie.patch1.89 KBfgm

Comments

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

(typo: I meant: it should do as in sess_destroy_sid()).

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Active » Needs review
StatusFileSize
new cookie.patch1.89 KB

Also, not unsetting that cookie when logging out causes errors on logout. Suggested patch fixed both issues.

JirkaRybka’s picture

JirkaRybka commented

Maybe I came across this problem as well - the unfamous problem with dual URLs www . foo . tld and just foo . tld both working. With CacheRouter active (fastpath page cache), I got an event in Drupal watchdog log, clicked on the logged URL of the page where the problem was - and oops, I saw that page as anonymous. Only then I realized, that the original visitor (whose exact URL I followed through the log, in an attempt to reproduce their problem), used www prefix, otherwise not used on the site. Definitely a cookie domain / caching problem, but I didn't investigate further yet.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
StatusFileSize
new cacherouter_cookie.patch1.91 KB

Typo in previous patch.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 6.x-1.0-rc1 » 6.x-1.x-dev

@fgm Your patch looks good, but I'm planing to check D7 codebase before commit this

PS: Confirm the problem with [www.]example.com but this should be changed at .htaccess with redirect

andypost’s picture

andypost
he/him
Primary language Russian
commented

version check should include 5.2.0 version
version_compare(PHP_VERSION, '5.2.0') >= 0

As http://api.drupal.org/api/function/sess_regenerate/6
setcookie should expire time() - 42000 for IE visitors

What for unset($_COOKIE['DRUPAL_UID']);

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

IIRC the unset() is there because when it is missing in some scenarios (using Pressflow 6.15), at request shutdown, drupal checks if $_COOKIE is empty and, if not, tries to save the cookie to the session, causing an error because the session can no longer be created (no objects after shutdown): if there is nothing else in the session, this unset will cause session destruction, and allow PF to avoid keeping it in the sessions table, which is significant for large sites.

Regarding the setcookie(), -42000 (+/- 12 hours) is not a magic value; AIUI the only important point is to set it in the past.

andypost’s picture

andypost
he/him
Primary language Russian
commented

@fgm Could you reroll patch with version_compare and setcookie 42000?
I'm going to commit this change before next RC

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Fixed

Thx, fixed http://drupal.org/cvs?commit=410202

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.