fix(overrides): refuse a save that would drop tokens, and keep the file path out of errors

lib/Controller/OverridesController.php

@@ -53,6 +53,14 @@
  */
 class OverridesController extends Controller {
 
+	/**
+	 * The answer to a failed write. The exception text names the absolute file
+	 * path, which is not for the browser.
+	 *
+	 * @var string
+	 */
+	private const WRITE_FAILED = 'The token overrides could not be saved. Check that the web server can write to the app\'s css/ directory.';
+
 	/**
 	 * The custom overrides service.
 	 *
@@ -127,7 +135,8 @@ public function getOverrides(): JSONResponse {
 	 * Write new custom token overrides to custom-overrides.css.
 	 *
 	 * Accepts a JSON body with an 'overrides' key containing token name => value pairs.
-	 * Only tokens in the TokenRegistry are accepted; others are silently ignored.
+	 * A save with any token outside the TokenRegistry, or with a value the writer
+	 * would drop, is refused with 400 naming those tokens, and nothing is written.
 	 *
 	 * @return JSONResponse Status and count of written tokens.
 	 *
@@ -143,12 +152,25 @@ public function setOverrides(): JSONResponse {
 			return new JSONResponse(['error' => 'overrides must be an object'], 400);
 		}
 
+		// Refuse the whole save when any token would be dropped, so the answer,
+		// the written count and the audit entry all describe what reached the file.
+		$rejected = $this->overridesService->findRejected(tokens: $overrides);
+		if (empty($rejected) === false) {
+			return new JSONResponse(
+				[
+					'error' => 'Some tokens were not saved: ' . implode(', ', array_keys($rejected)),
+					'rejected' => $rejected,
+				],
+				400
+			);
+		}
+
 		$before = $this->overridesService->read();
 
 		try {
 			$this->overridesService->write(tokens: $overrides);
-		} catch (\RuntimeException $e) {
-			return new JSONResponse(['error' => $e->getMessage()], 500);
+		} catch (\RuntimeException) {
+			return new JSONResponse(['error' => self::WRITE_FAILED], 500);
 		}
 
 		$this->auditService->log(
@@ -298,8 +320,8 @@ private function writeImportedTokens(array $parsed, string $rawContent): JSONRes
 
 		try {
 			$this->overridesService->write(tokens: $toImport);
-		} catch (\RuntimeException $e) {
-			return new JSONResponse(['error' => $e->getMessage()], 500);
+		} catch (\RuntimeException) {
+			return new JSONResponse(['error' => self::WRITE_FAILED], 500);
 		}
 
 		$this->auditService->log(

lib/Service/CustomOverridesService.php

@@ -158,6 +158,50 @@ public function write(array $tokens): void {
 
 	}//end write()
 
+	/**
+	 * List the tokens in a map that write() would not persist, with the reason.
+	 *
+	 * A token is refused when its name is not in the TokenRegistry, when its value
+	 * is not a string, or when its value carries a character the writer strips to
+	 * keep the file a single :root block. Callers that answer an admin use this to
+	 * refuse the whole save instead of reporting tokens that never reached the file.
+	 *
+	 * @param array<array-key, mixed> $tokens Input token map.
+	 *
+	 * @return array<string, string> Map of refused token name => reason; empty when all are accepted.
+	 *
+	 * @SuppressWarnings(PHPMD.StaticAccess) - TokenRegistry uses static methods by design
+	 *
+	 * @spec openspec/changes/authoring-token-value-types/tasks.md#task-2.1
+	 */
+	public function findRejected(array $tokens): array {
+		$rejected = [];
+		foreach ($tokens as $name => $value) {
+			$name = (string)$name;
+			if (TokenRegistry::isEditable(tokenName: $name) === false) {
+				$rejected[$name] = 'not an editable token';
+				continue;
+			}
+
+			if (is_string($value) === false || $this->isUnsafeValue(value: $value) === true) {
+				$rejected[$name] = 'not an allowed value';
+			}
+		}
+
+		return $rejected;
+	}//end findRejected()
+
+	/**
+	 * Tell whether a value carries a character that would break out of the :root block.
+	 *
+	 * @param string $value The token value.
+	 *
+	 * @return bool True when the writer would drop the value.
+	 */
+	private function isUnsafeValue(string $value): bool {
+		return preg_match('/[{};]|\/\*|\*\//', $value) === 1;
+	}//end isUnsafeValue()
+
 	/**
 	 * Filter a token map to only those present in the registry.
 	 *
@@ -250,7 +294,7 @@ private function buildDeclarationLines(array $tokens): array {
 		$lines = [];
 		foreach ($tokens as $name => $value) {
 			// Reject any value containing CSS injection characters.
-			if (preg_match('/[{};]|\/\*|\*\//', $value) === 1) {
+			if ($this->isUnsafeValue(value: $value) === true) {
 				continue;
 			}
 

tests/Unit/Controller/OverridesControllerValidationTest.php

@@ -0,0 +1,197 @@
+<?php
+
+/**
+ * Unit tests for what OverridesController::setOverrides() accepts and reports.
+ *
+ * SPDX-License-Identifier: EUPL-1.2
+ * SPDX-FileCopyrightText: 2026 Conduction B.V.
+ *
+ * @spec openspec/changes/authoring-token-value-types/tasks.md#task-2.1
+ */
+
+declare(strict_types=1);
+
+namespace OCA\Thematiq\Tests\Unit\Controller;
+
+use OCA\Thematiq\Controller\OverridesController;
+use OCA\Thematiq\Service\CssParserService;
+use OCA\Thematiq\Service\CustomOverridesService;
+use OCA\Thematiq\Service\ThemingAuditService;
+use OCP\App\IAppManager;
+use OCP\IRequest;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Thematiq#694: a save with an unknown token name or an unsafe value answered
+ * 200 while the token was dropped, the audit entry listed tokens that never
+ * reached the file, and a write failure returned the absolute file path.
+ *
+ * The overrides service here is the REAL CustomOverridesService writing to a
+ * temporary app directory, so what the controller reports is compared with
+ * what actually lands in custom-overrides.css.
+ */
+class OverridesControllerValidationTest extends TestCase {
+
+	/**
+	 * Temporary app root holding css/custom-overrides.css.
+	 *
+	 * @var string
+	 */
+	private string $appDir;
+
+	/**
+	 * The real overrides service.
+	 *
+	 * @var CustomOverridesService
+	 */
+	private CustomOverridesService $overridesService;
+
+	/**
+	 * The mocked audit service.
+	 *
+	 * @var ThemingAuditService&\PHPUnit\Framework\MockObject\MockObject
+	 */
+	private ThemingAuditService $auditService;
+
+	/**
+	 * The mocked request.
+	 *
+	 * @var IRequest&\PHPUnit\Framework\MockObject\MockObject
+	 */
+	private IRequest $request;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->appDir = sys_get_temp_dir() . '/thematiq-694-' . bin2hex(random_bytes(4));
+		mkdir($this->appDir . '/css', 0777, true);
+
+		$appManager = $this->createMock(IAppManager::class);
+		$appManager->method('getAppPath')->willReturn($this->appDir);
+
+		$this->overridesService = new CustomOverridesService($appManager, new CssParserService());
+		$this->overridesService->write(tokens: ['--color-primary' => '#000000']);
+
+		$this->auditService = $this->createMock(ThemingAuditService::class);
+		$this->request = $this->createMock(IRequest::class);
+	}//end setUp()
+
+	protected function tearDown(): void {
+		foreach (glob($this->appDir . '/css/*') ?: [] as $file) {
+			unlink($file);
+		}
+
+		rmdir($this->appDir . '/css');
+		rmdir($this->appDir);
+		parent::tearDown();
+	}//end tearDown()
+
+	/**
+	 * Build the controller around a given overrides service.
+	 *
+	 * @param CustomOverridesService $service The overrides service.
+	 *
+	 * @return OverridesController
+	 */
+	private function controller(CustomOverridesService $service): OverridesController {
+		return new OverridesController('thematiq', $this->request, $service, new CssParserService(), $this->auditService);
+	}//end controller()
+
+	/**
+	 * An unknown token name is refused with 400 naming it, and nothing is written.
+	 */
+	public function testUnknownTokenIs400(): void {
+		$this->request->method('getParams')->willReturn(
+			['overrides' => ['--color-primary' => '#112233', '--not-a-registry-token' => '#445566']]
+		);
+		$this->auditService->expects($this->never())->method('log');
+
+		$response = $this->controller(service: $this->overridesService)->setOverrides();
+
+		$this->assertSame(400, $response->getStatus());
+		$this->assertStringContainsString('--not-a-registry-token', json_encode($response->getData()));
+		$this->assertSame(['--color-primary' => '#000000'], $this->overridesService->read());
+	}//end testUnknownTokenIs400()
+
+	/**
+	 * A value the writer would drop (a CSS injection attempt) is refused with 400
+	 * naming the token, and nothing is written.
+	 */
+	public function testWrongValueIs400(): void {
+		$this->request->method('getParams')->willReturn(
+			['overrides' => ['--color-primary' => 'red; } body { display: none']]
+		);
+		$this->auditService->expects($this->never())->method('log');
+
+		$response = $this->controller(service: $this->overridesService)->setOverrides();
+
+		$this->assertSame(400, $response->getStatus());
+		$this->assertStringContainsString('--color-primary', json_encode($response->getData()));
+		$this->assertSame(['--color-primary' => '#000000'], $this->overridesService->read());
+	}//end testWrongValueIs400()
+
+	/**
+	 * A value that is not a string is refused with 400 instead of a type error.
+	 */
+	public function testNonStringValueIs400(): void {
+		$this->request->method('getParams')->willReturn(
+			['overrides' => ['--color-primary' => ['#112233']]]
+		);
+
+		$response = $this->controller(service: $this->overridesService)->setOverrides();
+
+		$this->assertSame(400, $response->getStatus());
+		$this->assertSame(['--color-primary' => '#000000'], $this->overridesService->read());
+	}//end testNonStringValueIs400()
+
+	/**
+	 * A valid save reports the count that reached the file, and the audit entry
+	 * records exactly what was written.
+	 */
+	public function testValidSaveReportsAndAuditsWhatWasWritten(): void {
+		$this->request->method('getParams')->willReturn(
+			['overrides' => ['--color-primary' => '#112233', '--color-primary-element' => '#445566']]
+		);
+		$this->auditService->expects($this->once())
+			->method('log')
+			->with(
+				'overrides_written',
+				[
+					'old' => ['--color-primary' => '#000000'],
+					'new' => ['--color-primary' => '#112233', '--color-primary-element' => '#445566'],
+				]
+			);
+
+		$response = $this->controller(service: $this->overridesService)->setOverrides();
+
+		$this->assertSame(200, $response->getStatus());
+		$this->assertSame(2, $response->getData()['written']);
+		$this->assertSame(
+			['--color-primary' => '#112233', '--color-primary-element' => '#445566'],
+			$this->overridesService->read()
+		);
+	}//end testValidSaveReportsAndAuditsWhatWasWritten()
+
+	/**
+	 * A write failure answers 500 with a generic message, never the file path
+	 * the service puts in its exception text.
+	 */
+	public function testWriteFailureHidesPath(): void {
+		$failing = $this->getMockBuilder(CustomOverridesService::class)
+			->disableOriginalConstructor()
+			->onlyMethods(['read', 'write', 'findRejected'])
+			->getMock();
+		$failing->method('read')->willReturn([]);
+		$failing->method('findRejected')->willReturn([]);
+		$failing->method('write')->willThrowException(
+			new \RuntimeException('Could not write /var/www/html/custom_apps/thematiq/css/custom-overrides.css.tmp.')
+		);
+		$this->request->method('getParams')->willReturn(['overrides' => ['--color-primary' => '#112233']]);
+
+		$response = $this->controller(service: $failing)->setOverrides();
+
+		$this->assertSame(500, $response->getStatus());
+		$this->assertStringNotContainsString('/var/www', json_encode($response->getData()));
+		$this->assertStringNotContainsString('custom-overrides.css', json_encode($response->getData()));
+	}//end testWriteFailureHidesPath()
+}//end class