nextcloud/server

[Bug]: \OC::$server->getSession()->set('key', 'value') not saved

the-djmaze opened this issue · 13 comments

the-djmaze commented

⚠️ This issue respects the following points: ⚠️

  • This is a bug, not a question or a configuration/webserver/proxy issue.
  • This issue is not already reported on Github (I've searched it).
  • Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
  • Nextcloud Server is running on 64bit capable CPU, PHP and OS.
  • I agree to follow Nextcloud's Code of Conduct.

Bug description

My app is trying to set a custom session value using. But NULL is always returned.

Steps to reproduce

  1. first request set value
\OC::$server->getSession()->set('snappymail-uid', 'value');
  1. second request fetch value
\var_export($ocSession['snappymail-uid']);

Expected behavior

Return 'value' instead of NULL

Installation method

Community Manual installation with Archive

Operating system

RHEL/CentOS

PHP engine version

PHP 7.4

Web server

Apache (supported)

Database engine version

SQlite

Is this bug present after an update or on a fresh install?

Fresh Nextcloud Server install

Are you using the Nextcloud Server Encryption module?

Encryption is Enabled

What user-backends are you using?

  • Default user-backend (database)
  • LDAP/ Active Directory
  • SSO - SAML
  • Other

Configuration report

Not relevant

List of activated Apps

SnappyMail

Nextcloud Signing status

Not relevant

Nextcloud Logs

Not relevant

Additional info

CryptoSessionData::__construct OC\Session\Internal
CryptoSessionData::close by lib/private/AppFramework/Middleware/SessionMiddleware.php#55
CryptoSessionData::isModified => $this->session->set()
CryptoSessionData->set('key', 'value');
CryptoSessionData::__destruct
CryptoSessionData::isModified => $this->session->set()
szaimen commented

Which NC version?

the-djmaze commented

Which NC version?

24.0.6

CarlSchwan commented

You are probably using lib/private/Session/Memory.php instead of lib/private/Session/Internal.php, unfortunately I have no idea how the Internal session is used and it seems only the Memory session are actually used

@juliushaertl do you have an idea why we are using the in memory session handling?

juliushaertl commented

The only scenario I would have in mind is if your call to getSession is actually taking place before the initSession call of Nextcloud.

server/lib/base.php

Line 418 in 11bedf1

public static function initSession() {

Maybe you could add some logging to that to see if that is the case. Otherwise maybe you can provide the link to where the setSession in your apps code doesn't take effect, then I could have a quick look.

the-djmaze commented

The code is executed after the App Controller is called.

Go to: /nextcloud/index.php/apps/snappymail/
appinfo/routes.php has `page#index`
Nextcloud calls `OCA\SnappyMail\Controller\PageController::index()`
That calls SnappyMailHelper::startApp()
startApp() tries to do a check due complications with the Impersonate app

Here is the code block (with ocSession disabled):
https://github.com/the-djmaze/snappymail/blob/39a827aa2e4fb11ea7cbea488b768ac2bee1012b/integrations/nextcloud/snappymail/lib/Util/SnappyMailHelper.php#L104-L127

juliushaertl commented

You may need to add the @UseSession annotation to your controller method then, as otherwise ISession::set is called after the session has been closed for writing already.

https://docs.nextcloud.com/server/latest/developer_manual/basics/controllers.html#reading-and-writing-session-variables

the-djmaze commented

Well that is a problem.
As i also have class Provider implements IProvider but the documentation of search has no @UseSession
https://docs.nextcloud.com/server/latest/developer_manual/digging_deeper/search.html

juliushaertl commented

But why would search need to write the session?

the-djmaze commented

Because of the horrible Impersonate app.

My SnappyMail app has an automatic login system.
When user logs in Nextcloud it stores password encrypted in the session.

When doing a unified search or open the app, it tries to login.
When login succeeds:

  • the unified search shows found emails
  • the app opens without requesting login

Now the impersonate app is used and you impersonate someone:

  • SnappyMail logged in, so unified search shows mail of original Nextcloud login (not the impersonate)
  • SnappyMail logged in so app shows mail of original Nextcloud login (not the impersonate)

To prevent this, i want to:

  • When SnappyMail logged in: store the Nextcloud UID in session
  • Nextcloud UID not the same as in session, then someone uses impersonate
  • Force a logout in this case
  • Try to login with current (impersonate) Nextcloud UID

If the impersonate app properly triggers the logout event OR Nextcloud then all these checks were not needed.

the-djmaze commented

I know that you want to close the session immediately.
Else there will be a lock on the data and simultaneous requests have to wait for the unlock.

However, with simultaneous requests there are issues:

  1. Request 1 loads session data and closes
  2. Request 2 loads session data
  3. Request 2 modifies session data and saves
  4. Request 1 still running but with old session data (not the changed by request 2)

As this issue is always there, a different approach to sessions could be written.

  1. Request 1 loads session data and closes
  2. Request 2 loads session data
  3. Request 2 modifies session data and saves
  4. Request 1 modifies session data
  • Load current session data
  • Set the modified session data
  • Write merged result session data
juliushaertl commented

Ok, so for Nextcloud 25 there would be a way to reopen the session with #32162

Now I don't have much insight into the impersonate app but with the described behavior I would imagine this also has all kind of issues with external storages where the login data is stored in the session and probably other session-related cases.

Maybe @blizzz has any further insight as the main contributor to the impersonate app?

the-djmaze commented

Yes that would be better.

As for Nextcloud my approach for sessions might be easy to implement to improve #32162

class Internal extends Session {
	/**
	 * @param string $name
	 * @throws \Exception
	 */
	public function __construct(string $name) {
		set_error_handler([$this, 'trapError']);
		$this->invoke('session_name', [$name]);
		try {
			$this->startSession();
			$this->invoke('session_abort'); // ADDED, close immediately
		} catch (\Exception $e) {
			setcookie($this->invoke('session_name'), '', -1, \OC::$WEBROOT ?: '/');
		}
		restore_error_handler();
		if (!isset($_SESSION)) {
			throw new \Exception('Failed to start session');
		}
	}

	public function set(string $key, $value) {
		// ADDED
		$closed = $this->sessionClosed;
		$closed && $this->startSession(); // reopen() ?!?
//		$closed && $this->invoke('session_reset'); // possible? no idea


//		$this->validateSession();
		$_SESSION[$key] = $value;

		// ADDED
		$closed && $this->close();
	}

This approach has more IOPS when setting multiple.
So the ->close() could also be done by process/app itself.

This way the session:

  • has shortest lock ever
  • session_abort closes session without write
  • set() reopens session with lock
  • lock stays active until close()

This would also solve my problem if the Impersonate (or other app) provides similar issues.
Then i can write to Session any time i want and (auto)->close() to unlock asap

the-djmaze commented

Oh never mind my response. I looked at the first code but the real solution was later on in the thread.
You all did what i mention using the $this->reopen() 👍

I will use that if Impersonate can't solve this.

  • 👍1