Skip to content
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

local/session storage is mismanaged, particularly when it doesn't exist #16568

Closed
dan-ding opened this issue Jul 5, 2018 · 17 comments
Closed
Labels
Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed

Comments

@dan-ding
Copy link

dan-ding commented Jul 5, 2018

Essentially, users carts will disappear or they won't be able to login, or they'll be logged out or some other session related wackyiness.

creating here from #7931 as @orlangur said to make a new one

There are multiple reasons why this is occurring -- the follow is the first way I've found to recreate an a problem easily.

https://github.com/julien-maurel/jQuery-Storage-API/blob/d2ce10b0fb53d7cb919f14e70853a5c232477f93/jquery.storageapi.js#L423

Preconditions

  1. magento 2.2.0 + (updated)
  2. customers who want to buy things

Steps to reproduce

One way to reproduce is:

  1. install firefox;
  2. disable localstorage in firefox (about:config dom.storage.enabled == false);
  3. browse around the catalog;
  4. while on a product listing page, identify a product that really is interesting;
  5. open the product detail page in a new window/tab;
  6. decide the product isn't for you and close the product detail window/tab;
  7. browse around some more;
  8. on a product listing page identify a product you'd like to know more about;
  9. open the product detail page in a new tab/window;
  10. decide you don't want it and close the tab/window;
  11. do this eight more times until you find a product you want;
  12. try to log in or add it to your cart;
  13. try to checkout;
  14. look for your cart;
  15. look at your cookies and see billions (exaggeration) of them.
  16. look at your magento log and see maximum cookies error

Expected result

  1. customers are happy and can buy stuff
  2. clients make money
  3. cookies minimal

Actual result

  1. customers are unhappy

  2. clients are really unhappy

  3. cookies are a sogging mess
    depending on surfing habits there will be multiple ss_\d{8}* named cookies and multiple ls* cookies, possibly far too many depending on a variety of normal operating conditions.

  4. possibly get a "too many cookies" error

  5. possibly get a "request header too large" error

changing
https://github.com/julien-maurel/jQuery-Storage-API/blob/d2ce10b0fb53d7cb919f14e70853a5c232477f93/jquery.storageapi.js#L423
to be a static string instead of a random number for the window.name helps with the ss_* cookies
it will not help with the ls_* cookies

again, disabling localstorage here in firefox is just one example of causing the issue

@magento-engcom-team
Copy link
Contributor

Hi @dan-ding. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me {$VERSION} instance

where {$VERSION} is version tags (starting from 2.2.0+) or develop branches (2.2-develop +).
For more details, please, review the Magento Contributor Assistant documentation.

@dan-ding do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

  • yes
  • no

@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Jul 5, 2018
@dan-ding
Copy link
Author

dan-ding commented Jul 5, 2018

ya, any version of magento should do. start with 2.2.0!

@orlangur
Copy link
Contributor

orlangur commented Jul 6, 2018

@dan-ding,

disable localstorage in firefox

This is not a valid case. Additional safeguard should be added probably similar to the one appearing when JavaScript is disabled.

@dan-ding
Copy link
Author

dan-ding commented Jul 6, 2018

@orlangur whhhaaa? come on. it's totally valid.

(a) comparing it to having javascript disabled makes as little sense as saying people can only use IE 10.
(b) disabling localstorage is the easiest means of recreating and testing an issue that is definitely occurring in the universe with unknown conditions.

@dan-ding
Copy link
Author

dan-ding commented Jul 7, 2018

What do you need to give this some deeper consideration?

@orlangur
Copy link
Contributor

orlangur commented Jul 9, 2018

occurring in the universe with unknown conditions

They should be determined. It is obvious that with local storage disabled Magento will not function properly, check https://devdocs.magento.com/guides/v2.2/extension-dev-guide/cache/page-caching/private-content.html for instance.

@dan-ding
Copy link
Author

dan-ding commented Jul 9, 2018

Ok! Discussion --
There's dozens browsers, variants thereof, and even more plugins for them. All these may conspire to handle storage differently, for different reasons, and for reasons that people actually choose. For example, private browsing is probably going to in continued use and browsers handle localstorage in that differently.
I'd ask how you think all these could be determined, but that's quite a tangent.

That page doesn't say localstorage is required, it just says magento uses it ( and forget for a moment how silly storing someones name or address or viewed products in localstorage actually is). However, if one is going to use it, one of big decisions and case(s) to test, is going to be what to do when it's not available.

To that last item, it's not obvious magento won't work if localstorage isn't available. Take the library (that is part of the core) to manage this so-called localstorage:
https://github.com/magento/magento2/blob/2.2-develop/lib/web/jquery/jquery.storageapi.min.js
or if one prefers to actually read the code
https://github.com/julien-maurel/jQuery-Storage-API/blob/d2ce10b0fb53d7cb919f14e70853a5c232477f93/jquery.storageapi.js
Specifically if it can't use localstorage, it falls back to cookies. We can infer whomever decided to use the library, wanted the fallback -- therefore localstorage is definitely not required,

