From 6408ed0b51532b5d9c7a8c2664e8d8d88173d6b2 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 17 Sep 2025 15:54:17 +0200 Subject: [PATCH] feat(AppFramework): Add missing NoSameSiteCookieRequired attribute Allow to replace the old annotation. Signed-off-by: Carl Schwan --- .../Controller/PublicPreviewController.php | 4 +- .../lib/Controller/ShareController.php | 11 ++--- .../lib/Controller/ThemingController.php | 4 +- core/Controller/AvatarController.php | 7 ++- core/Controller/ClientFlowLoginController.php | 5 +- .../ClientFlowLoginV2Controller.php | 5 +- core/Controller/CssController.php | 10 ++-- core/Controller/JsController.php | 5 +- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../Attributes/TwoFactorSetUpDoneRequired.php | 2 +- .../Security/SameSiteCookieMiddleware.php | 34 ++++++++++++-- .../Attribute/NoSameSiteCookieRequired.php | 25 ++++++++++ .../Http/Attribute/NoTwoFactorRequired.php | 7 ++- .../Security/SameSiteCookieMiddlewareTest.php | 46 ++++++++++++------- 15 files changed, 115 insertions(+), 52 deletions(-) create mode 100644 lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php index d917f6e0ebb..9056161af84 100644 --- a/apps/files_sharing/lib/Controller/PublicPreviewController.php +++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php @@ -8,6 +8,7 @@ namespace OCA\Files_Sharing\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\DataResponse; @@ -144,8 +145,6 @@ class PublicPreviewController extends PublicShareController { } /** - * @NoSameSiteCookieRequired - * * Get a direct link preview for a shared file * * @param string $token Token of the share @@ -159,6 +158,7 @@ class PublicPreviewController extends PublicShareController { #[PublicPage] #[NoCSRFRequired] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] + #[NoSameSiteCookieRequired] public function directLink(string $token) { // No token no image if ($token === '') { diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index 3c51a95f1d5..ab7556e1af9 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -15,12 +15,12 @@ use OCA\Files_Sharing\Event\ShareLinkAccessedEvent; use OCP\Accounts\IAccountManager; use OCP\AppFramework\AuthPublicShareController; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\NotFoundResponse; use OCP\AppFramework\Http\RedirectResponse; -use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\TemplateResponse; use OCP\Constants; use OCP\Defaults; @@ -343,18 +343,13 @@ class ShareController extends AuthPublicShareController { } /** - * @NoSameSiteCookieRequired - * - * @param string $token - * @param string|null $files - * @param string $path - * @return void|Response * @throws NotFoundException * @deprecated 31.0.0 Users are encouraged to use the DAV endpoint */ #[PublicPage] #[NoCSRFRequired] - public function downloadShare($token, $files = null, $path = '') { + #[NoSameSiteCookieRequired] + public function downloadShare(string $token, ?string $files = null, string $path = ''): NotFoundResponse|RedirectResponse|DataResponse { \OC_User::setIncognitoMode(true); $share = $this->shareManager->getShareByToken($token); diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index a581605c374..467caef40a0 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -17,6 +17,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; use OCP\AppFramework\Http\Attribute\BruteForceProtection; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; @@ -377,8 +378,6 @@ class ThemingController extends Controller { } /** - * @NoSameSiteCookieRequired - * * Get the CSS stylesheet for a theme * * @param string $themeId ID of the theme @@ -393,6 +392,7 @@ class ThemingController extends Controller { #[NoCSRFRequired] #[NoTwoFactorRequired] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] + #[NoSameSiteCookieRequired] public function getThemeStylesheet(string $themeId, bool $plain = false, bool $withCustomCss = false) { $themes = $this->themesService->getThemes(); if (!in_array($themeId, array_keys($themes))) { diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index 1413c9d678e..3d6f79151e8 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -13,6 +13,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\FileDisplayResponse; @@ -50,8 +51,6 @@ class AvatarController extends Controller { } /** - * @NoSameSiteCookieRequired - * * Get the dark avatar * * @param string $userId ID of the user @@ -67,6 +66,7 @@ class AvatarController extends Controller { #[PublicPage] #[FrontpageRoute(verb: 'GET', url: '/avatar/{userId}/{size}/dark')] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] + #[NoSameSiteCookieRequired] public function getAvatarDark(string $userId, int $size, bool $guestFallback = false) { if ($size <= 64) { if ($size !== 64) { @@ -102,8 +102,6 @@ class AvatarController extends Controller { /** - * @NoSameSiteCookieRequired - * * Get the avatar * * @param string $userId ID of the user @@ -119,6 +117,7 @@ class AvatarController extends Controller { #[PublicPage] #[FrontpageRoute(verb: 'GET', url: '/avatar/{userId}/{size}')] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] + #[NoSameSiteCookieRequired] public function getAvatar(string $userId, int $size, bool $guestFallback = false) { if ($size <= 64) { if ($size !== 64) { diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 0e83ef283a9..836bba6d8b7 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -17,6 +17,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\Attribute\PublicPage; @@ -171,13 +172,11 @@ class ClientFlowLoginController extends Controller { return $response; } - /** - * @NoSameSiteCookieRequired - */ #[NoAdminRequired] #[NoCSRFRequired] #[UseSession] #[FrontpageRoute(verb: 'GET', url: '/login/flow/grant')] + #[NoSameSiteCookieRequired] public function grantPage( string $stateToken = '', string $clientIdentifier = '', diff --git a/core/Controller/ClientFlowLoginV2Controller.php b/core/Controller/ClientFlowLoginV2Controller.php index 9a1694a93be..85b82b388f4 100644 --- a/core/Controller/ClientFlowLoginV2Controller.php +++ b/core/Controller/ClientFlowLoginV2Controller.php @@ -18,6 +18,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\Attribute\PublicPage; @@ -142,14 +143,12 @@ class ClientFlowLoginV2Controller extends Controller { ); } - /** - * @NoSameSiteCookieRequired - */ #[NoAdminRequired] #[NoCSRFRequired] #[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)] #[UseSession] #[FrontpageRoute(verb: 'GET', url: '/login/v2/grant')] + #[NoSameSiteCookieRequired] public function grantPage(?string $stateToken, int $direct = 0): StandaloneTemplateResponse { if ($stateToken === null) { return $this->stateTokenMissingResponse(); diff --git a/core/Controller/CssController.php b/core/Controller/CssController.php index 37e7edc530f..b6b0777fe40 100644 --- a/core/Controller/CssController.php +++ b/core/Controller/CssController.php @@ -13,11 +13,11 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\FileDisplayResponse; use OCP\AppFramework\Http\NotFoundResponse; -use OCP\AppFramework\Http\Response; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\IAppData; use OCP\Files\NotFoundException; @@ -41,21 +41,19 @@ class CssController extends Controller { } /** - * @NoSameSiteCookieRequired - * * @param string $fileName css filename with extension * @param string $appName css folder name - * @return FileDisplayResponse|NotFoundResponse */ #[PublicPage] #[NoCSRFRequired] #[FrontpageRoute(verb: 'GET', url: '/css/{appName}/{fileName}')] - public function getCss(string $fileName, string $appName): Response { + #[NoSameSiteCookieRequired] + public function getCss(string $fileName, string $appName): FileDisplayResponse|NotFoundResponse { try { $folder = $this->appData->getFolder($appName); $gzip = false; $file = $this->getFile($folder, $fileName, $gzip); - } catch (NotFoundException $e) { + } catch (NotFoundException) { return new NotFoundResponse(); } diff --git a/core/Controller/JsController.php b/core/Controller/JsController.php index b74e0583683..f214d3043c2 100644 --- a/core/Controller/JsController.php +++ b/core/Controller/JsController.php @@ -13,12 +13,12 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\FileDisplayResponse; use OCP\AppFramework\Http\NotFoundResponse; -use OCP\AppFramework\Http\Response; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\IAppData; use OCP\Files\NotFoundException; @@ -42,8 +42,6 @@ class JsController extends Controller { } /** - * @NoSameSiteCookieRequired - * * @param string $fileName js filename with extension * @param string $appName js folder name */ @@ -51,6 +49,7 @@ class JsController extends Controller { #[NoCSRFRequired] #[FrontpageRoute(verb: 'GET', url: '/js/{appName}/{fileName}')] #[NoTwoFactorRequired] + #[NoSameSiteCookieRequired] public function getJs(string $fileName, string $appName): FileDisplayResponse|NotFoundResponse { try { $folder = $this->appData->getFolder($appName); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 165c7c05cfd..ba20ae33098 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -93,6 +93,7 @@ return array( 'OCP\\AppFramework\\Http\\Attribute\\IgnoreOpenAPI' => $baseDir . '/lib/public/AppFramework/Http/Attribute/IgnoreOpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php', + 'OCP\\AppFramework\\Http\\Attribute\\NoSameSiteCookieRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\NoTwoFactorRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\OpenAPI' => $baseDir . '/lib/public/AppFramework/Http/Attribute/OpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 4dbfb44cb8c..3fb5e222e25 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -134,6 +134,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\AppFramework\\Http\\Attribute\\IgnoreOpenAPI' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/IgnoreOpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php', + 'OCP\\AppFramework\\Http\\Attribute\\NoSameSiteCookieRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\NoTwoFactorRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\OpenAPI' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/OpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php', diff --git a/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php b/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php index f298fff77eb..377d4b959ab 100644 --- a/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php +++ b/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php @@ -12,7 +12,7 @@ namespace OC\AppFramework\Http\Attributes; use Attribute; -#[Attribute] +#[Attribute(Attribute::TARGET_METHOD)] class TwoFactorSetUpDoneRequired { } diff --git a/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php b/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php index 097ed1b2b8f..3bc733931d4 100644 --- a/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php @@ -1,5 +1,7 @@ reflector->hasAnnotation('NoSameSiteCookieRequired'); + $reflectionMethod = new ReflectionMethod($controller, $methodName); + $noSSC = $this->hasAnnotationOrAttribute($reflectionMethod, 'NoSameSiteCookieRequired', NoSameSiteCookieRequired::class); if ($noSSC) { return; } @@ -80,4 +87,25 @@ class SameSiteCookieMiddleware extends Middleware { ); } } + + /** + * @template T + * + * @param ReflectionMethod $reflectionMethod + * @param ?string $annotationName + * @param class-string $attributeClass + * @return boolean + */ + protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, ?string $annotationName, string $attributeClass): bool { + if (!empty($reflectionMethod->getAttributes($attributeClass))) { + return true; + } + + if ($annotationName && $this->reflector->hasAnnotation($annotationName)) { + $this->logger->debug($reflectionMethod->getDeclaringClass()->getName() . '::' . $reflectionMethod->getName() . ' uses the @' . $annotationName . ' annotation and should use the #[' . $attributeClass . '] attribute instead'); + return true; + } + + return false; + } } diff --git a/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php b/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php new file mode 100644 index 00000000000..5bbb65cbe29 --- /dev/null +++ b/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php @@ -0,0 +1,25 @@ +request = $this->createMock(Request::class); $this->reflector = $this->createMock(ControllerMethodReflector::class); - $this->middleware = new SameSiteCookieMiddleware($this->request, $this->reflector); + $this->logger = $this->createMock(LoggerInterface::class); + $this->middleware = new SameSiteCookieMiddleware($this->request, $this->reflector, $this->logger); } public function testBeforeControllerNoIndex(): void { $this->request->method('getScriptName') ->willReturn('/ocs/v2.php'); - $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + $this->middleware->beforeController(new NoAnnotationController('foo', $this->request), 'foo'); $this->addToAssertionCount(1); } @@ -50,7 +64,7 @@ class SameSiteCookieMiddlewareTest extends TestCase { ->with('NoSameSiteCookieRequired') ->willReturn(true); - $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + $this->middleware->beforeController(new HasAnnotationController('foo', $this->request), 'foo'); $this->addToAssertionCount(1); } @@ -65,7 +79,7 @@ class SameSiteCookieMiddlewareTest extends TestCase { $this->request->method('passesLaxCookieCheck') ->willReturn(true); - $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + $this->middleware->beforeController(new NoAnnotationController('foo', $this->request), 'foo'); $this->addToAssertionCount(1); } @@ -82,14 +96,14 @@ class SameSiteCookieMiddlewareTest extends TestCase { $this->request->method('passesLaxCookieCheck') ->willReturn(false); - $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + $this->middleware->beforeController(new NoAnnotationController('foo', $this->request), 'foo'); } public function testAfterExceptionNoLaxCookie(): void { $ex = new SecurityException(); try { - $this->middleware->afterException($this->createMock(Controller::class), 'foo', $ex); + $this->middleware->afterException(new NoAnnotationController('foo', $this->request), 'foo', $ex); $this->fail(); } catch (\Exception $e) { $this->assertSame($ex, $e); @@ -103,14 +117,14 @@ class SameSiteCookieMiddlewareTest extends TestCase { ->willReturn('/myrequri'); $middleware = $this->getMockBuilder(SameSiteCookieMiddleware::class) - ->setConstructorArgs([$this->request, $this->reflector]) + ->setConstructorArgs([$this->request, $this->reflector, $this->logger]) ->onlyMethods(['setSameSiteCookie']) ->getMock(); $middleware->expects($this->once()) ->method('setSameSiteCookie'); - $resp = $middleware->afterException($this->createMock(Controller::class), 'foo', $ex); + $resp = $middleware->afterException(new NoAnnotationController('foo', $this->request), 'foo', $ex); $this->assertSame(Http::STATUS_FOUND, $resp->getStatus());