Fix #10249 cookies do not set `samesite` attribute required by current browsers

chris001 commented 4 months ago

Fix #10249 and https://github.com/salesagility/SuiteCRM-Core/issues/447

Adds samesite default parameter to SugarApplication::setCookie(). Required in modern web applications and current browsers.
Adds default value setting for samesite, in php.ini session.cookie_samesite, to make this value upgrade safe. Without upgrade safe, every upgrade to the application could result in failure to login, when the deployment environment requires a default value other than Strict for samesite on the login related cookies.

Description

Adds optional parameter samesite to the function SugarApplication::setCookie(). Default value is set to Strict. Default value can be changed in php.ini session.cookie_samesite, to make this setting be upgrade safe. Some users who embed Suite in an iFrame of another web application on the top browser tab (e.g. call center) must use None for the application to load, because Suite's URL in its iFrame is a different domain than the main browser tab address URL.

Motivation and Context

See RFC. To prevent Cross Site Request Forgery (CSRF), a hacking technique used to steal Suite and other web application user accounts, current browsers now expect samesite attribute on cookies, or else they'll be set to the default value Lax. Setting this to Strict by default, allows only the Suite application domain to access application cookies while the browser URL is the same as the cookie's. Third parties must not obtain user login cookies, otherwise they could steal user login session, and access private data in the system, etc.

How To Test This

Before this change. Browse in the application in Chrome and Firefox. You'll see console warnings about soon ending support for cookies that do no have the attribute samesite. Checking with an extension "Cookies and Headers Analyzer" should show many cookies used by the application which have "secure" set to "false", and "samesite" unset. After the change. Those browser console warnings should be cleared. Cookies used by the application should be all "secure" set to "true" and "samesite" set to "Strict".

NOTE: for those running Suite on Apache in http mode behind a TLS proxy, this should continue to work, because the attribute "secure" is only set to "true" when the function isSSL() returns true, which should be when the Suite Apache server is set to secure (SSL or TLS) mode, running on https.

NOTE 2: There may be scenarios where setting cookie attribute "samesite" to "Strict" may break third party code, however, there must be other ways of passing information rather than allowing a third party application to have access to the application's user login cookies and have the ability to hijack user login sessions, steal user accounts, and obtain sensitive data stored by the application.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

[x] My code follows the code style of this project found here.
[x] My change requires a change to the documentation.
[x] I have read the How to Contribute guidelines.

SuiteBot commented 4 months ago

This pull request has been mentioned on SuiteCRM. There might be relevant details there:

https://community.suitecrm.com/t/ck-login-id-others-samesite-attribue-missing/92126/2

serhiisamko091184 commented 3 months ago

Hi @chris001,

thanks for your PR!

Regards, Serhii

salesagility / SuiteCRM