Skip to content

Allow to configure "allowed domains" for CORS on DAV #40537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
susnux wants to merge 6 commits into
base: master
Choose a base branch
from
+1,652 −296
Draft

Allow to configure "allowed domains" for CORS on DAV #40537

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c1fa640
Enabled CORS on webdav and ocs
noveens Jul 21, 2017
0cb950e
Removed beforeController Logic
noveens Aug 30, 2017
e608e5d
fix: Make CORS handling admin configurable and fix tests
susnux Sep 18, 2023
33ae58e
feat: Implement settings frontend for allowed CORS domains
susnux Sep 20, 2023
bcfaa85
chore: Compile assets
susnux Sep 20, 2023
d00b9cd
fix: Resolve some psalm issues
susnux Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
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] owncloud/core#28457
[2] owncloud/core#28864
  • Loading branch information
@noveens @susnux
noveens authored and susnux committed Sep 20, 2023
commit 0cb950e4db100f60de52b17055d79263967f7a8f
80 changes: 21 additions & 59 deletions lib/private/AppFramework/Middleware/Security/CORSMiddleware.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,9 @@

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;
Expand All @@ -58,63 +56,7 @@ public function __construct(private IRequest $request,
}

/**
* 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
Expand Down Expand Up @@ -172,4 +114,24 @@ public function afterException($controller, $methodName, \Exception $exception)

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;
Comment on lines +127 to +135
Copy link
Collaborator

@Altahrim Altahrim Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
if ($this->reflector->hasAnnotation($annotationName)) {
return true;
}
if (!empty($reflectionMethod->getAttributes($attributeClass))) {
return true;
}
return false;
return
$this->reflector->hasAnnotation($annotationName)
|| !empty($reflectionMethod->getAttributes($attributeClass));

}
}
6 changes: 5 additions & 1 deletion lib/private/Route/Router.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ public function findMatchingRoute(string $url): array {
\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
Expand Down Expand Up @@ -340,6 +340,10 @@ public function findMatchingRoute(string $url): array {
*/
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'])) {
Expand Down
156 changes: 0 additions & 156 deletions tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,162 +179,6 @@ public function dataNoCORSOnAnonymousPublicPage(): array {
];
}

/**
* @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' => [
Expand Down