Skip to content

Commit 75f17b6

Browse files
nickvergessennickvergessen
authored
Merge pull request #37905 from nextcloud/techdebt/noid/add-attributes-for-remaining-security-annotations
feat(security): Add PHP \Attribute for remaining security annotations
2 parents d601725 + ecb8b55 commit 75f17b6

24 files changed

Lines changed: 1278 additions & 278 deletions

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,15 @@
3737
'OCP\\AppFramework\\Http' => $baseDir . '/lib/public/AppFramework/Http.php',
3838
'OCP\\AppFramework\\Http\\Attribute\\ARateLimit' => $baseDir . '/lib/public/AppFramework/Http/Attribute/ARateLimit.php',
3939
'OCP\\AppFramework\\Http\\Attribute\\AnonRateLimit' => $baseDir . '/lib/public/AppFramework/Http/Attribute/AnonRateLimit.php',
40+
'OCP\\AppFramework\\Http\\Attribute\\AuthorizedAdminSetting' => $baseDir . '/lib/public/AppFramework/Http/Attribute/AuthorizedAdminSetting.php',
4041
'OCP\\AppFramework\\Http\\Attribute\\BruteForceProtection' => $baseDir . '/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php',
42+
'OCP\\AppFramework\\Http\\Attribute\\CORS' => $baseDir . '/lib/public/AppFramework/Http/Attribute/CORS.php',
43+
'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php',
44+
'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php',
45+
'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php',
46+
'OCP\\AppFramework\\Http\\Attribute\\PublicPage' => $baseDir . '/lib/public/AppFramework/Http/Attribute/PublicPage.php',
47+
'OCP\\AppFramework\\Http\\Attribute\\StrictCookiesRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/StrictCookiesRequired.php',
48+
'OCP\\AppFramework\\Http\\Attribute\\SubAdminRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/SubAdminRequired.php',
4149
'OCP\\AppFramework\\Http\\Attribute\\UseSession' => $baseDir . '/lib/public/AppFramework/Http/Attribute/UseSession.php',
4250
'OCP\\AppFramework\\Http\\Attribute\\UserRateLimit' => $baseDir . '/lib/public/AppFramework/Http/Attribute/UserRateLimit.php',
4351
'OCP\\AppFramework\\Http\\ContentSecurityPolicy' => $baseDir . '/lib/public/AppFramework/Http/ContentSecurityPolicy.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,15 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
7070
'OCP\\AppFramework\\Http' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http.php',
7171
'OCP\\AppFramework\\Http\\Attribute\\ARateLimit' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/ARateLimit.php',
7272
'OCP\\AppFramework\\Http\\Attribute\\AnonRateLimit' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/AnonRateLimit.php',
73+
'OCP\\AppFramework\\Http\\Attribute\\AuthorizedAdminSetting' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/AuthorizedAdminSetting.php',
7374
'OCP\\AppFramework\\Http\\Attribute\\BruteForceProtection' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php',
75+
'OCP\\AppFramework\\Http\\Attribute\\CORS' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/CORS.php',
76+
'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php',
77+
'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php',
78+
'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php',
79+
'OCP\\AppFramework\\Http\\Attribute\\PublicPage' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/PublicPage.php',
80+
'OCP\\AppFramework\\Http\\Attribute\\StrictCookiesRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/StrictCookiesRequired.php',
81+
'OCP\\AppFramework\\Http\\Attribute\\SubAdminRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/SubAdminRequired.php',
7482
'OCP\\AppFramework\\Http\\Attribute\\UseSession' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/UseSession.php',
7583
'OCP\\AppFramework\\Http\\Attribute\\UserRateLimit' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/UserRateLimit.php',
7684
'OCP\\AppFramework\\Http\\ContentSecurityPolicy' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/ContentSecurityPolicy.php',

‎lib/private/AppFramework/Middleware/Security/CORSMiddleware.php‎

Lines changed: 45 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,13 @@
3333
use OC\User\Session;
3434
use OCP\AppFramework\Controller;
3535
use OCP\AppFramework\Http;
36+
use OCP\AppFramework\Http\Attribute\CORS;
37+
use OCP\AppFramework\Http\Attribute\PublicPage;
3638
use OCP\AppFramework\Http\JSONResponse;
3739
use OCP\AppFramework\Http\Response;
3840
use OCP\AppFramework\Middleware;
3941
use OCP\IRequest;
42+
use ReflectionMethod;
4043

