PR-staging

Failure

User tests: Successful: Unsuccessful:

avatar niklas-deworetzki-thm
niklas-deworetzki-thm
26 Jul 2018

Pull Request for #15742

Original issue closes:

Summary of Changes

Moved ini_set out of constructor into JSessionHandlerJoomla::start and added a check to test, if there is an active session, so ini_set doesn't get called. This prevents an error from being thrown, if a PHP session was setup before Joomla! is started.

Additionally added a similar check in JSessionHandlerJoomla::setCookieParams , because the last call inside this function session_set_cookie_params also fails, if there is already a PHP session.

Testing Instructions

You can test this code by putting a session_start() statement somewhere, before the Joomla! application starts. For example in the beginning of your index.php or inside a custom php-application using the Joomla! framework.

Expected result

No warnings or errors.

Actual result

A warning is displayed, telling you, that you can't change cookie parameters when a session is active:
Warning: session_set_cookie_params(): Cannot change session cookie parameters when session is active in xxx/libraries/joomla/session/handler/joomla.php on line 151

Or a warning is displayed, telling you, that you can't change the ini when a session is active:
Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in xxx/libraries/joomla/session/handler/joomla.php on line 48

Documentation Changes Required

Manipulating the PHP session outside of Joomla! leads to Joomla! being unable to setup the session by itself. This prevents the cookie params from being set (with and without the fix).
Perhaps there should be a warning, that you should not mess with the session unless you're knowing what you are doing.

@icampus

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
4.00

avatar niklas-deworetzki-thm niklas-deworetzki-thm - open - 26 Jul 2018
avatar niklas-deworetzki-thm niklas-deworetzki-thm - change - 26 Jul 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Jul 2018
Category Libraries
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 26 Jul 2018

@niklas-deworetzki-thm #15742 is an Pull Request, so you wan't to close this?

avatar jeckodevelopment
jeckodevelopment - comment - 26 Jul 2018

@franz-wohlkoenig i guess it's because of this #15742 (comment)

Phil is not able to fix conflicts these days

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 26 Jul 2018

@jeckodevelopment thanks, so all fine.

avatar niklas-deworetzki-thm
niklas-deworetzki-thm - comment - 30 Jul 2018

I looked into Joomla! 4.0, where this issue also exists. But after adding the two checks from this PR to /libraries/src/Session/Storage/JoomlaStorage.php the application still fails and I'm greeted with following error site:
sessionerror

The reason for this lies within /libraries/vendor/joomla/session/src/Storage/NativeStorage.php:

The method setName on line 403 checks for an active session and terminates

	public function setName(string $name)
	{
		if ($this->isActive())
		{
			throw new \LogicException('Cannot change the name of an active session');
		}

		session_name($name);

		return $this;
	}

So I have two questions now:

  1. Should we add this behavior to the current version instead of ignoring the active session?
  2. Should the current 'fixes' be applied to 4.0 if its intended to fail?
avatar niklas-deworetzki-thm niklas-deworetzki-thm - change - 30 Jul 2018
Labels Added: PR-staging
avatar niklas-deworetzki-thm
niklas-deworetzki-thm - comment - 1 Aug 2018

@wilsonge @mbabker perhaps you can help me, deciding how Joomla! 3.8 should behave, when a php session was started before Joomla! itself is starting?

With this fix Joomla! 3.8 ignores the fact, that it can't change the ini values
But Joomla! 4.0 displays the error message shown above.

avatar wilsonge
wilsonge - comment - 3 Aug 2018

I'm happy for 3.8 to ignore I think - especially as in 3.x we don't have good session management in the CLI scripts - and just to tighten the screw in 4.x

avatar niklas-deworetzki-thm
niklas-deworetzki-thm - comment - 5 Aug 2018

Okay, that's fine. So we just need some testers before we can finally merge this PR


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21260.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 6 Aug 2018

So we just need some testers before we can finally merge this PR

@niklas-deworetzki-thm correct. Every Pull Request need 2 successfully Tests (not by PR-Dev.) before Maintainer decide if PR gets merged.

avatar andrewpthompson
andrewpthompson - comment - 3 Jan 2019

@niklas-deworetzki-thm I have tested this patch on Joomla 3.9.1 and it works as expected.

My interest in it is that it would resolve a longstanding problem (since J3.8) with CiviCRM's cron jobs failing because of Error: Failed to start application: Failed to start the session because headers have already been sent by "/var/www/html/libraries/joomla/session/handler/joomla.php" at line 48.. I used this in part to test the patch: works fine with it applied and broken without.

