Should VCL file unset cookies on 404's?

[alankent]

Ben Lessani (twitter @sonassi) reported: "The default VCL in M2 is alarming. Why is there a cookie unset in vcl_recv - this will cause session/cookie loss on 404's " https://twitter.com/sonassi/status/528250395991490562

Alan: I am not sure why losing a session cookie on a 404 is a major issue - should not be getting 404's in the first place. But recording to make sure it gets looked into.

[piotrekkaminski]

@alankent 404 can happen for any reason, old link on a landing page or bad link shared somewhere. If it causes to disconnect session (you loose cart contents) it is pretty bad.

[blessani]

Hi Guys,

If I'd known a simple message was all that was needed, I would have done this myself, I feel awfully lazy now. I thought that a PR would be required to suggest the fix.

In any case, the problem is high on the severity scale. The reason being that if you unset a cookie on specific file extensions when a request is received (regardless of whether the asset exists or not), if that asset no longer exists (ie. A missing image/CSS/JS file), then the request will be redirected to the Magento bootstrap so Magento itself can process the 404.

Because there is no longer a cookie, Magento assumes it's a new session, creates a new cookie, sends it back to the browser, then the browser deletes the previous cookie in favour of the new one.

So any session related activity is immediately lost, ie. cart, login, wish list etc.

Cookies should only be unset on fetch and only if the response status is 200.

Almost every website out there has a rogue 404 on static content.

[alankent]

They way I see it working is pull requests are more precise, and quicker to get processed. If people want to help and have time, that does help us! But we know not everyone has time constraints. We would rather hear about faults and address them than not hear about them! So yes, if a problem (even if just a suspicion of a problem) we would love to hear it. It is cheaper to fix early than late in dev lifecycle. So thanks for reporting this - and I don't care if it turns out everything is OK in the current design or not.

Copying from tweet thread. (I will point some of the core engineers at this issue too - just be aware of potential timezone induced delays.) From Ben: No, its not a concern, I'm pointing out a massive failing in your core VCL. It WILL cause session loss on 404'ed static assets. You can't unset a cookie the moment a request is received, if the asset doesn't exist, bootstrap is hit, Magento session is lost.

Alan: I am not a VCL config file expert, so I will loop in some of the guys more directly. Obviously we want to get this right! My understanding is we remove any cookie setting in pages we cache - this will not actually unset any cookie in the web browser will it? We are not adding an 'unset cookie' header. So it won't trigger a new session (it will not delete any existing cookies).

Also note for static assets (JS, CSS etc files) in production (and somewhat in dev) we use Apache URL rewrites. There is a deploy.php script that generates all the static assets. So these won't hit PHP at all - just fetch a file directly from disk. (If the file is not on disk, then PHP code gets invoked.) In developer mode we did not generate the cached version, but we are about to (or just did?) change this. It was too slow in developer mode. So it will generate the file on first request, but then cache the result as a file on disk and return that until it is deleted (which will trigger automatic regeneration of the file). So if the resource does not exist, it will hit PHP in dev. (Production will be faster.)

"Almost every website out there has a rogue 404 on static content" - Hmmm. Sounds like a tool to spot that would be useful. Not sure how easy to automate with PHTML files etc. Interesting.

[alankent]

I was poking around for self edification (I am not the expert here). https://github.com/magento/magento2/blob/master/app/code/Magento/PageCache/etc/varnish.vcl I assume is the template file. Are these the lines you are concerned with?

if (req.url ~ "\.(css|js|jpg|png|gif|tiff|bmp|gz|tgz|bz2|tbz|mp3|ogg|svg|swf|woff)(\?|$)") {
     unset req.http.Cookie;
}

The VCL file does assume these are static assets. The mod_rewrite rules I was referring to can be found in https://github.com/magento/magento2/blob/master/pub/static/.htaccess

RewriteEngine On

# Remove signature of the static files that is used to overcome the browser cache
RewriteRule ^version.+?/(.+)$ $1 [L]

RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-l

RewriteRule .* ../static.php?resource=$0 [L]

My understanding is this redirects to the static.php file only if the local file does not exist.

Ah, now I get you. You are saying a new session is being created at that time because to the web server it looks like the user does not have a session cookie. So when a static referenced file is missing, a new cookie will be set. If we guaranteed that requests for static assets never set a cookie then it would be safe.

Thanks, I will dig some more. Yes, your description above was useful. Thank you!

[kokoc]

Thank you for reporting the issue. Appropriate item created in internal backlog

[barbazul]

I don't there is an issue here unless varnish itself is sending a cookie.

All assets requests should be striped of cookies. Ideally you would use a
different domain for assets to prevent sending meaningless cookies back and
forth.

I am also not an expert but my understanding is that Varnish should strip
all cookies from the response, so it doesn't really matter what it does to
cookies in the request.
El 05/11/2014 14:58, "kokoc" notifications@github.com escribió:

Thank you for reporting the issue. Appropriate item created in internal
backlog

—
Reply to this email directly or view it on GitHub
#721 (comment).