4144
/**
4245
* This middleware sets the correct CORS headers on a response if the
@@ -81,9 +84,12 @@ public function __construct(IRequest $request,
8184
* @since 6.0.0
8285
*/
8386
public function beforeController($controller, $methodName) {
87+
$reflectionMethod = new ReflectionMethod($controller, $methodName);
88+
8489
// ensure that @CORS annotated API routes are not used in conjunction
8590
// with session authentication since this enables CSRF attack vectors
86-
if ($this->reflector->hasAnnotation('CORS') && (!$this->reflector->hasAnnotation('PublicPage') || $this->session->isLoggedIn())) {
91+
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) &&
92+
(!$this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn())) {
8793
$user = array_key_exists('PHP_AUTH_USER', $this->request->server) ? $this->request->server['PHP_AUTH_USER'] : null;
8894
$pass = array_key_exists('PHP_AUTH_PW', $this->request->server) ? $this->request->server['PHP_AUTH_PW'] : null;
8995

@@ -103,7 +109,28 @@ public function beforeController($controller, $methodName) {
103109
}
104110

105111
/**
106-
* This is being run after a successful controllermethod call and allows
112+
* @template T
113+
*
114+
* @param ReflectionMethod $reflectionMethod
115+
* @param string $annotationName
116+
* @param class-string<T> $attributeClass
117+
* @return boolean
118+
*/
119+
protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {
120+
if ($this->reflector->hasAnnotation($annotationName)) {
121+
return true;
122+
}
123+
124+
125+
if (!empty($reflectionMethod->getAttributes($attributeClass))) {
126+
return true;
127+
}
128+
129+
return false;
130+
}
131+
132+
/**
133+
* This is being run after a successful controller method call and allows
107134
* the manipulation of a Response object. The middleware is run in reverse order
108135
*
109136
* @param Controller $controller the controller that is being called
@@ -114,23 +141,25 @@ public function beforeController($controller, $methodName) {
114141
* @throws SecurityException
115142
*/
116143
public function afterController($controller, $methodName, Response $response) {
117-
// only react if its a CORS request and if the request sends origin and
144+
// only react if it's a CORS request and if the request sends origin and
118145

119-
if (isset($this->request->server['HTTP_ORIGIN']) &&
120-
$this->reflector->hasAnnotation('CORS')) {
121-
// allow credentials headers must not be true or CSRF is possible
122-
// otherwise
123-
foreach ($response->getHeaders() as $header => $value) {
124-
if (strtolower($header) === 'access-control-allow-credentials' &&
125-
strtolower(trim($value)) === 'true') {
126-
$msg = 'Access-Control-Allow-Credentials must not be '.
127-
'set to true in order to prevent CSRF';
128-
throw new SecurityException($msg);
146+
if (isset($this->request->server['HTTP_ORIGIN'])) {
147+
$reflectionMethod = new ReflectionMethod($controller, $methodName);
148+
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class)) {
149+
// allow credentials headers must not be true or CSRF is possible
150+
// otherwise
151+
foreach ($response->getHeaders() as $header => $value) {
152+
if (strtolower($header) === 'access-control-allow-credentials' &&
153+
strtolower(trim($value)) === 'true') {
154+
$msg = 'Access-Control-Allow-Credentials must not be '.
155+
'set to true in order to prevent CSRF';
156+
throw new SecurityException($msg);
157+
}
129158
}
130-
}
131159

132-
$origin = $this->request->server['HTTP_ORIGIN'];
133-
$response->addHeader('Access-Control-Allow-Origin', $origin);
160+
$origin = $this->request->server['HTTP_ORIGIN'];
161+
$response->addHeader('Access-Control-Allow-Origin', $origin);
162+
}
134163
}
135164
return $response;
136165
}

‎lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,13 @@
2626
use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException;
2727
use OC\AppFramework\Utility\ControllerMethodReflector;
2828
use OCP\AppFramework\Controller;
29+
use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
2930
use OCP\AppFramework\Middleware;
3031
use OCP\AppFramework\Utility\ITimeFactory;
3132
use OCP\ISession;
3233
use OCP\IUserSession;
3334
use OCP\User\Backend\IPasswordConfirmationBackend;
35+
use ReflectionMethod;
3436

3537
class PasswordConfirmationMiddleware extends Middleware {
3638
/** @var ControllerMethodReflector */
@@ -68,7 +70,9 @@ public function __construct(ControllerMethodReflector $reflector,
6870
* @throws NotConfirmedException
6971
*/
7072
public function beforeController($controller, $methodName) {
71-
if ($this->reflector->hasAnnotation('PasswordConfirmationRequired')) {
73+
$reflectionMethod = new ReflectionMethod($controller, $methodName);
74+
75+
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'PasswordConfirmationRequired', PasswordConfirmationRequired::class)) {
7276
$user = $this->userSession->getUser();
7377
$backendClassName = '';
7478
if ($user !== null) {
@@ -89,4 +93,24 @@ public function beforeController($controller, $methodName) {
8993
}
9094
}
9195
}
96+
97+
/**
98+
* @template T
99+
*
100+
* @param ReflectionMethod $reflectionMethod
101+
* @param string $annotationName
102+
* @param class-string<T> $attributeClass
103+
* @return boolean
104+
*/
105+
protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {
106+
if (!empty($reflectionMethod->getAttributes($attributeClass))) {
107+
return true;
108+
}
109+
110+
if ($this->reflector->hasAnnotation($annotationName)) {
111+
return true;
112+
}
113+
114+
return false;
115+
}
92116
}

‎lib/private/AppFramework/Middleware/Security/ReloadExecutionMiddleware.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public function afterException($controller, $methodName, \Exception $exception)
5858

5959
return new RedirectResponse($this->urlGenerator->linkToRouteAbsolute(
6060
'core.login.showLoginForm',
61-
['clear' => true] // this param the the code in login.js may be removed when the "Clear-Site-Data" is working in the browsers
61+
['clear' => true] // this param the code in login.js may be removed when the "Clear-Site-Data" is working in the browsers
6262
));
6363
}
6464

‎lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php‎

Lines changed: 61 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,12 @@
4848
use OCP\App\AppPathNotFoundException;
4949
use OCP\App\IAppManager;
5050
use OCP\AppFramework\Controller;
51+
use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting;
52+
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
53+
use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
54+
use OCP\AppFramework\Http\Attribute\PublicPage;
55+
use OCP\AppFramework\Http\Attribute\StrictCookiesRequired;
56+
use OCP\AppFramework\Http\Attribute\SubAdminRequired;
5157
use OCP\AppFramework\Http\JSONResponse;
5258
use OCP\AppFramework\Http\RedirectResponse;
5359
use OCP\AppFramework\Http\Response;
@@ -61,6 +67,7 @@
6167
use OCP\IUserSession;
6268
use OCP\Util;
6369
use Psr\Log\LoggerInterface;
70+
use ReflectionMethod;
6471

6572
/**
6673
* Used to do all the authentication and checking stuff for a controller method
@@ -145,22 +152,24 @@ public function beforeController($controller, $methodName) {
145152
$this->navigationManager->setActiveEntry('spreed');
146153
}
147154

155+
$reflectionMethod = new ReflectionMethod($controller, $methodName);
156+
148157
// security checks
149-
$isPublicPage = $this->reflector->hasAnnotation('PublicPage');
158+
$isPublicPage = $this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class);
150159
if (!$isPublicPage) {
151160
if (!$this->isLoggedIn) {
152161
throw new NotLoggedInException();
153162
}
154163
$authorized = false;
155-
if ($this->reflector->hasAnnotation('AuthorizedAdminSetting')) {
164+
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class)) {
156165
$authorized = $this->isAdminUser;
157166

158-
if (!$authorized && $this->reflector->hasAnnotation('SubAdminRequired')) {
167+
if (!$authorized && $this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)) {
159168
$authorized = $this->isSubAdmin;
160169
}
161170

162171
if (!$authorized) {
163-
$settingClasses = explode(';', $this->reflector->getAnnotationParameter('AuthorizedAdminSetting', 'settings'));
172+
$settingClasses = $this->getAuthorizedAdminSettingClasses($reflectionMethod);
164173
$authorizedClasses = $this->groupAuthorizationMapper->findAllClassesForUser($this->userSession->getUser());
165174
foreach ($settingClasses as $settingClass) {
166175
$authorized = in_array($settingClass, $authorizedClasses, true);
@@ -174,29 +183,30 @@ public function beforeController($controller, $methodName) {
174183
throw new NotAdminException($this->l10n->t('Logged in user must be an admin, a sub admin or gotten special right to access this setting'));
175184
}
176185
}
177-
if ($this->reflector->hasAnnotation('SubAdminRequired')
186+
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
178187
&& !$this->isSubAdmin
179188
&& !$this->isAdminUser
180189
&& !$authorized) {
181190
throw new NotAdminException($this->l10n->t('Logged in user must be an admin or sub admin'));
182191
}
183-
if (!$this->reflector->hasAnnotation('SubAdminRequired')
184-
&& !$this->reflector->hasAnnotation('NoAdminRequired')
192+
if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
193+
&& !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class)
185194
&& !$this->isAdminUser
186195
&& !$authorized) {
187196
throw new NotAdminException($this->l10n->t('Logged in user must be an admin'));
188197
}
189198
}
190199

191200
// Check for strict cookie requirement
192-
if ($this->reflector->hasAnnotation('StrictCookieRequired') || !$this->reflector->hasAnnotation('NoCSRFRequired')) {
201+
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'StrictCookieRequired', StrictCookiesRequired::class) ||
202+
!$this->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) {
193203
if (!$this->request->passesStrictCookieCheck()) {
194204
throw new StrictCookieMissingException();
195205
}
196206
}
197207
// CSRF check - also registers the CSRF token since the session may be closed later
198208
Util::callRegister();
199-
if (!$this->reflector->hasAnnotation('NoCSRFRequired')) {
209+
if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) {
200210
/*
201211
* Only allow the CSRF check to fail on OCS Requests. This kind of
202212
* hacks around that we have no full token auth in place yet and we
@@ -232,6 +242,48 @@ public function beforeController($controller, $methodName) {
232242
}
233243
}
234244

245+
/**
246+
* @template T
247+
*
248+
* @param ReflectionMethod $reflectionMethod
249+
* @param string $annotationName
250+
* @param class-string<T> $attributeClass
251+
* @return boolean
252+
*/
253+
protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {
254+
if (!empty($reflectionMethod->getAttributes($attributeClass))) {
255+
return true;
256+
}
257+
258+
if ($this->reflector->hasAnnotation($annotationName)) {
259+
return true;
260+
}
261+
262+
return false;
263+
}
264+
265+
/**
266+
* @param ReflectionMethod $reflectionMethod
267+
* @return string[]
268+
*/
269+
protected function getAuthorizedAdminSettingClasses(ReflectionMethod $reflectionMethod): array {
270+
$classes = [];
271+
if ($this->reflector->hasAnnotation('AuthorizedAdminSetting')) {
272+
$classes = explode(';', $this->reflector->getAnnotationParameter('AuthorizedAdminSetting', 'settings'));
273+
}
274+
275+
$attributes = $reflectionMethod->getAttributes(AuthorizedAdminSetting::class);
276+
if (!empty($attributes)) {
277+
foreach ($attributes as $attribute) {
278+
/** @var AuthorizedAdminSetting $setting */
279+
$setting = $attribute->newInstance();
280+
$classes[] = $setting->getSettings();
281+
}
282+
}
283+
284+
return $classes;
285+
}
286+
235287
/**
236288
* If an SecurityException is being caught, ajax requests return a JSON error
237289
* response and non ajax requests redirect to the index

‎lib/public/AppFramework/ApiController.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
*/
2424
namespace OCP\AppFramework;
2525

26+
use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
27+
use OCP\AppFramework\Http\Attribute\PublicPage;
2628
use OCP\AppFramework\Http\Response;
2729
use OCP\IRequest;
2830

@@ -70,6 +72,8 @@ public function __construct($appName,
7072
* @PublicPage
7173
* @since 7.0.0
7274
*/
75+
#[NoCSRFRequired]
76+
#[PublicPage]
7377
public function preflightedCors() {
7478
if (isset($this->request->server['HTTP_ORIGIN'])) {
7579
$origin = $this->request->server['HTTP_ORIGIN'];

‎lib/public/AppFramework/AuthPublicShareController.php‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@
2828
*/
2929
namespace OCP\AppFramework;
3030

31+
use OCP\AppFramework\Http\Attribute\BruteForceProtection;
32+
use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
33+
use OCP\AppFramework\Http\Attribute\PublicPage;
34+
use OCP\AppFramework\Http\Attribute\UseSession;
3135
use OCP\AppFramework\Http\RedirectResponse;
3236
use OCP\AppFramework\Http\TemplateResponse;
3337
use OCP\IRequest;
@@ -70,6 +74,8 @@ public function __construct(string $appName,
7074
*
7175
* @since 14.0.0
7276
*/
77+
#[NoCSRFRequired]
78+
#[PublicPage]
7379
public function showAuthenticate(): TemplateResponse {
7480
return new TemplateResponse('core', 'publicshareauth', [], 'guest');
7581
}
@@ -129,7 +135,7 @@ protected function authFailed() {
129135
}
130136

131137
/**
132-
* Function called after successfull authentication
138+
* Function called after successful authentication
133139
*
134140
* You can use this to do some logging for example
135141
*
@@ -147,6 +153,9 @@ protected function authSucceeded() {
147153
*
148154
* @since 14.0.0
149155
*/
156+
#[BruteForceProtection(action: 'publicLinkAuth')]
157+
#[PublicPage]
158+
#[UseSession]
150159
final public function authenticate(string $password = '', string $passwordRequest = 'no', string $identityToken = '') {
151160
// Already authenticated
152161
if ($this->isAuthenticated()) {

0 commit comments

Comments
 (0)