[BUGFIX] Encrypt api_key before DataHandler records the history diff

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.

Classes/Hooks/EncryptApiKey.php

@@ -18,17 +18,42 @@
 /**
  * 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
 {
     private const TABLE = 'tx_aim_configuration';
 
     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,

Tests/Functional/DataHandling/EncryptApiKeyHistoryTest.php

@@ -0,0 +1,101 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of TYPO3 CMS-based extension "aim" by b13.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ */
+
+namespace B13\Aim\Tests\Functional\DataHandling;
+
+use B13\Aim\Crypto\ApiKeyEncryption;
+use PHPUnit\Framework\Attributes\Test;
+use TYPO3\CMS\Core\DataHandling\DataHandler;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
+use TYPO3\TestingFramework\Core\Functional\FunctionalTestCase;
+
+/**
+ * On an update, DataHandler captures the sys_history diff in
+ * compareFieldArrayWithCurrentAndUnset() before it calls
+ * processDatamap_postProcessFieldArray(). Encrypting only there leaves the
+ * plaintext key in the record history. Inserts are not affected.
+ *
+ * The unit test can't show this, it calls the hook directly.
+ */
+final class EncryptApiKeyHistoryTest extends FunctionalTestCase
+{
+    private const INITIAL_KEY = 'sk-history-insert-probe';
+    private const NEW_KEY = 'sk-history-leak-probe';
+
+    protected array $testExtensionsToLoad = [
+        'b13/aim',
+    ];
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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<int|string, array<string, mixed>> $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'));
+    }
+}