diff --git a/lib/Horde/Session.php b/lib/Horde/Session.php index 02159dec..1094d277 100644 --- a/lib/Horde/Session.php +++ b/lib/Horde/Session.php @@ -60,7 +60,7 @@ * @package Core * * @property-read integer $begin The timestamp when this session - * began (0 if session is not active). + * began (0 if unknown). * @property-read boolean $regenerate_due True if session ID is due for * regeneration (since 2.5.0). * @property-read integer $regenerate_interval The regeneration interval @@ -222,9 +222,15 @@ public function __get($name) { switch ($name) { case 'begin': - if (!$this->_active) { - return 0; - } + // Read through to the modern session regardless of + // `_active`. close() (used by read-only service scripts + // such as download/view) flips `_active` to false but + // never clears the underlying data, so the previous + // "!$this->_active -> 0" short-circuit made `begin` (and + // thus Registry::checkExistingAuth()'s max_time check) + // report an unauthenticated session in every closed, + // read-only request once `session.max_time` was + // configured. See horde/imp#88. $begin = $this->modern->getSessionBegin(); if ($begin !== null) { return $begin->getTimestamp(); diff --git a/test/Unit/Session/HordeSessionShimBeginTest.php b/test/Unit/Session/HordeSessionShimBeginTest.php new file mode 100644 index 00000000..84d11d58 --- /dev/null +++ b/test/Unit/Session/HordeSessionShimBeginTest.php @@ -0,0 +1,66 @@ + + */ + +namespace Horde\Core\Test\Unit\Session; + +use Horde\Core\Session\HordeSession; +use Horde\SessionHandler\SessionId; +use Horde_Session; +use PHPUnit\Framework\TestCase; +use ReflectionProperty; + +/** + * Horde_Session::$begin must keep reading through to the modern + * HordeSession even after close() clears the shim's `_active` flag. + * + * Read-only service scripts (download, view) call close() right after + * bootstrap, but close() never clears the underlying session data - + * only session_write_close() runs. Registry::checkExistingAuth() reads + * $session->begin for the `session.max_time` check; if that getter + * collapsed to 0 once `_active` was false, max_time + 0 compares less + * than the current timestamp for any non-zero max_time, incorrectly + * failing auth for every closed, read-only request. See horde/imp#88. + */ +class HordeSessionShimBeginTest extends TestCase +{ + private function hordeSession(): HordeSession + { + return new HordeSession(new SessionId('begin-test'), []); + } + + public function testBeginReadsThroughModernSessionWhenInactive(): void + { + $modern = $this->hordeSession(); + $modern->setSessionBegin(1700000000); + + $shim = new Horde_Session($modern); + + $active = new ReflectionProperty(Horde_Session::class, '_active'); + $active->setAccessible(true); + $active->setValue($shim, false); + + self::assertFalse($shim->isActive()); + self::assertSame(1700000000, $shim->begin); + } + + public function testBeginFallsBackToZeroWhenNeverStarted(): void + { + $shim = new Horde_Session($this->hordeSession()); + + $active = new ReflectionProperty(Horde_Session::class, '_active'); + $active->setAccessible(true); + $active->setValue($shim, false); + + self::assertSame(0, $shim->begin); + } +}