From 920974ab680dcd95f6c94212c1b7c10e02075299 Mon Sep 17 00:00:00 2001 From: Oleksander Piskun Date: Mon, 2 Mar 2026 08:49:09 +0200 Subject: [PATCH] fix: ExApp upgrade 401 on set_init_status Signed-off-by: Oleksander Piskun --- .github/workflows/phpunit.yml | 77 +++++++ lib/Service/AppAPIService.php | 28 ++- tests/php/Service/AppAPIServiceTest.php | 295 ++++++++++++++++++++++++ 3 files changed, 398 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/phpunit.yml create mode 100644 tests/php/Service/AppAPIServiceTest.php diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml new file mode 100644 index 00000000..91fcb9da --- /dev/null +++ b/.github/workflows/phpunit.yml @@ -0,0 +1,77 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: MIT +name: PHPUnit + +on: + pull_request: + push: + branches: [main] + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: phpunit-${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + phpunit: + runs-on: ubuntu-22.04 + name: PHPUnit • PHP ${{ matrix.php-version }} • ${{ matrix.server-version }} + strategy: + fail-fast: false + matrix: + php-version: ['8.2', '8.3'] + server-version: ['master'] + + steps: + - name: Set app env + run: echo "APP_NAME=${GITHUB_REPOSITORY##*/}" >> $GITHUB_ENV + + - name: Checkout server + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + submodules: true + repository: nextcloud/server + ref: ${{ matrix.server-version }} + + - name: Checkout AppAPI + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + path: apps/${{ env.APP_NAME }} + + - name: Set up PHP ${{ matrix.php-version }} + uses: shivammathur/setup-php@44454db4f0199b8b9685a5d763dc37cbf79108e1 # v2 + with: + php-version: ${{ matrix.php-version }} + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite3, pdo_sqlite + coverage: none + ini-file: development + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + - name: Install app dependencies + working-directory: apps/${{ env.APP_NAME }} + run: composer i + + - name: Set up Nextcloud + run: | + mkdir data + ./occ maintenance:install --verbose --database=sqlite --admin-user admin --admin-pass admin + ./occ app:enable --force ${{ env.APP_NAME }} + + - name: Run PHPUnit + working-directory: apps/${{ env.APP_NAME }} + run: composer run test:unit + + phpunit-summary: + permissions: + contents: none + runs-on: ubuntu-22.04 + needs: [phpunit] + name: PHPUnit-OK + steps: + - run: echo "PHPUnit tests passed successfully" diff --git a/lib/Service/AppAPIService.php b/lib/Service/AppAPIService.php index cfbd0cc8..66f6a82d 100644 --- a/lib/Service/AppAPIService.php +++ b/lib/Service/AppAPIService.php @@ -309,7 +309,7 @@ public function validateExAppRequestToNC(IRequest $request, bool $isDav = false) $this->logger->error(sprintf('Error getting path info. Error: %s', $e->getMessage()), ['exception' => $e]); return false; } - if (($this->sanitizeOcsRoute($path) !== '/apps/app_api/ex-app/state') && !$exApp->getEnabled()) { + if (!$exApp->getEnabled() && !$this->isExemptFromEnabledCheck($path, $exApp)) { $this->logger->error(sprintf('ExApp with appId %s is disabled (%s)', $request->getHeader('EX-APP-ID'), $request->getRequestUri())); return false; } @@ -368,6 +368,26 @@ private function sanitizeOcsRoute(string $route): string { return $route; } + /** + * Check if the given path is exempt from the ExApp enabled check. + * /ex-app/state is always exempt (query enabled state). + * Init status endpoints are only exempt while the ExApp is actively being + * installed or updated (status type set server-side by Register/Update commands), + * preventing a disabled ExApp from re-enabling itself via set_init_status(100). + */ + private function isExemptFromEnabledCheck(string $path, ExApp $exApp): bool { + $sanitizedPath = $this->sanitizeOcsRoute($path); + if ($sanitizedPath === '/apps/app_api/ex-app/state') { + return true; + } + $status = $exApp->getStatus(); + $isInitializing = in_array($status['type'] ?? '', ['install', 'update'], true); + if ($isInitializing && $sanitizedPath === '/apps/app_api/ex-app/status') { + return true; + } + return false; + } + private function getCustomLogger(string $name): LoggerInterface { $path = $this->config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data') . '/' . $name; return $this->logFactory->getCustomPsrLogger($path); @@ -411,7 +431,11 @@ public function dispatchExAppInitInternal(ExApp $exApp): void { } $this->setAppInitProgress($exApp, 0); - $this->exAppService->enableExAppInternal($exApp); + if (!$this->exAppService->enableExAppInternal($exApp)) { + $this->logger->error(sprintf('Failed to enable ExApp %s before init dispatch', $exApp->getAppid())); + $this->setAppInitProgress($exApp, 0, 'Failed to enable ExApp before init'); + return; + } try { $this->client->post($initUrl, $options); } catch (\Exception $e) { diff --git a/tests/php/Service/AppAPIServiceTest.php b/tests/php/Service/AppAPIServiceTest.php new file mode 100644 index 00000000..9e4b3289 --- /dev/null +++ b/tests/php/Service/AppAPIServiceTest.php @@ -0,0 +1,295 @@ +logger = $this->createMock(LoggerInterface::class); + $logFactory = $this->createMock(\OCP\Log\ILogFactory::class); + $this->throttler = $this->createMock(IThrottler::class); + $this->config = $this->createMock(IConfig::class); + $this->client = $this->createMock(IClient::class); + $clientService = $this->createMock(IClientService::class); + $clientService->method('newClient')->willReturn($this->client); + $this->userSession = $this->createMock(IUserSession::class); + $this->session = $this->createMock(ISession::class); + $this->userManager = $this->createMock(IUserManager::class); + $l10nFactory = $this->createMock(IFactory::class); + $this->exAppService = $this->createMock(ExAppService::class); + $this->dockerActions = $this->createMock(DockerActions::class); + $this->manualActions = $this->createMock(ManualActions::class); + $this->commonService = $this->createMock(AppAPICommonService::class); + $daemonConfigService = $this->createMock(DaemonConfigService::class); + $harpService = $this->createMock(HarpService::class); + + $this->service = new AppAPIService( + $this->logger, + $logFactory, + $this->throttler, + $this->config, + $clientService, + $this->userSession, + $this->session, + $this->userManager, + $l10nFactory, + $this->exAppService, + $this->dockerActions, + $this->manualActions, + $this->commonService, + $daemonConfigService, + $harpService, + ); + } + + private function createExApp( + string $appId = 'test_app', + bool $enabled = true, + string $version = '1.0.0', + string $statusType = '', + ): ExApp { + $exApp = new ExApp(); + $exApp->setAppid($appId); + $exApp->setVersion($version); + $exApp->setName('Test App'); + $exApp->setPort(23000); + $exApp->setSecret(self::TEST_SECRET); + $exApp->setEnabled($enabled ? 1 : 0); + $exApp->setStatus([ + 'deploy' => 100, + 'init' => 0, + 'action' => '', + 'type' => $statusType, + 'error' => '', + ]); + $exApp->setDaemonConfigName('test_daemon'); + $exApp->setProtocol('http'); + $exApp->setHost('127.0.0.1'); + $exApp->setAcceptsDeployId('manual-install'); + $exApp->setDeployConfig([]); + return $exApp; + } + + private function createMockRequest(string $appId, string $version, string $secret, string $path): MockObject&\OCP\IRequest { + $request = $this->createMock(\OCP\IRequest::class); + $request->method('getRemoteAddress')->willReturn('127.0.0.1'); + $request->method('getHeader')->willReturnCallback(function (string $name) use ($appId, $version, $secret) { + return match ($name) { + 'EX-APP-ID' => $appId, + 'EX-APP-VERSION' => $version, + 'AUTHORIZATION-APP-API' => base64_encode(':' . $secret), + default => '', + }; + }); + $request->method('getPathInfo')->willReturn($path); + $request->method('getRequestUri')->willReturn($path); + return $request; + } + + /** + * Set up mocks needed for getExAppUrl to resolve without errors. + */ + private function setupExAppUrlMocks(): void { + // Make getExAppUrl take the manual actions path and return a test URL + $this->dockerActions->method('getAcceptsDeployId')->willReturn('docker-install'); + $this->manualActions->method('getAcceptsDeployId')->willReturn('manual-install'); + $this->manualActions->method('resolveExAppUrl')->willReturn('http://localhost:23000'); + } + + /** + * Test that dispatchExAppInitInternal aborts when enableExAppInternal fails. + * This is the primary fix for issue #803. + */ + public function testDispatchExAppInitInternalAbortsOnEnableFail(): void { + $exApp = $this->createExApp(enabled: false); + + $this->setupExAppUrlMocks(); + + $this->exAppService->expects(self::once()) + ->method('enableExAppInternal') + ->with($exApp) + ->willReturn(false); + + $this->exAppService->method('updateExApp')->willReturn(true); + + $this->commonService->method('buildAppAPIAuthHeaders') + ->willReturn(['AUTHORIZATION-APP-API' => 'test']); + + // The HTTP client must NOT be called if enable fails + $this->client->expects(self::never()) + ->method('post'); + + $this->service->dispatchExAppInitInternal($exApp); + } + + /** + * Test that dispatchExAppInitInternal proceeds normally when enable succeeds. + */ + public function testDispatchExAppInitInternalProceedsOnEnableSuccess(): void { + $exApp = $this->createExApp(enabled: false); + + $this->setupExAppUrlMocks(); + + $this->exAppService->expects(self::once()) + ->method('enableExAppInternal') + ->with($exApp) + ->willReturn(true); + + $this->exAppService->method('updateExApp')->willReturn(true); + + $this->commonService->method('buildAppAPIAuthHeaders') + ->willReturn(['AUTHORIZATION-APP-API' => 'test']); + + // POST /init SHOULD be called when enable succeeds + $this->client->expects(self::once()) + ->method('post'); + + $this->service->dispatchExAppInitInternal($exApp); + } + + /** + * Test that a disabled ExApp CAN access the /ex-app/state endpoint. + * This was already working before; verify it's still working. + */ + public function testValidateDisabledExAppCanAccessExAppState(): void { + $exApp = $this->createExApp('test_app', false); + $request = $this->createMockRequest('test_app', '1.0.0', self::TEST_SECRET, + '/ocs/v1.php/apps/app_api/ex-app/state'); + + $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); + $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + + self::assertTrue($this->service->validateExAppRequestToNC($request)); + } + + /** + * Test that a disabled ExApp being updated CAN access the new /ex-app/status endpoint. + * This is part of the fix for issue #803. + */ + public function testValidateDisabledExAppDuringUpdateCanAccessExAppStatus(): void { + $exApp = $this->createExApp('test_app', false, statusType: 'update'); + $request = $this->createMockRequest('test_app', '1.0.0', self::TEST_SECRET, + '/ocs/v1.php/apps/app_api/ex-app/status'); + + $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); + $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + + self::assertTrue($this->service->validateExAppRequestToNC($request)); + } + + /** + * Test that a disabled ExApp being installed CAN access the new /ex-app/status endpoint. + * This is part of the fix for issue #803. + */ + public function testValidateDisabledExAppDuringInstallCanAccessExAppStatus(): void { + $exApp = $this->createExApp('test_app', false, statusType: 'install'); + $request = $this->createMockRequest('test_app', '1.0.0', self::TEST_SECRET, + '/ocs/v1.php/apps/app_api/ex-app/status'); + + $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); + $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + + self::assertTrue($this->service->validateExAppRequestToNC($request)); + } + + /** + * Test that a disabled ExApp NOT in install/update CANNOT access status endpoints. + * Prevents a disabled ExApp from re-enabling itself via set_init_status(100). + */ + public function testValidateDisabledExAppCannotAccessStatusWhenNotInitializing(): void { + $exApp = $this->createExApp('test_app', false); // statusType defaults to '' + $request = $this->createMockRequest('test_app', '1.0.0', self::TEST_SECRET, + '/ocs/v1.php/apps/app_api/ex-app/status'); + + $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); + $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + + self::assertFalse($this->service->validateExAppRequestToNC($request)); + } + + /** + * Test that a disabled ExApp is STILL rejected for non-exempt endpoints even during update. + */ + public function testValidateDisabledExAppRejectedForNonExemptEndpoint(): void { + $exApp = $this->createExApp('test_app', false, statusType: 'update'); + $request = $this->createMockRequest('test_app', '1.0.0', self::TEST_SECRET, + '/ocs/v1.php/apps/app_api/api/v1/log'); + + $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); + $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + + self::assertFalse($this->service->validateExAppRequestToNC($request)); + } + + /** + * Test that an enabled ExApp passes validation for any endpoint. + */ + public function testValidateEnabledExAppAllowedEverywhere(): void { + $exApp = $this->createExApp('test_app', true); + $request = $this->createMockRequest('test_app', '1.0.0', self::TEST_SECRET, + '/ocs/v1.php/apps/app_api/api/v1/log'); + + $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); + $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + + self::assertTrue($this->service->validateExAppRequestToNC($request)); + } + + /** + * Test that an invalid secret is rejected regardless of the path. + */ + public function testValidateInvalidSecretRejected(): void { + $exApp = $this->createExApp('test_app', true); + $request = $this->createMockRequest('test_app', '1.0.0', 'wrong_secret', + '/ocs/v1.php/apps/app_api/ex-app/status'); + + $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); + $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + + self::assertFalse($this->service->validateExAppRequestToNC($request)); + } +}