From 919c0256dffedd9efbd39614c811186d80f55d5a Mon Sep 17 00:00:00 2001 From: Josh Date: Sat, 18 Jul 2026 12:55:56 -0400 Subject: [PATCH 1/7] refactor(security): clarify controller security middleware checks - Move metadata into clear predicates for cleaner authorization, cookie, and CSRF checks. - Document compatibility behavior and policy rationale. - Clean up outdated comments. - Crucial: Retain original validation order to ensure error exceptions remain identical. Signed-off-by: Josh --- .../Security/SecurityMiddleware.php | 190 +++++++++--------- 1 file changed, 99 insertions(+), 91 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 3b3028c0a20d7..3b50dd7915dcf 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -53,10 +53,11 @@ use ReflectionMethod; /** - * Used to do all the authentication and checking stuff for a controller method - * It reads out the annotations of a controller method and checks which if - * security things should be checked and also handles errors in case a security - * check fails + * Enforces baseline access-control and request-security requirements for controller methods. + * + * Inspects controller-method annotations and attributes to apply authentication, + * authorization, admin IP, strict-cookie, CSRF, and app-availability checks. + * Handles security-check failures by returning an appropriate response. */ class SecurityMiddleware extends Middleware { private ?bool $isAdminUser = null; @@ -97,56 +98,72 @@ private function isSubAdmin(): bool { } /** - * This runs all the security checks before a method call. The - * security checks are determined by inspecting the controller method - * annotations + * Applies access-control and request-security requirements before a controller method runs. * - * @param Controller $controller the controller - * @param string $methodName the name of the method - * @throws SecurityException when a security check fails + * Inspects controller-method annotations and attributes to enforce authentication, + * authorization, admin IP restrictions, strict-cookie requirements, CSRF protection, + * and app availability. + * + * @param Controller $controller The controller instance. + * @param string $methodName The controller method name. + * @throws SecurityException When a security requirement is not met. * * @suppress PhanUndeclaredClassConstant */ #[\Override] public function beforeController($controller, $methodName) { - // this will set the current navigation entry of the app, use this only - // for normal HTML requests and not for AJAX requests - $this->navigationManager->setActiveEntry($this->appName); - + // Mark the appropriate navigation entry as active for responses that render navigation. + $navEntry = $this->appName; /** @psalm-suppress UndefinedClass */ + // Talk call pages belong to Talk's navigation entry rather than the current app. if (get_class($controller) === TalkPageController::class && $methodName === 'showCall') { - $this->navigationManager->setActiveEntry('spreed'); + $navEntry = 'spreed'; } + $this->navigationManager->setActiveEntry($navEntry); $reflectionMethod = new ReflectionMethod($controller, $methodName); - // security checks $isPublicPage = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class); + $isExAppRequired = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'ExAppRequired', ExAppRequired::class); + $requiresSubAdmin = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class); + $requiresAuthorizedAdminSetting = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class); + $doesNotRequireAdmin = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class); + $allowsAppApiAdminAccessWithoutUser = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, null, AppApiAdminAccessWithoutUser::class); + // Keep support for the legacy @StrictCookieRequired annotation while accepting the StrictCookiesRequired attribute. + $requiresStrictCookies = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'StrictCookieRequired', StrictCookiesRequired::class); + $csrfProtectionDisabled = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class); - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'ExAppRequired', ExAppRequired::class)) { - if (!$this->userSession instanceof Session || $this->userSession->getSession()->get('app_api') !== true) { + $requiresAdmin = !$requiresSubAdmin && !$doesNotRequireAdmin; + $requiresPrivilegedAccess = $requiresSubAdmin || $requiresAdmin; + + if ($isExAppRequired) { + if ( + !$this->userSession instanceof Session + || $this->userSession->getSession()->get('app_api') !== true + ) { throw new ExAppRequiredException(); } } elseif (!$isPublicPage) { $authorized = false; - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, null, AppApiAdminAccessWithoutUser::class)) { - // this attribute allows ExApp to access admin endpoints only if "userId" is "null" - if ($this->userSession instanceof Session && $this->userSession->getSession()->get('app_api') === true && $this->userSession->getUser() === null) { - $authorized = true; - } + // Allow an AppAPI request without an associated user to satisfy the route's + // authorization requirement when it explicitly opts in. Admin IP restrictions + // still apply to routes that require privileged access. + $isAppApiRequestWithoutUser = $this->userSession instanceof Session + && $this->userSession->getSession()->get('app_api') === true + && $this->userSession->getUser() === null; + + if ($allowsAppApiAdminAccessWithoutUser && $isAppApiRequestWithoutUser) { + $authorized = true; } if (!$authorized && !$this->isLoggedIn) { throw new NotLoggedInException(); } - if (!$authorized && $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { - $authorized = $this->isAdminUser(); - - if (!$authorized && $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)) { - $authorized = $this->isSubAdmin(); - } + if (!$authorized && $requiresAuthorizedAdminSetting) { + $authorized = $this->isAdminUser() || ($requiresSubAdmin && $this->isSubAdmin()); + // Allow delegated administrators access to settings authorized for their groups. if (!$authorized) { $settingClasses = $this->middlewareUtils->getAuthorizedAdminSettingClasses($reflectionMethod); $authorizedClasses = $this->groupAuthorizationMapper->findAllClassesForUser($this->userSession->getUser()); @@ -158,100 +175,91 @@ public function beforeController($controller, $methodName) { } } } + if (!$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin, a sub admin or gotten special right to access this setting')); } + if (!$this->remoteAddress->allowsAdminActions()) { throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); } } - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) - && !$this->isSubAdmin() - && !$this->isAdminUser() - && !$authorized) { + + if ($requiresSubAdmin && !$this->isSubAdmin() && !$this->isAdminUser() && !$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin or sub admin')); } - if (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) - && !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) - && !$this->isAdminUser() - && !$authorized) { + + if ($requiresAdmin && !$this->isAdminUser() && !$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin')); } - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) - && !$this->remoteAddress->allowsAdminActions()) { - throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); - } - if (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) - && !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) - && !$this->remoteAddress->allowsAdminActions()) { + + if ($requiresPrivilegedAccess && !$this->remoteAddress->allowsAdminActions()) { throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); } - } - // Check for strict cookie requirement - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'StrictCookieRequired', StrictCookiesRequired::class) - || !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) { + if ($requiresStrictCookies || !$csrfProtectionDisabled) { if (!$this->request->passesStrictCookieCheck()) { throw new StrictCookieMissingException(); } } - // CSRF check - also registers the CSRF token since the session may be closed later + + // Generate the session token before controller execution because the session may + // be closed before the response is rendered. Server::get(CsrfTokenManager::class)->generateSessionToken(); - if ($this->isInvalidCSRFRequired($reflectionMethod)) { - /* - * Only allow the CSRF check to fail on OCS Requests. This kind of - * hacks around that we have no full token auth in place yet and we - * do want to offer CSRF checks for web requests. - * - * Additionally we allow Bearer authenticated requests to pass on OCS routes. - * This allows oauth apps (e.g. moodle) to use the OCS endpoints - */ - if (!$controller instanceof OCSController || !$this->isValidOCSRequest()) { - throw new CrossSiteRequestForgeryException(); - } - } - /** - * Checks if app is enabled (also includes a check whether user is allowed to access the resource) - * The getAppPath() check is here since components such as settings also use the AppFramework and - * therefore won't pass this check. - * If page is public, app does not need to be enabled for current user/visitor - */ - try { - $appPath = $this->appManager->getAppPath($this->appName); - } catch (AppPathNotFoundException $e) { - $appPath = false; + // Restrict the OCS compatibility bypass to OCS controllers so regular web + // routes cannot bypass CSRF validation. + if ( + !$csrfProtectionDisabled + && !$this->request->passesCSRFCheck() + && !$this->canBypassCsrfForOcsRequest($controller) + ) { + throw new CrossSiteRequestForgeryException(); } - if ($appPath !== false && !$isPublicPage && !$this->appManager->isEnabledForUser($this->appName)) { - throw new AppNotEnabledException(); + // Per-user app enablement does not apply to public routes. + if ($isPublicPage) { + return; } - } + // Only enforce per-user enablement for real apps. + try { + $this->appManager->getAppPath($this->appName); - private function isInvalidCSRFRequired(ReflectionMethod $reflectionMethod): bool { - if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) { - return false; + if (!$this->appManager->isEnabledForUser($this->appName)) { + throw new AppNotEnabledException(); + } + } catch (AppPathNotFoundException $e) { + // AppFramework consumers that aren't apps do not have an app path. } - - return !$this->request->passesCSRFCheck(); } - private function isValidOCSRequest(): bool { - return $this->request->getHeader('OCS-APIREQUEST') === 'true' - || str_starts_with($this->request->getHeader('Authorization'), 'Bearer '); + /** + * Determines whether a failed CSRF check may be bypassed for an OCS request. + * + * The bypass is restricted to OCS controllers and requests that either identify + * themselves as OCS requests or contain a Bearer authorization header. + */ + private function canBypassCsrfForOcsRequest(Controller $controller): bool { + return $controller instanceof OCSController + && ( + $this->request->getHeader('OCS-APIREQUEST') === 'true' + || stripos($this->request->getHeader('Authorization'), 'Bearer ') === 0 + ); } /** - * If an SecurityException is being caught, ajax requests return a JSON error - * response and non ajax requests redirect to the index + * Converts security exceptions into responses appropriate for the requested format. + * + * Returns a JSON error response for requests that do not accept HTML. For HTML + * requests, redirects unauthenticated users to the login form and renders a + * forbidden response for other security failures. * - * @param Controller $controller the controller that is being called - * @param string $methodName the name of the method that will be called on - * the controller - * @param \Exception $exception the thrown exception - * @return Response a Response object or null in case that the exception could not be handled - * @throws \Exception the passed in exception if it can't handle it + * @param Controller $controller The controller being called. + * @param string $methodName The controller method being called. + * @param \Exception $exception The exception thrown during request handling. + * @return Response The response for a handled security exception. + * @throws \Exception When the exception is not a SecurityException. */ #[\Override] public function afterException($controller, $methodName, \Exception $exception): Response { From 2f8c8d48ca6da2b507796a13cce70ea0cf8e0f10 Mon Sep 17 00:00:00 2001 From: Josh Date: Sat, 18 Jul 2026 13:08:51 -0400 Subject: [PATCH 2/7] chore(security): trim/polish SecurityMiddleware docs Signed-off-by: Josh --- .../Middleware/Security/SecurityMiddleware.php | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 3b50dd7915dcf..ef143d6fddb37 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -57,7 +57,6 @@ * * Inspects controller-method annotations and attributes to apply authentication, * authorization, admin IP, strict-cookie, CSRF, and app-availability checks. - * Handles security-check failures by returning an appropriate response. */ class SecurityMiddleware extends Middleware { private ?bool $isAdminUser = null; @@ -100,10 +99,6 @@ private function isSubAdmin(): bool { /** * Applies access-control and request-security requirements before a controller method runs. * - * Inspects controller-method annotations and attributes to enforce authentication, - * authorization, admin IP restrictions, strict-cookie requirements, CSRF protection, - * and app availability. - * * @param Controller $controller The controller instance. * @param string $methodName The controller method name. * @throws SecurityException When a security requirement is not met. @@ -145,9 +140,8 @@ public function beforeController($controller, $methodName) { } } elseif (!$isPublicPage) { $authorized = false; - // Allow an AppAPI request without an associated user to satisfy the route's - // authorization requirement when it explicitly opts in. Admin IP restrictions - // still apply to routes that require privileged access. + // Explicitly opted-in AppAPI requests without a user bypass normal user and role + // checks; privileged routes still enforce the admin IP restriction. $isAppApiRequestWithoutUser = $this->userSession instanceof Session && $this->userSession->getSession()->get('app_api') === true && $this->userSession->getUser() === null; @@ -218,11 +212,10 @@ public function beforeController($controller, $methodName) { throw new CrossSiteRequestForgeryException(); } - // Per-user app enablement does not apply to public routes. if ($isPublicPage) { return; } - // Only enforce per-user enablement for real apps. + try { $this->appManager->getAppPath($this->appName); @@ -230,7 +223,7 @@ public function beforeController($controller, $methodName) { throw new AppNotEnabledException(); } } catch (AppPathNotFoundException $e) { - // AppFramework consumers that aren't apps do not have an app path. + // Non-app components that use the App Framework lack an app path. } } From 09cea073a4671fed014e41037624507ca04b8158 Mon Sep 17 00:00:00 2001 From: Josh Date: Sat, 18 Jul 2026 13:42:20 -0400 Subject: [PATCH 3/7] fix(AppFramework): polish SecurityMiddleware user-facing messages Align / make more consistent Signed-off-by: Josh --- .../Security/SecurityMiddleware.php | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index ef143d6fddb37..4d804498d6de9 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -171,24 +171,34 @@ public function beforeController($controller, $methodName) { } if (!$authorized) { - throw new NotAdminException($this->l10n->t('Logged in account must be an admin, a sub admin or gotten special right to access this setting')); + throw new NotAdminException($this->l10n->t( + 'You must be an administrator, sub-administrator, or have been granted access to this setting.' + )); } if (!$this->remoteAddress->allowsAdminActions()) { - throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); + throw new AdminIpNotAllowedException($this->l10n->t( + 'Your IP address is not allowed to perform administrative actions.' + )); } } if ($requiresSubAdmin && !$this->isSubAdmin() && !$this->isAdminUser() && !$authorized) { - throw new NotAdminException($this->l10n->t('Logged in account must be an admin or sub admin')); + throw new NotAdminException($this->l10n->t( + 'You must be an administrator or sub-administrator.' + )); } if ($requiresAdmin && !$this->isAdminUser() && !$authorized) { - throw new NotAdminException($this->l10n->t('Logged in account must be an admin')); + throw new NotAdminException($this->l10n->t( + 'You must be an administrator.' + )); } if ($requiresPrivilegedAccess && !$this->remoteAddress->allowsAdminActions()) { - throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); + throw new AdminIpNotAllowedException($this->l10n->t( + 'Your IP address is not allowed to perform administrative actions.' + )); } } From 0f354ccc303706184eb975bc55cc1bcc7b1211ce Mon Sep 17 00:00:00 2001 From: Josh Date: Sat, 18 Jul 2026 13:50:59 -0400 Subject: [PATCH 4/7] chore: update StrictCookieMissingException message Signed-off-by: Josh --- .../Security/Exceptions/StrictCookieMissingException.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php b/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php index 47ed1b6a0905b..c176278e49bad 100644 --- a/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php +++ b/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php @@ -12,13 +12,16 @@ use OCP\AppFramework\Http; /** - * Class StrictCookieMissingException is thrown when the strict cookie has not - * been sent with the request but is required. + * Class StrictCookieMissingException is thrown when a required strict cookie + * is missing from the request. * * @package OC\AppFramework\Middleware\Security\Exceptions */ class StrictCookieMissingException extends SecurityException { public function __construct() { - parent::__construct('Strict Cookie has not been found in request.', Http::STATUS_PRECONDITION_FAILED); + parent::__construct( + 'Required strict cookie is missing from the request.', + Http::STATUS_PRECONDITION_FAILED, + ); } } From b8ac0c426b5414c962730df6a164daeec5b0db75 Mon Sep 17 00:00:00 2001 From: Josh Date: Sat, 18 Jul 2026 13:53:02 -0400 Subject: [PATCH 5/7] chore: update NotLoggedInException message Signed-off-by: Josh --- .../Middleware/Security/Exceptions/NotLoggedInException.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/private/AppFramework/Middleware/Security/Exceptions/NotLoggedInException.php b/lib/private/AppFramework/Middleware/Security/Exceptions/NotLoggedInException.php index dfb8abe3c50f6..24559cf078a0a 100644 --- a/lib/private/AppFramework/Middleware/Security/Exceptions/NotLoggedInException.php +++ b/lib/private/AppFramework/Middleware/Security/Exceptions/NotLoggedInException.php @@ -20,6 +20,9 @@ */ class NotLoggedInException extends SecurityException { public function __construct() { - parent::__construct('Current user is not logged in', Http::STATUS_UNAUTHORIZED); + parent::__construct( + 'You are not logged in.', + Http::STATUS_UNAUTHORIZED, + ); } } From 2ed02ed29995fdc7e80c620b6a9be35eb4aad11d Mon Sep 17 00:00:00 2001 From: Josh Date: Sat, 18 Jul 2026 13:54:24 -0400 Subject: [PATCH 6/7] chore: update AppNotEnabledException message Signed-off-by: Josh --- .../Security/Exceptions/AppNotEnabledException.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php b/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php index 3741c2e8c7bdf..7f196c539eaab 100644 --- a/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php +++ b/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php @@ -20,6 +20,9 @@ */ class AppNotEnabledException extends SecurityException { public function __construct() { - parent::__construct('App is not enabled', Http::STATUS_PRECONDITION_FAILED); + parent::__construct( + 'This app is not enabled.', + Http::STATUS_PRECONDITION_FAILED, + ); } } From 484c55378cf42f58f1cd2fec60ac50d8fa9523cb Mon Sep 17 00:00:00 2001 From: Josh Date: Sat, 18 Jul 2026 13:55:26 -0400 Subject: [PATCH 7/7] chore: update ExAppRequiredException message Signed-off-by: Josh --- .../Security/Exceptions/ExAppRequiredException.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/Exceptions/ExAppRequiredException.php b/lib/private/AppFramework/Middleware/Security/Exceptions/ExAppRequiredException.php index 89ceaefd3dae9..145d4cecb78d2 100644 --- a/lib/private/AppFramework/Middleware/Security/Exceptions/ExAppRequiredException.php +++ b/lib/private/AppFramework/Middleware/Security/Exceptions/ExAppRequiredException.php @@ -12,10 +12,14 @@ use OCP\AppFramework\Http; /** - * Class ExAppRequiredException is thrown when an endpoint can only be called by an ExApp but the caller is not an ExApp. + * Class ExAppRequiredException is thrown when an endpoint can only be called + * by an ExApp but the caller is not an ExApp. */ class ExAppRequiredException extends SecurityException { public function __construct() { - parent::__construct('ExApp required', Http::STATUS_PRECONDITION_FAILED); + parent::__construct( + 'This endpoint can only be accessed by an ExApp.', + Http::STATUS_PRECONDITION_FAILED, + ); } }