I am wondering about the other instances of headers_sent() in the Joomla code (or maybe what I'm about to say would be scope creep...). This commit is a good place to see the most relevant instances, i.e. where there is an ini_set() after if (!headers_sent()), and as is the case inJSessionHandlerNative as it currently stands (before this PR), these don't have && session_id() == '' so is there potential for the same problem? I don't know if there is a real problem though, because with this PR's patch applied I can't produce any problem with Joomla's session handler set to database or redis (I didn't test others).

avatar franz-wohlkoenig franz-wohlkoenig - change - 28 Apr 2019
The description was changed
avatar franz-wohlkoenig franz-wohlkoenig - edited - 28 Apr 2019
avatar franz-wohlkoenig franz-wohlkoenig - change - 28 Apr 2019
The description was changed
avatar franz-wohlkoenig franz-wohlkoenig - edited - 28 Apr 2019
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 28 Apr 2019

@niklas-deworetzki-thm can you please comment @andrewpthompson above?

avatar niklas-deworetzki-thm
niklas-deworetzki-thm - comment - 28 Apr 2019

@andrewpthompson I don't know about any other instances of ini_set() in the Joomla code. But having an "unprotected" ini_set() doesn't necessarily cause problems or a crash, since it's a timing issue. As long as no data has been sent, there is nothing to fear from setting ini-variables.

Since those variables should only be changed during Joomlas startup procedure, it is relatively safe to assume no other errors will occur at runtime.

avatar andrewpthompson
andrewpthompson - comment - 29 Apr 2019

Thanks @niklas-deworetzki-thm. That makes sense. I hadn't seen any problem arise from other instances of ini_set() anyway. This patch worked well for me.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 29 Apr 2019
avatar andrewpthompson
andrewpthompson - comment - 1 May 2019

I have tested this item successfully on 1d52b99

First, I reproduced the problem using the CiviCRM extension. CiviCRM has had a longstanding problem (since J3.8) with its cron jobs failing to run. I used this command to test:

wget -O - -q -t 1 'https://localhost/testsite/administrator/components/com_civicrm/civicrm/bin/cron.php?name=civicrm_cron&pass=password&key=mysitekey'

Prior to applying the patch I received this error:

Error: Failed to start application: Failed to start the session because headers have already been sent by "/var/www/html/testsite/libraries/joomla/session/handler/joomla.php" at line 48.

After applying the patch, no error.

No adverse effects seen from the patch. I tested logging in and out of J Admin console and frontend, editing article.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21260.

avatar andrewpthompson
andrewpthompson - comment - 1 May 2019

I have tested this item successfully on 1d52b99

First, I reproduced the problem using the CiviCRM extension. CiviCRM has had a longstanding problem (since J3.8) with its cron jobs failing to run. I used this command to test:

wget -O - -q -t 1 'https://localhost/testsite/administrator/components/com_civicrm/civicrm/bin/cron.php?name=civicrm_cron&pass=password&key=mysitekey'

Prior to applying the patch I received this error:

Error: Failed to start application: Failed to start the session because headers have already been sent by "/var/www/html/membership/libraries/joomla/session/handler/joomla.php" at line 48.

After applying the patch, no error.

No adverse effects seen from the patch. I tested logging in and out of J Admin console and frontend, editing article.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21260.

avatar andrewpthompson andrewpthompson - test_item - 1 May 2019 - Tested successfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 1 May 2019

@PhilETaylor can you please test as Opener of #15742?

avatar PhilETaylor
PhilETaylor - comment - 1 May 2019

@franz-wohlkoenig No. Sorry but I no longer contribute to the Joomla project as is not a safe place for contributors.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 1 May 2019

as is not a safe place for contributors.

didn't get what this mean.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 1 May 2019

@PhilETaylor got it and sorry to read. I try to make this better.

avatar anibalsanchez
anibalsanchez - comment - 15 Nov 2019

It can't be applied:

The file marked for modification does not exist: libraries/joomla/session/handler/joomla.php


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21260.
avatar SharkyKZ
SharkyKZ - comment - 15 Nov 2019

@anibalsanchez did you by chance test this on 4.0? This PR is for 3.9.x.

avatar anibalsanchez
anibalsanchez - comment - 15 Nov 2019

We are testing on J4.

Add a Comment

Login with GitHub to post a comment