From aee7553a43f59ff29089c1c7d8904322d735c6d7 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 28 Sep 2026 16:41:16 +0200 Subject: [PATCH] fix(overrides): refuse a save that would drop tokens, and keep the file path out of errors A save with an unknown token name or a value the writer strips answered 200 with written set to the input size, and the audit entry listed tokens that never reached the file. It now answers 400 naming those tokens and writes nothing. A write failure answers a generic message instead of the exception text, which carried the absolute file path. Fixes #694 --- lib/Controller/OverridesController.php | 32 ++- lib/Service/CustomOverridesService.php | 46 +++- .../OverridesControllerValidationTest.php | 197 ++++++++++++++++++ 3 files changed, 269 insertions(+), 6 deletions(-) create mode 100644 tests/Unit/Controller/OverridesControllerValidationTest.php diff --git a/lib/Controller/OverridesController.php b/lib/Controller/OverridesController.php index b4b63e29..22da28b3 100644 --- a/lib/Controller/OverridesController.php +++ b/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( diff --git a/lib/Service/CustomOverridesService.php b/lib/Service/CustomOverridesService.php index 94364ddf..7e4a4cb2 100644 --- a/lib/Service/CustomOverridesService.php +++ b/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 $tokens Input token map. + * + * @return array 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; } diff --git a/tests/Unit/Controller/OverridesControllerValidationTest.php b/tests/Unit/Controller/OverridesControllerValidationTest.php new file mode 100644 index 00000000..dd04a5bc --- /dev/null +++ b/tests/Unit/Controller/OverridesControllerValidationTest.php @@ -0,0 +1,197 @@ +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