Cookie mage-messages does not respect cookie configuration settings

PromInc commented 2 years ago

Preconditions (*)

version 2.4.3

Steps to reproduce (*)

Load any page of the website
View the mage-messages cookie in the browsers developer tools Application panel and observe the Expires, Path, and Domain and compare to the admin configuration

Expected result (*)

The configuration would match

Actual result (*)

The configuration does not match

Notes about this issue

The configuration values for lifetime and path are not respected.

The configuration values for domain are not used when setting the mage-messages cookie.

Effects of this bug

Configure the cookie domain to not have a subdomain (ie. .example.com) but load the site with a subdomain (ie. www.example.com). Because the cookie doesn't respect the configuration for domain the browser sets the cookie with the subdomain (ie. www.example.com). If the site uses a secondary subdomain (ie. blog.example.com) the cookie will not persist as it was set only for www.example.com.

Configuration

Configuration Path: Stores -> Configuration -> General -> Web -> Default Cookie Settings:

Scope: Default Config

Cookie Lifetime: 3600
Cookie Path: empty
Cookie Domain: empty
Use HTTP Only: Yes
Cookie Restriction Mode: No

Scope: Main Website

Cookie Lifetime: 604800
Cookie Path: empty
Cookie Domain: .example.com
Use HTTP Only: Yes
Cookie Restriction Mode: No

Please provide Severity assessment for the Issue as Reporter. This information will help during Confirmation and Issue triage processes.

[x] Severity: S0 - Affects critical data or functionality and leaves users without workaround.
[ ] Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
[ ] Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
[ ] Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
[ ] Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.

m2-assistant[bot] commented 2 years ago

Hi @PromInc. Thank you for your report. To speed up processing of this issue, make sure that you provided the following information:

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

Make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:

@magento give me 2.4-develop instance - upcoming 2.4.x release

For more details, review the Magento Contributor Assistant documentation.

Add a comment to assign the issue: @magento I am working on this

To learn more about issue processing workflow, refer to the Code Contributions.

Join Magento Community Engineering Slack and ask your questions in #github channel.

:warning: According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

:clock10: You can find the schedule on the Magento Community Calendar page.

:telephone_receiver: The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

:movie_camera: You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

:pencil2: Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

PromInc commented 2 years ago

I believe this issue relates to / causes #33611

m2-assistant[bot] commented 2 years ago

Hi @engcom-November. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

[ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
Details
If the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.
[ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.
[ ] 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
[ ] 4. Verify that the issue is reproducible on 2.4-develop branch
Details
- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 5. Add label Issue: Confirmed once verification is complete.
[ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

engcom-November commented 2 years ago

Verified the issue on Magento 2.4-develop branch but cannot able to reproduce the issue. mage-messages cookie in the browsers developer tools Application panel has same - "Expires", "Path", and "Domain" values in front-end and Admin configurations all the pages - No issue. Also - Stores -> Configuration -> General -> Web -> Default Cookie Settings values are same for Default Config and Main Website scopes @PromInc kindly verify the issue on Magento 2.4-develop branch and provide missing steps if any if the issue is still reproducible. Thank you.

PromInc commented 2 years ago

@magento give me 2.4-develop instance

magento-deployment-service[bot] commented 2 years ago

Hi @PromInc. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 2 years ago

Hi @PromInc, unfortunately there is no ability to deploy Magento instance at the moment. Please try again later.

PromInc commented 2 years ago

@magento give me 2.4-develop instance

magento-deployment-service[bot] commented 2 years ago

Hi @PromInc. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 2 years ago

Hi @PromInc, unfortunately there is no ability to deploy Magento instance at the moment. Please try again later.

PromInc commented 2 years ago

@engcom-November Please ensure that you set the Cookie Domain for the Scope: Main Website as noted in my configuration in the original post.

I wasn't able to get a Magento develop instance at this time to test the vanilla install. I'll have to try again at a later date.

I did put together a pull request to address this issue. https://github.com/magento/magento2/pull/34890

PromInc commented 2 years ago

@magento give me 2.4-develop instance

magento-deployment-service[bot] commented 2 years ago

Hi @PromInc. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 2 years ago

Hi @PromInc, unfortunately there is no ability to deploy Magento instance at the moment. Please try again later.

PromInc commented 2 years ago

@magento give me 2.4-develop instance

magento-deployment-service[bot] commented 2 years ago

Hi @PromInc. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 2 years ago

Hi @PromInc, unfortunately there is no ability to deploy Magento instance at the moment. Please try again later.

PromInc commented 2 years ago

@engcom-November I still can't get a Magento instance to test on. Thoughts?

engcom-November commented 2 years ago

Hi @PromInc , I re-verified the issue on Magento 2.4-develop branch by setting Main Website scope using below values but cannot able to reproduce the issue:

Magento bot instance has limited access. Please try again and recheck the issue on Magento 2.4-develop branch and provide missing steps if any if you are still facing the issue. Thank you.

PromInc commented 2 years ago

Thank you @engcom-November

I'll try to get an instance when I have time to test, however your screenshot highlights the issue here. In the configuration you set the domain to .magento24.local but in the browser you can see it is only magento24.local (the leading period is not present).

If you look at the pull request I submitted recently you can see that the domain is not being set based on the configuration by Javascript when it creates the cookie. As highlighted here.

There are other configuration values that aren't being set either on the PHP side (the lifetime and path).

I will say that I know an Amasty module was also contributing to the issue on the site I found this issue on (I patched both Magento and Amasty code to resolve my sites issue), however I have analyzed the code to feel that the core Magento code should be improved as well.

engcom-November commented 2 years ago

Verified the issue again on Magento 2.4-develop vanilla instance from bot but cannot able to reproduce the issue: Instance URL: https://1c9dcf6e7e90deed522db184295c2434.instances.magento-community.engineering/ Stores - Settings - Configuration - General - Web - Scope: Default Config

Stores - Settings - Configuration - General - Web - Scope: Main Website: cookie domain set to: .instances.magento-community.engineering Cookie Lifetime: 604800

Save and clear cache Load both front end and back end admin pages mage-messages - configurations(Domain, Path and Expires) are same for both front-end and backend - No issue front-end: Domain: 1c9dcf6e7e90deed522db184295c2434.instances.magento-community.engineering Expires: 2022-03-09T13:07:11.000Z Backend admin domain: 1c9dcf6e7e90deed522db184295c2434.instances.magento-community.engineering Expires: 2022-03-09T13:07:11.000Z @PromInc kindly recheck the issue on Magento 2.4-develop branch and let us know if the issue is still reproducible. Thank you.

PromInc commented 2 years ago

@magento give me 2.4-develop instance

magento-deployment-service[bot] commented 2 years ago

Hi @PromInc. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 2 years ago

Hi @PromInc, here is your Magento Instance: https://14591c84ae7a8c153a688c82fa390c6e.instances.magento-community.engineering Admin access: https://14591c84ae7a8c153a688c82fa390c6e.instances.magento-community.engineering/admin_8b35 Login: 8cc5f51e Password: 9b9259a39e77

PromInc commented 2 years ago

I was able to get a Magento instance and test this.

In my tests below you can see that there are some cookies that respect the configuration and some that do not. In my testing it seems that only the Cookie Domain setting is an issue. I did some tests with the Cookie Lifetime and Cooke Path fields and found them to work for the cookies I was paying attention to. Though the expiration isn't the same for all cookies, so maybe that has issues as well?

Issues I was able to create:

Duplicate cookies (one with leading . and one without). Confirmed on the frontend, did not test in the admin.
Cookies in the admin are using the Main Website configuration as opposed to the Default Config**
Only some cookies respect the configuration for the Cookie Domain setting
- The last comment from @engcom-November shows this as well in that the frontend was configured to use a leading . but it was stated that the cookie did not have a leading .
Not all cookies have the same expiration, which implies they are not respecting the Cookie Lifetime configuration

Scope: Default Config

Scope: Main Website

Cleared all caches

I then deleted all of the cookies from the admin and logged back in.

In the admin the cookie domain has the leading . (period). Based on the configuration I wouldn't expect that to b set as it was only set for the Main Website.

On the frontend, I deleted all my cookies and reloaded the page. I then added a product to the cart. Notice how the cookie domain doesn't match across all of the cookies. Some have the leading . (period) and others don't.

Also notice that there are two form_key cookies. One with the leading . and one without.

I did confirm that the expires date for mages-messages does follow the configuration - despite my claim when opening this ticket. However I see other cookies don't all have the same expiration. Maybe that is desired???

To really try and highlight this issue, I updated the Main Website configuration to add a leading TEST instead of a leading . to the Cookie Domain.

After clearing the cache in the admin, and then the cookies on the frontend I reloaded the page and added a product to the cart (same testing steps as done above). Look at the differences here. The mage-messages cookie is sill here - but if it were respecting the configuration it wouldn't show as the cookie domain with the leading TEST doesn't match the URL. For that same reason, you'll notice that the PHPSESSID cookie doesn't show here. That cookie respected the configuration change and adds the leading TEST but that doesn't match this domain and thus isn't displayed here. You'll also see the duplicate form_key cookie that I called out in the last test isn't present - that one also respected the leading TEST set in the configuration.

This test clearly highlights that some cookies respect the configuration and some don't.

mad-develop commented 2 years ago

I confirm the issues @PromInc mentioned. I noticed that the JavaScript component for cookies get dot prefix for domain configuration and its configured without leading dot. pic1: grafik

Further investigation shows that in \Magento\Framework\View\Element\Js\Cookie::getDomain() the value get a dot prefix if ip validation fails. That could at least explain the issues with dublicate form_key cookie. pic2: grafik

As for mage-messages cookie: The browser domain gets used because in Magento_Theme::js/view/messages.js domain option is set as empty, which overrides the default settings shown in pic1.

engcom-November commented 2 years ago

Thank you for the update @PromInc . Re-verified the issue again on Magento 2.4-develop branch and the issue is reproducible. On the front-end, After clearing the cache in the admin, and then the cookies on the frontend - added a product to the cart - cookie domain doesn't match across all of the cookies. Some have the leading . (period) and others don't. Hence confirming the issue.

github-jira-sync-bot commented 2 years ago

:white_check_mark: Jira issue https://jira.corp.magento.com/browse/AC-2607 is successfully created for this GitHub issue.

m2-assistant[bot] commented 2 years ago

:white_check_mark: Confirmed by @engcom-November. Thank you for verifying the issue.
Issue Available: @engcom-November, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

oaguilar5 commented 5 months ago

Thank you @engcom-November

I'll try to get an instance when I have time to test, however your screenshot highlights the issue here. In the configuration you set the domain to .magento24.local but in the browser you can see it is only magento24.local (the leading period is not present).

If you look at the pull request I submitted recently you can see that the domain is not being set based on the configuration by Javascript when it creates the cookie. As highlighted here.

There are other configuration values that aren't being set either on the PHP side (the lifetime and path).

I will say that I know an Amasty module was also contributing to the issue on the site I found this issue on (I patched both Magento and Amasty code to resolve my sites issue), however I have analyzed the code to feel that the core Magento code should be improved as well.

Hi @PromInc, We also have Amasty modules installed and having the same issue with success/error messages not showing up across the site with the most common happening on the login screen (i.e. no error if your login is incorrect). We believe it could be the one page checkout which seems to cause issues with other modules. May I ask what your issue and solution was and if you'd be willing to share code or insights on the patch you applied? So far we've tried to set the cookie domain on MessagePlugin.php which correctly adds the leading dot (which was missing before). The mage-messages cookie looks correct but error messages are not appearing. We also noticed a duplicate form key with one of them missing the leading dot in the domain so we tried $.cookieStorage.setConf({path: '/', expires: -1}).set('form_key', null); in messages.js which did actually remove the dup form key without the leading dot but still no messages.

magento / magento2