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