Skip to content

fix!: prevent plaintext API key storage via setup wizard #97

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
CybotTM merged 2 commits into from
Mar 6, 2026
Merged

fix!: prevent plaintext API key storage via setup wizard #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5665d0f
fix!: prevent plaintext API key storage via setup wizard
CybotTM Mar 6, 2026
03330a3
refactor: use Symfony Uuid::v7() instead of manual UUID generation
CybotTM Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion Classes/Controller/Backend/SetupWizardController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@
use Netresearch\NrLlm\Service\SetupWizard\DTO\SuggestedConfiguration;
use Netresearch\NrLlm\Service\SetupWizard\ModelDiscoveryInterface;
use Netresearch\NrLlm\Service\SetupWizard\ProviderDetector;
use Netresearch\NrVault\Service\VaultServiceInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Symfony\Component\Uid\Uuid;
use Throwable;
use TYPO3\CMS\Backend\Attribute\AsController;
use TYPO3\CMS\Backend\Routing\UriBuilder as BackendUriBuilder;
Expand Down Expand Up @@ -60,6 +62,7 @@ public function __construct(
private readonly PageRenderer $pageRenderer,
private readonly BackendUriBuilder $backendUriBuilder,
private readonly IconFactory $iconFactory,
private readonly VaultServiceInterface $vaultService,
) {}

protected function initializeAction(): void
Expand Down Expand Up @@ -277,12 +280,30 @@ public function saveAction(ServerRequestInterface $request): ResponseInterface
$providerEndpoint = is_string($providerData['endpoint'] ?? null) ? $providerData['endpoint'] : '';
$providerApiKey = is_string($providerData['apiKey'] ?? null) ? $providerData['apiKey'] : '';

// Store the API key in the vault and use the vault identifier
$vaultIdentifier = '';
if ($providerApiKey !== '') {
try {
$vaultIdentifier = $this->generateVaultIdentifier();
$this->vaultService->store($vaultIdentifier, $providerApiKey, [
'table' => 'tx_nrllm_provider',
'field' => 'api_key',
'source' => 'setup_wizard',
]);
} catch (Throwable $e) {
return new JsonResponse([
'success' => false,
'error' => 'Failed to store API key securely: ' . $e->getMessage(),
], 500);
}
}

$provider = new Provider();
$provider->setIdentifier($this->generateIdentifier($providerName));
$provider->setName($providerName !== '' ? $providerName : 'New Provider');
$provider->setAdapterType($providerAdapter);
$provider->setEndpointUrl($providerEndpoint);
$provider->setApiKey($providerApiKey);
$provider->setApiKey($vaultIdentifier);
$provider->setIsActive(true);
if ($pid >= 0) {
$provider->setPid($pid);
Expand Down Expand Up @@ -488,6 +509,11 @@ private function extractIntFromBody(mixed $body, string $key, int $default = 0):
return is_numeric($value) ? (int)$value : $default;
}

private function generateVaultIdentifier(): string
{
return Uuid::v7()->toRfc4122();
}

/**
* Generate a unique identifier from a name.
*/
Expand Down
49 changes: 46 additions & 3 deletions Classes/Domain/Model/Provider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

namespace Netresearch\NrLlm\Domain\Model;

