Check for valid session before messing with session ini · Pull Request #21260 · joomla/joomla-cms

ghost · 2018-07-26T09:40:02Z

Pull Request for #15742

Original issue closes:

Error:"A session is active. You cannot change the session module's ini settings at this time" #15680
E_WARNING in libraries/joomla/session/handler/joomla.php - J3.6.2 #12153

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

… is no error thrown.

ghost · 2018-07-26T09:45:36Z

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

jeckodevelopment · 2018-07-26T09:48:57Z

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

Phil is not able to fix conflicts these days

ghost · 2018-07-26T09:53:06Z

@jeckodevelopment thanks, so all fine.

Quy · 2018-07-26T13:30:32Z

libraries/joomla/session/handler/joomla.php

+                ini_set('session.use_only_cookies', 1);
+            }
+        }
+


Remove blank line.

Quy · 2018-07-26T13:31:09Z

libraries/joomla/session/handler/joomla.php

 		{
 			return;
 		}
-


Revert deleting blank line.

Quy · 2018-07-26T13:31:30Z

libraries/joomla/session/handler/joomla.php


-		return parent::start();
+		// Only change ini if there is no active session.
+        if (!headers_sent() && session_id() == '')


Use tabs and not spaces to indent.

brianteeman · 2018-07-26T17:41:29Z

libraries/joomla/session/handler/joomla.php

 	protected function setCookieParams()
 	{
-		if (headers_sent())
+	    // We can't change cookie params, if there is a valid session or headers have already been sent.


might seem pedantic but please remove the , from this comment as it changes the meaning of the sentence.

ghost · 2018-07-30T07:29:24Z

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:

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:

Should we add this behavior to the current version instead of ignoring the active session?
Should the current 'fixes' be applied to 4.0 if its intended to fail?

ghost · 2018-08-01T13:08:31Z

@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.

wilsonge · 2018-08-03T11:09:14Z

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

ghost · 2018-08-05T19:35:32Z

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.}

ghost · 2018-08-06T03:55:34Z

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.

andrewpthompson · 2019-01-03T06:26:19Z

@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).

ghost · 2019-04-28T10:30:05Z

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

ghost · 2019-04-28T11:57:14Z

@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.

andrewpthompson · 2019-04-29T10:42:36Z

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.

ghost · 2019-04-29T12:22:26Z

@andrewpthompson please mark your test as successfully > https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results

andrewpthompson · 2019-05-01T01:36:01Z

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.}

ghost · 2019-05-01T04:10:20Z

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

ghost · 2019-05-01T09:51:10Z

as is not a safe place for contributors.

didn't get what this mean.

ghost · 2019-05-01T10:29:41Z

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

anibalsanchez · 2019-11-15T19:18:29Z

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.}

SharkyKZ · 2019-11-15T19:22:17Z

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

anibalsanchez · 2019-11-15T19:29:00Z

We are testing on J4.

ghost · 2020-11-24T09:49:19Z

@anibalsanchez:

We are testing on J4.

one year later …

laoneo · 2022-03-28T07:25:09Z

As Joomla is an application in itself where a session management is completely handled internally, a session should not be started before invoking an index.php. If you want to start the session before the app is loaded, then you can do in Joomla 4 your own session service provider and start the app with that one, so you have full control over the session. I also really suggest to use the web service API to interact nowadays with Joomla from an external application. So I'm closing as this is expected behavior.

Added checks for an active php session, before changing ini, so there…

71e262f

… is no error thrown.

joomla-cms-bot added the PR-staging label Jul 26, 2018

Quy reviewed Jul 26, 2018

View reviewed changes

libraries/joomla/session/handler/joomla.php

ini_set('session.use_only_cookies', 1);

}

}

Copy link
Copy Markdown

Contributor

Quy Jul 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove blank line.

Quy reviewed Jul 26, 2018

View reviewed changes

libraries/joomla/session/handler/joomla.php

{

return;

}

Copy link
Copy Markdown

Contributor

Quy Jul 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert deleting blank line.

Quy reviewed Jul 26, 2018

View reviewed changes

brianteeman reviewed Jul 26, 2018

View reviewed changes

Reformatted code according to the Joomla! Coding Standard.

1d52b99

ghost mentioned this pull request Aug 1, 2018

4.0 check for valid session before messing with ini #21337

Merged

ghost added the J3 Issue label Apr 5, 2019

ghost removed the J3 Issue label Apr 19, 2019

This comment was marked as abuse.

Sign in to view

zero-24 added PR-3.10-dev and removed PR-staging labels Aug 9, 2021

laoneo closed this Mar 28, 2022

Uh oh!

Conversation

ghost commented Jul 26, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Uh oh!

ghost commented Jul 26, 2018

Uh oh!

jeckodevelopment commented Jul 26, 2018

Uh oh!

ghost commented Jul 26, 2018

Uh oh!

Quy Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

Quy Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

Quy Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

brianteeman Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 30, 2018

Uh oh!

ghost commented Aug 1, 2018

Uh oh!

wilsonge commented Aug 3, 2018

Uh oh!

ghost commented Aug 5, 2018

Uh oh!

ghost commented Aug 6, 2018

Uh oh!

andrewpthompson commented Jan 3, 2019

Uh oh!

ghost commented Apr 28, 2019

Uh oh!

ghost commented Apr 28, 2019

Uh oh!

andrewpthompson commented Apr 29, 2019

Uh oh!

ghost commented Apr 29, 2019

Uh oh!

andrewpthompson commented May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 1, 2019

Uh oh!

This comment was marked as abuse.

ghost commented May 1, 2019

Uh oh!

ghost commented May 1, 2019

Uh oh!

anibalsanchez commented Nov 15, 2019

Uh oh!

SharkyKZ commented Nov 15, 2019

Uh oh!

anibalsanchez commented Nov 15, 2019

Uh oh!

ghost commented Nov 24, 2020

Uh oh!

laoneo commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

ghost commented Jul 26, 2018 •

edited by ghost

Loading

andrewpthompson commented May 1, 2019 •

edited

Loading

laoneo commented Mar 28, 2022 •

edited

Loading