fix(admin-cabinet): handle 404s cleanly, plug menu info leak, polish error UX (#1002)

Three related changes driven by Sentry issue #1002
("Action 'start' was not found on handler 'Session'", 148 events, 4 users).

1. Attach NotFoundPlugin unconditionally.

   DispatcherProvider gated NotFoundPlugin behind
   class_exists(Whoops\Handler\PrettyPageHandler::class), but filp/whoops
   is in require (not require-dev), so the class always exists and the
   plugin was never attached in production. Every non-existent route
   propagated as an unhandled dispatcher exception to Whoops and Sentry.

   NotFoundPlugin itself is also narrowed: it now only forwards
   EXCEPTION_HANDLER_NOT_FOUND / EXCEPTION_ACTION_NOT_FOUND to
   errors/show404 and lets every other exception propagate, so real
   bugs still reach WhoopsErrorHandlerProvider and Sentry.

2. Close information-disclosure via left sidebar.

   Once NotFoundPlugin started working, the error page began rendering
   via the main admin layout, whose partials/leftsidebar iterates
   Elements::getMenu(). The ACL check in SecurityPlugin::isAllowedAction
   fell back to ROLE_ADMINS when no JWT/refreshToken was present
   ("backward compatibility"), so anonymous callers were treated as
   admin and the full module list was rendered into the HTML —
   revealing installed modules, AMI/ARI feature flags, and admin URLs.

   - isAllowedAction(): secure-by-default. If role is null, fall back
     to ROLE_ADMINS only for isLocalHostRequest() (internal workers
     and health checks), otherwise ROLE_GUESTS which the ACL denies.
     Return value now uses (bool) cast — Phalcon 5 isAllowed() returns
     bool, not AclEnum int, which would silently fail strict comparison.
   - Elements::getMenu(): defense-in-depth early return when the caller
     is not authenticated. The menu structure is never iterated and no
     module information is emitted for anonymous requests.

3. Polished, self-contained 404/401/500 pages.

   BaseController now gives ErrorsController a dedicated "error"
   layout and sets LEVEL_AFTER_TEMPLATE so Views/index.volt is NOT
   rendered. That matters because index.volt loads advice-worker,
   user-page-tracker, connection-check-worker and pbx-api-client, and
   those fire REST calls on DOM ready. On a 404 the calls can race
   TokenManager warmup, return 401, and PbxApiClient.handleAuthError
   redirects to /session/index — causing a redirect loop on the error
   page for authenticated users.

   layouts/error.volt is a self-contained HTML document with inline
   style and direct Semantic UI base CSS links. It shows the MikoPBX
   logo, the show40x body, and a branded "Go home" button whose target
   switches between /extensions/index (authenticated) and /session/index
   (anonymous). show404/show401/show500 views were shrunk accordingly.

   ErrorsController now also sets the correct HTTP status code
   (404/401/500) — previously the body said "not found" but the
   response code was 200, which is exactly the kind of thing that
   confused Sentry volume tracking for #1002.

   BaseController additionally skips the "Breadcrumb{Controller}{Action}"
   title assembly for ErrorsController so the title set in
   ErrorsController::initialize ("Oops!") is preserved instead of
   being overwritten with a non-existent translation key
   (e.g. BreadcrumbErrorsshow404).

4. Narrow SecurityPlugin stale-cookie branch.

   SecurityPlugin::beforeDispatch had a pre-existing branch that
   clearAuthCookies() for any authenticated visit to any SessionController
   action except "end". With the NotFoundPlugin fix above, visiting
   /session/start (or any unknown session action) started reaching
   that branch and silently logging authenticated users out before the
   error page rendered. Narrowed to only trigger on the legitimate
   login page (action === 'index'). SessionController only exposes
   indexAction and endAction, so no other flows are affected.

5. Drop debug syslog noise from SecurityPlugin.

   checkUserAuth() and extractRoleFromJwt() emitted LOG_DEBUG messages
   on every request including the request URI, which is both a hot-path
   cost and a modest log-leak. Removed.

Verified end-to-end in a real browser (Playwright) against production
and local Docker, including: anonymous 404, protected-controller
login redirect, real admin login, authenticated 404 in a regular
controller, authenticated visit to /session/start, and a follow-up
check that the user remained authenticated with a 75-item sidebar.

Author: Alexey Portnov

src/AdminCabinet/Controllers/BaseController.php

@@ -94,20 +94,24 @@ protected function prepareView(): void
             $this->view->urlToSupport = 'https://www.mikopbx.com/support/?fromPBX=true';
         }
 