And that unfortunately brings us to this issue because how magento "uses localstorage", it ends up being just a matter of time before some customers are paralyzed. Either by too many cookies, or too large of a request header.

Can we move forward now?

@orlangur
Copy link
Contributor

orlangur commented Jul 9, 2018

how silly storing someones name or address or viewed products in localstorage actually is

This is totally not silly, please try to be more constructive.

What would you suggest to use for fallback instead of cookies when local storage is not available for some reason?

That page doesn't say localstorage is required, it just says magento uses it

I can say you more, you cannot turn this mechanism off. I do agree that it looks like cookies storage as fallback simply does not work.

What is the % of users having local storage disabled?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jul 9, 2018

Hi @orlangur on our website some customers also have this issue - OS: Android (including 7.1.1, that is quite new) + opened website in Facebook (in app) - as result - no local storage support. Probably the same issue will be with twitter/another apps

Another example: Incognito mode on Safari iOS/MacOS - no local storage support.

As a workaround I we could NOT use cookies as fallback from localstorage

@orlangur
Copy link
Contributor

orlangur commented Jul 9, 2018

@ihor-sviziev no objections from my side if it would allow a decent UX. Considered it by myself but I don't think doing request for section data on each request is acceptable especially on mobile devices. If UX is poor, we can show warning to open site in a proper browser.

@dan-ding could you please play with such variant really quick? Just comment out fallback you already found in code and check how it works with disabled local storage.

@dan-ding
Copy link
Author

dan-ding commented Jul 9, 2018

I'm sorry, @orlangur, I am being constructive (to the ends of having a better application) . Storing a customers name in localstorage is silly. I'd open up an issue for it, but I don't know where would get the consideration it deserves.

I'm confused. You've rebuffed the issue twice saying in so many words it's not an issue, now you're asking me questions as if you think it is an issue.
Which is it please?
Are those little checkboxes going to get checked?

@dan-ding
Copy link
Author

dan-ding commented Jul 9, 2018

And I firmly object to:

if UX is poor, we can show warning to open site in a proper browser.

That is poor UX.
It's one thing to wave warnings with IE6. It's quite another to do it with the latest Chrome (or whatever).

And I have no idea what percentage of customers do what with their browsers. I do know that I see, or hear about, symptoms of this issue almost hourly, which has been slowly increasing over the year.

@orlangur
Copy link
Contributor

orlangur commented Jul 9, 2018

Storing a customers name in localstorage is silly

It means only that you know nothing about FPC in Magento :)

I never said this is not an issue. Just that there are multiple approaches to circumvent this situation:

  1. Forbid Magento usage without local storage (from information provided by Ihor it seems to be to restrictive)
  2. Remove check for too many cookies
  3. Remove fallback into cookie storage (this require a proper usability testing - whether it's feasible to make request for private data with each page request)

@dan-ding
Copy link
Author

Haha so silly @orlangur

Yeah, you're right, I know nothing about magento or fpc. Geez, I was going to suggest

  1. Fix the code that is messing up the cookies when local/session storage isn't there.

How dumb is that idea, right?!

@orlangur
Copy link
Contributor

@dan-ding it's the same as

Remove fallback into cookie storage

And I just suggested you to check how usable will be Magento with such removal.

@dan-ding
Copy link
Author

NO it's not the same. I ... I ...

@orlangur this is a ridiculous conversation which you seem incapable, and why I closed the ticket. You want to experiment, then YOU do it. Geezz, you want to talk about adding buggery code to show warnings or other nonsense -- yet that already exists in magento 😱 (and thankfully it can still be turned off).


For anyone else getting a headache reading this, AND with cookie issues or customers sessions being lost the following may help some:

search and replace these strings in:
https://github.com/magento/magento2/blob/2.2-develop/lib/web/jquery/jquery.storageapi.min.js

change _path:null to _path:'/' or if your magento url root like /store/ change it to that. this is the base cookie URI;
change window.name||(window.name=Math.floor(1e8*Math.random()) to window.name||(window.name='') or whatever string you want as long as it's constant;
change _expires:3650 to _expires:78 (this is days) some number because ten years is silly

this will make the cookies be reused instead of regenerating themselves with random names and various paths. it could be more clever, but ...

there may be other issues (private browsing in safari possibly?) with this library since it's four years old and unmaintained, but I'll let someone else open that issue.

@ihor-sviziev
Copy link
Contributor

Hi @dan-ding,
FYI I reported issue that was acknowledged #17195. So you can watch it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed
Projects
None yet
Development

No branches or pull requests

4 participants