Fix incorrect setting of the SameSite cookie param

[ihor-sviziev]

Description (*)

• Fix incorrect concatenation for the cookie params that lead to appending the lex suffix to the value, domain, or other params.
• Add support of the samesite param in places that didn't set it at all

/CC @hostep

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes SameSite cookie, posible issues #26377
2. Fixes cookie lax append issue magento 2.4.2 upgrade enterprise b2b #32440
3. Replaces Fix syntax error in jQuery cookie when samesite option not set #32266

Manual testing scenarios (*)

1. ...
2. ...

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @ihor-sviziev. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

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

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

[ihor-sviziev]

@magento run all tests

[Den4ik]

Hi, @ihor-sviziev. Thanks for the fixes.
Please, look at my suggestions, that doesn't change the main logic implemented by your PR, but improve code style

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

I'll just add some info about reproducing this issue that we discussed with @engcom-Foxtrot in Slack:

Example 1:

1. Go to frontend, open developer tools
2. Add breakpoint before document.cookie in the mage/cookie.js file.
3. Run in console following code:

jQuery.mage.cookies.set('test', 'value', {samesite: null})

4. See what going to be set in document.cookie

Actual result (before this PR)
❌ added 'lax' to the end of cookie domain; no samesite attribute

Expected result (with this PR)
✔ "lax" shouldn't be added to the cookie domain; the samesite attribute should be present with default value - "lax"

Example 2:

1. Go to frontend, open developer tools
2. Add breakpoint before document.cookie in the jquery.cookie.js file.
3. Run in console following code:

jQuery.cookie('foo', 'bar', {domain: 'foobar.com', samesite: null})

4. See what going to be set in document.cookie

Actual result (before this PR)
the output looks like this:

"foo=bar; path=/; domain=foobar.comlax"

❌ added 'lax' to the end of cookie domain; no samesite attribute

Expected result (with this PR)
✔ "lax" shouldn't be added to the cookie domain; the samesite attribute should be present with default value - "lax"

[mrtuvn]

Should we change capitalize first letter of 'lax' ? Change to 'Lax'
Some document i saw they used Lax

[ihor-sviziev]

@mrtuvn according to RFC - it's case insensitive:
https://tools.ietf.org/id/draft-ietf-httpbis-rfc6265bis-03.html#the-samesite-attribute-1

[m2-assistant]

Hi @ihor-sviziev, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[zaietsv]

While waiting for a regular fix I can propose an easy way to workaround a "}lax" problem in cookies. As for me I don't like to penetrate deep into such complicated materials like overriding core jquery libraries. So I suggest to override a phtml template.

For a hot-fix it is necessary to customize at your theme either at any other module a template vendor/magento/module-cookie/view/base/templates/html/cookie.phtml . You need to add to the template a new row
window.cookiesConfig.samesite = 'lax';
which initializes a "samesite" value at a storefront.

http://screenshots.collabstar.com/vza/Selection_d75f109.png

This will initialize a previously undefined field "_samesite" at a frontend script vendor/magento/module-cookie/view/base/web/js/jquery.storageapi.extended.js
_samesite: window.cookiesConfig ? window.cookiesConfig.samesite : 'lax',

http://screenshots.collabstar.com/vza/Selection_53b8cb2.png

The above mentioned operation allows to bypass a buggy part of a statement
options.samesite ? '; samesite=' + options.samesite : 'lax'
at a script lib/web/jquery/jquery.cookie.js .
http://screenshots.collabstar.com/vza/Selection_255d05c.png

Since a "false" branch of the statement is incorrect and a "true" branch is OK we only need to define an argument "options.samesite". This allows to implement a positive scenario and omit a wrong "lax" assignment.

Of course one can implement the fix in a gentle way by providing additional arguments to a block
block class="Magento\Framework\View\Element\Js\Cookie" name="cookie_config"
at a layout.

Have a nice fixing!

[ihor-sviziev]

Just note the fix also available in the quality patches tool as MC-41359.

[salehawal]

@ihor-sviziev

i have already pulled and merged the latest updates, and seems that your fix is already applied, but the issue still active, the only thing i need to know now where the cookie is coming from in order to add "secure" at the end.

[ihor-sviziev]

Hi @salehawal,
Could you create a separate issue with the information on how to reproduce and check if the issue reproducing on the test instance?

[salehawal]

@ihor-sviziev
nothing is different than this post, and below is the steps i did to produce the issue:
1- installed magento
2- added ssl on localhost
3- setup admin to be secure

i am facing this issue on the admin only, the site is working fine

[ihor-sviziev]

@salehawal, again, it's not clear to me what are steps to reproduce, actual and expected results. Are the cookies getting set with js or with backend? Is it reproducing on clean magento installation? (It might be cause by some 3rd party extension).
Please, report the separate issue just to separate a bit this PR that was fixing exactly the listed issues from the issue you have. It might be a bit different case