mirror of
https://github.com/nextcloud/server.git
synced 2026-06-11 09:42:09 -04:00
Removed beforeController Logic
I removed the beforeController logic here due to the change of handling CORS since PR 28457[1]
According to previous implementation, CORS was only allowed with methods that had @PublicPage notation for preventing CSRF attacks.
But in the latest PR by me, the current implementations is as follows:
* maintain a white-list of domains for whom CORS is enabled
* This list can be viewed and edited under settings -> personal -> security
This implementation removes the need for `@PublicPage`[2].
[1] https://github.com/owncloud/core/pull/28457
[2] https://github.com/owncloud/core/pull/28864/
This commit is contained in:
noveens
Ferdinand Thiessen
parent
c1fa6401a3
commit
0cb950e4db
3 changed files with 26 additions and 216 deletions
|
|
@ -28,11 +28,9 @@ namespace OC\AppFramework\Middleware\Security;
|
|||
|
||||
use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
|
||||
use OC\AppFramework\Utility\ControllerMethodReflector;
|
||||
use OC\Authentication\Exceptions\PasswordLoginForbiddenException;
|
||||
use OCP\AppFramework\Controller;
|
||||
use OCP\AppFramework\Http;
|
||||
use OCP\AppFramework\Http\Attribute\CORS;
|
||||
use OCP\AppFramework\Http\Attribute\PublicPage;
|
||||
use OCP\AppFramework\Http\JSONResponse;
|
||||
use OCP\AppFramework\Http\Response;
|
||||
use OCP\AppFramework\Middleware;
|
||||
|
|
@ -58,63 +56,7 @@ class CORSMiddleware extends Middleware {
|
|||
}
|
||||
|
||||
/**
|
||||
* This is being run in normal order before the controller is being
|
||||
* called which allows several modifications and checks
|
||||
*
|
||||
* @param Controller $controller the controller that is being called
|
||||
* @param string $methodName the name of the method that will be called on
|
||||
* the controller
|
||||
* @throws SecurityException
|
||||
* @since 6.0.0
|
||||
*/
|
||||
public function beforeController($controller, $methodName) {
|
||||
$reflectionMethod = new ReflectionMethod($controller, $methodName);
|
||||
|
||||
// ensure that @CORS annotated API routes are not used in conjunction
|
||||
// with session authentication since this enables CSRF attack vectors
|
||||
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) &&
|
||||
(!$this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn())) {
|
||||
$user = array_key_exists('PHP_AUTH_USER', $this->request->server) ? $this->request->server['PHP_AUTH_USER'] : null;
|
||||
$pass = array_key_exists('PHP_AUTH_PW', $this->request->server) ? $this->request->server['PHP_AUTH_PW'] : null;
|
||||
|
||||
// Allow to use the current session if a CSRF token is provided
|
||||
if ($this->request->passesCSRFCheck()) {
|
||||
return;
|
||||
}
|
||||
$this->session->logout();
|
||||
try {
|
||||
if ($user === null || $pass === null || !$this->session->logClientIn($user, $pass, $this->request, $this->throttler)) {
|
||||
throw new SecurityException('CORS requires basic auth', Http::STATUS_UNAUTHORIZED);
|
||||
}
|
||||
} catch (PasswordLoginForbiddenException $ex) {
|
||||
throw new SecurityException('Password login forbidden, use token instead', Http::STATUS_UNAUTHORIZED);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @template T
|
||||
*
|
||||
* @param ReflectionMethod $reflectionMethod
|
||||
* @param string $annotationName
|
||||
* @param class-string<T> $attributeClass
|
||||
* @return boolean
|
||||
*/
|
||||
protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {
|
||||
if ($this->reflector->hasAnnotation($annotationName)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
|
||||
if (!empty($reflectionMethod->getAttributes($attributeClass))) {
|
||||
return true;
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* This is being run after a successful controller method call and allows
|
||||
* This is being run after a successful controllermethod call and allows
|
||||
* the manipulation of a Response object. The middleware is run in reverse order
|
||||
*
|
||||
* @param Controller $controller the controller that is being called
|
||||
|
|
@ -172,4 +114,24 @@ class CORSMiddleware extends Middleware {
|
|||
|
||||
throw $exception;
|
||||
}
|
||||
|
||||
/**
|
||||
* @template T
|
||||
*
|
||||
* @param ReflectionMethod $reflectionMethod
|
||||
* @param string $annotationName
|
||||
* @param class-string<T> $attributeClass
|
||||
* @return boolean
|
||||
*/
|
||||
protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {
|
||||
if ($this->reflector->hasAnnotation($annotationName)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if (!empty($reflectionMethod->getAttributes($attributeClass))) {
|
||||
return true;
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -291,7 +291,7 @@ class Router implements IRouter {
|
|||
\OC_API::respond($response, \OC_API::requestedFormat());
|
||||
|
||||
// Return since no more processing for an OPTIONS request is required
|
||||
return;
|
||||
return [];
|
||||
} catch (ResourceNotFoundException $e) {
|
||||
if (substr($url, -1) !== '/') {
|
||||
// We allow links to apps/files? for backwards compatibility reasons
|
||||
|
|
@ -340,6 +340,10 @@ class Router implements IRouter {
|
|||
*/
|
||||
public function match($url) {
|
||||
$parameters = $this->findMatchingRoute($url);
|
||||
if (\OC::$server->getRequest()->getMethod() === "OPTIONS" && count($parameters) === 0) {
|
||||
// nothing to do here as this is a CORS preflight
|
||||
return;
|
||||
}
|
||||
|
||||
$this->eventLogger->start('route:run', 'Run route');
|
||||
if (isset($parameters['caller'])) {
|
||||
|
|
|
|||
|
|
@ -179,162 +179,6 @@ class CORSMiddlewareTest extends \Test\TestCase {
|
|||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* @dataProvider dataNoCORSOnAnonymousPublicPage
|
||||
*/
|
||||
public function testNoCORSOnAnonymousPublicPage(string $method): void {
|
||||
$request = new Request(
|
||||
[],
|
||||
$this->createMock(IRequestId::class),
|
||||
$this->createMock(IConfig::class)
|
||||
);
|
||||
$this->reflector->reflect($this->controller, $method);
|
||||
$middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config);
|
||||
$this->session->expects($this->once())
|
||||
->method('isLoggedIn')
|
||||
->willReturn(false);
|
||||
$this->session->expects($this->never())
|
||||
->method('logout');
|
||||
$this->session->expects($this->never())
|
||||
->method('logClientIn')
|
||||
->with($this->equalTo('user'), $this->equalTo('pass'))
|
||||
->willReturn(true);
|
||||
$this->reflector->reflect($this->controller, $method);
|
||||
|
||||
$middleware->beforeController($this->controller, $method);
|
||||
}
|
||||
|
||||
public function dataCORSShouldNeverAllowCookieAuth(): array {
|
||||
return [
|
||||
['testCORSShouldNeverAllowCookieAuth'],
|
||||
['testCORSShouldNeverAllowCookieAuthAttribute'],
|
||||
['testCORSAttributeShouldNeverAllowCookieAuth'],
|
||||
['testCORSAttributeShouldNeverAllowCookieAuthAttribute'],
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* @dataProvider dataCORSShouldNeverAllowCookieAuth
|
||||
*/
|
||||
public function testCORSShouldNeverAllowCookieAuth(string $method): void {
|
||||
$request = new Request(
|
||||
[],
|
||||
$this->createMock(IRequestId::class),
|
||||
$this->createMock(IConfig::class)
|
||||
);
|
||||
$this->reflector->reflect($this->controller, $method);
|
||||
$middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config);
|
||||
$this->session->expects($this->once())
|
||||
->method('isLoggedIn')
|
||||
->willReturn(true);
|
||||
$this->session->expects($this->once())
|
||||
->method('logout');
|
||||
$this->session->expects($this->never())
|
||||
->method('logClientIn')
|
||||
->with($this->equalTo('user'), $this->equalTo('pass'))
|
||||
->willReturn(true);
|
||||
|
||||
$this->expectException(SecurityException::class);
|
||||
$middleware->beforeController($this->controller, $method);
|
||||
}
|
||||
|
||||
public function dataCORSShouldRelogin(): array {
|
||||
return [
|
||||
['testCORSShouldRelogin'],
|
||||
['testCORSAttributeShouldRelogin'],
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* @dataProvider dataCORSShouldRelogin
|
||||
*/
|
||||
public function testCORSShouldRelogin(string $method): void {
|
||||
$request = new Request(
|
||||
['server' => [
|
||||
'PHP_AUTH_USER' => 'user',
|
||||
'PHP_AUTH_PW' => 'pass'
|
||||
]],
|
||||
$this->createMock(IRequestId::class),
|
||||
$this->createMock(IConfig::class)
|
||||
);
|
||||
$this->session->expects($this->once())
|
||||
->method('logout');
|
||||
$this->session->expects($this->once())
|
||||
->method('logClientIn')
|
||||
->with($this->equalTo('user'), $this->equalTo('pass'))
|
||||
->willReturn(true);
|
||||
$this->reflector->reflect($this->controller, $method);
|
||||
$middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config);
|
||||
|
||||
$middleware->beforeController($this->controller, $method);
|
||||
}
|
||||
|
||||
public function dataCORSShouldFailIfPasswordLoginIsForbidden(): array {
|
||||
return [
|
||||
['testCORSShouldFailIfPasswordLoginIsForbidden'],
|
||||
['testCORSAttributeShouldFailIfPasswordLoginIsForbidden'],
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* @dataProvider dataCORSShouldFailIfPasswordLoginIsForbidden
|
||||
*/
|
||||
public function testCORSShouldFailIfPasswordLoginIsForbidden(string $method): void {
|
||||
$this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\SecurityException::class);
|
||||
|
||||
$request = new Request(
|
||||
['server' => [
|
||||
'PHP_AUTH_USER' => 'user',
|
||||
'PHP_AUTH_PW' => 'pass'
|
||||
]],
|
||||
$this->createMock(IRequestId::class),
|
||||
$this->createMock(IConfig::class)
|
||||
);
|
||||
$this->session->expects($this->once())
|
||||
->method('logout');
|
||||
$this->session->expects($this->once())
|
||||
->method('logClientIn')
|
||||
->with($this->equalTo('user'), $this->equalTo('pass'))
|
||||
->will($this->throwException(new \OC\Authentication\Exceptions\PasswordLoginForbiddenException));
|
||||
$this->reflector->reflect($this->controller, $method);
|
||||
$middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config);
|
||||
|
||||
$middleware->beforeController($this->controller, $method);
|
||||
}
|
||||
|
||||
public function dataCORSShouldNotAllowCookieAuth(): array {
|
||||
return [
|
||||
['testCORSShouldNotAllowCookieAuth'],
|
||||
['testCORSAttributeShouldNotAllowCookieAuth'],
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* @dataProvider dataCORSShouldNotAllowCookieAuth
|
||||
*/
|
||||
public function testCORSShouldNotAllowCookieAuth(string $method): void {
|
||||
$this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\SecurityException::class);
|
||||
|
||||
$request = new Request(
|
||||
['server' => [
|
||||
'PHP_AUTH_USER' => 'user',
|
||||
'PHP_AUTH_PW' => 'pass'
|
||||
]],
|
||||
$this->createMock(IRequestId::class),
|
||||
$this->createMock(IConfig::class)
|
||||
);
|
||||
$this->session->expects($this->once())
|
||||
->method('logout');
|
||||
$this->session->expects($this->once())
|
||||
->method('logClientIn')
|
||||
->with($this->equalTo('user'), $this->equalTo('pass'))
|
||||
->willReturn(false);
|
||||
$this->reflector->reflect($this->controller, $method);
|
||||
$middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config);
|
||||
|
||||
$middleware->beforeController($this->controller, $method);
|
||||
}
|
||||
|
||||
public function testAfterExceptionWithSecurityExceptionNoStatus() {
|
||||
$request = new Request(
|
||||
['server' => [
|
||||
|
|
|
|||
Loading…
Reference in a new issue