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 bec5893..d833397 100644 --- a/Classes/Hooks/EncryptApiKey.php +++ b/Classes/Hooks/EncryptApiKey.php @@ -18,10 +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. + * 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 { @@ -29,6 +36,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/DataHandling/EncryptApiKeyHistoryTest.php b/Tests/Functional/DataHandling/EncryptApiKeyHistoryTest.php new file mode 100644 index 0000000..870bd87 --- /dev/null +++ b/Tests/Functional/DataHandling/EncryptApiKeyHistoryTest.php @@ -0,0 +1,101 @@ +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' => 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 = $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.'); + + 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')); + } +}