use InvalidArgumentException;
use Netresearch\NrVault\Service\VaultServiceInterface;
use Throwable;
use TYPO3\CMS\Core\Utility\GeneralUtility;
Expand Down Expand Up @@ -104,15 +105,32 @@ public function getApiKey(): string
*
* This method retrieves the API key from nr-vault using the stored identifier.
* The value is retrieved on-demand and not cached to minimize exposure.
*
* If the stored value is not a valid vault identifier (legacy plaintext data),
* a security warning is logged and an empty string is returned to prevent
* accidental use of unencrypted secrets.
*/
public function getDecryptedApiKey(): string
{
if ($this->apiKey === '') {
return '';
}

// Detect legacy plaintext API keys that were stored before vault integration
if (!self::isVaultIdentifier($this->apiKey)) {
trigger_error(
\sprintf(
'Provider %d has a plaintext API key instead of a vault identifier. '
. 'Re-save the provider record to migrate it to the vault.',
$this->uid,
),
E_USER_WARNING,
);
Comment thread
CybotTM marked this conversation as resolved.

return '';
}

try {
// apiKey contains a vault identifier (e.g., "tx_nrllm_provider__api_key__123")
$vault = GeneralUtility::makeInstance(VaultServiceInterface::class);
return $vault->retrieve($this->apiKey) ?? '';
} catch (Throwable) {
Expand Down Expand Up @@ -250,14 +268,39 @@ public function setEndpointUrl(string $endpointUrl): void
/**
* Set the API key vault identifier.
*
* The actual secret storage is handled by nr-vault's TCA form element.
* This method stores the vault identifier that references the encrypted secret.
* The actual secret storage is handled by nr-vault's TCA form element
* or by the SetupWizardController via VaultServiceInterface::store().
* This method only accepts vault identifiers (UUIDs), not raw secrets.
*
* @throws InvalidArgumentException If the value looks like a raw API key instead of a vault UUID
*/
public function setApiKey(string $apiKey): void
{
// Allow empty string (no key / clearing)
// Reject values that look like raw API keys (not vault UUIDs)
if ($apiKey !== '' && !self::isVaultIdentifier($apiKey)) {
throw new InvalidArgumentException(
'API key must be a vault identifier (UUID), not a raw secret. '
. 'Use VaultServiceInterface::store() first.',
1741268400,
);
Comment thread
CybotTM marked this conversation as resolved.
}

$this->apiKey = $apiKey;
}

/**
* Check whether a value looks like a vault identifier (UUID v7).
*/
private static function isVaultIdentifier(string $value): bool
{
// UUID v7 format: 8-4-4-4-12 hex digits with version 7
return (bool)preg_match(
'/^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i',
$value,
);
}

public function setOrganizationId(string $organizationId): void
{
$this->organizationId = $organizationId;
Expand Down
48 changes: 21 additions & 27 deletions Tests/E2E/Backend/ErrorPathwaysE2ETest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

namespace Netresearch\NrLlm\Tests\E2E\Backend;

use InvalidArgumentException;
use Netresearch\NrLlm\Controller\Backend\ConfigurationController;
use Netresearch\NrLlm\Controller\Backend\LlmModuleController;
use Netresearch\NrLlm\Controller\Backend\ModelController;
Expand Down Expand Up @@ -201,7 +202,7 @@ public function pathway7_1_invalidApiKey_providerTestReturnsError(): void
$provider->setIdentifier('invalid-key-provider');
$provider->setName('Invalid Key Provider');
$provider->setAdapterType('openai');
$provider->setApiKey('invalid-api-key-12345');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setIsActive(true);

$this->providerRepository->add($provider);
Expand Down Expand Up @@ -239,7 +240,7 @@ public function pathway7_1_invalidApiKey_modelTestReturnsError(): void
$provider->setIdentifier('invalid-key-provider-2');
$provider->setName('Invalid Key Provider 2');
$provider->setAdapterType('openai');
$provider->setApiKey('invalid-api-key-67890');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-5a6b7c8d9e0f');
$provider->setIsActive(true);

$this->providerRepository->add($provider);
Expand Down Expand Up @@ -292,7 +293,7 @@ public function pathway7_1_invalidApiKey_quickTestReturnsError(): void
$provider->setIdentifier('invalid-key-provider-3');
$provider->setName('Invalid Key Provider 3');
$provider->setAdapterType('openai');
$provider->setApiKey('invalid-api-key-abcde');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-6a7b8c9d0e1f');
$provider->setIsActive(true);

$this->providerRepository->add($provider);
Expand Down Expand Up @@ -371,7 +372,7 @@ public function pathway7_3_networkTimeout_handledGracefully(): void
$provider->setIdentifier('timeout-test-provider');
$provider->setName('Timeout Test Provider');
$provider->setAdapterType('openai');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setTimeout(1); // 1 second timeout
$provider->setIsActive(true);

Expand Down Expand Up @@ -404,7 +405,7 @@ public function pathway7_3_invalidEndpoint_handledGracefully(): void
$provider->setIdentifier('unreachable-provider');
$provider->setName('Unreachable Provider');
$provider->setAdapterType('openai');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setEndpointUrl('https://nonexistent.invalid.domain.local/v1');
$provider->setTimeout(5);
$provider->setIsActive(true);
Expand Down Expand Up @@ -651,7 +652,7 @@ public function pathway7_5_specialCharactersInProviderName_handledSafely(): void
$provider->setIdentifier('special-chars-provider');
$provider->setName('<script>alert("xss")</script>');
$provider->setAdapterType('openai');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setIsActive(true);

$this->providerRepository->add($provider);
Expand Down Expand Up @@ -715,7 +716,7 @@ public function pathway7_5_sqlInjectionAttempt_handledSafely(): void
$provider->setIdentifier("'; DROP TABLE tx_nrllm_domain_model_provider; --");
$provider->setName('SQL Injection Test');
$provider->setAdapterType('openai');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setIsActive(true);

$this->providerRepository->add($provider);
Expand Down Expand Up @@ -780,7 +781,7 @@ public function pathway7_5_nullBytes_handledSafely(): void
$provider->setIdentifier("null-byte-test\x00suffix");
$provider->setName("Null\x00Byte");
$provider->setAdapterType('openai');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setIsActive(true);

$this->providerRepository->add($provider);
Expand All @@ -793,27 +794,20 @@ public function pathway7_5_nullBytes_handledSafely(): void
}

#[Test]
public function pathway7_5_veryLongApiKey_handledSafely(): void
public function pathway7_5_rawApiKey_rejectedByValidation(): void
{
// Test very long API key (should be stored or truncated, not crash)
$longApiKey = str_repeat('a', 10000);
// setApiKey() now rejects raw API keys and requires vault identifiers (UUID v7)
$rawApiKey = str_repeat('a', 10000);

$provider = new Provider();
$provider->setPid(0);
$provider->setIdentifier('long-key-provider');
$provider->setName('Long Key Provider');
$provider->setAdapterType('openai');
$provider->setApiKey($longApiKey);
$provider->setIsActive(true);

$this->providerRepository->add($provider);
$this->persistenceManager->persistAll();
$this->persistenceManager->clearState();

$addedProvider = $this->providerRepository->findOneByIdentifier('long-key-provider');
self::assertNotNull($addedProvider);
// API key should be stored (possibly truncated by database)
self::assertNotEmpty($addedProvider->getApiKey());
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionCode(1741268400);
$provider->setApiKey($rawApiKey);
}

// =========================================================================
Expand Down Expand Up @@ -906,7 +900,7 @@ public function pathway7_7_connectionFailure_returnsStructuredError(): void
$provider->setIdentifier('connection-fail-provider-' . time());
$provider->setName('Connection Fail Provider');
$provider->setAdapterType('openai');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setEndpointUrl('https://unreachable.invalid.local/v1');
$provider->setTimeout(2);
$provider->setIsActive(true);
Expand Down Expand Up @@ -1187,7 +1181,7 @@ public function pathway7_12_zeroTimeout_handledGracefully(): void
$provider->setIdentifier('zero-timeout-provider-' . time());
$provider->setName('Zero Timeout Provider');
$provider->setAdapterType('openai');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setTimeout(0);
$provider->setIsActive(true);

Expand All @@ -1210,7 +1204,7 @@ public function pathway7_12_negativeTimeout_handledGracefully(): void
$provider->setIdentifier('negative-timeout-provider-' . time());
$provider->setName('Negative Timeout Provider');
$provider->setAdapterType('openai');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setTimeout(-100);
$provider->setIsActive(true);

Expand Down Expand Up @@ -1374,7 +1368,7 @@ public function pathway7_15_unknownAdapterType_handledGracefully(): void
$provider->setIdentifier('unknown-adapter-provider-' . time());
$provider->setName('Unknown Adapter Provider');
$provider->setAdapterType('nonexistent_adapter_xyz');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setIsActive(true);

$this->providerRepository->add($provider);
Expand Down Expand Up @@ -1402,7 +1396,7 @@ public function pathway7_15_emptyAdapterType_handledGracefully(): void
$provider->setIdentifier('empty-adapter-provider-' . time());
$provider->setName('Empty Adapter Provider');
$provider->setAdapterType('');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setIsActive(true);

$this->providerRepository->add($provider);
Expand Down Expand Up @@ -1618,7 +1612,7 @@ public function pathway7_19_providerWithXssInName_sanitized(): void
$provider->setIdentifier('xss-name-provider-' . time());
$provider->setName($xssName);
$provider->setAdapterType('openai');
$provider->setApiKey('sk-test');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setIsActive(true);

$this->providerRepository->add($provider);
Expand Down
4 changes: 2 additions & 2 deletions Tests/E2E/Backend/MultiProviderWorkflowsE2ETest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ public function pathway8_2_fallbackScenario_primaryProviderDisabled(): void
$fallbackProvider->setIdentifier('fallback-test-provider');
$fallbackProvider->setName('Fallback Test Provider');
$fallbackProvider->setAdapterType('openai');
$fallbackProvider->setApiKey('fallback-key');
$fallbackProvider->setApiKey('0190a5e0-7a1c-7b2d-8a1b-2c3d4e5f6a7b');
$fallbackProvider->setIsActive(true);
$fallbackProvider->setPriority(10); // Lower priority

Expand Down Expand Up @@ -772,7 +772,7 @@ public function providerChain_createFullStack(): void
$provider->setIdentifier('chain-test-provider-' . time());
$provider->setName('Chain Test Provider');
$provider->setAdapterType('openai');
$provider->setApiKey('test-key');
$provider->setApiKey('0190a5e0-7a1c-7b2d-8f3e-4a5b6c7d8e9f');
$provider->setIsActive(true);

$this->providerRepository->add($provider);
Expand Down
Loading
Loading