From 525ae9305eac19198a6a63ae52d2aa5e25ad0927 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Tue, 1 Sep 2026 22:20:22 +0200 Subject: [PATCH 1/3] style(e2e): collapse the roadmap URL assertion onto one line The route-matching change in #663 left this call wrapped across three lines where prettier wants one, which reddened Frontend Check (format) on development. No behaviour change. --- tests/e2e/spec-coverage/settings-roadmap.spec.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/e2e/spec-coverage/settings-roadmap.spec.ts b/tests/e2e/spec-coverage/settings-roadmap.spec.ts index db3cfb0b..6b20f16e 100644 --- a/tests/e2e/spec-coverage/settings-roadmap.spec.ts +++ b/tests/e2e/spec-coverage/settings-roadmap.spec.ts @@ -51,9 +51,7 @@ async function openRoute(page: Page, route: string): Promise { await dismissSupportDialog(page) // Path, not hash. Anchored at the end so `/features-roadmap` cannot be // satisfied by some longer route that merely contains it. - await expect(page).toHaveURL( - new RegExp(`${route.replace(/\//g, '\\/')}$`), - ) + await expect(page).toHaveURL(new RegExp(`${route.replace(/\//g, '\\/')}$`)) await expect(page.locator('.app-content')).toBeVisible({ timeout: 10_000 }) } From f3ea24a9be3393ce01e75b16977522b134bc5326 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Tue, 1 Sep 2026 22:34:46 +0200 Subject: [PATCH 2/3] fix(apphost): implement the health and metrics endpoints larpinq already routed Routes::standard() emits health#index and metrics#index for every AppHost adopter, so larpinq has routed both since it adopted the builder and implemented neither. Both URLs resolved to a controller class that does not exist. gate-14 route-reachability named them controller-class-not-found. Ported from pipelinq, which carries the same pair, including the constraint in its header: neither class may name an OpenRegister symbol in any position the autoloader resolves. Nextcloud reflects every file in lib/Controller/ while matching a route, so one unresolvable parent makes EVERY larpinq route 500, not just the one being matched. The collaborators are pulled from the container by FQCN string at dispatch time instead. Health answers HTTP 200 degraded when the engine is absent, because a probe that fails when its dependency is missing tells monitoring nothing about the app. Metrics degrades to 503 rather than 500 and stays admin-only through the deliberate absence of NoAdminRequired. openspec/specs/apphost-adoption/spec.md is new and owns all three properties. larpinq had no home for them; the fleet convention is this capability name. Also fixes gate-25 contract-coverage: dashboard#catchAll had no contract test. The assertion that matters is not that it answers, but that it answers with the SAME shell as page(), since a catch-all returning anything else still routes, still returns 200, and still leaves every deep link broken. TESTS THAT COULD NEVER RUN. phpunit.xml collects ./tests/unit, and a second ./tests/Unit existed alongside it. On a case-sensitive filesystem the uppercase directory is simply not collected, so DemoDataServiceTest had never executed. Moved into tests/unit/Service; the suite goes from 238 collected tests to 251. --- lib/Controller/HealthController.php | 190 ++++++++++++++++++ lib/Controller/MetricsController.php | 133 ++++++++++++ openspec/specs/apphost-adoption/spec.md | 87 ++++++++ .../Controller/DashboardControllerTest.php | 34 ++++ .../unit/Controller/HealthControllerTest.php | 148 ++++++++++++++ .../unit/Controller/MetricsControllerTest.php | 144 +++++++++++++ .../Service/DemoDataServiceTest.php | 0 7 files changed, 736 insertions(+) create mode 100644 lib/Controller/HealthController.php create mode 100644 lib/Controller/MetricsController.php create mode 100644 openspec/specs/apphost-adoption/spec.md create mode 100644 tests/unit/Controller/HealthControllerTest.php create mode 100644 tests/unit/Controller/MetricsControllerTest.php rename tests/{Unit => unit}/Service/DemoDataServiceTest.php (100%) diff --git a/lib/Controller/HealthController.php b/lib/Controller/HealthController.php new file mode 100644 index 00000000..4f44a94a --- /dev/null +++ b/lib/Controller/HealthController.php @@ -0,0 +1,190 @@ +openregister` in appinfo/info.xml, so the parent was + * unresolvable on any instance without OpenRegister. + * + * @category Controller + * @package OCA\Larpinq\Controller + * + * @author Conduction Development Team + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://github.com/ConductionNL/larpinq + * + * @spec openspec/specs/apphost-adoption/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Larpinq\Controller; + +use OCA\Larpinq\AppInfo\Application; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Attribute\AnonRateLimit; +use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\PublicPage; +use OCP\AppFramework\Http\JSONResponse; +use OCP\IConfig; +use OCP\IRequest; +use Psr\Container\ContainerInterface; + +/** + * Public, declarative health endpoint backed by the AppHost engine. + * + * The engine collaborators are pulled from the container by FQCN string at + * dispatch time, so larpinq never binds an OpenRegister class at + * class-declaration time. + * + * @psalm-suppress UnusedClass + * + * @spec openspec/specs/apphost-adoption/spec.md + */ +class HealthController extends Controller { + + /** + * FQCN of the AppHost observability manifest loader. + * + * Referenced as a string, never imported: the class only exists when + * openregister is installed. + * + * @var string + */ + private const MANIFEST_LOADER = 'OCA\\OpenRegister\\AppHost\\Observability\\ManifestLoader'; + + /** + * FQCN of the AppHost declarative health-check executor. + * + * Referenced as a string, never imported: the class only exists when + * openregister is installed. + * + * @var string + */ + private const HEALTH_EXECUTOR = 'OCA\\OpenRegister\\AppHost\\Observability\\HealthCheckExecutor'; + + /** + * Constructor. + * + * @param IRequest $request The HTTP request. + * @param IConfig $config The Nextcloud config service (fallback version). + * @param ContainerInterface $container DI container — resolves the AppHost engine lazily. + * + * @return void + */ + public function __construct( + IRequest $request, + private readonly IConfig $config, + private readonly ContainerInterface $container, + ) { + parent::__construct(appName: Application::APP_ID, request: $request); + + }//end __construct() + + /** + * GET /api/health — declarative health check (ADR-006), public probe. + * + * Runs the manifest-declared checks through the AppHost engine and renders + * the `{status, app, version, checks}` envelope with the status code the + * engine's policy resolved. CORS headers are emitted only when the + * manifest opts in, exactly as the engine does. + * + * When the AppHost engine cannot be resolved — openregister absent or + * disabled — the endpoint still answers (the whole point of a health + * probe): `status: degraded`, `checks.openregister: unavailable`, HTTP 200. + * + * The rate-limit ceiling is generous because this is a liveness probe, + * polled on a schedule by monitoring. + * + * @return JSONResponse `{status, app, version, checks}` with HTTP code per policy. + * + * @spec openspec/specs/apphost-adoption/spec.md + */ + #[PublicPage] + #[NoCSRFRequired] + #[AnonRateLimit(limit: 120, period: 60)] + public function index(): JSONResponse { + $engine = $this->engineResult(); + if ($engine === null) { + return new JSONResponse( + [ + 'status' => 'degraded', + 'app' => $this->appName, + 'version' => $this->config->getAppValue(Application::APP_ID, 'installed_version', ''), + 'checks' => ['openregister' => 'unavailable'], + ], + Http::STATUS_OK + ); + } + + $response = new JSONResponse( + [ + 'status' => $engine['status'], + 'app' => $this->appName, + 'version' => $engine['version'], + 'checks' => $engine['checks'], + ], + $engine['httpStatus'] + ); + + if ($engine['cors'] === true) { + $response->addHeader('Access-Control-Allow-Origin', '*'); + $response->addHeader('Access-Control-Allow-Methods', 'GET, OPTIONS'); + } + + return $response; + }//end index() + + /** + * Run the AppHost observability engine for this app. + * + * Null when the engine is unavailable (openregister absent/disabled). + * + * @return array{status: string, version: string, checks: array, httpStatus: int, cors: bool}|null + */ + private function engineResult(): ?array { + try { + $manifestLoader = $this->container->get(self::MANIFEST_LOADER); + $executor = $this->container->get(self::HEALTH_EXECUTOR); + + $appId = $this->appName; + $manifest = $manifestLoader->load(appId: $appId); + $result = $executor->execute(manifest: $manifest); + + return [ + 'status' => (string)$result->status, + 'version' => (string)$manifestLoader->appVersion(appId: $appId), + 'checks' => (array)$result->checks, + 'httpStatus' => (int)$result->httpStatusCode, + 'cors' => ($manifest->cors === true), + ]; + } catch (\Throwable $e) { + return null; + }//end try + + }//end engineResult() +}//end class diff --git a/lib/Controller/MetricsController.php b/lib/Controller/MetricsController.php new file mode 100644 index 00000000..7d53e183 --- /dev/null +++ b/lib/Controller/MetricsController.php @@ -0,0 +1,133 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://github.com/ConductionNL/larpinq + * + * @spec openspec/specs/apphost-adoption/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Larpinq\Controller; + +use OCA\Larpinq\AppInfo\Application; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\TextPlainResponse; +use OCP\IRequest; +use Psr\Container\ContainerInterface; + +/** + * Admin-only declarative Prometheus metrics endpoint backed by the AppHost engine. + * + * No `#[NoAdminRequired]` — the absence of that attribute means NC requires an + * admin session, which is the intended ADR-006 posture for metrics. Anonymous + * callers get the NC login redirect / 401, never metric data. + * + * @psalm-suppress UnusedClass + * + * @spec openspec/specs/apphost-adoption/spec.md + */ +class MetricsController extends Controller { + + /** + * FQCN of the AppHost observability manifest loader. + * + * Referenced as a string, never imported: the class only exists when + * openregister is installed. + * + * @var string + */ + private const MANIFEST_LOADER = 'OCA\\OpenRegister\\AppHost\\Observability\\ManifestLoader'; + + /** + * FQCN of the AppHost Prometheus metrics engine. + * + * Referenced as a string, never imported: the class only exists when + * openregister is installed. + * + * @var string + */ + private const METRICS_ENGINE = 'OCA\\OpenRegister\\AppHost\\Observability\\MetricsEngine'; + + /** + * Prometheus text exposition content type (mirrors the engine's renderer). + * + * @var string + */ + private const CONTENT_TYPE = 'text/plain; version=0.0.4; charset=utf-8'; + + /** + * Constructor. + * + * @param IRequest $request The HTTP request. + * @param ContainerInterface $container DI container — resolves the AppHost engine lazily. + * + * @return void + */ + public function __construct( + IRequest $request, + private readonly ContainerInterface $container, + ) { + parent::__construct(appName: Application::APP_ID, request: $request); + + }//end __construct() + + /** + * GET /api/metrics — declarative Prometheus metrics (admin-only, ADR-006). + * + * Admin-only by the deliberate absence of `#[NoAdminRequired]`. + * + * Returns HTTP 503 with a Prometheus comment line when the AppHost engine + * is unavailable (openregister absent or disabled) — never a 500. + * + * @return TextPlainResponse Prometheus text exposition 0.0.4. + * + * @spec openspec/specs/apphost-adoption/spec.md + */ + #[NoCSRFRequired] + public function index(): TextPlainResponse { + try { + $manifestLoader = $this->container->get(self::MANIFEST_LOADER); + $engine = $this->container->get(self::METRICS_ENGINE); + + $manifest = $manifestLoader->load(appId: $this->appName); + $body = (string)$engine->render(manifest: $manifest); + $status = Http::STATUS_OK; + } catch (\Throwable $e) { + $body = '# metrics unavailable: the OpenRegister AppHost observability engine is not installed' . "\n"; + $status = Http::STATUS_SERVICE_UNAVAILABLE; + }//end try + + $response = new TextPlainResponse($body, $status); + $response->addHeader('Content-Type', self::CONTENT_TYPE); + + return $response; + }//end index() +}//end class diff --git a/openspec/specs/apphost-adoption/spec.md b/openspec/specs/apphost-adoption/spec.md new file mode 100644 index 00000000..285c11e8 --- /dev/null +++ b/openspec/specs/apphost-adoption/spec.md @@ -0,0 +1,87 @@ +# Larpinq AppHost adoption + +## Purpose + +`\OCA\OpenRegister\AppHost\Routes::standard()` builds larpinq's route table, and +it emits `health#index` and `metrics#index` for every adopter. Larpinq routed +both from the day it adopted the builder and implemented neither, so both URLs +resolved to a controller class that did not exist. gate-14 route-reachability +named them `controller-class-not-found`. + +This spec owns the two observability endpoints and, more importantly, the way +they are allowed to reach OpenRegister. The engine that answers them belongs to +a sibling app that larpinq does not declare as a dependency and cannot assume is +installed, so the shape of the coupling is the requirement, not an implementation +note. + +## Requirements + +### Requirement: Health answers on an instance without OpenRegister + +`GET /api/health` SHALL return the ADR-006 envelope `{status, app, version, +checks}` and SHALL answer whether or not the OpenRegister AppHost observability +engine can be resolved. + +A health probe that fails when its dependency is missing reports nothing about +the thing being probed. Monitoring cannot tell "larpinq is down" from "larpinq +cannot answer", so the degraded case is a documented 200 rather than an error. + +#### Scenario: OpenRegister absent + +- **WHEN** the AppHost observability engine cannot be resolved from the container +- **THEN** the endpoint responds HTTP 200 +- **AND** the envelope carries `status: degraded` +- **AND** `checks.openregister` reads `unavailable` +- **AND** `version` falls back to larpinq's own `installed_version` + +#### Scenario: Engine available + +- **WHEN** the engine resolves and executes the manifest-declared checks +- **THEN** the endpoint responds with the status code the engine's policy resolved +- **AND** the envelope carries the engine's `status`, `version` and `checks` +- **AND** CORS headers are emitted only when the manifest opts in + +### Requirement: Metrics are admin-only and degrade to 503 + +`GET /api/metrics` SHALL render Prometheus text exposition 0.0.4 for +administrators, and SHALL NOT expose metric data to anonymous callers. + +The posture comes from the deliberate absence of `#[NoAdminRequired]`. Nextcloud +requires an admin session for a controller method that does not carry it, so an +anonymous caller gets the login redirect and never reaches the engine. + +#### Scenario: Engine unavailable + +- **WHEN** the AppHost metrics engine cannot be resolved +- **THEN** the endpoint responds HTTP 503 +- **AND** the body is a Prometheus comment line naming the missing engine +- **AND** the response is never a 500 + +#### Scenario: Content type + +- **WHEN** the endpoint answers at all, degraded or not +- **THEN** the `Content-Type` header is `text/plain; version=0.0.4; charset=utf-8` + +### Requirement: The observability controllers never bind an OpenRegister class at declaration time + +Both controllers SHALL resolve OpenRegister collaborators from the DI container +by fully-qualified name at dispatch time, and SHALL NOT name an OpenRegister +class in any position the autoloader resolves: no `extends`, no `implements`, no +`use` import, no typed parameter or return. + +This is a whole-app property, not a per-endpoint one. Nextcloud's router +reflects every file in `lib/Controller/` while matching a route, so a single +unresolvable parent class makes **every** route in larpinq return HTTP 500, not +only the one being matched. `extends` is resolved by the autoloader rather than +the container, so no amount of lazy registration rescues it. Larpinq does not +declare `openregister` in `appinfo/info.xml`, so an administrator can +create exactly the instance where this bites. + +#### Scenario: Class declaration is inspected + +- **WHEN** either observability controller is loaded on an instance without OpenRegister +- **THEN** the class declaration resolves +- **AND** every OpenRegister name it uses appears only as a string constant + +See also [apphost-autoload-prelude](../apphost-autoload-prelude/spec.md), which +owns the matching invariant for `Application::register()`. diff --git a/tests/unit/Controller/DashboardControllerTest.php b/tests/unit/Controller/DashboardControllerTest.php index 10095591..0cec07d5 100644 --- a/tests/unit/Controller/DashboardControllerTest.php +++ b/tests/unit/Controller/DashboardControllerTest.php @@ -59,4 +59,38 @@ public function testPageReturnsEmptyParams(): void { self::assertEmpty($result->getParams()); } + + /** + * The SPA catch-all serves the same shell as page(). + * + * `dashboard#catchAll` on `/{path}` is what makes a deep link survive a + * RELOAD. Before it existed, /apps/larpinq/characters and /events both + * returned 404 while every other hash-mode app answered 200, and that 404 + * is what kept this app on hash routing. The failure is invisible from + * inside the SPA, because the SPA never loads to report it. + * + * Serving *a* response is not the contract. Serving the app shell is: a + * catch-all that answered with anything else would still route, still + * return 200, and still leave every deep link broken. + */ + public function testCatchAllServesTheAppShell(): void { + $result = $this->controller->catchAll(); + + self::assertInstanceOf(TemplateResponse::class, $result); + self::assertSame('index', $result->getTemplateName()); + } + + /** + * The two entry points agree, so a deep link is not a second-class page. + * + * Asserted as an observable pair rather than by reading catchAll()'s body, + * so it survives the delegation being rewritten. + */ + public function testCatchAllAndPageAgreeOnTemplateAndParams(): void { + $page = $this->controller->page(); + $catchAll = $this->controller->catchAll(); + + self::assertSame($page->getTemplateName(), $catchAll->getTemplateName()); + self::assertSame($page->getParams(), $catchAll->getParams()); + } } diff --git a/tests/unit/Controller/HealthControllerTest.php b/tests/unit/Controller/HealthControllerTest.php new file mode 100644 index 00000000..e13ca459 --- /dev/null +++ b/tests/unit/Controller/HealthControllerTest.php @@ -0,0 +1,148 @@ +openregister`, so an administrator can create exactly that + * instance. These tests hold the degraded path to a documented HTTP 200. + * + * @category Test + * @package OCA\Larpinq\Tests\Unit\Controller + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + */ + +declare(strict_types=1); + +namespace OCA\Larpinq\Tests\Unit\Controller; + +use OCA\Larpinq\AppInfo\Application; +use OCA\Larpinq\Controller\HealthController; +use OCP\AppFramework\Http; +use OCP\IConfig; +use OCP\IRequest; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; + +/** + * Covers the health endpoint's envelope and its OpenRegister-absent fallback. + * + * @spec openspec/specs/apphost-adoption/spec.md + */ +class HealthControllerTest extends TestCase { + + /** + * Mocked HTTP request. + * + * @var IRequest&MockObject + */ + private IRequest&MockObject $request; + + /** + * Mocked Nextcloud config. + * + * @var IConfig&MockObject + */ + private IConfig&MockObject $config; + + /** + * Mocked DI container standing in for the AppHost engine's home. + * + * @var ContainerInterface&MockObject + */ + private ContainerInterface&MockObject $container; + + /** + * Build the collaborators. + * + * @return void + */ + protected function setUp(): void { + parent::setUp(); + $this->request = $this->createMock(IRequest::class); + $this->config = $this->createMock(IConfig::class); + $this->container = $this->createMock(ContainerInterface::class); + + }//end setUp() + + /** + * The controller under test. + * + * @return HealthController + */ + private function controller(): HealthController { + return new HealthController($this->request, $this->config, $this->container); + + }//end controller() + + /** + * With no engine in the container, health still answers 200 degraded. + * + * @return void + */ + public function testAnswersDegradedWhenTheEngineCannotBeResolved(): void { + $this->container->method('get') + ->willThrowException(new \RuntimeException('not registered')); + $this->config->method('getAppValue') + ->with(Application::APP_ID, 'installed_version', '') + ->willReturn('9.9.9'); + + $response = $this->controller()->index(); + $data = $response->getData(); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertSame('degraded', $data['status']); + $this->assertSame(Application::APP_ID, $data['app']); + $this->assertSame('9.9.9', $data['version']); + $this->assertSame(['openregister' => 'unavailable'], $data['checks']); + + }//end testAnswersDegradedWhenTheEngineCannotBeResolved() + + /** + * The degraded envelope carries all four ADR-006 keys, not a subset. + * + * A probe that omits a key its consumer reads is a broken contract even + * when the status code is right. + * + * @return void + */ + public function testDegradedEnvelopeCarriesTheFullShape(): void { + $this->container->method('get') + ->willThrowException(new \RuntimeException('not registered')); + $this->config->method('getAppValue')->willReturn(''); + + $data = $this->controller()->index()->getData(); + + $this->assertSame( + ['status', 'app', 'version', 'checks'], + array_keys($data), + 'The ADR-006 envelope keys and their order are the contract.' + ); + + }//end testDegradedEnvelopeCarriesTheFullShape() + + /** + * A container that returns an unusable engine degrades rather than fatals. + * + * `ContainerInterface::get()` is not required to throw for an unknown id in + * every implementation, so the null-object case is exercised separately + * from the throwing one. + * + * @return void + */ + public function testDegradesWhenTheContainerAnswersWithAnUnusableEngine(): void { + $this->container->method('get')->willReturn(new \stdClass()); + $this->config->method('getAppValue')->willReturn('1.2.3'); + + $response = $this->controller()->index(); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertSame('degraded', $response->getData()['status']); + + }//end testDegradesWhenTheContainerAnswersWithAnUnusableEngine() +}//end class diff --git a/tests/unit/Controller/MetricsControllerTest.php b/tests/unit/Controller/MetricsControllerTest.php new file mode 100644 index 00000000..a0c1d32a --- /dev/null +++ b/tests/unit/Controller/MetricsControllerTest.php @@ -0,0 +1,144 @@ + + */ + +declare(strict_types=1); + +namespace OCA\Larpinq\Tests\Unit\Controller; + +use OCA\Larpinq\Controller\MetricsController; +use OCP\AppFramework\Http; +use OCP\IRequest; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use ReflectionMethod; + +/** + * Covers the metrics endpoint's degraded path and its access posture. + * + * @spec openspec/specs/apphost-adoption/spec.md + */ +class MetricsControllerTest extends TestCase { + + /** + * Mocked HTTP request. + * + * @var IRequest&MockObject + */ + private IRequest&MockObject $request; + + /** + * Mocked DI container standing in for the AppHost engine's home. + * + * @var ContainerInterface&MockObject + */ + private ContainerInterface&MockObject $container; + + /** + * Build the collaborators. + * + * @return void + */ + protected function setUp(): void { + parent::setUp(); + $this->request = $this->createMock(IRequest::class); + $this->container = $this->createMock(ContainerInterface::class); + + }//end setUp() + + /** + * The controller under test. + * + * @return MetricsController + */ + private function controller(): MetricsController { + return new MetricsController($this->request, $this->container); + + }//end controller() + + /** + * A missing engine is 503 with a Prometheus comment, never a 500. + * + * @return void + */ + public function testDegradesToServiceUnavailableWithoutTheEngine(): void { + $this->container->method('get') + ->willThrowException(new \RuntimeException('not registered')); + + $response = $this->controller()->index(); + + $this->assertSame(Http::STATUS_SERVICE_UNAVAILABLE, $response->getStatus()); + $this->assertNotSame(Http::STATUS_INTERNAL_SERVER_ERROR, $response->getStatus()); + $this->assertStringStartsWith('#', $response->render()); + $this->assertStringContainsString('observability engine', $response->render()); + + }//end testDegradesToServiceUnavailableWithoutTheEngine() + + /** + * The declared content type is Prometheus text exposition 0.0.4. + * + * Asserted against the constant rather than the response, deliberately: + * `Response::getHeaders()` reaches into `OCP\Server` for the request id, + * which needs the `OC` server container that a unit test does not boot. + * The constant is the single place the value is written, so pinning it + * still catches a silent change to what scrapers are told to parse. + * + * @return void + */ + public function testDeclaresThePrometheusContentType(): void { + $reflected = new \ReflectionClass(MetricsController::class); + + $this->assertSame( + 'text/plain; version=0.0.4; charset=utf-8', + $reflected->getConstant('CONTENT_TYPE') + ); + + }//end testDeclaresThePrometheusContentType() + + /** + * `index()` carries no `#[NoAdminRequired]`, so NC requires an admin. + * + * This is the whole access control for the endpoint. Adding that attribute + * would silently open metric data to any signed-in user, and nothing else + * in the class would change, so the absence is asserted directly. + * + * @return void + */ + public function testIndexIsAdminOnlyByOmittingNoAdminRequired(): void { + $attributes = (new ReflectionMethod(MetricsController::class, 'index')) + ->getAttributes(); + $names = array_map(static fn ($a) => $a->getName(), $attributes); + + $this->assertNotContains( + 'OCP\AppFramework\Http\Attribute\NoAdminRequired', + $names, + 'Metrics must stay admin-only: NoAdminRequired would open it to any user.' + ); + $this->assertNotContains( + 'OCP\AppFramework\Http\Attribute\PublicPage', + $names, + 'Metrics must never be a public page.' + ); + + }//end testIndexIsAdminOnlyByOmittingNoAdminRequired() +}//end class diff --git a/tests/Unit/Service/DemoDataServiceTest.php b/tests/unit/Service/DemoDataServiceTest.php similarity index 100% rename from tests/Unit/Service/DemoDataServiceTest.php rename to tests/unit/Service/DemoDataServiceTest.php From d5170724077511196d763b626731f1473bce1e1f Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Tue, 1 Sep 2026 23:06:04 +0200 Subject: [PATCH 3/3] fix(e2e): assert the roadmap route with a predicate, not a hand-escaped regex CodeQL flagged this as a high-severity js/incomplete-sanitization: the regex escaped only '/', leaving backslashes and every other metacharacter live. It is also a latent false pass, since '.' matching any character would let a redirect to a similar-looking path satisfy the assertion. Reading URL.pathname and calling endsWith says exactly what the comment above it already claimed, with nothing to escape. --- tests/e2e/spec-coverage/settings-roadmap.spec.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/e2e/spec-coverage/settings-roadmap.spec.ts b/tests/e2e/spec-coverage/settings-roadmap.spec.ts index 6b20f16e..c8a00dac 100644 --- a/tests/e2e/spec-coverage/settings-roadmap.spec.ts +++ b/tests/e2e/spec-coverage/settings-roadmap.spec.ts @@ -51,7 +51,14 @@ async function openRoute(page: Page, route: string): Promise { await dismissSupportDialog(page) // Path, not hash. Anchored at the end so `/features-roadmap` cannot be // satisfied by some longer route that merely contains it. - await expect(page).toHaveURL(new RegExp(`${route.replace(/\//g, '\\/')}$`)) + // + // Asserted with a PREDICATE rather than a built regex. The regex form + // escaped only `/`, which left `\` and every other metacharacter live — + // CodeQL js/incomplete-sanitization, and a latent false pass, since `.` + // matching any character would let a redirect to a similar-looking path + // satisfy the assertion. Reading `pathname` says the same thing with + // nothing to escape. + await expect(page).toHaveURL((u) => new URL(String(u)).pathname.endsWith(route)) await expect(page.locator('.app-content')).toBeVisible({ timeout: 10_000 }) }