diff options
-rw-r--r-- | apps/provisioning_api/lib/Controller/AppConfigController.php | 7 | ||||
-rw-r--r-- | apps/provisioning_api/tests/Controller/AppConfigControllerTest.php | 27 | ||||
-rw-r--r-- | apps/settings/lib/Controller/AppSettingsController.php | 11 | ||||
-rw-r--r-- | apps/theming/tests/UtilTest.php | 31 | ||||
-rw-r--r-- | core/Command/L10n/CreateJs.php | 7 | ||||
-rw-r--r-- | lib/private/App/AppManager.php | 9 | ||||
-rw-r--r-- | lib/private/Authentication/TwoFactorAuth/ProviderLoader.php | 30 | ||||
-rw-r--r-- | lib/private/L10N/Factory.php | 2 | ||||
-rw-r--r-- | lib/private/Route/Router.php | 26 | ||||
-rw-r--r-- | lib/private/legacy/OC_App.php | 3 | ||||
-rw-r--r-- | lib/public/App/IAppManager.php | 16 | ||||
-rw-r--r-- | remote.php | 19 | ||||
-rw-r--r-- | tests/apps.php | 11 | ||||
-rw-r--r-- | tests/lib/App/AppManagerTest.php | 4 | ||||
-rw-r--r-- | tests/lib/InfoXmlTest.php | 12 |
15 files changed, 115 insertions, 100 deletions
diff --git a/apps/provisioning_api/lib/Controller/AppConfigController.php b/apps/provisioning_api/lib/Controller/AppConfigController.php index 5061a0ffc8f..bb161f6cb8c 100644 --- a/apps/provisioning_api/lib/Controller/AppConfigController.php +++ b/apps/provisioning_api/lib/Controller/AppConfigController.php @@ -10,6 +10,7 @@ namespace OCA\Provisioning_API\Controller; use OC\AppConfig; use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException; +use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; @@ -35,6 +36,7 @@ class AppConfigController extends OCSController { private IL10N $l10n, private IGroupManager $groupManager, private IManager $settingManager, + private IAppManager $appManager, ) { parent::__construct($appName, $request); } @@ -171,11 +173,10 @@ class AppConfigController extends OCSController { } /** - * @param string $app * @throws \InvalidArgumentException */ - protected function verifyAppId(string $app) { - if (\OC_App::cleanAppId($app) !== $app) { + protected function verifyAppId(string $app): void { + if ($this->appManager->cleanAppId($app) !== $app) { throw new \InvalidArgumentException('Invalid app id given'); } } diff --git a/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php b/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php index 0bdee39cebc..23a1f2ee195 100644 --- a/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php +++ b/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php @@ -7,6 +7,7 @@ namespace OCA\Provisioning_API\Tests\Controller; use OC\AppConfig; use OCA\Provisioning_API\Controller\AppConfigController; +use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; use OCP\Exceptions\AppConfigUnknownKeyException; @@ -17,6 +18,7 @@ use OCP\IRequest; use OCP\IUser; use OCP\IUserSession; use OCP\Settings\IManager; +use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; use function json_decode; use function json_encode; @@ -28,16 +30,12 @@ use function json_encode; */ class AppConfigControllerTest extends TestCase { - /** @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject */ - private $appConfig; - /** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */ - private $userSession; - /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */ - private $l10n; - /** @var IManager|\PHPUnit\Framework\MockObject\MockObject */ - private $settingManager; - /** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */ - private $groupManager; + private IAppConfig&MockObject $appConfig; + private IUserSession&MockObject $userSession; + private IL10N&MockObject $l10n; + private IManager&MockObject $settingManager; + private IGroupManager&MockObject $groupManager; + private IAppManager $appManager; protected function setUp(): void { parent::setUp(); @@ -45,8 +43,9 @@ class AppConfigControllerTest extends TestCase { $this->appConfig = $this->createMock(AppConfig::class); $this->userSession = $this->createMock(IUserSession::class); $this->l10n = $this->createMock(IL10N::class); - $this->groupManager = $this->createMock(IGroupManager::class); $this->settingManager = $this->createMock(IManager::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->appManager = \OCP\Server::get(IAppManager::class); } /** @@ -64,7 +63,8 @@ class AppConfigControllerTest extends TestCase { $this->userSession, $this->l10n, $this->groupManager, - $this->settingManager + $this->settingManager, + $this->appManager, ); } else { return $this->getMockBuilder(AppConfigController::class) @@ -75,7 +75,8 @@ class AppConfigControllerTest extends TestCase { $this->userSession, $this->l10n, $this->groupManager, - $this->settingManager + $this->settingManager, + $this->appManager, ]) ->setMethods($methods) ->getMock(); diff --git a/apps/settings/lib/Controller/AppSettingsController.php b/apps/settings/lib/Controller/AppSettingsController.php index 760584888c0..2c682ac4600 100644 --- a/apps/settings/lib/Controller/AppSettingsController.php +++ b/apps/settings/lib/Controller/AppSettingsController.php @@ -14,7 +14,6 @@ use OC\App\AppStore\Version\VersionParser; use OC\App\DependencyAnalyzer; use OC\App\Platform; use OC\Installer; -use OC_App; use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; use OCP\AppFramework\Controller; @@ -479,7 +478,7 @@ class AppSettingsController extends Controller { $updateRequired = false; foreach ($appIds as $appId) { - $appId = OC_App::cleanAppId($appId); + $appId = $this->appManager->cleanAppId($appId); // Check if app is already downloaded /** @var Installer $installer */ @@ -537,7 +536,7 @@ class AppSettingsController extends Controller { public function disableApps(array $appIds): JSONResponse { try { foreach ($appIds as $appId) { - $appId = OC_App::cleanAppId($appId); + $appId = $this->appManager->cleanAppId($appId); $this->appManager->disableApp($appId); } return new JSONResponse([]); @@ -553,7 +552,7 @@ class AppSettingsController extends Controller { */ #[PasswordConfirmationRequired] public function uninstallApp(string $appId): JSONResponse { - $appId = OC_App::cleanAppId($appId); + $appId = $this->appManager->cleanAppId($appId); $result = $this->installer->removeApp($appId); if ($result !== false) { $this->appManager->clearAppsCache(); @@ -567,7 +566,7 @@ class AppSettingsController extends Controller { * @return JSONResponse */ public function updateApp(string $appId): JSONResponse { - $appId = OC_App::cleanAppId($appId); + $appId = $this->appManager->cleanAppId($appId); $this->config->setSystemValue('maintenance', true); try { @@ -594,7 +593,7 @@ class AppSettingsController extends Controller { } public function force(string $appId): JSONResponse { - $appId = OC_App::cleanAppId($appId); + $appId = $this->appManager->cleanAppId($appId); $this->appManager->ignoreNextcloudRequirementForApp($appId); return new JSONResponse(); } diff --git a/apps/theming/tests/UtilTest.php b/apps/theming/tests/UtilTest.php index 4c9f9f10c97..474daab6030 100644 --- a/apps/theming/tests/UtilTest.php +++ b/apps/theming/tests/UtilTest.php @@ -18,22 +18,17 @@ use Test\TestCase; class UtilTest extends TestCase { - /** @var Util */ - protected $util; - /** @var IConfig|MockObject */ - protected $config; - /** @var IAppData|MockObject */ - protected $appData; - /** @var IAppManager|MockObject */ - protected $appManager; - /** @var ImageManager|MockObject */ - protected $imageManager; + protected Util $util; + protected IConfig&MockObject $config; + protected IAppData&MockObject $appData; + protected IAppManager $appManager; + protected ImageManager&MockObject $imageManager; protected function setUp(): void { parent::setUp(); $this->config = $this->createMock(IConfig::class); $this->appData = $this->createMock(IAppData::class); - $this->appManager = $this->createMock(IAppManager::class); + $this->appManager = \OCP\Server::get(IAppManager::class); $this->imageManager = $this->createMock(ImageManager::class); $this->util = new Util($this->config, $this->appManager, $this->appData, $this->imageManager); } @@ -152,19 +147,15 @@ class UtilTest extends TestCase { ->method('getFolder') ->with('global/images') ->willThrowException(new NotFoundException()); - $this->appManager->expects($this->once()) - ->method('getAppPath') - ->with($app) - ->willReturn(\OC_App::getAppPath($app)); $icon = $this->util->getAppIcon($app); $this->assertEquals($expected, $icon); } public function dataGetAppIcon() { return [ - ['user_ldap', \OC_App::getAppPath('user_ldap') . '/img/app.svg'], + ['user_ldap', \OCP\Server::get(IAppManager::class)->getAppPath('user_ldap') . '/img/app.svg'], ['noapplikethis', \OC::$SERVERROOT . '/core/img/logo/logo.svg'], - ['comments', \OC_App::getAppPath('comments') . '/img/comments.svg'], + ['comments', \OCP\Server::get(IAppManager::class)->getAppPath('comments') . '/img/comments.svg'], ]; } @@ -187,12 +178,6 @@ class UtilTest extends TestCase { * @dataProvider dataGetAppImage */ public function testGetAppImage($app, $image, $expected) { - if ($app !== 'core') { - $this->appManager->expects($this->once()) - ->method('getAppPath') - ->with($app) - ->willReturn(\OC_App::getAppPath($app)); - } $this->assertEquals($expected, $this->util->getAppImage($app, $image)); } diff --git a/core/Command/L10n/CreateJs.php b/core/Command/L10n/CreateJs.php index 517016b7e16..27d537b093e 100644 --- a/core/Command/L10n/CreateJs.php +++ b/core/Command/L10n/CreateJs.php @@ -11,6 +11,7 @@ namespace OC\Core\Command\L10n; use DirectoryIterator; +use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; use Stecman\Component\Symfony\Console\BashCompletion\Completion\CompletionAwareInterface; use Stecman\Component\Symfony\Console\BashCompletion\CompletionContext; @@ -150,7 +151,11 @@ class CreateJs extends Command implements CompletionAwareInterface { return \OC_App::getAllApps(); } elseif ($argumentName === 'lang') { $appName = $context->getWordAtIndex($context->getWordIndex() - 1); - return $this->getAllLanguages(\OC_App::getAppPath($appName)); + try { + return $this->getAllLanguages($this->appManager->getAppPath($appName)); + } catch(AppPathNotFoundException) { + return []; + } } return []; } diff --git a/lib/private/App/AppManager.php b/lib/private/App/AppManager.php index fe2f7b74b22..974545cfe92 100644 --- a/lib/private/App/AppManager.php +++ b/lib/private/App/AppManager.php @@ -647,11 +647,9 @@ class AppManager implements IAppManager { /** * Get the directory for the given app. * - * @param string $appId - * @return string * @throws AppPathNotFoundException if app folder can't be found */ - public function getAppPath($appId) { + public function getAppPath(string $appId): string { $appPath = \OC_App::getAppPath($appId); if ($appPath === false) { throw new AppPathNotFoundException('Could not find path for ' . $appId); @@ -877,4 +875,9 @@ class AppManager implements IAppManager { return false; } + + public function cleanAppId(string $app): string { + // FIXME should list allowed characters instead + return str_replace(['<', '>', '"', "'", '\0', '/', '\\', '..'], '', $app); + } } diff --git a/lib/private/Authentication/TwoFactorAuth/ProviderLoader.php b/lib/private/Authentication/TwoFactorAuth/ProviderLoader.php index b9a0a97bec4..7e674a01dd8 100644 --- a/lib/private/Authentication/TwoFactorAuth/ProviderLoader.php +++ b/lib/private/Authentication/TwoFactorAuth/ProviderLoader.php @@ -9,8 +9,7 @@ declare(strict_types=1); namespace OC\Authentication\TwoFactorAuth; use Exception; -use OC; -use OC_App; +use OC\AppFramework\Bootstrap\Coordinator; use OCP\App\IAppManager; use OCP\AppFramework\QueryException; use OCP\Authentication\TwoFactorAuth\IProvider; @@ -19,15 +18,10 @@ use OCP\IUser; class ProviderLoader { public const BACKUP_CODES_APP_ID = 'twofactor_backupcodes'; - /** @var IAppManager */ - private $appManager; - - /** @var OC\AppFramework\Bootstrap\Coordinator */ - private $coordinator; - - public function __construct(IAppManager $appManager, OC\AppFramework\Bootstrap\Coordinator $coordinator) { - $this->appManager = $appManager; - $this->coordinator = $coordinator; + public function __construct( + private IAppManager $appManager, + private Coordinator $coordinator, + ) { } /** @@ -58,12 +52,12 @@ class ProviderLoader { } } - $registeredProviders = $this->coordinator->getRegistrationContext()->getTwoFactorProviders(); + $registeredProviders = $this->coordinator->getRegistrationContext()?->getTwoFactorProviders() ?? []; foreach ($registeredProviders as $provider) { try { $this->loadTwoFactorApp($provider->getAppId()); - $provider = \OCP\Server::get($provider->getService()); - $providers[$provider->getId()] = $provider; + $providerInstance = \OCP\Server::get($provider->getService()); + $providers[$providerInstance->getId()] = $providerInstance; } catch (QueryException $exc) { // Provider class can not be resolved throw new Exception('Could not load two-factor auth provider ' . $provider->getService()); @@ -75,12 +69,10 @@ class ProviderLoader { /** * Load an app by ID if it has not been loaded yet - * - * @param string $appId */ - protected function loadTwoFactorApp(string $appId) { - if (!OC_App::isAppLoaded($appId)) { - OC_App::loadApp($appId); + protected function loadTwoFactorApp(string $appId): void { + if (!$this->appManager->isAppLoaded($appId)) { + $this->appManager->loadApp($appId); } } } diff --git a/lib/private/L10N/Factory.php b/lib/private/L10N/Factory.php index 6b6dc5d3b40..fc76a15b07b 100644 --- a/lib/private/L10N/Factory.php +++ b/lib/private/L10N/Factory.php @@ -81,7 +81,7 @@ class Factory implements IFactory { */ public function get($app, $lang = null, $locale = null) { return new LazyL10N(function () use ($app, $lang, $locale) { - $app = \OC_App::cleanAppId($app); + $app = $this->appManager->cleanAppId($app); if ($lang !== null) { $lang = str_replace(['\0', '/', '\\', '..'], '', $lang); } diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index 646d1d4e6ed..ba369eecac0 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -104,7 +104,7 @@ class Router implements IRouter { */ public function loadRoutes($app = null) { if (is_string($app)) { - $app = \OC_App::cleanAppId($app); + $app = $this->appManager->cleanAppId($app); } $requestedApp = $app; @@ -123,11 +123,15 @@ class Router implements IRouter { if (isset($this->loadedApps[$app])) { return; } - $appPath = \OC_App::getAppPath($app); - $file = $appPath . '/appinfo/routes.php'; - if ($appPath !== false && file_exists($file)) { - $routingFiles = [$app => $file]; - } else { + try { + $appPath = $this->appManager->getAppPath($app); + $file = $appPath . '/appinfo/routes.php'; + if (file_exists($file)) { + $routingFiles = [$app => $file]; + } else { + $routingFiles = []; + } + } catch (AppPathNotFoundException) { $routingFiles = []; } @@ -238,14 +242,14 @@ class Router implements IRouter { // empty string / 'apps' / $app / rest of the route [, , $app,] = explode('/', $url, 4); - $app = \OC_App::cleanAppId($app); + $app = $this->appManager->cleanAppId($app); \OC::$REQUESTEDAPP = $app; $this->loadRoutes($app); } elseif (str_starts_with($url, '/ocsapp/apps/')) { // empty string / 'ocsapp' / 'apps' / $app / rest of the route [, , , $app,] = explode('/', $url, 5); - $app = \OC_App::cleanAppId($app); + $app = $this->appManager->cleanAppId($app); \OC::$REQUESTEDAPP = $app; $this->loadRoutes($app); } elseif (str_starts_with($url, '/settings/')) { @@ -433,7 +437,11 @@ class Router implements IRouter { $appControllerPath = __DIR__ . '/../../../core/Controller'; $appNameSpace = 'OC\\Core'; } else { - $appControllerPath = \OC_App::getAppPath($app) . '/lib/Controller'; + try { + $appControllerPath = $this->appManager->getAppPath($app) . '/lib/Controller'; + } catch (AppPathNotFoundException) { + return []; + } $appNameSpace = App::buildAppNamespace($app); } diff --git a/lib/private/legacy/OC_App.php b/lib/private/legacy/OC_App.php index f48f4fd0b98..d72d5fe8522 100644 --- a/lib/private/legacy/OC_App.php +++ b/lib/private/legacy/OC_App.php @@ -45,8 +45,7 @@ class OC_App { * @psalm-taint-escape html * @psalm-taint-escape has_quotes * - * @param string $app AppId that needs to be cleaned - * @return string + * @deprecated 31.0.0 use IAppManager::cleanAppId */ public static function cleanAppId(string $app): string { return str_replace(['<', '>', '"', "'", '\0', '/', '\\', '..'], '', $app); diff --git a/lib/public/App/IAppManager.php b/lib/public/App/IAppManager.php index b735b0d7c64..68c3cc771f4 100644 --- a/lib/public/App/IAppManager.php +++ b/lib/public/App/IAppManager.php @@ -141,12 +141,10 @@ interface IAppManager { /** * Get the directory for the given app. * - * @param string $appId - * @return string * @since 11.0.0 * @throws AppPathNotFoundException */ - public function getAppPath($appId); + public function getAppPath(string $appId): string; /** * Get the web path for the given app. @@ -282,4 +280,16 @@ interface IAppManager { * @since 30.0.0 */ public function isBackendRequired(string $backend): bool; + + /** + * Clean the appId from forbidden characters + * + * @psalm-taint-escape file + * @psalm-taint-escape include + * @psalm-taint-escape html + * @psalm-taint-escape has_quotes + * + * @since 31.0.0 + */ + public function cleanAppId(string $app): string; } diff --git a/remote.php b/remote.php index 1cdb74c4139..9b56e6c97f9 100644 --- a/remote.php +++ b/remote.php @@ -8,6 +8,7 @@ require_once __DIR__ . '/lib/versioncheck.php'; use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin; +use OCP\App\IAppManager; use Psr\Log\LoggerInterface; use Sabre\DAV\Exception\ServiceUnavailable; use Sabre\DAV\Server; @@ -20,10 +21,7 @@ use Sabre\DAV\Server; class RemoteException extends \Exception { } -/** - * @param Exception|Error $e - */ -function handleException($e) { +function handleException(Exception|Error $e): void { try { $request = \OC::$server->getRequest(); // in case the request content type is text/xml - we assume it's a WebDAV request @@ -126,20 +124,21 @@ try { // Load all required applications \OC::$REQUESTEDAPP = $app; - OC_App::loadApps(['authentication']); - OC_App::loadApps(['extended_authentication']); - OC_App::loadApps(['filesystem', 'logging']); + $appManager = \OCP\Server::get(IAppManager::class); + $appManager->loadApps(['authentication']); + $appManager->loadApps(['extended_authentication']); + $appManager->loadApps(['filesystem', 'logging']); switch ($app) { case 'core': $file = OC::$SERVERROOT .'/'. $file; break; default: - if (!\OC::$server->getAppManager()->isInstalled($app)) { + if (!$appManager->isInstalled($app)) { throw new RemoteException('App not installed: ' . $app); } - OC_App::loadApp($app); - $file = OC_App::getAppPath($app) .'/'. $parts[1]; + $appManager->loadApp($app); + $file = $appManager->getAppPath($app) .'/'. ($parts[1] ?? ''); break; } $baseuri = OC::$WEBROOT . '/remote.php/'.$service.'/'; diff --git a/tests/apps.php b/tests/apps.php index c01e364c0a8..c2015fecda9 100644 --- a/tests/apps.php +++ b/tests/apps.php @@ -44,10 +44,15 @@ function getSubclasses($parentClassName): array { } $apps = OC_App::getEnabledApps(); +$appManager = \OCP\Server::get(\OCP\App\IAppManager::class); foreach ($apps as $app) { - $dir = OC_App::getAppPath($app); - if (is_dir($dir . '/tests')) { - loadDirectory($dir . '/tests'); + try { + $dir = $appManager->getAppPath($app); + if (is_dir($dir . '/tests')) { + loadDirectory($dir . '/tests'); + } + } catch (\OCP\App\AppPathNotFoundException) { + /* ignore */ } } diff --git a/tests/lib/App/AppManagerTest.php b/tests/lib/App/AppManagerTest.php index 82a3f0d2045..ac470c00335 100644 --- a/tests/lib/App/AppManagerTest.php +++ b/tests/lib/App/AppManagerTest.php @@ -341,7 +341,7 @@ class AppManagerTest extends TestCase { $manager->expects($this->once()) ->method('getAppPath') ->with('test') - ->willReturn(null); + ->willReturn(''); $manager->expects($this->once()) ->method('getAppInfo') @@ -402,7 +402,7 @@ class AppManagerTest extends TestCase { $manager->expects($this->once()) ->method('getAppPath') ->with('test') - ->willReturn(null); + ->willReturn(''); $manager->expects($this->once()) ->method('getAppInfo') diff --git a/tests/lib/InfoXmlTest.php b/tests/lib/InfoXmlTest.php index 702eca4c0ce..1527e363bd7 100644 --- a/tests/lib/InfoXmlTest.php +++ b/tests/lib/InfoXmlTest.php @@ -7,6 +7,7 @@ namespace Test; use OCP\App\IAppManager; +use OCP\Server; /** * Class InfoXmlTest @@ -15,6 +16,13 @@ use OCP\App\IAppManager; * @package Test */ class InfoXmlTest extends TestCase { + private IAppManager $appManager; + + protected function setUp(): void { + parent::setUp(); + $this->appManager = Server::get(IAppManager::class); + } + public function dataApps() { return [ ['admin_audit'], @@ -45,8 +53,8 @@ class InfoXmlTest extends TestCase { * @param string $app */ public function testClasses($app) { - $appInfo = \OCP\Server::get(IAppManager::class)->getAppInfo($app); - $appPath = \OC_App::getAppPath($app); + $appInfo = $this->appManager->getAppInfo($app); + $appPath = $this->appManager->getAppPath($app); \OC_App::registerAutoloading($app, $appPath); //Add the appcontainer |