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

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

Open
wants to merge 1 commit into
base: hotfix
Choose a base branch
from

Conversation

chris001
Copy link
Contributor

@chris001 chris001 commented Mar 8, 2024

Fix #10249 and salesagility/SuiteCRM-Core#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

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

Final checklist

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

@SuiteBot
Copy link

SuiteBot commented Mar 8, 2024

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

@chris001 chris001 changed the title Fix #10249 https://github.com/salesagility/SuiteCRM-Core/issues/447 Fix #10249 cookies do not set samesite attribute required by 2024 browsers Mar 9, 2024
@chris001 chris001 changed the title Fix #10249 cookies do not set samesite attribute required by 2024 browsers Fix #10249 cookies do not set samesite attribute required by current browsers Mar 9, 2024
…ed by current browsers

- uses php.ini for default value
- sets samesite attr to value in php ini or strict
- adds required samesite parameter
@serhiisamko091184
Copy link
Contributor

Hi @chris001,

thanks for your PR!

Regards,
Serhii

@serhiisamko091184 serhiisamko091184 added Status: Needs Feature Review Status: Requires Code Review Needs the core team to code review Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member Branch:Hotfix labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Branch:Hotfix Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member Status: Needs Feature Review Status: Requires Code Review Needs the core team to code review
Projects
None yet
3 participants