cookie_domain config option for setting cookie on a wider domain #51657

SBizienFilippiPEReN · 2025-03-24T09:47:20Z

Summary

Adds a cookie_domain option to define to which domain(s) the cookies sent by Nextcloud are valid. By default, it is set to '' which is the safe option (i.e. the browser is instructed to send the cookie only for request to the exact same domain that issued it).

But when your instance is accessible over 2 domains, for example 'mycloud.mydomain.example' and 'sub.mycloud.mydomain.example', setting cookie_domain to 'mycloud.mydomain.example' will make the cookie valid for mycloud.mydomain.example and any subdomain (but not for mydomain.example).

Documentation : MDN / Cookies / Define where cookies are sent.

TODO

I've updated config.sample.php, but it's not clear where should this functionality should be documented.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included : not clear if applicable
Screenshots before/after for front-end changes (n.a.)
Documentation (manuals or wiki) has been updated or is not required
Backports not requested where applicable (ex: critical bugfixes)

icewind1991

Looks good otherwise

icewind1991 · 2025-03-25T17:26:37Z

lib/base.php

@@ -398,6 +398,9 @@ public static function initSession(): void {
 		$cookie_path = OC::$WEBROOT ? : '/';
 		ini_set('session.cookie_path', $cookie_path);

+		// set the cookie domain to the Nextcloud domain
+		ini_set('session.cookie_domain', self::$config->getValue('cookie_domain', ''));


might be worth only doing this if the value is set to anything but '' to prevent overwriting any value set by the admin in the php configuration

Fixed. (I amended and force-pushed the new commit : do you prefer individual fix commits for review before a final squash for merge ?)

SBizienFilippiPEReN · 2025-03-28T06:57:20Z

I also wondered if / how this option would be enforced for nextcloud apps. I don't know the codebase, so I can't tell if they will automatically pick this option for apps-defined cookies.

icewind1991 · 2025-03-28T14:39:52Z

Apps should generally not be doing their own cookie logic

come-nc

Please use typed version of the getter when possble (not possible in base.php as it’s not the same config class in use)

come-nc · 2025-03-31T10:19:44Z

lib/private/Session/CryptoWrapper.php

@@ -59,7 +59,7 @@ public function __construct(
 					[
 						'expires' => 0,
 						'path' => $webRoot,
-						'domain' => '',
+						'domain' => $this->config->getSystemValue('cookie_domain', ''),


Suggested change

'domain' => $this->config->getSystemValue('cookie_domain', ''),

'domain' => $this->config->getSystemValueString('cookie_domain'),

Fixed in last commit.

come-nc · 2025-03-31T10:19:53Z

lib/private/User/Session.php

@@ -968,14 +968,15 @@ public function setMagicInCookie($username, $token) {
 		if ($webRoot === '') {
 			$webRoot = '/';
 		}
+		$domain = $this->config->getSystemValue('cookie_domain', '');


Suggested change

$domain = $this->config->getSystemValue('cookie_domain', '');

$domain = $this->config->getSystemValueString('cookie_domain');

come-nc · 2025-03-31T10:20:36Z

lib/private/User/Session.php

@@ -1012,18 +1013,19 @@ public function setMagicInCookie($username, $token) {
 	public function unsetMagicInCookie() {
 		//TODO: DI for cookies and IRequest
 		$secureCookie = OC::$server->getRequest()->getServerProtocol() === 'https';
+		$domain = $this->config->getSystemValue('cookie_domain', '');


Suggested change

$domain = $this->config->getSystemValue('cookie_domain', '');

$domain = $this->config->getSystemValueString('cookie_domain');

github is misbehaving or you forgot to push, but I do not see the update?

Indeed : I forgot to push the changes. Now, it's fixed (at least, I see the fix in github, too).

come-nc · 2025-04-07T14:21:39Z

lib/private/Session/CryptoWrapper.php

@@ -59,7 +59,7 @@ public function __construct(
 					[
 						'expires' => 0,
 						'path' => $webRoot,
-						'domain' => '',
+						'domain' => $this->config->getSystemValueString('cookie_domain'),


As you can see in psalm output, there is no config property in this class.

So you need to either inject it in the constructor, or directly pull it here:

Suggested change

'domain' => $this->config->getSystemValueString('cookie_domain'),

'domain' => \OCP\Server::get(\OCP\IConfig::class)->getSystemValueString('cookie_domain'),

OK! Included.

Signed-off-by: Samuel Bizien Filippi <[email protected]>

github-actions · 2025-04-08T02:04:45Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

SBizienFilippiPEReN requested a review from a team as a code owner March 24, 2025 09:47

SBizienFilippiPEReN requested review from icewind1991, nfebe and come-nc and removed request for a team March 24, 2025 09:47

icewind1991 approved these changes Mar 25, 2025

View reviewed changes

SBizienFilippiPEReN force-pushed the cookie-domain branch from 0add860 to 56074a9 Compare March 28, 2025 06:54

icewind1991 approved these changes Mar 28, 2025

View reviewed changes

SBizienFilippiPEReN force-pushed the cookie-domain branch from 56074a9 to 0f78922 Compare March 31, 2025 06:15

come-nc requested changes Mar 31, 2025

View reviewed changes

SBizienFilippiPEReN force-pushed the cookie-domain branch 2 times, most recently from 6c904c5 to f5a89f0 Compare April 1, 2025 06:32

icewind1991 force-pushed the cookie-domain branch from 12c6c07 to f5a89f0 Compare April 5, 2025 20:49

come-nc requested changes Apr 7, 2025

View reviewed changes

feat(core): add cookie_domain config option

2b03b8e

Signed-off-by: Samuel Bizien Filippi <[email protected]>

SBizienFilippiPEReN force-pushed the cookie-domain branch from f5a89f0 to 2b03b8e Compare April 7, 2025 15:32

github-actions bot added the feedback-requested label Apr 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cookie_domain config option for setting cookie on a wider domain #51657

cookie_domain config option for setting cookie on a wider domain #51657

SBizienFilippiPEReN commented Mar 24, 2025 •

edited

Loading

icewind1991 left a comment

icewind1991 Mar 25, 2025

SBizienFilippiPEReN Mar 28, 2025

SBizienFilippiPEReN commented Mar 28, 2025

icewind1991 commented Mar 28, 2025

come-nc left a comment

come-nc Mar 31, 2025

SBizienFilippiPEReN Mar 31, 2025

come-nc Mar 31, 2025

SBizienFilippiPEReN Mar 31, 2025

come-nc Mar 31, 2025

SBizienFilippiPEReN Mar 31, 2025

come-nc Mar 31, 2025

SBizienFilippiPEReN Apr 7, 2025

come-nc Apr 7, 2025

SBizienFilippiPEReN Apr 7, 2025

github-actions bot commented Apr 8, 2025

	'domain' => $this->config->getSystemValue('cookie_domain', ''),
	'domain' => $this->config->getSystemValueString('cookie_domain'),

	$domain = $this->config->getSystemValue('cookie_domain', '');
	$domain = $this->config->getSystemValueString('cookie_domain');

	'domain' => $this->config->getSystemValueString('cookie_domain'),
	'domain' => \OCP\Server::get(\OCP\IConfig::class)->getSystemValueString('cookie_domain'),

cookie_domain config option for setting cookie on a wider domain #51657

Are you sure you want to change the base?

cookie_domain config option for setting cookie on a wider domain #51657

Conversation

SBizienFilippiPEReN commented Mar 24, 2025 • edited Loading

Summary

TODO

Checklist

icewind1991 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SBizienFilippiPEReN commented Mar 28, 2025

icewind1991 commented Mar 28, 2025

come-nc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 8, 2025

SBizienFilippiPEReN commented Mar 24, 2025 •

edited

Loading