From 4d1b4480a94baa48af265b999e6c19793790613a Mon Sep 17 00:00:00 2001 From: Julian Ammann Date: Fri, 28 Aug 2026 17:09:13 +0200 Subject: [PATCH 1/2] [BUGFIX] Encrypt api_key before DataHandler records the history diff --- Classes/Hooks/EncryptApiKey.php | 21 ++++ .../Hooks/EncryptApiKeyHistoryTest.php | 98 +++++++++++++++++++ 2 files changed, 119 insertions(+) create mode 100644 Tests/Functional/Hooks/EncryptApiKeyHistoryTest.php diff --git a/Classes/Hooks/EncryptApiKey.php b/Classes/Hooks/EncryptApiKey.php index bec5893..297313a 100644 --- a/Classes/Hooks/EncryptApiKey.php +++ b/Classes/Hooks/EncryptApiKey.php @@ -22,6 +22,9 @@ * what DataHandler persists. Idempotent: already-encrypted values pass * through unchanged, which means re-saving an unchanged row does not * double-encrypt the value. + * Also runs in processDatamap_preProcessFieldArray: on updates, DataHandler + * captures the sys_history diff befire the post-process hook, so the + * plaintext key would otherwise end up in the record history. */ final class EncryptApiKey { @@ -29,6 +32,24 @@ final class EncryptApiKey public function __construct(private readonly ApiKeyEncryption $encryption) {} + public function processDatamap_preProcessFieldArray( + array &$incomingFieldArray, + string $table, + $id, + DataHandler $dataHandler, + ): void { + if ($table !== self::TABLE) { + return; + } + + $value = (string)($incomingFieldArray['api_key'] ?? ''); + if ($value === '') { + return; + } + + $incomingFieldArray['api_key'] = $this->encryption->encrypt($value); + } + public function processDatamap_postProcessFieldArray( string $status, string $table, diff --git a/Tests/Functional/Hooks/EncryptApiKeyHistoryTest.php b/Tests/Functional/Hooks/EncryptApiKeyHistoryTest.php new file mode 100644 index 0000000..ac81067 --- /dev/null +++ b/Tests/Functional/Hooks/EncryptApiKeyHistoryTest.php @@ -0,0 +1,98 @@ +getConnectionPool()->getConnectionForTable('be_users') + ->insert('be_users', ['uid' => 1, 'username' => 'admin', 'admin' => 1]); + $this->setUpBackendUser(1); + } + + #[Test] + public function changingTheApiKeyDoesNotRecordThePlaintextInHistory(): void + { + $uid = $this->process(['NEW1' => [ + 'pid' => 0, + 'ai_provider' => 'openai', + 'title' => 'probe', + 'api_key' => 'sk-initial', + ]]); + + $this->process([$uid => ['api_key' => self::NEW_KEY]]); + + $encryption = new ApiKeyEncryption(); + $stored = $this->storedApiKey($uid); + self::assertTrue($encryption->isEncrypted($stored), 'The api_key column is not encrypted.'); + self::assertSame(self::NEW_KEY, $encryption->decrypt($stored), 'The api_key was encrypted twice or not replaced.'); + + self::assertStringNotContainsString(self::NEW_KEY, $this->historyData($uid), 'sys_history contains the api_key in plaintext.'); + } + + /** + * @param array> $records + */ + private function process(array $records): int + { + $dataHandler = GeneralUtility::makeInstance(DataHandler::class); + $dataHandler->start(['tx_aim_configuration' => $records], []); + $dataHandler->process_datamap(); + self::assertSame([], $dataHandler->errorLog, 'DataHandler rejected the datamap: ' . implode('; ', $dataHandler->errorLog)); + + return (int)($dataHandler->substNEWwithIDs['NEW1'] ?? array_key_first($records)); + } + + private function storedApiKey(int $uid): string + { + $row = $this->getConnectionPool()->getConnectionForTable('tx_aim_configuration') + ->select(['api_key'], 'tx_aim_configuration', ['uid' => $uid]) + ->fetchAssociative(); + self::assertNotFalse($row, 'The configuration record does not exist.'); + + return (string)$row['api_key']; + } + + private function historyData(int $uid): string + { + $rows = $this->getConnectionPool()->getConnectionForTable('sys_history') + ->select(['history_data'], 'sys_history', ['tablename' => 'tx_aim_configuration', 'recuid' => $uid]) + ->fetchAllAssociative(); + self::assertNotEmpty($rows, 'DataHandler did not write a sys_history entry.'); + + return implode("\n", array_column($rows, 'history_data')); + } +} From 61a857b32bf05878a0edad848b3718c80d5942c1 Mon Sep 17 00:00:00 2001 From: Oliver Bartsch Date: Sat, 29 Aug 2026 23:15:31 +0200 Subject: [PATCH 2/2] [TASK] Move the api_key history test and document the hook order CHANGELOG documents the fix and the one-off cleanup for history entries written before it. Dropping the whole row rather than editing history_data keeps that a single portable statement: the payload comes in two shapes (flat for ACTION_ADD, oldRecord/newRecord for ACTION_MODIFY) and would otherwise need JSON manipulation in four SQL dialects. sys_log.log_data references the history id, but only as a soft reference, and the backend history view queries sys_history by tablename/recuid. --- CHANGELOG.md | 10 ++++++++++ Classes/Hooks/EncryptApiKey.php | 18 +++++++++++------- .../EncryptApiKeyHistoryTest.php | 7 +++++-- 3 files changed, 26 insertions(+), 9 deletions(-) rename Tests/Functional/{Hooks => DataHandling}/EncryptApiKeyHistoryTest.php (91%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9649abc..e84d458 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changelog +## Unreleased + +- Bugfix: provider API keys are encrypted in `processDatamap_preProcessFieldArray` instead of only in the post-process hook. On an update, DataHandler captures the `sys_history` diff before the post-process hook runs, so the record history kept the unencrypted value even though the `api_key` column itself was encrypted. Inserts were not affected (#30) + + Existing history entries are not rewritten. To drop them for a configuration whose key was changed before this fix: + + ```sql + DELETE FROM sys_history WHERE tablename = 'tx_aim_configuration'; + ``` + ## 0.4.1 Bugfix release: third-party Symfony AI bridge auto-discovery, and a small footer addition. diff --git a/Classes/Hooks/EncryptApiKey.php b/Classes/Hooks/EncryptApiKey.php index 297313a..d833397 100644 --- a/Classes/Hooks/EncryptApiKey.php +++ b/Classes/Hooks/EncryptApiKey.php @@ -18,13 +18,17 @@ /** * Encrypts AiM provider API keys before they are written to the database. * - * Runs in processDatamap_postProcessFieldArray so the encrypted value is - * what DataHandler persists. Idempotent: already-encrypted values pass - * through unchanged, which means re-saving an unchanged row does not - * double-encrypt the value. - * Also runs in processDatamap_preProcessFieldArray: on updates, DataHandler - * captures the sys_history diff befire the post-process hook, so the - * plaintext key would otherwise end up in the record history. + * Encryption happens in processDatamap_preProcessFieldArray, because on an + * update DataHandler captures the sys_history diff in + * compareFieldArrayWithCurrentAndUnset() before it calls the post-process + * hook - encrypting only there would leave the plaintext key in the record + * history even though the column itself is encrypted. Inserts were never + * affected, since insertDB() writes the history entry after the hook. + * + * processDatamap_postProcessFieldArray stays for the "empty means keep the + * stored key" handling, and re-encrypts as a safety net for callers that + * bypass the pre-process stage. Both are idempotent: encrypt() passes + * already-encrypted values through unchanged, so nothing is encrypted twice. */ final class EncryptApiKey { diff --git a/Tests/Functional/Hooks/EncryptApiKeyHistoryTest.php b/Tests/Functional/DataHandling/EncryptApiKeyHistoryTest.php similarity index 91% rename from Tests/Functional/Hooks/EncryptApiKeyHistoryTest.php rename to Tests/Functional/DataHandling/EncryptApiKeyHistoryTest.php index ac81067..870bd87 100644 --- a/Tests/Functional/Hooks/EncryptApiKeyHistoryTest.php +++ b/Tests/Functional/DataHandling/EncryptApiKeyHistoryTest.php @@ -28,6 +28,7 @@ */ final class EncryptApiKeyHistoryTest extends FunctionalTestCase { + private const INITIAL_KEY = 'sk-history-insert-probe'; private const NEW_KEY = 'sk-history-leak-probe'; protected array $testExtensionsToLoad = [ @@ -50,12 +51,14 @@ public function changingTheApiKeyDoesNotRecordThePlaintextInHistory(): void 'pid' => 0, 'ai_provider' => 'openai', 'title' => 'probe', - 'api_key' => 'sk-initial', + 'api_key' => self::INITIAL_KEY, ]]); + self::assertStringNotContainsString(self::INITIAL_KEY, $this->historyData($uid), 'sys_history contains the inserted api_key in plaintext.'); + $this->process([$uid => ['api_key' => self::NEW_KEY]]); - $encryption = new ApiKeyEncryption(); + $encryption = $this->get(ApiKeyEncryption::class); $stored = $this->storedApiKey($uid); self::assertTrue($encryption->isEncrypted($stored), 'The api_key column is not encrypted.'); self::assertSame(self::NEW_KEY, $encryption->decrypt($stored), 'The api_key was encrypted twice or not replaced.');