-        // Set the title based on the current action
-        $title = 'MikoPBX';
-        switch ($this->actionName) {
-            case 'index':
-            case 'delete':
-            case 'save':
-            case 'modify':
-            case '*** WITHOUT ACTION ***':
-                $title .= '|' . $this->translation->_("Breadcrumb{$this->controllerName}");
-                break;
-            default:
-                $title .= '|' . $this->translation->_("Breadcrumb{$this->controllerName}{$this->actionName}");
+        // Set the title based on the current action.
+        // ErrorsController sets its own title in initialize() — don't overwrite it
+        // with a non-existent Breadcrumb* translation key.
+        if ($this->controllerClass !== ErrorsController::class) {
+            $title = 'MikoPBX';
+            switch ($this->actionName) {
+                case 'index':
+                case 'delete':
+                case 'save':
+                case 'modify':
+                case '*** WITHOUT ACTION ***':
+                    $title .= '|' . $this->translation->_("Breadcrumb{$this->controllerName}");
+                    break;
+                default:
+                    $title .= '|' . $this->translation->_("Breadcrumb{$this->controllerName}{$this->actionName}");
+            }
+            Tag::setTitle($title);
         }
-        Tag::setTitle($title);
 
         // Set other view variables
         $this->view->t = $this->translation;
@@ -127,7 +131,20 @@ protected function prepareView(): void
         $this->view->PBXName = PbxSettings::getValueByKey(PbxSettings::PBX_NAME);
         $this->view->MetaTegHeadDescription = $this->translation->_('MetaTegHeadDescription');
         $this->view->isExternalModuleController = $this->isExternalModuleController;
-        if ($this->controllerClass!==SessionController::class) {
+        // Layout selection:
+        //   - SessionController      : no template (login page renders itself)
+        //   - ErrorsController       : dedicated self-contained "error" layout.
+        //                              Render stops at LEVEL_AFTER_TEMPLATE so the
+        //                              main Views/index.volt (and all its polling
+        //                              workers) is NOT loaded. This keeps the error
+        //                              page lightweight and avoids stray API calls
+        //                              that would 401 and trigger a redirect loop.
+        //   - everything else        : main admin layout with sidebar/top menu.
+        if ($this->controllerClass === ErrorsController::class) {
+            $this->view->setTemplateAfter('error');
+            $this->view->setRenderLevel(View::LEVEL_AFTER_TEMPLATE);
+            $this->view->isUserAuthenticated = $this->isAuthenticated();
+        } elseif ($this->controllerClass !== SessionController::class) {
             $this->view->setTemplateAfter('main');
         }
 

src/AdminCabinet/Controllers/ErrorsController.php

@@ -31,18 +31,21 @@ public function initialize(): void
 
     public function show404Action(): void
     {
+        $this->response->setStatusCode(404, 'Not Found');
         $this->view->success = false;
         $this->view->message = 'Page not found - 404';
     }
 
     public function show401Action(): void
     {
+        $this->response->setStatusCode(401, 'Unauthorized');
         $this->view->success = false;
         $this->view->message =  'Unauthorized - 401';
     }
 
     public function show500Action(): void
     {
+        $this->response->setStatusCode(500, 'Internal Server Error');
         $this->view->success = false;
         $this->view->message =  'Server error - 500';
     }

src/AdminCabinet/Library/Elements.php

@@ -47,6 +47,7 @@
 use MikoPBX\AdminCabinet\Controllers\SystemDiagnosticController;
 use MikoPBX\AdminCabinet\Controllers\TimeSettingsController;
 use MikoPBX\AdminCabinet\Controllers\UpdateController;
+use MikoPBX\AdminCabinet\Plugins\SecurityPlugin;
 use MikoPBX\AdminCabinet\Providers\SecurityPluginProvider;
 use MikoPBX\Common\Models\PbxExtensionModules;
 use MikoPBX\Common\Models\PbxSettings;
@@ -317,6 +318,12 @@ class Elements extends Injectable
      */
     public function getMenu(): void
     {
+        // Anonymous requests must never see the menu — it leaks installed modules,
+        // feature flags and attack surface. Render nothing for unauthenticated callers.
+        if (!SecurityPlugin::isAuthenticated($this->request, $this->cookies)) {
+            return;
+        }
+
         $resultHtml = '';
         $this->addMenuItemSSHMenu();
         $this->addMenuItemsFromExternalModules();

src/AdminCabinet/Plugins/NotFoundPlugin.php

@@ -29,45 +29,37 @@
 /**
  * NotFoundPlugin
  *
- * Handles not-found controller/actions
+ * Forwards dispatcher "handler/action not found" exceptions to the 404 page.
+ * Other exceptions are left to propagate so WhoopsErrorHandlerProvider can log
+ * them (including to Sentry).
  */
 class NotFoundPlugin extends Injectable
 {
-    /**
-     * This action is executed before perform any action in the application
-     *
-     * @param Event $event
-     * @param MvcDispatcher $dispatcher
-     * @param Exception $exception
-     *
-     * @return bool
-     */
     public function beforeException(
         /** @scrutinizer ignore-unused */ Event $event,
         MvcDispatcher $dispatcher,
         Exception $exception
     ): bool {
-        if ($exception instanceof DispatcherException) {
-            switch ($exception->getCode()) {
-                case DispatcherException::EXCEPTION_HANDLER_NOT_FOUND:
-                case DispatcherException::EXCEPTION_ACTION_NOT_FOUND:
-                    $dispatcher->forward(
-                        [
-                            'controller' => 'errors',
-                            'action'     => 'show404',
-                        ]
-                    );
+        // Return true → Phalcon re-throws the original exception from the
+        // dispatcher loop, so WhoopsErrorHandlerProvider still catches it and
+        // reports to Sentry. Only silently-recoverable 404-type exceptions are
+        // forwarded to the errors/show404 action.
+        if (! $exception instanceof DispatcherException) {
+            return true;
+        }
 
-                    return false;
-            }
+        $code = $exception->getCode();
+        if (
+            $code !== DispatcherException::EXCEPTION_HANDLER_NOT_FOUND
+            && $code !== DispatcherException::EXCEPTION_ACTION_NOT_FOUND
+        ) {
+            return true;
         }
 
-        $dispatcher->forward(
-            [
-                'controller' => 'errors',
-                'action'     => 'show500',
-            ]
-        );
+        $dispatcher->forward([
+            'controller' => 'errors',
+            'action'     => 'show404',
+        ]);
 
         return false;
     }

src/AdminCabinet/Plugins/SecurityPlugin.php

@@ -27,7 +27,6 @@
 use MikoPBX\Common\Providers\AclProvider;
 use MikoPBX\Common\Providers\JwtProvider;
 use MikoPBX\Common\Providers\ManagedCacheProvider;
-use Phalcon\Acl\Enum as AclEnum;
 use Phalcon\Di\Injectable;
 use Phalcon\Events\Event;
 use Phalcon\Mvc\Dispatcher;
@@ -89,13 +88,17 @@ public function beforeDispatch(/** @scrutinizer ignore-unused */ Event $event, D
 
         // Authenticated users: validate access to the requested resource
         if ($isAuthenticated) {
-            // Handle authenticated users on login page (except END action)
-            // This can happen when refreshToken cookie exists but is expired in Redis
-            if ($controllerClass === SessionController::class && strtoupper($action) !== 'END') {
-                // Clear stale cookies to prevent redirect loop
+            // Handle authenticated users on the login page itself.
+            // This covers the edge case where refreshToken cookie exists but the
+            // Redis-side session is gone — we clear the stale cookie so the login
+            // form can be used. Narrowed to the 'index' action only — other Session
+            // actions (end, or any unknown action that will 404) must NOT clear the
+            // cookie, otherwise visiting /session/<anything> logs the user out.
+            if (
+                $controllerClass === SessionController::class
+                && strtolower($action) === 'index'
+            ) {
                 $this->clearAuthCookies();
-
-                // Allow access to login page (don't redirect to home)
                 return true;
             }
 
@@ -132,20 +135,7 @@ public function beforeDispatch(/** @scrutinizer ignore-unused */ Event $event, D
      */
     private function checkUserAuth(): bool
     {
-        $isAuth = self::isAuthenticated($this->request, $this->cookies);
-
-        // Debug logging
-        if (class_exists(\MikoPBX\Core\System\SystemMessages::class)) {
-            $hasBearer = $this->request->getHeader('Authorization') ? 'yes' : 'no';
-            $hasCookie = $this->cookies->has('refreshToken') ? 'yes' : 'no';
-            \MikoPBX\Core\System\SystemMessages::sysLogMsg(
-                __METHOD__,
-                "Auth check: result={$isAuth}, Bearer={$hasBearer}, Cookie={$hasCookie}, URI={$this->request->getURI()}",
-                LOG_DEBUG
-            );
-        }
-
-        return $isAuth;
+        return self::isAuthenticated($this->request, $this->cookies);
     }
 
     /**
@@ -343,22 +333,19 @@ private function forwardTo401Error(Dispatcher $dispatcher): void
      */
     public function isAllowedAction(string $controller, string $action): bool
     {
-        // Extract role from JWT token if Bearer header present
         $role = $this->extractRoleFromJwt();
 
-        // Fallback to admins for localhost requests or initial page loads (backward compatibility)
+        // Secure-by-default: anonymous remote requests are GUESTS (default-deny in ACL).
+        // Localhost keeps ADMINS so internal workers and health checks still work.
         if ($role === null) {
-            $role = AclProvider::ROLE_ADMINS;
+            $role = $this->isLocalHostRequest()
+                ? AclProvider::ROLE_ADMINS
+                : AclProvider::ROLE_GUESTS;
         }
 
         $acl = $this->di->get(AclProvider::SERVICE_NAME);
-        $allowed = $acl->isAllowed($role, $controller, $action);
 
-        if ($allowed != AclEnum::ALLOW) {
-            return false;
-        }
-
-        return true;
+        return (bool) $acl->isAllowed($role, $controller, $action);
     }
 
     /**
@@ -377,15 +364,7 @@ private function extractRoleFromJwt(): ?string
         // 1. Try Bearer header first (AJAX requests)
         $authHeader = $this->request->getHeader('Authorization');
         $role = $jwt->extractRoleFromHeader($authHeader);
-
         if ($role !== null) {
-            if (class_exists(\MikoPBX\Core\System\SystemMessages::class)) {
-                \MikoPBX\Core\System\SystemMessages::sysLogMsg(
-                    __METHOD__,
-                    "Role from Bearer header: {$role}",
-                    LOG_DEBUG
-                );
-            }
             return $role;
         }
 
@@ -394,24 +373,10 @@ private function extractRoleFromJwt(): ?string
             try {
                 $refreshToken = $this->cookies->get('refreshToken')->getValue();
                 if (!empty($refreshToken)) {
-                    $role = $jwt->extractRoleFromRefreshToken($refreshToken);
-                    if (class_exists(\MikoPBX\Core\System\SystemMessages::class)) {
-                        \MikoPBX\Core\System\SystemMessages::sysLogMsg(
-                            __METHOD__,
-                            "Role from refreshToken cookie: " . ($role ?? 'null') . ", token length: " . strlen($refreshToken),
-                            LOG_DEBUG
-                        );
-                    }
-                    return $role;
-                }
-            } catch (\Throwable $e) {
-                if (class_exists(\MikoPBX\Core\System\SystemMessages::class)) {
-                    \MikoPBX\Core\System\SystemMessages::sysLogMsg(
-                        __METHOD__,
-                        "Cookie decryption failed: " . $e->getMessage(),
-                        LOG_DEBUG
-                    );
+                    return $jwt->extractRoleFromRefreshToken($refreshToken);
                 }
+            } catch (\Throwable) {
+                // Cookie decryption failed — treat as unauthenticated.
             }
         }
 

src/AdminCabinet/Providers/DispatcherProvider.php

@@ -30,7 +30,6 @@
 use Phalcon\Di\ServiceProviderInterface;
 use Phalcon\Events\Manager as EventsManager;
 use Phalcon\Mvc\Dispatcher;
-use Whoops\Handler\PrettyPageHandler;
 
 /**
  * DispatcherProvider
@@ -68,14 +67,11 @@ function () {
                 $eventsManager->attach('dispatch:beforeDispatch', new SecurityPlugin());
 
                 /**
-                 * Handle exceptions and not-found exceptions using NotFoundPlugin
+                 * Forward unknown controllers/actions to the 404 page.
+                 * Other exceptions propagate to WhoopsErrorHandlerProvider so
+                 * they still reach Sentry and the global error handler.
                  */
-                if (! class_exists(PrettyPageHandler::class)) {
-                    $eventsManager->attach(
-                        'dispatch:beforeException',
-                        new NotFoundPlugin()
-                    );
-                }
+                $eventsManager->attach('dispatch:beforeException', new NotFoundPlugin());
                 $dispatcher = new Dispatcher();
                 $dispatcher->setEventsManager($eventsManager);
 

src/AdminCabinet/Views/Errors/show401.volt

@@ -1,15 +1,7 @@
-<div class="sixteen wide column">
-    {{ content() }}
-
-
-    <div class="ui warning message">
-        <i class="close icon"></i>
-        <div class="header">
-            {{ t._('er_Unauthorized') }}
-        </div>
-        {{ t._("er_UnauthorizedDescription") }}
+<h2 class="ui center aligned icon header">
+    <i class="lock icon"></i>
+    <div class="content">
+        {{ t._('er_Unauthorized') }}
+        <div class="sub header">{{ t._('er_UnauthorizedDescription') }}</div>
     </div>
-
-    {{ link_to('session/end', t._('er_Home'), 'class': 'ui button') }}
-
-</div>
+</h2>

src/AdminCabinet/Views/Errors/show404.volt

@@ -1,16 +1,7 @@
-<div class="sixteen wide column">
-    {{ content() }}
-
-
-    <div class="ui warning message">
-        <i class="close icon"></i>
-        <div class="header">
-            {{ t._('er_PageNotFound') }}
-        </div>
-        {{ t._('er_PageNotFoundDescription') }}
-
+<h2 class="ui center aligned icon header">
+    <i class="question circle outline icon"></i>
+    <div class="content">
+        {{ t._('er_PageNotFound') }}
+        <div class="sub header">{{ t._('er_PageNotFoundDescription') }}</div>
     </div>
-
-    {{ link_to('index', t._('er_Home'), 'class': 'ui button') }}
-
-</div>
+</h2>

src/AdminCabinet/Views/Errors/show500.volt

@@ -1,11 +1,7 @@
-<div class="sixteen wide column">
-    {{ content() }}
-    <div class="ui warning message">
-        <i class="close icon"></i>
-        <div class="header">
-            {{ t._('er_InternalServerError') }}
-        </div>
-        {{ t._('er_InternalServerErrorDescription') }}
+<h2 class="ui center aligned icon header">
+    <i class="exclamation triangle icon"></i>
+    <div class="content">
+        {{ t._('er_InternalServerError') }}
+        <div class="sub header">{{ t._('er_InternalServerErrorDescription') }}</div>
     </div>
-    {{ link_to('index', t._('er_Home'), 'class': 'ui button') }}
-</div>
+</h2>