fix!: prevent plaintext API key storage via setup wizard - #97
Conversation
BREAKING CHANGE: Provider::setApiKey() now rejects raw API keys and only accepts vault identifiers (UUID v7 format) or empty strings. Code that previously set raw API keys must use VaultServiceInterface to store the secret first and pass the resulting UUID. The SetupWizardController stored API keys via Extbase persistence ($this->providerRepository->add()), which completely bypasses TYPO3 DataHandler hooks where nr_vault intercepts writes to store secrets securely. This resulted in raw API keys (e.g., sk-proj-...) being stored as plaintext in the tx_nrllm_provider.api_key column. Security impact: plaintext API keys are accessible to anyone with database read access, visible in backups, logs, and the TYPO3 List module. The provider was also non-functional since getDecryptedApiKey() tried to use the raw key as a vault identifier. Fixes: - SetupWizardController now uses VaultServiceInterface to store the API key before Extbase persistence, saving only the vault UUID - Provider::setApiKey() validates UUID format, rejecting raw secrets as defense-in-depth - Provider::getDecryptedApiKey() detects legacy plaintext values and emits E_USER_WARNING instead of silently failing vault lookups - Returns HTTP 500 if vault storage fails, preventing silent fallback to plaintext Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
|
Thanks for your first pull request! We are excited to have you contribute. |
github-actions
Bot
added
tests
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for solo maintainer project
This PR has passed all automated quality gates:
- ✅ Static analysis (PHPStan)
- ✅ Code style (PHP-CS-Fixer)
- ✅ Unit & functional tests
- ✅ Security scanning
- ✅ Dependency review
See SECURITY_CONTROLS.md for compensating controls documentation.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical security vulnerability where API keys were stored in plaintext in the database by the Setup Wizard, bypassing existing encryption mechanisms. The changes ensure that all API keys are now securely stored in a vault, with only an encrypted reference (UUID v7) persisted in the database. This significantly enhances data security by preventing direct exposure of sensitive credentials and includes validation to prevent future plaintext storage, along with mechanisms to handle and migrate existing legacy data. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent security fix that correctly addresses the plaintext storage of API keys by integrating with nr_vault. The defense-in-depth approach in the Provider model, which now validates for a vault identifier format, is a great addition to prevent accidental misuse. The test suite has also been thoroughly updated to reflect these critical changes.
I have one suggestion to improve maintainability by replacing the manual UUID v7 generation with a call to a standard library component, which is detailed in the specific comment.
There was a problem hiding this comment.
Pull request overview
Security-focused change to ensure provider API keys are never persisted as plaintext by the Setup Wizard, aligning wizard behavior with nr-vault’s encrypted secret storage model.
Changes:
- Setup Wizard now stores the raw API key in nr-vault first and persists only the vault identifier.
- Provider model hardens against plaintext secrets by validating
apiKeyas a vault identifier and warning on legacy plaintext values. - Updates unit/functional/fuzzy/E2E tests + fixtures to use vault-identifier-shaped values and assert rejection of raw secrets.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Classes/Controller/Backend/SetupWizardController.php | Stores API key via VaultServiceInterface and saves only a generated vault identifier |
| Classes/Domain/Model/Provider.php | Rejects non-vault identifiers in setApiKey() and warns on legacy plaintext in getDecryptedApiKey() |
| Tests/Unit/Controller/Backend/ModelControllerTest.php | Updates provider fixture API key to UUID v7-shaped identifier |
| Tests/Fuzzy/Security/InputSanitizationFuzzyTest.php | Changes expectation to reject raw/injection-like API keys |
| Tests/Fuzzy/Domain/ProviderFuzzyTest.php | Adds acceptance test for UUID v7/empty and rejection property test for raw secrets |
| Tests/Functional/Repository/ProviderRepositoryTest.php | Updates persisted provider API key to UUID v7-shaped identifier |
| Tests/Functional/Fixtures/Providers.csv | Updates fixture data to store UUID v7-shaped identifier instead of plaintext |
| Tests/E2E/Backend/ProviderManagementE2ETest.php | Updates E2E setup flows to use UUID v7-shaped identifiers |
| Tests/E2E/Backend/MultiProviderWorkflowsE2ETest.php | Updates multi-provider E2E fixtures to use UUID v7-shaped identifiers |
| Tests/E2E/Backend/ErrorPathwaysE2ETest.php | Updates error-path E2E fixtures and adds assertion that raw keys are rejected |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace the manual UUID v7 implementation in SetupWizardController with Symfony\Component\Uid\Uuid::v7()->toRfc4122() as suggested in code review. The symfony/uid component is available as a transitive dependency through TYPO3. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
There was a problem hiding this comment.
Automated approval for solo maintainer project
This PR has passed all automated quality gates:
- ✅ Static analysis (PHPStan)
- ✅ Code style (PHP-CS-Fixer)
- ✅ Unit & functional tests
- ✅ Security scanning
- ✅ Dependency review
See SECURITY_CONTROLS.md for compensating controls documentation.
Summary
Security fix: The Setup Wizard stored raw API keys in plaintext in the database, bypassing nr_vault encryption entirely. This was caused by using Extbase persistence (
$this->providerRepository->add()) which does not trigger TYPO3 DataHandler hooks where nr_vault intercepts writes.Security impact
sk-proj-...) stored as plaintext intx_nrllm_provider.api_keygetDecryptedApiKey()tried to use raw key as vault identifier →SecretNotFoundException→ returned empty stringChanges
SetupWizardController.php (P0 Critical)
VaultServiceInterfaceas constructor dependencysaveAction()now stores the API key in vault via$this->vaultService->store()before Extbase persistencegenerateVaultIdentifier()method producing UUID v7 formatProvider.php (P1 Defense-in-depth)
setApiKey()now validates that the value is a UUID v7 vault identifier or empty stringInvalidArgumentExceptionif a raw secret is passed (defense-in-depth)isVaultIdentifier()private static method for UUID v7 format validationgetDecryptedApiKey()detects legacy plaintext values and emitsE_USER_WARNINGinstead of silently failing vault lookupsTests (8 files updated)
Breaking Change
Provider::setApiKey()now rejects raw API keys. Code that previously set raw keys must:VaultServiceInterface::store($identifier, $secret, $metadata)firstsetApiKey()Remediation for existing data
Use the vault CLI to migrate existing plaintext values:
Test plan
vault:migrate-fieldremediates existing plaintext values