From 616a265fdf0296d4b6f7557eb839131a9b30c6dc Mon Sep 17 00:00:00 2001
From: Torben Dannhauer <torben@dannhauer.de>
Date: Thu, 9 Jul 2026 22:13:13 +0200
Subject: [PATCH] fix(core): keep Horde_Session::$begin readable after close()
 (imp#88)

Read-only service scripts (the attachment download service, and
potentially other read-only endpoints) call close() right after
session bootstrap. close() flips the legacy Horde_Session shims
`_active` flag to false but never clears the underlying session
data - the modern HordeSession keeps it intact.

Horde_Session::__get(begin) short-circuited to 0 whenever `_active`
was false, so Registry::checkExistingAuth()s `session.max_time`
check (`max_time + $session->begin < time()`) incorrectly rejected
every closed, read-only request once session.max_time was
configured: max_time + 0 is always less than the current Unix
timestamp. That produced "User is not authenticated" on attachment
downloads even for a fully logged-in user.

Read through to the modern HordeSession unconditionally instead -
its data survives close() untouched. Add HordeSessionShimBeginTest
covering both the post-close and never-started cases.
---
 lib/Horde/Session.php                         | 14 ++--
 .../Session/HordeSessionShimBeginTest.php     | 66 +++++++++++++++++++
 2 files changed, 76 insertions(+), 4 deletions(-)
 create mode 100644 test/Unit/Session/HordeSessionShimBeginTest.php

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 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * Copyright 2026 The Horde Project (http://www.horde.org/)
+ *
+ * See the enclosed file LICENSE for license information (LGPL). If you
+ * did not receive this file, see http://www.horde.org/licenses/lgpl21.
+ *
+ * @author Torben Dannhauer <torben@dannhauer.de>
+ */
+
+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);
+    }
+}